Wire add-place, add-group and edit-place entry points into the map UI #75

Merged

rob merged 4 commits from feat/entry-points into main 2026-08-05 21:40:38 +00:00

Conversation 4 Commits 4 Files changed 37 +2229 -41

rob commented 2026-08-05 20:56:34 +00:00

Owner

Copy link

Implements task 159. PlaceForm and GroupForm existed but nothing opened either — this hosts both, plus editing, as floating panels matching GroupMembersPanel's own shape (ADR-0065).

• PlaceFormPanel and GroupFormPanel host the existing forms; Drawer opens them for add-place (general and per-group, pre-selecting the group) and add-group, Home opens the place form for editing from the detail panel's own Edit button.
• @key="Place?.Id" on PlaceForm inside PlaceFormPanel — the requirement PR #72's review found. PlaceFormPanelTests proves swapping which place is being edited on an already-rendered instance shows and submits the right place's data, and reddens if the key is removed.
• MapPlacesState.UpsertPlace patches the map and drawer locally after a save — no reload, per ADR-0062's own predicted extension point.
• Both add-place buttons (general and per-group) are gated on a role read independently of MapPlacesState (GroupMembershipLoader), closing the gap where a caller with no places anywhere yet would have no add-place affordance at all.
• Authorisation is a courtesy: hidden where the caller's role doesn't allow it, but the API's own 403/409 is still handled if a stale role slips through.

Reasoning, alternatives and what's left unreconciled (three independent GET /api/groups loaders) in ADR-0067 — next free number, confirmed against main and the one open PR (#74, which holds 0066).

Full solution build, dotnet test and dotnet format --verify-no-changes all clean under the pinned SDK.

Unverified in a browser (task 156, as ever): focus movement, whether the new panels visually clear the drawer on a narrow viewport, and screen-reader announcement of panel headings.

Implements task 159. `PlaceForm` and `GroupForm` existed but nothing opened either — this hosts both, plus editing, as floating panels matching `GroupMembersPanel`'s own shape (ADR-0065). - `PlaceFormPanel` and `GroupFormPanel` host the existing forms; `Drawer` opens them for add-place (general and per-group, pre-selecting the group) and add-group, `Home` opens the place form for editing from the detail panel's own Edit button. - `@key="Place?.Id"` on `PlaceForm` inside `PlaceFormPanel` — the requirement PR #72's review found. `PlaceFormPanelTests` proves swapping which place is being edited on an already-rendered instance shows and submits the right place's data, and reddens if the key is removed. - `MapPlacesState.UpsertPlace` patches the map and drawer locally after a save — no reload, per ADR-0062's own predicted extension point. - Both add-place buttons (general and per-group) are gated on a role read independently of `MapPlacesState` (`GroupMembershipLoader`), closing the gap where a caller with no places anywhere yet would have no add-place affordance at all. - Authorisation is a courtesy: hidden where the caller's role doesn't allow it, but the API's own 403/409 is still handled if a stale role slips through. Reasoning, alternatives and what's left unreconciled (three independent `GET /api/groups` loaders) in ADR-0067 — next free number, confirmed against `main` and the one open PR (#74, which holds 0066). Full solution build, `dotnet test` and `dotnet format --verify-no-changes` all clean under the pinned SDK. **Unverified in a browser** (task 156, as ever): focus movement, whether the new panels visually clear the drawer on a narrow viewport, and screen-reader announcement of panel headings.

rob added 1 commit 2026-08-05 20:56:34 +00:00

Wire add-place, add-group and edit-place entry points into the map UI edeab287a9

rob reviewed 2026-08-05 21:06:17 +00:00

rob left a comment

Copy link

Verdict: changes needed

