Select a place by clicking its map marker #81

Merged
rob merged 5 commits from feat/marker-click-select into main 2026-08-06 05:34:51 +00:00
Owner

Closes Vikunja task #158. Reverses the one clause of ADR-0062 that deferred this ("clicking a marker on the map itself does not select anything"); recorded as ADR-0071.

LeafletMap gains OnMarkerActivated (EventCallback<Guid>), the same shape ADR-0064 already used for OnPick: a DotNetObjectReference handed to map.js only when the parameter has a delegate, calling back one [JSInvokable] forwarding method. Home.razor binds it straight to MapPlacesState.SelectPlace — the same method the drawer's accordion already calls — so there is one selection path, not two implementations.

Correction to this description as originally written. It claimed the group-filter question "resolves by construction" because Home.Markers only plots VisiblePlaces. That was wrong: SelectPlace resolved against the unfiltered place list, leaving a window of one interop round trip in which a stale marker could select a place the filter hides. Review caught it. SelectPlace now takes a mandatory requireVisible parameter — mandatory so no future call site inherits the unguarded path by saying nothing — and all three call sites state their intent.

Investigating that guard surfaced a pre-existing defect, raised as Vikunja task 162 rather than fixed here: a place created into a filtered-out group is selected and panned to but never plotted. PlaceFormPanel therefore passes requireVisible: false deliberately.

Keyboard parity: Leaflet's own default (keyboard: true) already puts every marker in the tab order regardless of any of this — a latent instance of the tasks-139/157/PR-70 tab-stop defect, discovered rather than introduced here. map.js now sets keyboard to whether a marker can actually be activated, so a marker nothing can select is taken back out of the tab order, with click/keydown/keyup wired for Enter and Space (Space on keyup, matching a native <button>) plus an aria-label from a new MapMarker.Label.

Coverage: LeafletMapTests and HomeTests prove the .NET-side channel by invoking OnMapMarkerActivated directly, the same seam ADR-0064's picking tests use — bUnit has no JS runtime. Not covered: that a real browser click or key press reaches it, that focus is visible, or what a screen reader actually announces. No agent working this repository has a browser (task 156).

Closes Vikunja task #158. Reverses the one clause of ADR-0062 that deferred this ("clicking a marker on the map itself does not select anything"); recorded as ADR-0071. `LeafletMap` gains `OnMarkerActivated` (`EventCallback<Guid>`), the same shape ADR-0064 already used for `OnPick`: a `DotNetObjectReference` handed to `map.js` only when the parameter has a delegate, calling back one `[JSInvokable]` forwarding method. `Home.razor` binds it straight to `MapPlacesState.SelectPlace` — the same method the drawer's accordion already calls — so there is one selection path, not two implementations. **Correction to this description as originally written.** It claimed the group-filter question "resolves by construction" because `Home.Markers` only plots `VisiblePlaces`. That was wrong: `SelectPlace` resolved against the *unfiltered* place list, leaving a window of one interop round trip in which a stale marker could select a place the filter hides. Review caught it. `SelectPlace` now takes a mandatory `requireVisible` parameter — mandatory so no future call site inherits the unguarded path by saying nothing — and all three call sites state their intent. Investigating that guard surfaced a pre-existing defect, raised as Vikunja task 162 rather than fixed here: a place created into a filtered-out group is selected and panned to but never plotted. `PlaceFormPanel` therefore passes `requireVisible: false` deliberately. Keyboard parity: Leaflet's own default (`keyboard: true`) already puts every marker in the tab order regardless of any of this — a latent instance of the tasks-139/157/PR-70 tab-stop defect, discovered rather than introduced here. `map.js` now sets `keyboard` to whether a marker can actually be activated, so a marker nothing can select is taken back out of the tab order, with `click`/`keydown`/`keyup` wired for Enter and Space (Space on keyup, matching a native `<button>`) plus an `aria-label` from a new `MapMarker.Label`. Coverage: `LeafletMapTests` and `HomeTests` prove the .NET-side channel by invoking `OnMapMarkerActivated` directly, the same seam ADR-0064's picking tests use — bUnit has no JS runtime. **Not covered**: that a real browser click or key press reaches it, that focus is visible, or what a screen reader actually announces. No agent working this repository has a browser (task 156).
Select a place by clicking its map marker
All checks were successful
CI / build (pull_request) Successful in 2m34s
1cca144348
rob left a comment

