Click an empty part of the map to add a place there #120

Merged

rob merged 5 commits from feat/map-click-add-place into main

2026-08-11 14:54:51 +00:00

rob commented

2026-08-11 13:25:46 +00:00

Owner

Task 176. Clicking an empty part of the map starts adding a place there, pre-filled with the clicked coordinates.

LeafletMap.OnMapClicked (new MapCoordinates callback), independent of EnablePicking so the home map's existing markers stay fixed rather than becoming draggable.
Every marker now sets bubblingMouseEvents: false. Leaflet defaults it to true, so without this a click on an existing marker also fires a map click and opens the add-place form. Confirmed necessary rather than assumed, and proved by MapClickAddPlaceJourneyTests, which clicks the map, adds a place, then clicks the resulting marker and asserts only the detail panel opens. Reverting the flag reddens that journey.
PlaceForm.InitialCoordinates/DefaultGroupId (both optional) reuse the existing form rather than adding a second one. Default group is the expanded accordion group if the caller may add to it, else their personal group; a view-only group is never preselected.
A second map click on an already-open panel reseeds the coordinates. PlaceFormPanel keeps the form mounted across clicks and PlaceForm's @key is null for every add, so the instance is reused — without this the panel silently kept the first click's coordinates and would have saved a place at the wrong spot. PlaceForm now tracks the value it last seeded from and reseeds in OnParametersSet when a genuinely new one arrives, comparing against the last-seeded value rather than the live fields so a manually picked pin survives an unrelated re-render. Coordinates only — name, description and group are left intact, since a second click relocates the pin rather than restarting the form. Recorded in ADR-0105.
PR #117's duplicate-id race does not materialise here: the map's existing inert binding prevents a click landing while a panel is open or closing, verified in review with real forced-click races rather than accepted on argument.
The general "Add a place" button is kept, argued in ADR-0105: map-click needs a pointer and is not discoverable, so removing it would cost keyboard users their only route.

Review raised two findings, both fixed and mutation-tested — the reseed defect above, and a missing regression test for the view-only default-group fallback. Full suite, architecture tests, coverage ratchet (WebUI → 90.1%), dotnet format and the E2E suite all clean on the pinned SDK.

Task 176. Clicking an empty part of the map starts adding a place there, pre-filled with the clicked coordinates. - `LeafletMap.OnMapClicked` (new `MapCoordinates` callback), independent of `EnablePicking` so the home map's existing markers stay fixed rather than becoming draggable. - **Every marker now sets `bubblingMouseEvents: false`.** Leaflet defaults it to `true`, so without this a click on an existing marker also fires a map click and opens the add-place form. Confirmed necessary rather than assumed, and proved by `MapClickAddPlaceJourneyTests`, which clicks the map, adds a place, then clicks the resulting marker and asserts only the detail panel opens. Reverting the flag reddens that journey. - `PlaceForm.InitialCoordinates`/`DefaultGroupId` (both optional) reuse the existing form rather than adding a second one. Default group is the expanded accordion group if the caller may add to it, else their personal group; a view-only group is never preselected. - **A second map click on an already-open panel reseeds the coordinates.** `PlaceFormPanel` keeps the form mounted across clicks and `PlaceForm`'s `@key` is null for every add, so the instance is reused — without this the panel silently kept the *first* click's coordinates and would have saved a place at the wrong spot. `PlaceForm` now tracks the value it last seeded from and reseeds in `OnParametersSet` when a genuinely new one arrives, comparing against the last-**seeded** value rather than the live fields so a manually picked pin survives an unrelated re-render. Coordinates only — name, description and group are left intact, since a second click relocates the pin rather than restarting the form. Recorded in ADR-0105. - PR #117's duplicate-id race does not materialise here: the map's existing `inert` binding prevents a click landing while a panel is open or closing, verified in review with real forced-click races rather than accepted on argument. - The general "Add a place" button is **kept**, argued in ADR-0105: map-click needs a pointer and is not discoverable, so removing it would cost keyboard users their only route. Review raised two findings, both fixed and mutation-tested — the reseed defect above, and a missing regression test for the view-only default-group fallback. Full suite, architecture tests, coverage ratchet (WebUI → 90.1%), `dotnet format` and the E2E suite all clean on the pinned SDK.

rob added 1 commit

2026-08-11 13:25:46 +00:00

Click an empty part of the map to add a place there

CI / build (pull_request) Successful in 3m11s

Details

CI / e2e (pull_request) Successful in 1m48s

Details

e8dc9c0c1d

rob reviewed

2026-08-11 13:55:43 +00:00

rob left a comment

Verdict: changes needed

