Build the map shell: drawer accordion and route protection #70

Merged

rob merged 3 commits from feat/map-home-page into main 2026-08-05 19:08:43 +00:00

Conversation 5 Commits 3 Files changed 33 +2194 -49

rob commented 2026-08-05 18:33:36 +00:00

Owner

Copy link

Implements task 88. Home is now the full-bleed map ([Authorize]-protected, since GET /api/places is caller-scoped) with a PlaceAccordion in the drawer grouping the caller's places by group. Expanding a group filters the map's markers to it; expanding none shows everything. Selecting a place pans/zooms the map and opens a floating PlaceDetailPanel — a Blazor panel rather than a Leaflet popup (ADR-0062).

State is shared between the drawer (sibling of @Body, per ADR-0059) and the map through a new scoped MapPlacesState, which also owns the ADR-0049 page-to-completion loop.

Accessibility: the burger and every accordion header are real <button>s with aria-expanded. Opening the drawer moves focus into it, closing returns focus to the toggle, Escape closes it — none of this traps Tab inside the drawer, which nothing in the brief asked for.

NavMenu is repurposed as the drawer's secondary links ("Your account", "Groups", "Settings") beneath a new graphical divider, since Home/Places are superseded by the brand link and the accordion respectively.

Next free ADR number checked against main and both sibling worktrees/branches (feat/group-form, feat/blazor-auth-state et al.) — 0062 is free everywhere.

Not verifiable here (no browser, task 156): rendered layout, actual panning/zoom, marker placement, and whether the detail panel visually clears the drawer on a narrow viewport. Marker-click-to-select and a Leaflet-popup alternative are deliberately out of scope — see ADR-0062's alternatives.

Implements task 88. Home is now the full-bleed map (`[Authorize]`-protected, since `GET /api/places` is caller-scoped) with a `PlaceAccordion` in the drawer grouping the caller's places by group. Expanding a group filters the map's markers to it; expanding none shows everything. Selecting a place pans/zooms the map and opens a floating `PlaceDetailPanel` — a Blazor panel rather than a Leaflet popup (ADR-0062). State is shared between the drawer (sibling of `@Body`, per ADR-0059) and the map through a new scoped `MapPlacesState`, which also owns the ADR-0049 page-to-completion loop. Accessibility: the burger and every accordion header are real `<button>`s with `aria-expanded`. Opening the drawer moves focus into it, closing returns focus to the toggle, Escape closes it — none of this traps Tab inside the drawer, which nothing in the brief asked for. `NavMenu` is repurposed as the drawer's secondary links ("Your account", "Groups", "Settings") beneath a new graphical divider, since Home/Places are superseded by the brand link and the accordion respectively. Next free ADR number checked against `main` and both sibling worktrees/branches (`feat/group-form`, `feat/blazor-auth-state` et al.) — 0062 is free everywhere. **Not verifiable here (no browser, task 156):** rendered layout, actual panning/zoom, marker placement, and whether the detail panel visually clears the drawer on a narrow viewport. Marker-click-to-select and a Leaflet-popup alternative are deliberately out of scope — see ADR-0062's alternatives.

rob added 1 commit 2026-08-05 18:33:36 +00:00

Build the map home page: drawer accordion and route protection 228db02654

rob reviewed 2026-08-05 18:40:37 +00:00

rob left a comment

Copy link

No description provided.