Verdict: changes needed

The "resolves by construction" claim for the group filter doesn't hold at every instant. MapPlacesState.SelectPlace/SelectedPlace operate on the unfiltered _places list, not VisiblePlaces — confirmed in MapPlacesState.cs. Home.Markers only updates the JS-side marker set asynchronously (LeafletMap.OnParametersSetAsync awaits setMarkers after a filter-driven re-render), so there's a real window, one interop round trip wide, during which the DOM still shows and still has live click/keyboard listeners on a marker for a place VisiblePlaces has just excluded. A click landing in that window succeeds: SelectPlace sets it, SelectedPlace resolves it against _places, and the panel opens for a place outside the accordion's own expanded-group filter — exactly the "selected a place the filter would hide" state ADR-0071 asserts cannot exist ("there is no ... state for new code to prevent, because Markers never plots one"). That's true at rest, not true during the transition. Home_MarkerActivatedWhileAGroupIsExpandedInTheAccordion_SelectsWithoutChangingTheFilter only activates a marker that is in the expanded group, so it never exercises this path. Not a security issue (same caller-visible data either way) but the claim as written is factually wrong, and this project's other ADRs are otherwise scrupulous about naming exactly this kind of gap rather than asserting it away. Either guard SelectPlace/SelectedPlace against a place VisiblePlaces currently excludes, or correct ADR-0071 to acknowledge the window and say why it's accepted.

Everything else checks out:

  • Keyboard claim verified against the actual vendored leaflet.js (1.9.4): Marker options default keyboard: true, and _initIcon sets tabIndex="0"/role="button" whenever it's set — so the "every marker is already a tab stop" claim is correct, not asserted from memory. The keyboard: activatable gate applies on every toLeafletMarkers call, including from setMarkers, so it covers markers created after first render, not just the initial batch.
  • Enter/Space semantics are right: Enter activates on keydown, Space activates on keyup with preventDefault on both keys' keydown — matches native <button>.
  • Empty/whitespace aria-label: not reachable in practice — CreatePlaceRequest/UpdatePlaceRequest.Name use [Required] (which trims before checking) and the DB has ck_places_name_not_blank CHECK (length(btrim(name)) > 0), so a blank or whitespace-only place name can't reach MapMarker.Label.
  • DotNetObjectReference disposal is correctly ordered (map.js dispose() removes JS listeners before _markerActivationReference.Dispose() in finally), on the right hook (DisposeAsync), consistent with the ADR-0064 pattern.
  • One selection path confirmed — Home.razor binds OnMarkerActivated straight to PlacesState.SelectPlace, no wrapper, no second implementation.
  • ADR-0071's number is genuinely free against this branch's docs/adr/README.md index (0069 → 0071, no 0070), consistent with #79 owning 0070.
  • Coverage claims are honest: both LeafletMapTests and HomeTests only invoke the [JSInvokable] method directly and say so; nothing here exercises a real click, key press, focus outline or screen reader, and no test name implies otherwise.

Build and full test suite (1247 tests) pass clean on the pinned 10.0.100 SDK; CI run #383 on 1cca144 is green.