Reviewed at e8dc9c0 (unchanged since PR creation, mergeable, CI run #529 green).

Central claim verified. Reverted bubblingMouseEvents: false to Leaflet's own default (true) and reran MapClickAddPlaceJourneyTests against a real browser: it goes red, with a second "Add a place" panel visibly opening alongside the detail panel a marker click should only have selected. The journey genuinely discriminates.

Regression risk (PR #117's duplicate-id shape) — tested directly, not accepted on argument, and it holds. Built two E2E races against the real app: (1) start the drawer's own add-panel closing (200 ms animation) and fire a forced map click before it completes; (2) fire a forced map click immediately after opening the drawer's add-panel, before any settling wait. Both were run several times. Neither ever produced a second #place-form-name. inert (a real browser semantic, not merely a Playwright actionability check Force: true bypasses) blocks the click outright in both windows.

But there is a real gap in the field-level defence the PR describes as belt-and-braces, and it's reachable if HandleMapClicked is ever invoked twice for the same open panel. Home's own @key for the map-click panel is a fixed sentinel, and PlaceFormPanel's own <PlaceForm @key="Place?.Id"> is null for every add (Place is always null), so the same PlaceForm instance is reused across repeat clicks to the same panel — OnInitialized never re-runs, so InitialCoordinates is never re-seeded. Confirmed by bUnit: call LeafletMap.OnMapAreaClicked once, then again at different coordinates without closing the panel in between — the form keeps showing the first click's coordinates. A second, independent bUnit check confirms this is fine once the panel is genuinely closed and reopened (full unmount, fresh OnInitialized) — the bug is specific to a second call landing while the same instance is still live. Today inert is the only thing standing between a stray click and this — which the two race tests above show is currently solid — but the PR's own words ("nulls … defensively … rather than relying only on the browser argument") describe a second line of defence that doesn't actually cover this case: the null-guard only prevents edit and add fighting each other, not a second add landing on the same open add. If inert is ever weakened by an unrelated future change, this reseeds silently — no error, just a place saved at the wrong coordinates. Worth a guard (e.g. ignore a click while _addingPlaceAtCoordinates is already set, matching the eligibility-check early return already there) or, at minimum, a named, deliberate limit in ADR-0105 rather than the current "defensively" phrasing.

Minor: the viewer-only-expanded-group default is correct but untested. DefaultGroupIdForMapClick's { Role: GroupRole.Owner or GroupRole.Editor } pattern correctly excludes a Viewer-only expanded group and falls back to the personal group — confirmed by bUnit — but no test in this PR exercises that specific combination (every existing case is either no group expanded, or the expanded group is one the caller can add to). Worth a regression test given it's the one branch this ticket's own reasoning depends on for not preselecting a group the caller can't use.

Everything else checked out: the two "Add a place" routes don't diverge beyond the two new optional parameters (PlaceFormPanel/PlaceForm diffs are a clean, symmetric pass-through), no @code blocks, ADR-0105 numbered and indexed cleanly, coverage ratchet holds (WebUI 87.9% → 90%, reproduced independently), and dotnet format is clean.

Verdict: changes needed Reviewed at `e8dc9c0` (unchanged since PR creation, mergeable, CI run #529 green). **Central claim verified.** Reverted `bubblingMouseEvents: false` to Leaflet's own default (`true`) and reran `MapClickAddPlaceJourneyTests` against a real browser: it goes red, with a second "Add a place" panel visibly opening alongside the detail panel a marker click should only have selected. The journey genuinely discriminates. **Regression risk (PR #117's duplicate-id shape) — tested directly, not accepted on argument, and it holds.** Built two E2E races against the real app: (1) start the drawer's own add-panel closing (200 ms animation) and fire a forced map click before it completes; (2) fire a forced map click immediately after opening the drawer's add-panel, before any settling wait. Both were run several times. Neither ever produced a second `#place-form-name`. `inert` (a real browser semantic, not merely a Playwright actionability check `Force: true` bypasses) blocks the click outright in both windows. **But there is a real gap in the field-level defence the PR describes as belt-and-braces, and it's reachable if `HandleMapClicked` is ever invoked twice for the same open panel.** `Home`'s own `@key` for the map-click panel is a fixed sentinel, and `PlaceFormPanel`'s own `<PlaceForm @key="Place?.Id">` is `null` for every add (`Place` is always null), so the *same* `PlaceForm` instance is reused across repeat clicks to the same panel — `OnInitialized` never re-runs, so `InitialCoordinates` is never re-seeded. Confirmed by bUnit: call `LeafletMap.OnMapAreaClicked` once, then again at different coordinates *without closing the panel in between* — the form keeps showing the **first** click's coordinates. A second, independent bUnit check confirms this is fine once the panel is genuinely closed and reopened (full unmount, fresh `OnInitialized`) — the bug is specific to a second call landing while the same instance is still live. Today `inert` is the only thing standing between a stray click and this — which the two race tests above show is currently solid — but the PR's own words ("nulls … defensively … rather than relying only on the browser argument") describe a second line of defence that doesn't actually cover this case: the null-guard only prevents *edit* and *add* fighting each other, not a second add landing on the same open add. If `inert` is ever weakened by an unrelated future change, this reseeds silently — no error, just a place saved at the wrong coordinates. Worth a guard (e.g. ignore a click while `_addingPlaceAtCoordinates` is already set, matching the eligibility-check early return already there) or, at minimum, a named, deliberate limit in ADR-0105 rather than the current "defensively" phrasing. **Minor: the viewer-only-expanded-group default is correct but untested.** `DefaultGroupIdForMapClick`'s `{ Role: GroupRole.Owner or GroupRole.Editor }` pattern correctly excludes a Viewer-only expanded group and falls back to the personal group — confirmed by bUnit — but no test in this PR exercises that specific combination (every existing case is either no group expanded, or the expanded group is one the caller can add to). Worth a regression test given it's the one branch this ticket's own reasoning depends on for not preselecting a group the caller can't use. Everything else checked out: the two "Add a place" routes don't diverge beyond the two new optional parameters (`PlaceFormPanel`/`PlaceForm` diffs are a clean, symmetric pass-through), no `@code` blocks, ADR-0105 numbered and indexed cleanly, coverage ratchet holds (WebUI 87.9% → 90%, reproduced independently), and `dotnet format` is clean.

rob added 2 commits

2026-08-11 14:31:17 +00:00

Merge remote-tracking branch 'origin/main' into feat/map-click-add-place a0ef170555

Reseed the add-place form's coordinates on a second map click

CI / build (pull_request) Successful in 3m11s

Details

CI / e2e (pull_request) Successful in 1m45s

Details

3794b3d9cb

rob reviewed

2026-08-11 14:37:03 +00:00

rob left a comment

Verdict: changes needed

Re-reviewed only 3794b3d's diff over e8dc9c0. Both prior findings are fixed and independently mutation-tested by me, not just read: disabling OnParametersSet reddens PlaceForm_InitialCoordinatesChangesOnAnAlreadyMountedForm_ReseedsTheCoordinateFields; changing the guard to compare against live _latitude/_longitude instead of _seededInitialCoordinates reddens PlaceForm_RerenderedWithTheSameInitialCoordinatesAfterAManualPick_DoesNotOverwriteThePick — that second one is the test that actually proves the risky part (comparing against the last-seeded value, not the current fields), and it holds. MapCoordinates is a readonly record struct, so the != guard is genuine value equality, not reference — confirmed, not assumed. Traced the "Add a place" button path separately: InitialCoordinates stays null throughout that route, so the reseed condition never fires and there's nothing to clobber there. CI green on 3794b3d (build 614, e2e 615, both success, run #446).

Only outstanding item: "coordinates only, not name/description/group" is a real product decision, well-argued in the OnParametersSet XML doc, but it's not in ADR-0105 — the ADR wasn't touched by this commit. Every other consequential call in this file (the double? fix, the group-default fallback, the bubblingMouseEvents fix) got an ADR paragraph; this one should too, as an addendum to ADR-0105 rather than living only in a doc comment and a commit message.

Verdict: changes needed Re-reviewed only `3794b3d`'s diff over `e8dc9c0`. Both prior findings are fixed and independently mutation-tested by me, not just read: disabling `OnParametersSet` reddens `PlaceForm_InitialCoordinatesChangesOnAnAlreadyMountedForm_ReseedsTheCoordinateFields`; changing the guard to compare against live `_latitude`/`_longitude` instead of `_seededInitialCoordinates` reddens `PlaceForm_RerenderedWithTheSameInitialCoordinatesAfterAManualPick_DoesNotOverwriteThePick` — that second one is the test that actually proves the risky part (comparing against the last-seeded value, not the current fields), and it holds. `MapCoordinates` is a `readonly record struct`, so the `!=` guard is genuine value equality, not reference — confirmed, not assumed. Traced the "Add a place" button path separately: `InitialCoordinates` stays null throughout that route, so the reseed condition never fires and there's nothing to clobber there. CI green on `3794b3d` (build 614, e2e 615, both `success`, run #446). Only outstanding item: "coordinates only, not name/description/group" is a real product decision, well-argued in the `OnParametersSet` XML doc, but it's not in ADR-0105 — the ADR wasn't touched by this commit. Every other consequential call in this file (the `double?` fix, the group-default fallback, the `bubblingMouseEvents` fix) got an ADR paragraph; this one should too, as an addendum to ADR-0105 rather than living only in a doc comment and a commit message.

rob added 2 commits

2026-08-11 14:41:53 +00:00

Merge remote-tracking branch 'origin/main' into HEAD 56b2f1af43