Home's own independently-loaded group/role cache never refreshes after a group created via Drawer's GroupFormPanel — reachable in the exact "create a group → add a place → edit it" flow this PR wires up end to end, in one session. Drawer.HandleGroupSavedAsync reloads PlaceAccordion and GroupsSection after a save, but has no reference to Home (siblings, per ADR-0062) — so Home's own _groups (loaded once in OnInitializedAsync via GroupMembershipLoader) never learns about the new group. Concretely: caller creates a group, PlaceAccordion's general "Add a place" button correctly appears (its own loader was reloaded), caller adds a place to the new group, it gets selected and Home shows the detail panel — but RoleFor(newGroupId) returns null against the stale _groups, so CanEdit is false and the Edit button is silently hidden, even though the caller is that group's own Owner. Verified this concretely with a scratch bUnit test reproducing it against the built code (not committed). This is a fail-closed bug (no security issue), but it's misleading — it reads as "you don't have permission" for something the caller just created — and it directly answers the trade-off question ADR-0067 raises: the three independent GET /api/groups loaders aren't merely a redundant-request cost, they're a real staleness bug in this PR's own new surface, not a hypothetical about a rename days later. Suggest a small, scoped fix rather than the larger shared-service reconciliation ADR-0067 defers — e.g. have Home re-run GroupMembershipLoader.LoadAllAsync when PlacesState.Changed fires and PlacesState.SelectedPlace.GroupId isn't in _groups.

ADR-0067's Consequences section should be amended to name this concretely once fixed — as written it frames the three loaders as only "not worth avoiding" network overhead, not as a case that breaks an affordance in the caller's own just-created group.

Everything else checks out:

• @key="Place?.Id" is on PlaceForm itself, not a wrapper — removing it reddens PlaceFormPanel_PlaceParameterSwappedOnAnAlreadyRenderedInstance_ShowsTheNewPlacesFieldsAndVersionNotTheOldOnes (confirmed by mutation). The manual "close one place's edit form, open a different one" scenario doesn't itself exercise the key — Home's IsEditingSelectedPlace always drops to view mode first, fully unmounting PlaceFormPanel, before a different place's edit can open — but ADR-0067 says this plainly ("the @key is the correctness guarantee, not the UX nicety that usually avoids the case"), so this isn't a gap, just confirming the reasoning holds.
• Add-place (general and per-group, pre-selecting the group via PlaceForm's existing EligibleGroups[0] default), add-group, and edit-place from the detail panel are all wired and covered end to end (DrawerAuthenticatedTests, PlaceAccordionTests, HomeTests).
• MapPlacesState.UpsertPlace correctly patches both the new-place and edited-place cases with no duplication or loss — read the implementation and the existing tests cover both directions.
• Viewer is correctly denied both add-place affordances and the Edit button; a stale role reaching the API as 403 is handled by PlaceForm's pre-existing ApiProblemException handling (task 94, untouched here).
• Focus-on-open and Escape-to-close are asserted, and I mutated the focus call away to confirm the assertion genuinely fails without it.
• Full build, dotnet test (315 passed), dotnet format --verify-no-changes, and PlaceMark.Architecture.Tests all clean under the pinned 10.0.100 SDK.
• ADR-0067's number doesn't collide with PR #74's 0066.

Not verifiable without a browser (as the PR and ADR-0067 already say): whether focus actually lands visibly, whether either panel clears the drawer on a narrow viewport, and what a screen reader announces on open.

Verdict: changes needed **`Home`'s own independently-loaded group/role cache never refreshes after a group created via `Drawer`'s `GroupFormPanel` — reachable in the exact "create a group → add a place → edit it" flow this PR wires up end to end, in one session.** `Drawer.HandleGroupSavedAsync` reloads `PlaceAccordion` and `GroupsSection` after a save, but has no reference to `Home` (siblings, per ADR-0062) — so `Home`'s own `_groups` (loaded once in `OnInitializedAsync` via `GroupMembershipLoader`) never learns about the new group. Concretely: caller creates a group, `PlaceAccordion`'s general "Add a place" button correctly appears (its own loader was reloaded), caller adds a place to the new group, it gets selected and `Home` shows the detail panel — but `RoleFor(newGroupId)` returns `null` against the stale `_groups`, so `CanEdit` is `false` and the Edit button is silently hidden, even though the caller is that group's own Owner. Verified this concretely with a scratch bUnit test reproducing it against the built code (not committed). This is a fail-closed bug (no security issue), but it's misleading — it reads as "you don't have permission" for something the caller just created — and it directly answers the trade-off question ADR-0067 raises: the three independent `GET /api/groups` loaders aren't merely a redundant-request cost, they're a real staleness bug in this PR's own new surface, not a hypothetical about a rename days later. Suggest a small, scoped fix rather than the larger shared-service reconciliation ADR-0067 defers — e.g. have `Home` re-run `GroupMembershipLoader.LoadAllAsync` when `PlacesState.Changed` fires and `PlacesState.SelectedPlace.GroupId` isn't in `_groups`. ADR-0067's Consequences section should be amended to name this concretely once fixed — as written it frames the three loaders as only "not worth avoiding" network overhead, not as a case that breaks an affordance in the caller's own just-created group. Everything else checks out: - `@key="Place?.Id"` is on `PlaceForm` itself, not a wrapper — removing it reddens `PlaceFormPanel_PlaceParameterSwappedOnAnAlreadyRenderedInstance_ShowsTheNewPlacesFieldsAndVersionNotTheOldOnes` (confirmed by mutation). The manual "close one place's edit form, open a different one" scenario doesn't itself exercise the key — `Home`'s `IsEditingSelectedPlace` always drops to view mode first, fully unmounting `PlaceFormPanel`, before a different place's edit can open — but ADR-0067 says this plainly ("the `@key` is the correctness guarantee, not the UX nicety that usually avoids the case"), so this isn't a gap, just confirming the reasoning holds. - Add-place (general and per-group, pre-selecting the group via `PlaceForm`'s existing `EligibleGroups[0]` default), add-group, and edit-place from the detail panel are all wired and covered end to end (`DrawerAuthenticatedTests`, `PlaceAccordionTests`, `HomeTests`). - `MapPlacesState.UpsertPlace` correctly patches both the new-place and edited-place cases with no duplication or loss — read the implementation and the existing tests cover both directions. - Viewer is correctly denied both add-place affordances and the Edit button; a stale role reaching the API as 403 is handled by `PlaceForm`'s pre-existing `ApiProblemException` handling (task 94, untouched here). - Focus-on-open and Escape-to-close are asserted, and I mutated the focus call away to confirm the assertion genuinely fails without it. - Full build, `dotnet test` (315 passed), `dotnet format --verify-no-changes`, and `PlaceMark.Architecture.Tests` all clean under the pinned 10.0.100 SDK. - ADR-0067's number doesn't collide with PR #74's 0066. Not verifiable without a browser (as the PR and ADR-0067 already say): whether focus actually lands visibly, whether either panel clears the drawer on a narrow viewport, and what a screen reader announces on open.

rob added 1 commit 2026-08-05 21:21:25 +00:00

Share caller-group state between Drawer and Home to fix Edit-button drift 1bf55376b3

rob reviewed 2026-08-05 21:26:40 +00:00

rob left a comment

Copy link

Verdict: mergeable

Re-reviewed at 1bf5537.

Drift bug fixed, and correctly. Reproduced the original defect against pre-fix Home.razor/Home.razor.cs (reverted them in a worktree): the Edit button stays hidden after creating a group and adding a place to it in one session. On 1bf5537 the same steps show the Edit button, and HomeAndDrawerSharedGroupStateTests.Home_AlreadyMounted_ShowsEditForAPlaceAddedToAGroupCreatedAfterwardsInTheDrawer reddens against the reverted Home and passes against the fix — confirmed both directions myself rather than trusting the description. CallerGroupsState is correctly scoped, and GroupFormPanel reloading it directly (rather than Drawer reaching into siblings) is the right shape — no component reaches into another's internals to propagate the change.

Cross-user reset: present and correct in the code, but untested — confirm before merge. CallerGroupsState mirrors MapPlacesState's AuthenticationStateChanged handling correctly; I wrote a scratch test (sign in as A, load, sign out, sign in as B) and it passes, so the behaviour itself is right. But there is no CallerGroupsStateTests.cs in this PR at all, unlike MapPlacesStateTests' own AuthenticationStateChanged_ADifferentCallerSignsInAfterSignOut_... test. I commented out the AuthenticationStateChanged subscription in CallerGroupsState's constructor and ran the full PlaceMark.WebUI.Tests suite (320 tests) — all still pass. Nothing in this PR would catch a regression here. Given this exact class of bug (one user's state leaking to the next sign-in) was a PR #70 finding, this needs a dedicated test before merge, not just structural resemblance to MapPlacesState.

GroupsSection residual: judged and named honestly, and currently inert. Checked whether any live UI path can move GroupsSection and CallerGroupsState out of step: GroupForm's rename affordance has no host anywhere in the WebUI, and PlaceMarkApiClient.DeleteGroupAsync has no caller in any component — neither rename nor delete is reachable from the UI at all yet. GroupMembersPanel's role controls never render against the caller's own row (ADR-0065), so an in-app role change can't touch the caller's own role either. The only mutation currently wired end-to-end is group creation, and that's the one case already paired correctly (GroupFormPanel reloads CallerGroupsState, Drawer.HandleGroupSavedAsync reloads GroupsSection, same handler). So: no, it cannot drift today. It will become live the moment someone wires up rename or delete without remembering to touch both reload call sites — exactly the shape that caused the original bug — and the rewritten ADR names that trigger condition explicitly rather than leaving it implicit. Agreed with the record's own judgement here.

ADR rewrite: correct and honest. ADR-0001 binds immutability "from the moment a record is merged onto main" — ADR-0067 has never been merged, so rewriting in place rather than superseding is the right call, consistent with the precedent ADR-0001 itself names for ADRs 0001–0011. The rewrite doesn't smooth the mistake into a design choice: it states plainly that the first version's trade-off was wrong, names the reproducing test, and keeps the rejected first version in Alternatives considered rather than deleting it.

Build, full dotnet test (316 passed, 4 pre-existing skips), and dotnet format --verify-no-changes all clean under the pinned 10.0.100 SDK. Nothing else outstanding from the first round.

Verdict: mergeable Re-reviewed at `1bf5537`. **Drift bug fixed, and correctly.** Reproduced the original defect against pre-fix `Home.razor`/`Home.razor.cs` (reverted them in a worktree): the Edit button stays hidden after creating a group and adding a place to it in one session. On `1bf5537` the same steps show the Edit button, and `HomeAndDrawerSharedGroupStateTests.Home_AlreadyMounted_ShowsEditForAPlaceAddedToAGroupCreatedAfterwardsInTheDrawer` reddens against the reverted `Home` and passes against the fix — confirmed both directions myself rather than trusting the description. `CallerGroupsState` is correctly scoped, and `GroupFormPanel` reloading it directly (rather than `Drawer` reaching into siblings) is the right shape — no component reaches into another's internals to propagate the change. **Cross-user reset: present and correct in the code, but untested — confirm before merge.** `CallerGroupsState` mirrors `MapPlacesState`'s `AuthenticationStateChanged` handling correctly; I wrote a scratch test (sign in as A, load, sign out, sign in as B) and it passes, so the behaviour itself is right. But there is no `CallerGroupsStateTests.cs` in this PR at all, unlike `MapPlacesStateTests`' own `AuthenticationStateChanged_ADifferentCallerSignsInAfterSignOut_...` test. I commented out the `AuthenticationStateChanged` subscription in `CallerGroupsState`'s constructor and ran the full `PlaceMark.WebUI.Tests` suite (320 tests) — **all still pass.** Nothing in this PR would catch a regression here. Given this exact class of bug (one user's state leaking to the next sign-in) was a PR #70 finding, this needs a dedicated test before merge, not just structural resemblance to `MapPlacesState`. **`GroupsSection` residual: judged and named honestly, and currently inert.** Checked whether any live UI path can move `GroupsSection` and `CallerGroupsState` out of step: `GroupForm`'s rename affordance has no host anywhere in the WebUI, and `PlaceMarkApiClient.DeleteGroupAsync` has no caller in any component — neither rename nor delete is reachable from the UI at all yet. `GroupMembersPanel`'s role controls never render against the caller's own row (ADR-0065), so an in-app role change can't touch the caller's own role either. The only mutation currently wired end-to-end is group creation, and that's the one case already paired correctly (`GroupFormPanel` reloads `CallerGroupsState`, `Drawer.HandleGroupSavedAsync` reloads `GroupsSection`, same handler). So: no, it cannot drift *today*. It will become live the moment someone wires up rename or delete without remembering to touch both reload call sites — exactly the shape that caused the original bug — and the rewritten ADR names that trigger condition explicitly rather than leaving it implicit. Agreed with the record's own judgement here. **ADR rewrite: correct and honest.** ADR-0001 binds immutability "from the moment a record is merged onto `main`" — ADR-0067 has never been merged, so rewriting in place rather than superseding is the right call, consistent with the precedent ADR-0001 itself names for ADRs 0001–0011. The rewrite doesn't smooth the mistake into a design choice: it states plainly that the first version's trade-off was wrong, names the reproducing test, and keeps the rejected first version in *Alternatives considered* rather than deleting it. Build, full `dotnet test` (316 passed, 4 pre-existing skips), and `dotnet format --verify-no-changes` all clean under the pinned 10.0.100 SDK. Nothing else outstanding from the first round.

rob reviewed 2026-08-05 21:27:13 +00:00

rob left a comment

Copy link

Verdict: changes needed

Correcting my previous review on this same commit (1bf5537), posted moments ago with verdict mergeable — that verdict contradicted my own finding in the same comment and should not stand. Everything in that review's substance holds; only the verdict was wrong.

The blocking issue: CallerGroupsState has no test coverage for the cross-user reset at all. I commented out its AuthenticationStateChanged subscription and ran the full PlaceMark.WebUI.Tests suite — all 320 tests (316 run, 4 pre-existing skips) still passed. Nothing in this PR would catch a regression that leaks one user's groups into the next sign-in on the same tab — the exact bug class PR #70's review found in MapPlacesState. The behaviour itself is correct (confirmed with a scratch test: sign in as A, load, sign out, sign in as B, none of A's groups remain), but "I checked it by hand and it's fine" is not a substitute for the regression test MapPlacesStateTests itself sets as the house standard (AuthenticationStateChanged_ADifferentCallerSignsInAfterSignOut_NeverExposesThePreviousCallersPlaces). Add the equivalent for CallerGroupsState before merge.

Everything else from the previous review stands:

• Reproduction genuinely fixed, confirmed both directions (reverted Home to redden HomeAndDrawerSharedGroupStateTests, restored to see it pass).
• GroupsSection residual is real but currently inert — no live UI path renames or deletes a group, or changes the caller's own role, so it cannot drift today; the rewritten ADR names the future trigger condition honestly.
• ADR-0067's in-place rewrite is correct per ADR-0001 (binds only once merged) and describes the defect plainly rather than smoothing it over.
• Build, full test run, and dotnet format --verify-no-changes all clean under the pinned SDK.

Once CallerGroupsStateTests (or equivalent) exists and asserts the cross-user reset, this is mergeable.

Verdict: changes needed Correcting my previous review on this same commit (`1bf5537`), posted moments ago with verdict `mergeable` — that verdict contradicted my own finding in the same comment and should not stand. Everything in that review's substance holds; only the verdict was wrong. **The blocking issue**: `CallerGroupsState` has no test coverage for the cross-user reset at all. I commented out its `AuthenticationStateChanged` subscription and ran the full `PlaceMark.WebUI.Tests` suite — all 320 tests (316 run, 4 pre-existing skips) still passed. Nothing in this PR would catch a regression that leaks one user's groups into the next sign-in on the same tab — the exact bug class PR #70's review found in `MapPlacesState`. The behaviour itself is correct (confirmed with a scratch test: sign in as A, load, sign out, sign in as B, none of A's groups remain), but "I checked it by hand and it's fine" is not a substitute for the regression test `MapPlacesStateTests` itself sets as the house standard (`AuthenticationStateChanged_ADifferentCallerSignsInAfterSignOut_NeverExposesThePreviousCallersPlaces`). Add the equivalent for `CallerGroupsState` before merge. Everything else from the previous review stands: - Reproduction genuinely fixed, confirmed both directions (reverted `Home` to redden `HomeAndDrawerSharedGroupStateTests`, restored to see it pass). - `GroupsSection` residual is real but currently inert — no live UI path renames or deletes a group, or changes the caller's own role, so it cannot drift today; the rewritten ADR names the future trigger condition honestly. - ADR-0067's in-place rewrite is correct per ADR-0001 (binds only once merged) and describes the defect plainly rather than smoothing it over. - Build, full test run, and `dotnet format --verify-no-changes` all clean under the pinned SDK. Once `CallerGroupsStateTests` (or equivalent) exists and asserts the cross-user reset, this is mergeable.

rob added 1 commit 2026-08-05 21:31:58 +00:00

Add CallerGroupsState cross-user reset tests bb3f332129

rob reviewed 2026-08-05 21:35:08 +00:00

rob left a comment

Copy link

Verdict: mergeable

Re-reviewed at bb3f332. Diff is exactly one new file, CallerGroupsStateTests.cs (174 lines), as expected.

Verified independently rather than trusting the description:

• Disabled CallerGroupsState's AuthenticationStateChanged subscription: exactly the two cross-user tests fail, both with a genuine leaked-group assertion (namesVisibleToB containing "Alice's Group", and state.Groups non-empty after sign-out) — not an incidental crash. Restored, git status clean.
• Did the same for MapPlacesState's subscription: its two equivalents fail the same way (namesVisibleToB containing "Alice's Place"). Restored, clean.
• Removed the same-caller early-return guard in AuthenticationStateChanged: exactly AuthenticationStateChanged_SameCallerNotified_KeepsWhatIsAlreadyLoaded fails ("state.Groups should not be empty but was"), nothing else. Restored, clean. So the same-caller no-op is genuinely pinned, not merely asserted.

The eight tests each pin something distinct — first load, no-op second EnsureLoadedAsync call, API-error handling, explicit ReloadAsync re-reading, ReloadAsync raising Changed, sign-out clearing, cross-user isolation, and same-caller no-op — no padding or restatement.

Full test suite (324 passed, 4 pre-existing skips), dotnet format --verify-no-changes, and PlaceMark.Architecture.Tests all clean under the pinned 10.0.100 SDK.

Nothing outstanding from either of the last two rounds. This is clean.

Verdict: mergeable Re-reviewed at `bb3f332`. Diff is exactly one new file, `CallerGroupsStateTests.cs` (174 lines), as expected. Verified independently rather than trusting the description: - Disabled `CallerGroupsState`'s `AuthenticationStateChanged` subscription: exactly the two cross-user tests fail, both with a genuine leaked-group assertion (`namesVisibleToB` containing `"Alice's Group"`, and `state.Groups` non-empty after sign-out) — not an incidental crash. Restored, `git status` clean. - Did the same for `MapPlacesState`'s subscription: its two equivalents fail the same way (`namesVisibleToB` containing `"Alice's Place"`). Restored, clean. - Removed the same-caller early-return guard in `AuthenticationStateChanged`: exactly `AuthenticationStateChanged_SameCallerNotified_KeepsWhatIsAlreadyLoaded` fails ("`state.Groups` should not be empty but was"), nothing else. Restored, clean. So the same-caller no-op is genuinely pinned, not merely asserted. The eight tests each pin something distinct — first load, no-op second `EnsureLoadedAsync` call, API-error handling, explicit `ReloadAsync` re-reading, `ReloadAsync` raising `Changed`, sign-out clearing, cross-user isolation, and same-caller no-op — no padding or restatement. Full test suite (324 passed, 4 pre-existing skips), `dotnet format --verify-no-changes`, and `PlaceMark.Architecture.Tests` all clean under the pinned 10.0.100 SDK. Nothing outstanding from either of the last two rounds. This is clean.

rob added 1 commit 2026-08-05 21:37:27 +00:00

Merge remote-tracking branch 'origin/main' into feat/entry-points ... 71aabd6ec9

# Conflicts:
#	docs/adr/README.md

rob merged commit dc9f83e1b8 into main 2026-08-05 21:40:38 +00:00

rob deleted branch feat/entry-points 2026-08-05 21:40:38 +00:00

rob referenced this pull request from a commit 2026-08-05 21:40:38 +00:00

Wire the place and group forms into the map UI (#75)

rob referenced this pull request 2026-08-20 06:44:47 +00:00

Rework the place detail panel's controls and fix its width #197

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

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!75

No description provided.