Verdict: changes needed Build and full `PlaceMark.WebUI.Tests`/`PlaceMark.Architecture.Tests` suites are clean on the pinned SDK (183 + 15 passing). Mutation-tested the two headline a11y claims directly: reverting `aria-expanded` to a literal `"false"` on the burger reddens `DrawerTests`, and changing the accordion header from `<button>` to `<div>` reddens 6/8 of `PlaceAccordionTests` — both are genuinely enforced, not just class-toggle theatre. `DrawerFocusTests` asserts against `Blazor._internal.domWrapper.focus` directly, which is a real (if bUnit-limited) check that `FocusAsync()` fires rather than a no-op. Route protection is solid and covered end-to-end in `AuthorizationTests`. **Stale `MapPlacesState` leaks between users on sign-out (real disclosure).** `MapPlacesState` is scoped, which is correct for WASM (one scope per app load, per ADR-0062's own reasoning), but nothing resets it. `Account.razor.cs.SignOutAsync` clears `ITokenStore` and notifies auth state but never touches `MapPlacesState`; `PlaceMarkAuthenticationStateProvider` has no reference to it. Sign-out and sign-in both navigate client-side (`Navigation.NavigateTo(...)`, no `forceLoad`), so the same `MapPlacesState` instance survives into the next session in the same tab. `EnsureLoadedAsync`'s `_isLoaded` guard then means a second user signing in after a first signs out sees the first user's places (names, descriptions, coordinates) rendered in the accordion and on the map until a full reload happens to occur. Needs a `Reset()`/equivalent called from `SignOutAsync` (or `SignInAsync`, to also cover switching accounts without an intervening sign-out). **Closed/covered drawer content stays in the tab order.** `.drawer-closed` is `transform: translateX(-100%)` only — no `hidden`, `inert`, or `visibility: hidden`. The accordion buttons and `NavMenu` links inside a closed drawer are still focusable; a keyboard user tabbing from the toggle on a narrow viewport lands on invisible off-canvas controls before reaching the map. The mirror case exists when the drawer is open on mobile: nothing traps or redirects focus, so tabbing past the last drawer link lands on `main` content sitting behind the backdrop. ADR-0062 explicitly and reasonably declines a *focus trap* ("nothing in the brief asks for it"), but that's a different thing from making the currently-hidden region non-tabbable, which no acceptance criterion trades off explicitly and which is exactly the shape of defect flagged as already shipped twice. Suggest `inert` (or a `tabindex="-1"` sweep) on whichever of drawer/main is currently off-screen or covered. **ADR-0049 pagination: implementation reads correctly, but the requested boundary tests aren't there.** Traced the loop by hand — it strictly grows `_places.Count` by `response.Items.Count` (>0) each iteration and stops the instant that count reaches `TotalCount`, or immediately on an empty page, so it can't spin forever even against a malformed/repeating page. But `MapPlacesStateTests` only exercises 1-page and 2×1-item cases; there's no test at exactly 50, 51, 100, 101, and no test proving the empty-page break actually fires. Given this is called out as the most-expected defect location, it's worth the explicit boundary tests even though the code held up under inspection. **No test renders `Home` itself.** The central behavioural claim — selecting a place pans/zooms the map and opens `PlaceDetailPanel` without navigating — is only proven piecemeal: `MapPlacesState` sets `SelectedPlace`, `PlaceAccordion` calls `SelectPlace`, and `Home.razor.cs`'s `Centre` computed property reads `SelectedPlace` correctly by inspection. Nothing renders `<Home>`, selects a place, and asserts `LeafletMap`'s `Centre` parameter changed, `PlaceDetailPanel` appeared, and `NavigationManager.Uri` is unchanged. `RoutingTests.App_TheRootRoute_RendersTheHomePage` only covers the empty-places case. **Marker-click exclusion (ADR-0062).** The ADR's reasoning is sound — no marker-to-Blazor channel exists yet in `LeafletMap`/`map.js`, and wiring one is a second interop surface with its own id-bookkeeping, not a small add. Given the accordion gives equivalent functionality today, this is defensible as scoped out rather than missed. Worth turning "left for whoever next needs it" into an actual backlog ticket now, though, since a pin that doesn't respond to a click is the most obvious thing a first-time user will try on this page. **Not verifiable without a browser (task 156):** actual layout/rendering, whether the detail panel visually clears the drawer on narrow viewports, real DOM focus (only the JS interop call is provable), and how the closed-drawer tab-order issue above actually reads to a keyboard user in practice — the code reading is enough to be confident it's a real defect, but not to size how bad it feels.

rob added 1 commit 2026-08-05 18:55:36 +00:00

Reset places on sign-out, inert the closed drawer, harden paging 3950da346a

rob commented 2026-08-05 18:55:49 +00:00

Author

Owner

Copy link

Addressed all four findings:

1. MapPlacesState now takes AuthenticationStateProvider and subscribes to AuthenticationStateChanged for its own lifetime, clearing every field on a caller change (including sign-out → anonymous). Test: AuthenticationStateChanged_ADifferentCallerSignsInAfterSignOut_NeverExposesThePreviousCallersPlaces.
2. Closed drawer now carries inert="@(!IsOpen)" alongside the transform. Tests in DrawerTests assert the attribute directly.
3. EnsureLoadedAsync now freezes TotalCount from page one (matching ADR-0049's wording literally) and has a MaxPagesToRead backstop against a page that never satisfies it. Boundary tests at 50/51/100/101, plus a repeating-page test and a never-satisfied-total test proving termination, in MapPlacesStateTests. Also fixed an adjacent bug the new cap surfaced: a retry after a partial failure was appending onto stale data rather than starting clean.
4. HomeTests renders Home directly and drives MapPlacesState.SelectPlace (the same state a click in the accordion mutates), asserting setView is called with the place's coordinates, createMap is not called again, and NavigationManager.Uri is unchanged.

ADR-0062 updated with the reasoning for each. Full CI sequence (build, dotnet format --verify-no-changes, tests) clean on the pinned SDK. Pushed as 3950da3.

Addressed all four findings: 1. `MapPlacesState` now takes `AuthenticationStateProvider` and subscribes to `AuthenticationStateChanged` for its own lifetime, clearing every field on a caller change (including sign-out → anonymous). Test: `AuthenticationStateChanged_ADifferentCallerSignsInAfterSignOut_NeverExposesThePreviousCallersPlaces`. 2. Closed drawer now carries `inert="@(!IsOpen)"` alongside the transform. Tests in `DrawerTests` assert the attribute directly. 3. `EnsureLoadedAsync` now freezes `TotalCount` from page one (matching ADR-0049's wording literally) and has a `MaxPagesToRead` backstop against a page that never satisfies it. Boundary tests at 50/51/100/101, plus a repeating-page test and a never-satisfied-total test proving termination, in `MapPlacesStateTests`. Also fixed an adjacent bug the new cap surfaced: a retry after a partial failure was appending onto stale data rather than starting clean. 4. `HomeTests` renders `Home` directly and drives `MapPlacesState.SelectPlace` (the same state a click in the accordion mutates), asserting `setView` is called with the place's coordinates, `createMap` is not called again, and `NavigationManager.Uri` is unchanged. ADR-0062 updated with the reasoning for each. Full CI sequence (build, `dotnet format --verify-no-changes`, tests) clean on the pinned SDK. Pushed as 3950da3.

rob reviewed 2026-08-05 19:01:51 +00:00

rob left a comment

Copy link

No description provided.

Verdict: mergeable Re-reviewed at `3950da3`. Build and `PlaceMark.WebUI.Tests`/`PlaceMark.Architecture.Tests` clean on the pinned SDK (201 + 15 passing, up from 183). Mutation-tested every fix: - **Cross-user leak.** Removing the `AuthenticationStateChanged` subscription reddens both `AuthenticationStateChanged_SigningOutAfterLoading_ClearsThePreviousCallersPlaces` and `...ADifferentCallerSignsInAfterSignOut_NeverExposesThePreviousCallersPlaces`. `Dispose()` unsubscribes correctly; since `MapPlacesState` is scoped in a WASM host with exactly one scope for the app's life, there's one instance and one subscription, so this is not a leak in practice — matches the precedent `ITokenStore`/`PlaceMarkAuthenticationStateProvider` already set. `AuthorizationTests.ConfigureServices` wires the same real `PlaceMarkAuthenticationStateProvider` instance both `AuthorizeRouteView` and `MapPlacesState` use, so this is exercised against production wiring, not just a fake. - **`inert`.** Confirmed applied via `inert="@(!IsOpen)"` and lifted on open. Setting it to a static `inert="false"` reddens `Drawer_InitialRenderOnAWideViewport_IsNotInert` and `Drawer_ToggleOpensTheDrawer_RemovesInert` — the tests assert `HasAttribute("inert")`, not the CSS class, so they're genuinely checking the right thing. The transform still matters for the open-drawer, mobile-backdrop case ADR-0062 doesn't claim to fix (no trap), so keeping both is correct, not redundant. - **ADR-0049 boundaries.** `CreatePagingStub` genuinely slices by `page`/`pageSize`, so 50/51/100/101 discriminate real off-by-ones, not just canned responses. The repeating-page test uses a finite `TotalCount` satisfied by accumulation, proving termination is independent of the backstop. Setting `MaxPagesToRead = int.MaxValue` hangs `EnsureLoadedAsync_TotalCountNeverSatisfied_StopsAtTheHardPageCapRatherThanLoopingForever` (killed after 20s) — the backstop test is real, not vacuous. By inspection the `_places.Count >= totalCount` success check runs before the `page >= MaxPagesToRead` check, so a legitimate result needing exactly 500 pages still completes; the cap doesn't truncate one page early. No test exercises that exact edge (would need ~25,000 fake places), but the logic is sound and this is a minor gap, not a defect. - **Frozen `TotalCount` and stale-append fix.** `totalCount ??= response.TotalCount` matches ADR-0049's "read `TotalCount`" (singular, page one) rather than re-reading it every response — correct, not a paper-over. **However**: removing the new `_places.Clear()` at the top of `EnsureLoadedAsync` — the fix for the stale-append bug the author found while writing the boundary tests — does not redden any test; the full suite still passes at 201/201 with it deleted. The fix is real and correct, but it currently has no regression test, which is worth closing given this is the exact kind of bug this PR's own history shows slips through without one: a test like "first attempt fails after accumulating one page, second attempt's result contains no duplicates" (via a stub that fails page 2 once, or hits the `MaxPagesToRead` backstop, then succeeds on retry). - **`HomeTests`.** `Home_PlaceSelected_PansAndZoomsTheMapToItWithoutNavigatingAway` correctly asserts `setView` with the place's coordinates and zoom 16, `createMap` invoked exactly once (not recreated), and `NavigationManager.Uri` unchanged — this is exactly what was asked for and it's the right assertion shape. One non-blocking gap (missing regression test for the stale-append fix) — doesn't block merge given the fix itself is correct and verified by hand, but worth a follow-up commit or a note before this is forgotten.

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

Pin the partial-retry paging fix with a regression test c8bb65bb04

rob commented 2026-08-05 19:05:33 +00:00

Author

Owner

Copy link

Added the missing regression test: EnsureLoadedAsync_RetriedAfterAPartialFailure_DoesNotDoubleCountThePagesReadBeforeTheFailure (MapPlacesStateTests).

Proved it discriminates by removing _places.Clear(), running the test, and restoring it. Worth noting what actually happened: without the fix the loop doesn't just duplicate a place, it stops one page early — the stale duplicate from the failed attempt satisfies the total-reached check before the retry ever requests the second page, so a real place goes silently missing. Recorded that in ADR-0062, since it's a sharper failure mode than "double-counting" suggests.

Full CI sequence clean on the pinned SDK. Pushed as c8bb65b.

Added the missing regression test: `EnsureLoadedAsync_RetriedAfterAPartialFailure_DoesNotDoubleCountThePagesReadBeforeTheFailure` (`MapPlacesStateTests`). Proved it discriminates by removing `_places.Clear()`, running the test, and restoring it. Worth noting what actually happened: without the fix the loop doesn't just duplicate a place, it stops one page early — the stale duplicate from the failed attempt satisfies the total-reached check before the retry ever requests the second page, so a real place goes silently missing. Recorded that in ADR-0062, since it's a sharper failure mode than "double-counting" suggests. Full CI sequence clean on the pinned SDK. Pushed as c8bb65b.

rob reviewed 2026-08-05 19:08:16 +00:00

rob left a comment

Copy link

No description provided.

Verdict: mergeable Confirmed by removing `_places.Clear()` and inspecting the actual result directly (not just the test's pass/fail): with the fix reverted, `state.Groups` ends up with `["Alpha", "Alpha"]` — "Beta" is never fetched at all, because the retry's first page re-adds the stale "Alpha", satisfies `_places.Count >= totalCount` (2 >= 2) on that duplicate alone, and the loop breaks before ever requesting page 2. The author's account is right and sharper than either of ours: the observable failure is a silently missing place, not a cosmetic duplicate — there is a literal duplicate in the intermediate list, but the thing a user would actually notice is the second place never arriving, with `LoadError` clear and nothing indicating a problem. ADR-0062's new note describes this precisely and matches what was observed. Test genuinely discriminates (reddens on the revert, passes with the fix) and re-ran it five times with no flakiness — `callCount` is a closure-local counter driving strictly sequential awaited calls, no parallelism or timing dependency to worry about. Diff is exactly the three files expected: the new test, the `MapPlacesState.cs` comment (no logic change), and the ADR addendum. Full suite clean at 202 (`PlaceMark.WebUI.Tests`, up from 201) and 15 (`PlaceMark.Architecture.Tests`) on the pinned SDK. Nothing else to act on.

rob merged commit 7dd1e09dc8 into main 2026-08-05 19:08:43 +00:00

rob deleted branch feat/map-home-page 2026-08-05 19:08:43 +00:00

rob referenced this pull request from a commit 2026-08-05 19:08:44 +00:00

Make the home page a map with a place drawer (#70)

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

No description provided.