Verdict: changes needed **The "resolves by construction" claim for the group filter doesn't hold at every instant.** `MapPlacesState.SelectPlace`/`SelectedPlace` operate on the unfiltered `_places` list, not `VisiblePlaces` — confirmed in `MapPlacesState.cs`. `Home.Markers` only updates the JS-side marker set asynchronously (`LeafletMap.OnParametersSetAsync` awaits `setMarkers` after a filter-driven re-render), so there's a real window, one interop round trip wide, during which the DOM still shows and still has live click/keyboard listeners on a marker for a place `VisiblePlaces` has just excluded. A click landing in that window succeeds: `SelectPlace` sets it, `SelectedPlace` resolves it against `_places`, and the panel opens for a place outside the accordion's own expanded-group filter — exactly the "selected a place the filter would hide" state ADR-0071 asserts cannot exist ("there is no ... state for new code to prevent, because `Markers` never plots one"). That's true at rest, not true during the transition. `Home_MarkerActivatedWhileAGroupIsExpandedInTheAccordion_SelectsWithoutChangingTheFilter` only activates a marker that *is* in the expanded group, so it never exercises this path. Not a security issue (same caller-visible data either way) but the claim as written is factually wrong, and this project's other ADRs are otherwise scrupulous about naming exactly this kind of gap rather than asserting it away. Either guard `SelectPlace`/`SelectedPlace` against a place `VisiblePlaces` currently excludes, or correct ADR-0071 to acknowledge the window and say why it's accepted. Everything else checks out: - Keyboard claim verified against the actual vendored `leaflet.js` (1.9.4): `Marker` options default `keyboard: true`, and `_initIcon` sets `tabIndex="0"`/`role="button"` whenever it's set — so the "every marker is already a tab stop" claim is correct, not asserted from memory. The `keyboard: activatable` gate applies on every `toLeafletMarkers` call, including from `setMarkers`, so it covers markers created after first render, not just the initial batch. - Enter/Space semantics are right: Enter activates on `keydown`, Space activates on `keyup` with `preventDefault` on both keys' `keydown` — matches native `<button>`. - Empty/whitespace `aria-label`: not reachable in practice — `CreatePlaceRequest`/`UpdatePlaceRequest.Name` use `[Required]` (which trims before checking) and the DB has `ck_places_name_not_blank CHECK (length(btrim(name)) > 0)`, so a blank or whitespace-only place name can't reach `MapMarker.Label`. - `DotNetObjectReference` disposal is correctly ordered (`map.js dispose()` removes JS listeners before `_markerActivationReference.Dispose()` in `finally`), on the right hook (`DisposeAsync`), consistent with the ADR-0064 pattern. - One selection path confirmed — `Home.razor` binds `OnMarkerActivated` straight to `PlacesState.SelectPlace`, no wrapper, no second implementation. - ADR-0071's number is genuinely free against this branch's `docs/adr/README.md` index (0069 → 0071, no 0070), consistent with #79 owning 0070. - Coverage claims are honest: both `LeafletMapTests` and `HomeTests` only invoke the `[JSInvokable]` method directly and say so; nothing here exercises a real click, key press, focus outline or screen reader, and no test name implies otherwise. Build and full test suite (1247 tests) pass clean on the pinned 10.0.100 SDK; CI run #383 on 1cca144 is green.
Guard marker selection against a stale group filter
All checks were successful
CI / build (pull_request) Successful in 2m52s
7be4e4863d
Author
Owner

Addressed at 7be4e48. MapPlacesState.SelectPlace gains requireVisible (default false); Home.razor's marker binding is the only caller that sets it, since it's the only one crossing an interop round trip. Rejected selections are silently ignored (no Changed). ADR-0071 rewritten to describe the actual guard instead of the "by construction" claim, plus a new alternative on where the check lives. Added MapPlacesStateTests coverage for the guard directly and a HomeTests integration test that fails without it (confirmed by temporarily removing the guard and re-running).

Addressed at `7be4e48`. `MapPlacesState.SelectPlace` gains `requireVisible` (default `false`); `Home.razor`'s marker binding is the only caller that sets it, since it's the only one crossing an interop round trip. Rejected selections are silently ignored (no `Changed`). ADR-0071 rewritten to describe the actual guard instead of the "by construction" claim, plus a new alternative on where the check lives. Added `MapPlacesStateTests` coverage for the guard directly and a `HomeTests` integration test that fails without it (confirmed by temporarily removing the guard and re-running).
rob left a comment

Verdict: changes needed

