Select a place by clicking its map marker #81
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/marker-click-select"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.
LeafletMapgainsOnMarkerActivated(EventCallback<Guid>), the same shape ADR-0064 already used forOnPick: aDotNetObjectReferencehanded tomap.jsonly when the parameter has a delegate, calling back one[JSInvokable]forwarding method.Home.razorbinds it straight toMapPlacesState.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.Markersonly plotsVisiblePlaces. That was wrong:SelectPlaceresolved 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.SelectPlacenow takes a mandatoryrequireVisibleparameter — 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.
PlaceFormPaneltherefore passesrequireVisible: falsedeliberately.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.jsnow setskeyboardto whether a marker can actually be activated, so a marker nothing can select is taken back out of the tab order, withclick/keydown/keyupwired for Enter and Space (Space on keyup, matching a native<button>) plus anaria-labelfrom a newMapMarker.Label.Coverage:
LeafletMapTestsandHomeTestsprove the .NET-side channel by invokingOnMapMarkerActivateddirectly, 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).Verdict: changes needed
The "resolves by construction" claim for the group filter doesn't hold at every instant.
MapPlacesState.SelectPlace/SelectedPlaceoperate on the unfiltered_placeslist, notVisiblePlaces— confirmed inMapPlacesState.cs.Home.Markersonly updates the JS-side marker set asynchronously (LeafletMap.OnParametersSetAsyncawaitssetMarkersafter 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 placeVisiblePlaceshas just excluded. A click landing in that window succeeds:SelectPlacesets it,SelectedPlaceresolves 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, becauseMarkersnever plots one"). That's true at rest, not true during the transition.Home_MarkerActivatedWhileAGroupIsExpandedInTheAccordion_SelectsWithoutChangingTheFilteronly 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 guardSelectPlace/SelectedPlaceagainst a placeVisiblePlacescurrently excludes, or correct ADR-0071 to acknowledge the window and say why it's accepted.Everything else checks out:
leaflet.js(1.9.4):Markeroptions defaultkeyboard: true, and_initIconsetstabIndex="0"/role="button"whenever it's set — so the "every marker is already a tab stop" claim is correct, not asserted from memory. Thekeyboard: activatablegate applies on everytoLeafletMarkerscall, including fromsetMarkers, so it covers markers created after first render, not just the initial batch.keydown, Space activates onkeyupwithpreventDefaulton both keys'keydown— matches native<button>.aria-label: not reachable in practice —CreatePlaceRequest/UpdatePlaceRequest.Nameuse[Required](which trims before checking) and the DB hasck_places_name_not_blank CHECK (length(btrim(name)) > 0), so a blank or whitespace-only place name can't reachMapMarker.Label.DotNetObjectReferencedisposal is correctly ordered (map.js dispose()removes JS listeners before_markerActivationReference.Dispose()infinally), on the right hook (DisposeAsync), consistent with the ADR-0064 pattern.Home.razorbindsOnMarkerActivatedstraight toPlacesState.SelectPlace, no wrapper, no second implementation.docs/adr/README.mdindex (0069 → 0071, no 0070), consistent with #79 owning 0070.LeafletMapTestsandHomeTestsonly 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
1cca144is green.Addressed at
7be4e48.MapPlacesState.SelectPlacegainsrequireVisible(defaultfalse);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 (noChanged). ADR-0071 rewritten to describe the actual guard instead of the "by construction" claim, plus a new alternative on where the check lives. AddedMapPlacesStateTestscoverage for the guard directly and aHomeTestsintegration test that fails without it (confirmed by temporarily removing the guard and re-running).Verdict: changes needed
The guard itself is sound and closes the window it claims to:
OnMapMarkerActivated→EventCallback<Guid>.InvokeAsync→ theid => SelectPlace(id, requireVisible: true)lambda all execute synchronously in one dispatch, WASM is single-threaded, andSelectPlace's check and its write to_selectedPlaceIdhappen 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,..._DoesNotRaiseChangedandHome_MarkerActivatedForAPlaceTheCurrentFilterHides_IsSilentlyIgnoredall fail as expected, pass with it restored. Silent-ignore is confirmed genuinely silent —Home.razorrendersPlaceDetailPanelonly fromSelectedPlace, 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 bareSelectPlace(saved.Id)is pre-existing, unrelated to this PR, and correct —Home.Centre/PlaceDetailPanelare driven bySelectedPlacedirectly, not gated on the place also appearing inMarkers/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 on7be4e48is green.requireVisibledefaulting tofalseis 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 aboutrequireVisibleinheritsfalsesilently, 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 passedfalsebut 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 makingrequireVisiblea 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 explicitfalse. 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.Addressed at
03229d3.requireVisibleis now mandatory (no default) — all three call sites state it explicitly:Home's marker binding and the accordion's button both passtrue(accordion can never actually hit the race, stated as an assertion of that invariant),PlaceFormPanel.HandleSavedAsyncpassesfalse.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_placeslookup, unchanged by this PR), butHome.Markers(built fromVisiblePlaces) plots no pin — a real, pre-existing gap (predates task 158 entirely;PlaceFormPanel's unconditional selection is task 159/ADR-0067,VisiblePlacesfiltering 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
requireVisibleisn't a one-line removal now that all three sites depend on it explicitly.Addressed at
c4ccbaa. ADR-0071 and thePlaceFormPanel.HandleSavedAsync/MapPlacesState.SelectPlacecomments 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, notmainitself, 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.Verdict: mergeable
requireVisibleis now mandatory, and the three call sites are correctly categorised:Home's marker binding andPlaceAccordion's button both passtrue,PlaceFormPanel.HandleSavedAsyncpassesfalse. 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 passtrue, everything elsefalse), 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.Centrealready read the unfilteredSelectedPlace,Home.Markersalready read filteredVisiblePlaces, andPlaceFormPanel.HandleSavedAsyncalready calledSelectPlace(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 forrequireVisiblesince 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
c4ccbaais green.