The guard itself is sound and closes the window it claims to: OnMapMarkerActivatedEventCallback<Guid>.InvokeAsync → the id => SelectPlace(id, requireVisible: true) lambda all execute synchronously in one dispatch, WASM is single-threaded, and SelectPlace's check and its write to _selectedPlaceId happen in the same synchronous method — there's no second interop hop between the check and the effect. Confirmed by removing the guard: SelectPlace_RequireVisibleAndThePlaceIsHiddenByTheCurrentFilter_IsSilentlyIgnored, ..._DoesNotRaiseChanged and Home_MarkerActivatedForAPlaceTheCurrentFilterHides_IsSilentlyIgnored all fail as expected, pass with it restored. Silent-ignore is confirmed genuinely silent — Home.razor renders PlaceDetailPanel only from SelectedPlace, which the guard never touches, so a rejected click leaves map and panel in the same state as no click at all. ADR-0071's rewrite is accurate to the code, and the create-flow claim holds: PlaceFormPanel.HandleSavedAsync's bare SelectPlace(saved.Id) is pre-existing, unrelated to this PR, and correct — Home.Centre/PlaceDetailPanel are driven by SelectedPlace directly, not gated on the place also appearing in Markers/VisiblePlaces, so selecting a place outside the current filter already has defined, working behaviour independent of this change. No American spellings found in the touched files or the new ADR prose. Build and full suite (1224 tests across Api/Infrastructure/WebUI, plus Domain/Contracts/Architecture) pass clean on 10.0.100; CI run #386 on 7be4e48 is green.

requireVisible defaulting to false is the wrong way round, and the ADR doesn't consider the alternative despite considering everything else about this parameter. The failure mode this fix closes was exactly this shape: a caller that says nothing about visibility gets the unguarded behaviour. That's still true of the parameter itself — a fourth call site added later that forgets to think about requireVisible inherits false silently, and only misbehaves if it happens to cross an interop or other async boundary, which is precisely the kind of intermittent, hard-to-notice defect this PR exists to fix. Contrast with getting it wrong the other way: a caller that should have passed false but didn't just finds its selection silently does nothing, which is an obvious, immediately-visible break the first time anyone exercises it manually. That asymmetry is the argument for dropping the default and making requireVisible a required parameter — every call site, present and future, has to state its intent, and the three current sites (PlaceAccordion, PlaceFormPanel, Home's marker binding) all already state theirs explicitly or trivially gain an explicit false. This costs nothing today and removes the "silent unsafe default" shape entirely rather than documenting around it. Please make the change, or add this as a considered-and-rejected alternative in ADR-0071 with the actual reasoning for keeping an unsafe default over a mandatory parameter — right now the ADR's Decision section asserts the three call sites are all correctly categorised but never asks whether a fourth, unwritten one is safe by default, which is the same class of gap the Context section was rewritten to stop eliding.

Verdict: changes needed The guard itself is sound and closes the window it claims to: `OnMapMarkerActivated` → `EventCallback<Guid>.InvokeAsync` → the `id => SelectPlace(id, requireVisible: true)` lambda all execute synchronously in one dispatch, WASM is single-threaded, and `SelectPlace`'s check and its write to `_selectedPlaceId` happen in the same synchronous method — there's no second interop hop between the check and the effect. Confirmed by removing the guard: `SelectPlace_RequireVisibleAndThePlaceIsHiddenByTheCurrentFilter_IsSilentlyIgnored`, `..._DoesNotRaiseChanged` and `Home_MarkerActivatedForAPlaceTheCurrentFilterHides_IsSilentlyIgnored` all fail as expected, pass with it restored. Silent-ignore is confirmed genuinely silent — `Home.razor` renders `PlaceDetailPanel` only from `SelectedPlace`, which the guard never touches, so a rejected click leaves map and panel in the same state as no click at all. ADR-0071's rewrite is accurate to the code, and the create-flow claim holds: `PlaceFormPanel.HandleSavedAsync`'s bare `SelectPlace(saved.Id)` is pre-existing, unrelated to this PR, and correct — `Home.Centre`/`PlaceDetailPanel` are driven by `SelectedPlace` directly, not gated on the place also appearing in `Markers`/`VisiblePlaces`, so selecting a place outside the current filter already has defined, working behaviour independent of this change. No American spellings found in the touched files or the new ADR prose. Build and full suite (1224 tests across Api/Infrastructure/WebUI, plus Domain/Contracts/Architecture) pass clean on 10.0.100; CI run #386 on 7be4e48 is green. **`requireVisible` defaulting to `false` is the wrong way round, and the ADR doesn't consider the alternative despite considering everything else about this parameter.** The failure mode this fix closes was exactly this shape: a caller that says nothing about visibility gets the unguarded behaviour. That's still true of the parameter itself — a fourth call site added later that forgets to think about `requireVisible` inherits `false` silently, and only misbehaves if it happens to cross an interop or other async boundary, which is precisely the kind of intermittent, hard-to-notice defect this PR exists to fix. Contrast with getting it wrong the other way: a caller that should have passed `false` but didn't just finds its selection silently does nothing, which is an obvious, immediately-visible break the first time anyone exercises it manually. That asymmetry is the argument for dropping the default and making `requireVisible` a required parameter — every call site, present and future, has to state its intent, and the three current sites (`PlaceAccordion`, `PlaceFormPanel`, `Home`'s marker binding) all already state theirs explicitly or trivially gain an explicit `false`. This costs nothing today and removes the "silent unsafe default" shape entirely rather than documenting around it. Please make the change, or add this as a considered-and-rejected alternative in ADR-0071 with the actual reasoning for keeping an unsafe default over a mandatory parameter — right now the ADR's Decision section asserts the three call sites are all correctly categorised but never asks whether a fourth, unwritten one is safe by default, which is the same class of gap the Context section was rewritten to stop eliding.
Make SelectPlace's visibility guard a mandatory parameter
All checks were successful
CI / build (pull_request) Successful in 2m24s
03229d33ba
Author
Owner

Addressed at 03229d3. requireVisible is now mandatory (no default) — all three call sites state it explicitly: Home's marker binding and the accordion's button both pass true (accordion can never actually hit the race, stated as an assertion of that invariant), PlaceFormPanel.HandleSavedAsync passes false.

Checked the question directly: if a newly created place's own group isn't expanded, Home.Centre/the detail panel still select and pan there (unfiltered _places lookup, unchanged by this PR), but Home.Markers (built from VisiblePlaces) plots no pin — a real, pre-existing gap (predates task 158 entirely; PlaceFormPanel's unconditional selection is task 159/ADR-0067, VisiblePlaces filtering is ADR-0062). Bypassing is still correct there — guarding it would silently break the already-shipped "add a place always opens its detail" behaviour instead. Documented in ADR-0071's Decision section, with the reasoning laid out rather than asserted.

One limitation to flag: I don't have a Vikunja task-creation tool this session (only get/list/comment), so I couldn't raise the dedicated follow-up ticket myself — flagged it as a comment on task 158 instead, with a note that it still needs a proper ticket.

ADR-0071's "checking at the call site" alternative reframed against the mandatory signature; the "Reversing this" consequence corrected to note requireVisible isn't a one-line removal now that all three sites depend on it explicitly.

Addressed at `03229d3`. `requireVisible` is now mandatory (no default) — all three call sites state it explicitly: `Home`'s marker binding and the accordion's button both pass `true` (accordion can never actually hit the race, stated as an assertion of that invariant), `PlaceFormPanel.HandleSavedAsync` passes `false`. Checked the question directly: if a newly created place's own group isn't expanded, `Home.Centre`/the detail panel still select and pan there (unfiltered `_places` lookup, unchanged by this PR), but `Home.Markers` (built from `VisiblePlaces`) plots no pin — a real, pre-existing gap (predates task 158 entirely; `PlaceFormPanel`'s unconditional selection is task 159/ADR-0067, `VisiblePlaces` filtering is ADR-0062). Bypassing is still correct there — guarding it would silently break the already-shipped "add a place always opens its detail" behaviour instead. Documented in ADR-0071's Decision section, with the reasoning laid out rather than asserted. One limitation to flag: I don't have a Vikunja task-creation tool this session (only get/list/comment), so I couldn't raise the dedicated follow-up ticket myself — flagged it as a comment on task 158 instead, with a note that it still needs a proper ticket. ADR-0071's "checking at the call site" alternative reframed against the mandatory signature; the "Reversing this" consequence corrected to note `requireVisible` isn't a one-line removal now that all three sites depend on it explicitly.
Cite Vikunja task 162 for the create-flow marker gap
All checks were successful
CI / build (pull_request) Successful in 2m32s
c4ccbaa501
Author
Owner

Addressed at c4ccbaa. ADR-0071 and the PlaceFormPanel.HandleSavedAsync/MapPlacesState.SelectPlace comments now cite Vikunja task 162 by number instead of pointing at a comment thread.

Checked for the American spelling flagged — swept every line added across this branch (diffed against the actual merge-base with main, not main itself, since that also picks up PR #79's unrelated changes) for "-ize/-ise" family words and other common AmE/BrE differences. Found none in the committed content (existing "behaviour"/"honour" instances are already correct BrE); the "centralizing" you saw must have been in one of my earlier PR comments rather than the ADR file itself — I can't edit those after the fact, but the file content is clean.

Addressed at `c4ccbaa`. ADR-0071 and the `PlaceFormPanel.HandleSavedAsync`/`MapPlacesState.SelectPlace` comments now cite Vikunja task 162 by number instead of pointing at a comment thread. Checked for the American spelling flagged — swept every line added across this branch (diffed against the actual merge-base with `main`, not `main` itself, since that also picks up PR #79's unrelated changes) for "-ize/-ise" family words and other common AmE/BrE differences. Found none in the committed content (existing "behaviour"/"honour" instances are already correct BrE); the "centralizing" you saw must have been in one of my earlier PR comments rather than the ADR file itself — I can't edit those after the fact, but the file content is clean.
rob left a comment

Verdict: mergeable

requireVisible is now mandatory, and the three call sites are correctly categorised: Home's marker binding and PlaceAccordion's button both pass true, PlaceFormPanel.HandleSavedAsync passes false. No other production or test call site was missed — every test-code call site passes the value appropriate to what it's testing (the filter-guard tests pass true, everything else false), none is a compile-fix that quietly weakens a test.

Verified the create-flow claim independently against the merge-base, not just the branch's own prose: Home.Centre already read the unfiltered SelectedPlace, Home.Markers already read filtered VisiblePlaces, and PlaceFormPanel.HandleSavedAsync already called SelectPlace(saved.Id) unconditionally, all before this PR existed. The diagnosis and the "pre-existing, not introduced here" claim both hold; ADR-0071 and task 162 are correctly grounded.

ADR-0071 as it now stands is accurate: the mandatory-parameter reasoning, the reframed "asserts an invariant rather than bypasses one" treatment of the accordion's own true, the corrected "reversing this" consequence (additive for the JS/interop surface, not free for requireVisible since it's mandatory), and the task 162 citation all check out against the code.

Whole-branch diff against the merge-base found nothing beyond what the two prior reviews already covered.

Build and full suite (1376 tests) pass clean on the pinned 10.0.100 SDK; CI run #388 on c4ccbaa is green.

Verdict: mergeable `requireVisible` is now mandatory, and the three call sites are correctly categorised: `Home`'s marker binding and `PlaceAccordion`'s button both pass `true`, `PlaceFormPanel.HandleSavedAsync` passes `false`. No other production or test call site was missed — every test-code call site passes the value appropriate to what it's testing (the filter-guard tests pass `true`, everything else `false`), none is a compile-fix that quietly weakens a test. Verified the create-flow claim independently against the merge-base, not just the branch's own prose: `Home.Centre` already read the unfiltered `SelectedPlace`, `Home.Markers` already read filtered `VisiblePlaces`, and `PlaceFormPanel.HandleSavedAsync` already called `SelectPlace(saved.Id)` unconditionally, all before this PR existed. The diagnosis and the "pre-existing, not introduced here" claim both hold; ADR-0071 and task 162 are correctly grounded. ADR-0071 as it now stands is accurate: the mandatory-parameter reasoning, the reframed "asserts an invariant rather than bypasses one" treatment of the accordion's own `true`, the corrected "reversing this" consequence (additive for the JS/interop surface, not free for `requireVisible` since it's mandatory), and the task 162 citation all check out against the code. Whole-branch diff against the merge-base found nothing beyond what the two prior reviews already covered. Build and full suite (1376 tests) pass clean on the pinned 10.0.100 SDK; CI run #388 on c4ccbaa is green.
Merge remote-tracking branch 'origin/main' into feat/marker-click-select
All checks were successful
CI / build (pull_request) Successful in 2m34s
1af31f54ef
# Conflicts:
#	docs/adr/README.md
rob merged commit d0e0d1283f into main 2026-08-06 05:34:51 +00:00
rob deleted branch feat/marker-click-select 2026-08-06 05:34:51 +00:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
rob/PlaceMark!81
No description provided.