Show the reader's current location on the map and in the add-place form #198

rob · 2026-08-20T06:47:31Z

rob commented

2026-08-20 06:47:31 +00:00

Task 254 (#78). ADR-0167.

One shared CurrentLocationButton is the only thing that ever asks for the Geolocation permission, and only on a press — Home floats it top-right of the map ("Show my location", plots a non-interactive dot outside the cluster layer and takes the map there via the existing MapZoomRequest), PlaceForm puts it beside the coordinate fields ("Use current location", routes through HandlePick so a located coordinate is indistinguishable from a typed one downstream).

geolocation.js feature-detects navigator.geolocation and never rejects; window.isSecureContext distinguishes insecure from unsupported, the ADR-0142 posture applied to a second secure-context API.

30 new tests; WebUI suite 885 passing, 4 skipped. Every new test was watched failing against a deliberate break of the behaviour it guards — the press-only import, the coordinate-completeness guard, each distinct failure message, the message reset, the JSDisconnectedException catch, role="alert", the redundant-call guard, the first-render plot, Home's CurrentLocation/zoom/inert wiring, and the form's recentre and full-precision handling.

Two judgements a reviewer may want to overturn: pressing "Show my location" moves the map to it at zoom 16, and a refused permission shows "Location permission was refused. Allow it in your site settings, then try again." rather than anything more specific about which browser.

Task 254 (#78). ADR-0167. One shared `CurrentLocationButton` is the only thing that ever asks for the Geolocation permission, and only on a press — `Home` floats it top-right of the map ("Show my location", plots a non-interactive dot outside the cluster layer and takes the map there via the existing `MapZoomRequest`), `PlaceForm` puts it beside the coordinate fields ("Use current location", routes through `HandlePick` so a located coordinate is indistinguishable from a typed one downstream). `geolocation.js` feature-detects `navigator.geolocation` and never rejects; `window.isSecureContext` distinguishes `insecure` from `unsupported`, the ADR-0142 posture applied to a second secure-context API. 30 new tests; WebUI suite 885 passing, 4 skipped. Every new test was watched failing against a deliberate break of the behaviour it guards — the press-only import, the coordinate-completeness guard, each distinct failure message, the message reset, the `JSDisconnectedException` catch, `role="alert"`, the redundant-call guard, the first-render plot, Home's `CurrentLocation`/zoom/inert wiring, and the form's recentre and full-precision handling. Two judgements a reviewer may want to overturn: pressing "Show my location" moves the map to it at zoom 16, and a refused permission shows "Location permission was refused. Allow it in your site settings, then try again." rather than anything more specific about which browser.

rob added 1 commit

2026-08-20 06:47:31 +00:00

Show the reader's current location on the map and in the add-place form

CI / build (pull_request) Successful in 5m7s

Details

CI / container-images (pull_request) Successful in 7s

Details

CI / e2e (pull_request) Successful in 8m31s

Details

d8e69b42ea

rob reviewed

2026-08-20 06:56:11 +00:00

rob left a comment

Verdict: changes needed

Two blockers and three smaller items; inline.

MapZoomRequest does not survive alongside PR #197 as both branches stand. #197 deletes the type, LeafletMap.ZoomRequest, ExplicitZoomLevel, _renderedZoomRequestId, Home._zoomRequest and HandleZoomToPlace; this PR gives all of them a new caller. Whichever merges second fails to compile. See the inline note on Home.razor.cs for the resolution and for what has to change in #197 and ADR-0166.
The secure-context detect is wrong for this particular API, and wrong precisely on the deployment it was written for — inline on geolocation.js. navigator.geolocation is not [SecureContext]-gated the way navigator.share and navigator.clipboard are.
Zoom 16 on press zooms a close-in reader out, and the jump is then persisted over their saved viewport — inline on Home.razor.cs.
The inert test does not discriminate — inline on HomeTests.cs.
Coverage as reported (90.86%) is below the PlaceMark.WebUI floor of 91.5 in coverage-baseline.json, so the ratchet step fails on that number. Reconcile against CI's own measurement for run #808 before merging rather than a locally measured figure.
CurrentLocationButton_ModuleUnreachable_ShowsTheGenericMessageRatherThanThrowing throws from currentPositionAsync, not from the import; its comment claims the module failing to load is covered, and it is not. Cover it or narrow the comment.

Verdict: changes needed Two blockers and three smaller items; inline. 1. **`MapZoomRequest` does not survive alongside PR #197 as both branches stand.** #197 deletes the type, `LeafletMap.ZoomRequest`, `ExplicitZoomLevel`, `_renderedZoomRequestId`, `Home._zoomRequest` and `HandleZoomToPlace`; this PR gives all of them a new caller. Whichever merges second fails to compile. See the inline note on `Home.razor.cs` for the resolution and for what has to change in #197 and ADR-0166. 2. **The secure-context detect is wrong for this particular API**, and wrong precisely on the deployment it was written for — inline on `geolocation.js`. `navigator.geolocation` is not `[SecureContext]`-gated the way `navigator.share` and `navigator.clipboard` are. 3. Zoom 16 on press zooms a close-in reader *out*, and the jump is then persisted over their saved viewport — inline on `Home.razor.cs`. 4. The `inert` test does not discriminate — inline on `HomeTests.cs`. 5. Coverage as reported (90.86%) is below the `PlaceMark.WebUI` floor of 91.5 in `coverage-baseline.json`, so the ratchet step fails on that number. Reconcile against CI's own measurement for run #808 before merging rather than a locally measured figure. 6. `CurrentLocationButton_ModuleUnreachable_ShowsTheGenericMessageRatherThanThrowing` throws from `currentPositionAsync`, not from the `import`; its comment claims the module failing to load is covered, and it is not. Cover it or narrow the comment.

src/PlaceMark.WebUI/Pages/Home.razor.cs Outdated

					
				@ -234,0 +246,4 @@

					/// and the same exception ADR-0101 already carves out for a reader who asked directly. Plotting the

					/// dot without moving the view would answer "where am I" with a marker somewhere off-screen.

					/// </summary>

					private void HandleCurrentLocation(MapCoordinates coordinates)

MapZoomRequest moves to a fixed ExplicitZoomLevel of 16, so a reader who had zoomed to 17-19 is zoomed out by "Show my location" — and HandleViewportChanged then persists the new centre and zoom through ViewportStore, so the view they had is gone for good. There is no way back.

Centre on the dot at the map's current zoom, or at max(current, 16) if you want a floor for a reader who was looking at the whole world. That needs the zoom to become part of the request (or null meaning "keep what the map has") rather than a shared constant, since ADR-0132's 16 was chosen for reading a single place, not for answering "where am I".

`MapZoomRequest` moves to a fixed `ExplicitZoomLevel` of 16, so a reader who had zoomed to 17-19 is zoomed *out* by "Show my location" — and `HandleViewportChanged` then persists the new centre and zoom through `ViewportStore`, so the view they had is gone for good. There is no way back. Centre on the dot at the map's current zoom, or at `max(current, 16)` if you want a floor for a reader who was looking at the whole world. That needs the zoom to become part of the request (or `null` meaning "keep what the map has") rather than a shared constant, since ADR-0132's 16 was chosen for reading a single place, not for answering "where am I".

src/PlaceMark.WebUI/Pages/Home.razor.cs Outdated

					
				@ -234,0 +249,4 @@

					private void HandleCurrentLocation(MapCoordinates coordinates)

					{

						_currentLocation = coordinates;

						_zoomRequest = new MapZoomRequest(coordinates, Guid.NewGuid());

Cross-branch break. PR #197 (place-detail-panel-controls, task 251, ADR-0166) deletes MapZoomRequest.cs, LeafletMap.ZoomRequest, ExplicitZoomLevel, _renderedZoomRequestId and Home._zoomRequest/HandleZoomToPlace. There is no textual conflict worth trusting here, but whichever branch merges second stops compiling.

Resolution: the type survives — it is no longer dead, because this is a second legitimate consumer of "centre and zoom here, again if asked again". #197 should be reduced to removing the button, PlaceDetailPanel.OnZoomToPlace and Home.HandleZoomToPlace, keeping the mechanism; ADR-0166 is overclaiming where it says the chain went wholly, and MapZoomRequest's and LeafletMap.ZoomRequest's doc comments both need re-pointing from "PlaceDetailPanel's own zoom button" to task 254. Then rebase whichever branch merges second onto the other and re-run — a green run on a base predating the other PR proves nothing about this.

If you would rather drop MapZoomRequest from this PR instead, whatever replaces it still needs a per-press identity: CurrentLocation on its own is deduped by value in OnParametersSetAsync, so a second press after the reader has panned away would reach map.js with nothing and appear to do nothing.

Cross-branch break. PR #197 (`place-detail-panel-controls`, task 251, ADR-0166) deletes `MapZoomRequest.cs`, `LeafletMap.ZoomRequest`, `ExplicitZoomLevel`, `_renderedZoomRequestId` and `Home._zoomRequest`/`HandleZoomToPlace`. There is no textual conflict worth trusting here, but whichever branch merges second stops compiling. Resolution: **the type survives** — it is no longer dead, because this is a second legitimate consumer of "centre and zoom here, again if asked again". #197 should be reduced to removing the button, `PlaceDetailPanel.OnZoomToPlace` and `Home.HandleZoomToPlace`, keeping the mechanism; ADR-0166 is overclaiming where it says the chain went wholly, and `MapZoomRequest`'s and `LeafletMap.ZoomRequest`'s doc comments both need re-pointing from "PlaceDetailPanel's own zoom button" to task 254. Then rebase whichever branch merges second onto the other and re-run — a green run on a base predating the other PR proves nothing about this. If you would rather drop `MapZoomRequest` from this PR instead, whatever replaces it still needs a per-press identity: `CurrentLocation` on its own is deduped by value in `OnParametersSetAsync`, so a second press after the reader has panned away would reach `map.js` with nothing and appear to do nothing.

src/PlaceMark.WebUI/wwwroot/js/geolocation.js Outdated

					
				@ -0,0 +46,4 @@

					// Optional chaining on `navigator.geolocation` itself, not merely a `typeof navigator.geolocation`

					// check: the property is absent entirely in an insecure context, and reading `.getCurrentPosition`

					// off it directly would throw rather than degrade.

					if (typeof navigator.geolocation?.getCurrentPosition !== "function") {

navigator.geolocation is not removed on insecure origins. Unlike navigator.share and navigator.clipboard, it is not [SecureContext] in its IDL — Chrome and Firefox keep the property and fail the call with PERMISSION_DENIED (Chrome additionally logs its own deprecation warning). So on http://192.168.1.2:5017 this detect passes, getCurrentPosition runs, and the reader is told "Location permission was refused. Allow it in your site settings, then try again." — a false claim they will act on and find nothing, which is the exact failure ADR-0142 exists to prevent.

Test window.isSecureContext first and return insecure before touching the API at all; keep the existing check afterwards for the genuinely-unsupported case. That also avoids Chrome's own console warning, since the call never happens.

ADR-0167's "it is not merely restricted over plain HTTP — it is absent. navigator.geolocation is undefined, not a callable that fails" needs correcting with it, as does the comment block at the top of this file.

`navigator.geolocation` is not removed on insecure origins. Unlike `navigator.share` and `navigator.clipboard`, it is not `[SecureContext]` in its IDL — Chrome and Firefox keep the property and fail the *call* with `PERMISSION_DENIED` (Chrome additionally logs its own deprecation warning). So on `http://192.168.1.2:5017` this detect passes, `getCurrentPosition` runs, and the reader is told "Location permission was refused. Allow it in your site settings, then try again." — a false claim they will act on and find nothing, which is the exact failure ADR-0142 exists to prevent. Test `window.isSecureContext` **first** and return `insecure` before touching the API at all; keep the existing check afterwards for the genuinely-unsupported case. That also avoids Chrome's own console warning, since the call never happens. ADR-0167's "it is not merely restricted over plain HTTP — it is absent. `navigator.geolocation` is `undefined`, not a callable that fails" needs correcting with it, as does the comment block at the top of this file.

tests/PlaceMark.WebUI.Tests/Pages/HomeTests.cs Outdated

					
				@ -1076,0 +1133,4 @@

					/// panel that has just made everything behind it background content (ADR-0086).

					/// </summary>

					[Fact]

					public void Home_ModalTaskPanelOpen_MarksTheCurrentLocationControlAsBackgroundContent()

This passes unchanged if the binding were hardcoded inert="true" — it only ever observes the open case, so it does not discriminate the thing it guards. Add the closed case asserting the attribute is absent while no modal-task panel is open; without it, a control permanently unreachable to the keyboard would ship green.

This passes unchanged if the binding were hardcoded `inert="true"` — it only ever observes the open case, so it does not discriminate the thing it guards. Add the closed case asserting the attribute is absent while no modal-task panel is open; without it, a control permanently unreachable to the keyboard would ship green.

rob force-pushed current-location from d8e69b42ea

CI / build (pull_request) Successful in 5m7s

Details

CI / container-images (pull_request) Successful in 7s

Details

CI / e2e (pull_request) Successful in 8m31s

Details

to 3938a5f29e

CI / build (pull_request) Successful in 5m1s

Details

CI / container-images (pull_request) Successful in 26s

Details

CI / e2e (pull_request) Has been cancelled

Details

2026-08-20 07:11:17 +00:00

Compare

rob commented

2026-08-20 07:11:38 +00:00

Actioned in 3938a5f, rebased onto b3e1239.

geolocation.js reads window.isSecureContext first and returns insecure before the API is touched; the feature detect follows and now only answers "this browser has none". Compared against false explicitly, so a browser too old to implement the property is not refused. ADR-0167's Context, Decision and the file's own comment block are rewritten — the false claim that the property is absent is replaced with why it is not, and why that makes the ordering load-bearing. New GeolocationSecureContextFirstTests holds the ordering from source text (best-effort, states what it cannot see); watched red against both the swap and the deletion.
MapZoomRequest kept. Its doc comment and LeafletMap.ZoomRequest's now name the mechanism and both consumers rather than the button.
MapZoomRequest gains int? MinimumZoom: null keeps ADR-0132's fixed level exactly, a value routes to a new map.js centreAtLeast, which is Math.max(map.getZoom(), minimumZoom). Home names its own floor of 16 with its own reasoning rather than sharing ExplicitZoomLevel. Centring at the current zoom with no floor was rejected: the whole-world default (zoom 2) would put the dot in an ocean. The Math.max itself is not provable from this stack — named in the ADR alongside the other JS gaps.
Closed case added for inert; red with inert="true" hardcoded. The interop-failure theory is now 4 cases — both exception types against both the import and the call — with the import failing through a substituted IJSRuntime, since bUnit refuses Setup<IJSObjectReference> and SetupModule cannot be made to fail. Dropping the JSException catch reddens the import case, so the load path is genuinely covered rather than claimed.
My baseline reading was taken in Debug; CI builds and tests in Release. Debug emits ~950 extra coverable lines in this assembly (3737 vs 2786), which is the whole discrepancy. Re-measured in Release: main 91.565%, this branch 91.6433%, floor 91.5. Floor unchanged.

Full suite on the rebased branch: WebUI 895/4 skipped, Architecture 135, Domain 39, Infrastructure 360, Api 622.

Actioned in `3938a5f`, rebased onto `b3e1239`. 1. `geolocation.js` reads `window.isSecureContext` first and returns `insecure` before the API is touched; the feature detect follows and now only answers "this browser has none". Compared against `false` explicitly, so a browser too old to implement the property is not refused. ADR-0167's Context, Decision and the file's own comment block are rewritten — the false claim that the property is absent is replaced with why it is not, and why that makes the ordering load-bearing. New `GeolocationSecureContextFirstTests` holds the ordering from source text (best-effort, states what it cannot see); watched red against both the swap and the deletion. 2. `MapZoomRequest` kept. Its doc comment and `LeafletMap.ZoomRequest`'s now name the mechanism and both consumers rather than the button. 3. `MapZoomRequest` gains `int? MinimumZoom`: `null` keeps ADR-0132's fixed level exactly, a value routes to a new `map.js` `centreAtLeast`, which is `Math.max(map.getZoom(), minimumZoom)`. `Home` names its own floor of 16 with its own reasoning rather than sharing `ExplicitZoomLevel`. Centring at the current zoom with no floor was rejected: the whole-world default (zoom 2) would put the dot in an ocean. The `Math.max` itself is not provable from this stack — named in the ADR alongside the other JS gaps. 4. Closed case added for `inert`; red with `inert="true"` hardcoded. The interop-failure theory is now 4 cases — both exception types against both the `import` and the call — with the import failing through a substituted `IJSRuntime`, since bUnit refuses `Setup<IJSObjectReference>` and `SetupModule` cannot be made to fail. Dropping the `JSException` catch reddens the import case, so the load path is genuinely covered rather than claimed. 5. My baseline reading was taken in **Debug**; CI builds and tests in **Release**. Debug emits ~950 extra coverable lines in this assembly (3737 vs 2786), which is the whole discrepancy. Re-measured in Release: `main` 91.565%, this branch **91.6433%**, floor 91.5. Floor unchanged. Full suite on the rebased branch: WebUI 895/4 skipped, Architecture 135, Domain 39, Infrastructure 360, Api 622.

rob reviewed

2026-08-20 07:17:19 +00:00

rob left a comment

Verdict: changes needed

Nothing in the runtime code. All five findings from the previous round are correctly actioned and I checked each rather than taking the account: the secure check is the first statement of the only entry point with no module-level access ahead of it; === false is the right test and undefined falls through as documented; centreAtLeast's Math.max and setView(center, zoom) argument order are right and MinimumZoom = null emits the identical setView call ADR-0132's consumer had; ThrowingJSRuntime does fail the import itself; the closed inert case is there. Rebase onto b3e1239 is clean and the two commit subjects are fine.

Two things left.

ADR-0167 asserts something false, and leans on it. "This project has no JavaScript test runner" — tests/PlaceMark.E2E.Tests is a 46-test Playwright suite that runs in the e2e job of .forgejo/workflows/ci.yml on every pull request, and DoubleTapDragZoomJourneyTests already reads el.placeMarkMap.getZoom() out of a real Chromium. So centreAtLeast's Math.max and the dot's rendering are not unverifiable; they are unverified. Inline for what to do.
The tripwire has a cheap evasion, and is already comparing against a comment. Inline on GeolocationSecureContextFirstTests.

Also: run #812 has not finished. The Release/Debug coverage account is consistent with the only independent figure on record (#194's own CI reading of 91.5 for PlaceMark.WebUI), but I cannot measure it here — this machine has SDK 10.0.111 and global.json pins 10.0.100, so dotnet refuses to run at all. Gate the merge on #812's own ratchet step, not on the reported number.

Verdict: changes needed Nothing in the runtime code. All five findings from the previous round are correctly actioned and I checked each rather than taking the account: the secure check is the first statement of the only entry point with no module-level access ahead of it; `=== false` is the right test and `undefined` falls through as documented; `centreAtLeast`'s `Math.max` and `setView(center, zoom)` argument order are right and `MinimumZoom = null` emits the identical `setView` call ADR-0132's consumer had; `ThrowingJSRuntime` does fail the `import` itself; the closed `inert` case is there. Rebase onto `b3e1239` is clean and the two commit subjects are fine. Two things left. 1. **ADR-0167 asserts something false, and leans on it.** "This project has no JavaScript test runner" — `tests/PlaceMark.E2E.Tests` is a 46-test Playwright suite that runs in the `e2e` job of `.forgejo/workflows/ci.yml` on every pull request, and `DoubleTapDragZoomJourneyTests` already reads `el.placeMarkMap.getZoom()` out of a real Chromium. So `centreAtLeast`'s `Math.max` and the dot's rendering are not unverifiable; they are unverified. Inline for what to do. 2. **The tripwire has a cheap evasion, and is already comparing against a comment.** Inline on `GeolocationSecureContextFirstTests`. Also: run #812 has not finished. The Release/Debug coverage account is consistent with the only independent figure on record (#194's own CI reading of 91.5 for `PlaceMark.WebUI`), but I cannot measure it here — this machine has SDK 10.0.111 and `global.json` pins 10.0.100, so `dotnet` refuses to run at all. Gate the merge on #812's own ratchet step, not on the reported number.

docs/adr/0167-feature-detect-geolocation-and-ask-only-on-a-gesture.md Outdated

					
				@ -0,0 +193,4 @@

				**What was verified, and what was not — the same limit ADR-0142 already recorded, and one new guard

				against the part of it that bit.** bUnit stubs the interop boundary, so every test here proves this

				application's own handling of each outcome `geolocation.js` can report; none of them runs

				`geolocation.js` against a real `navigator`, secure or insecure. This project has no JavaScript test

This is not true, and it is doing real work in this paragraph — it is the reason centreAtLeast's Math.max and "the dot's actual appearance on a real basemap" are listed as reasoned-about rather than tested.

tests/PlaceMark.E2E.Tests is 46 [E2EFact] tests driving real Chromium through Playwright 1.61, in the e2e job of .forgejo/workflows/ci.yml, on every pull request (ADR-0091, ADR-0092). Several exist precisely to execute this application's JavaScript in a browser — LeafletControlThemingTests, ZoomControlPositionTests, ZIndexComputedStackingJourneyTests, MobileTileAndMarkerRetentionJourneyTests, RememberViewportJourneyTests — and DoubleTapDragZoomJourneyTests already reads the live Leaflet zoom with el.placeMarkMap.getZoom(), which is exactly the assertion centreAtLeast needs. Playwright grants the geolocation permission and pins a position through BrowserNewContextOptions, and PlaceMarkAppFixture already parameterises NewContextAsync three ways.

What is true, and is the honest version of this paragraph: the fixture serves the WebUI at http://localhost:5169, and localhost is a potentially trustworthy origin, so window.isSecureContext is true there and the insecure branch cannot be reached in E2E at any price. That is a real argument for GeolocationSecureContextFirstTests existing — a much better one than "no JavaScript test runner", which the same suite disproves.

So: correct the sentence, and either add the E2E case (zoom to 18, press "Show my location", assert getZoom() is still 18 and the dot is on the map) or record declining it as scope. Do not leave a false claim standing as the justification. The same sentence appears in GeolocationSecureContextFirstTests' own remarks.

This is not true, and it is doing real work in this paragraph — it is the reason `centreAtLeast`'s `Math.max` and "the dot's actual appearance on a real basemap" are listed as reasoned-about rather than tested. `tests/PlaceMark.E2E.Tests` is 46 `[E2EFact]` tests driving real Chromium through Playwright 1.61, in the `e2e` job of `.forgejo/workflows/ci.yml`, on every pull request (ADR-0091, ADR-0092). Several exist precisely to execute this application's JavaScript in a browser — `LeafletControlThemingTests`, `ZoomControlPositionTests`, `ZIndexComputedStackingJourneyTests`, `MobileTileAndMarkerRetentionJourneyTests`, `RememberViewportJourneyTests` — and `DoubleTapDragZoomJourneyTests` already reads the live Leaflet zoom with `el.placeMarkMap.getZoom()`, which is exactly the assertion `centreAtLeast` needs. Playwright grants the geolocation permission and pins a position through `BrowserNewContextOptions`, and `PlaceMarkAppFixture` already parameterises `NewContextAsync` three ways. What *is* true, and is the honest version of this paragraph: the fixture serves the WebUI at `http://localhost:5169`, and localhost is a potentially trustworthy origin, so `window.isSecureContext` is `true` there and the insecure branch cannot be reached in E2E at any price. That is a real argument for `GeolocationSecureContextFirstTests` existing — a much better one than "no JavaScript test runner", which the same suite disproves. So: correct the sentence, and either add the E2E case (zoom to 18, press "Show my location", assert `getZoom()` is still 18 and the dot is on the map) or record declining it as scope. Do not leave a false claim standing as the justification. The same sentence appears in `GeolocationSecureContextFirstTests`' own remarks.

tests/PlaceMark.Architecture.Tests/GeolocationSecureContextFirstTests.cs Outdated

					
				@ -0,0 +38,4 @@

						var body = source[source.IndexOf("export function currentPositionAsync", StringComparison.Ordinal)..];

						var secureContextCheck = body.IndexOf("window.isSecureContext", StringComparison.Ordinal);

						var geolocationUse = body.IndexOf("navigator.geolocation", StringComparison.Ordinal);

geolocationUse matches the comment on the line above the feature detect, not the code — the first navigator.geolocation in the sliced body is inside // Optional chaining on \navigator.geolocation` itself`. It passes today only because the comment happens to sit after the secure check.

That is also the evasion: move the secure check below the feature detect but leave any comment above it containing the literal window.isSecureContext — an entirely natural thing for someone rewriting this to write — and secureContextCheck matches the prose while geolocationUse matches code, and the test goes green on the exact defect it exists to catch. Nothing about that edit looks like an evasion to its author, which is what makes it the one worth closing.

Strip line comments from body before searching, or anchor on the code forms (if (window.isSecureContext === false) and navigator.geolocation?.getCurrentPosition). Either keeps the check on code positions only. The remarks are right that control flow is out of reach; this one is not.

`geolocationUse` matches the *comment* on the line above the feature detect, not the code — the first `navigator.geolocation` in the sliced body is inside `// Optional chaining on \`navigator.geolocation\` itself`. It passes today only because the comment happens to sit after the secure check. That is also the evasion: move the secure check below the feature detect but leave any comment above it containing the literal `window.isSecureContext` — an entirely natural thing for someone rewriting this to write — and `secureContextCheck` matches the prose while `geolocationUse` matches code, and the test goes green on the exact defect it exists to catch. Nothing about that edit looks like an evasion to its author, which is what makes it the one worth closing. Strip line comments from `body` before searching, or anchor on the code forms (`if (window.isSecureContext === false)` and `navigator.geolocation?.getCurrentPosition`). Either keeps the check on code positions only. The remarks are right that control flow is out of reach; this one is not.

rob referenced this pull request

2026-08-20 07:19:05 +00:00

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

rob referenced this pull request

2026-08-20 07:19:18 +00:00

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

rob added 1 commit

2026-08-20 07:26:58 +00:00

Prove the current-location zoom floor and dot in a real browser

CI / e2e (pull_request) Has been cancelled

Details

CI / container-images (pull_request) Has been cancelled

Details

CI / build (pull_request) Has been cancelled

Details

a739c82999

rob force-pushed current-location from a739c82999

CI / e2e (pull_request) Has been cancelled

Details

CI / container-images (pull_request) Has been cancelled

Details

CI / build (pull_request) Has been cancelled

Details

to 4ae93241cb

CI / build (pull_request) Successful in 7m54s

Details

CI / container-images (pull_request) Successful in 40s

Details

CI / e2e (pull_request) Successful in 5m33s

Details

2026-08-20 07:28:53 +00:00

Compare

rob commented

2026-08-20 07:29:45 +00:00

Actioned in 4ae9324.

1. "No JavaScript test runner" — false, corrected, and the E2E case added rather than declined.

ADR-0167's "What was verified" now reads:

bUnit stubs the interop boundary, so every test in PlaceMark.WebUI.Tests proves this application's own handling of each outcome geolocation.js can report, and none of them runs a line of it. What does run it is PlaceMark.E2E.Tests, against real headless Chromium on every pull request (ADR-0091, ADR-0092), with the browser's own Geolocation API emulated to a fixed position … An earlier draft of this record claimed this project has no JavaScript test runner. That was simply false, and it was doing real work in the argument: it was the stated reason centreAtLeast's Math.max and the dot's behaviour were left unproven, and the reason a source-text guard was reached for.

The same sentence is gone from CurrentLocationButtonTests, and the tripwire's remarks now carry the narrowed reason verbatim: the fixture serves http://localhost:5169, localhost is potentially trustworthy, so isSecureContext is true throughout the suite and the insecure branch is unreachable without rehosting every journey on a distrusted address.

New CurrentLocationJourneyTests (3 [E2EFact]s, suite now 49) with a NewPageAsync(latitude, longitude) fixture overload granting and pinning the position:

pressed from zoom 18 → still 18, centred on the reported position;
pressed from zoom 2 → 16, centred — the two sides of Math.max;
a place added at the dot's exact coordinate → one .placemark-current-location-icon, zero .placemark-cluster-icon (a dot in placeMarkMarkers would collapse both into a count badge), no leaflet-interactive class, no tabindex.

Refusal path declined, reason stated in the class and the ADR: what Chromium does with an ungranted geolocation permission under automation is a property of the harness, not of this application, and the outcome→message mapping it would exercise is already proved per outcome in bUnit.

2. Tripwire hole — real, closed, demonstrated both ways. Comments and string literals are now stripped before matching. I reproduced your evasion exactly (real check relocated below the feature detect, decoy line // window.isSecureContext is what decides the insecure case; see below. at the top of the function body): the previous check passed it, the comment-stripping one reddens on the identical file. Also still red against the plain swap and the outright deletion. My first attempt at the evasion failed for the wrong reason — the feature detect's own comment mentions navigator.geolocation — which is why the decoy had to go above everything, and is worth knowing if this is ever re-tested.

3. CI. Understood — my figure is not the gate. Run #814 (a739c82) was superseded by this amend; watching the run for 4ae9324 and will report its status rather than my own numbers. For the record, #808 on the original head passed the ratchet step.

Also noticed, not touched: ADR-0142 line 137 carries the same false "this project has no JavaScript test runner" claim. Out of scope here, but it is the record I inherited the phrasing from.

Actioned in `4ae9324`. **1. "No JavaScript test runner" — false, corrected, and the E2E case added rather than declined.** ADR-0167's "What was verified" now reads: > bUnit stubs the interop boundary, so every test in `PlaceMark.WebUI.Tests` proves this application's own handling of each outcome `geolocation.js` can report, and none of them runs a line of it. What does run it is `PlaceMark.E2E.Tests`, against real headless Chromium on every pull request (ADR-0091, ADR-0092), with the browser's own Geolocation API emulated to a fixed position … **An earlier draft of this record claimed this project has no JavaScript test runner. That was simply false**, and it was doing real work in the argument: it was the stated reason `centreAtLeast`'s `Math.max` and the dot's behaviour were left unproven, and the reason a source-text guard was reached for. The same sentence is gone from `CurrentLocationButtonTests`, and the tripwire's remarks now carry the narrowed reason verbatim: the fixture serves `http://localhost:5169`, `localhost` is potentially trustworthy, so `isSecureContext` is `true` throughout the suite and the insecure branch is unreachable without rehosting every journey on a distrusted address. New `CurrentLocationJourneyTests` (3 `[E2EFact]`s, suite now 49) with a `NewPageAsync(latitude, longitude)` fixture overload granting and pinning the position: - pressed from zoom 18 → still 18, centred on the reported position; - pressed from zoom 2 → 16, centred — the two sides of `Math.max`; - a place added at the dot's exact coordinate → one `.placemark-current-location-icon`, **zero** `.placemark-cluster-icon` (a dot in `placeMarkMarkers` would collapse both into a count badge), no `leaflet-interactive` class, no `tabindex`. Refusal path declined, reason stated in the class and the ADR: what Chromium does with an ungranted geolocation permission under automation is a property of the harness, not of this application, and the outcome→message mapping it would exercise is already proved per outcome in bUnit. **2. Tripwire hole — real, closed, demonstrated both ways.** Comments and string literals are now stripped before matching. I reproduced your evasion exactly (real check relocated below the feature detect, decoy line `// window.isSecureContext is what decides the insecure case; see below.` at the top of the function body): **the previous check passed it**, the comment-stripping one reddens on the identical file. Also still red against the plain swap and the outright deletion. My first attempt at the evasion failed for the wrong reason — the feature detect's own comment mentions `navigator.geolocation` — which is why the decoy had to go above everything, and is worth knowing if this is ever re-tested. **3. CI.** Understood — my figure is not the gate. Run #814 (`a739c82`) was superseded by this amend; watching the run for `4ae9324` and will report its status rather than my own numbers. For the record, #808 on the original head passed the ratchet step. Also noticed, not touched: ADR-0142 line 137 carries the same false "this project has no JavaScript test runner" claim. Out of scope here, but it is the record I inherited the phrasing from.

rob reviewed

2026-08-20 07:53:04 +00:00

rob left a comment

Verdict: mergeable

All three round-2 findings actioned, and I checked each rather than taking the account.

The E2E pair discriminates, and neither test would do it alone — worth recording, because it is the argument for keeping both: the zoom-18 case fails on any regression to a fixed level (Math.min, MinimumZoom dropped, setView(..., minimumZoom)), and the zoom-2 case fails on the opposite regression, a pan that never touches zoom. Only Math.max passes both. Lat/lng transposition fails the centre assertion at a tolerance three orders of magnitude tighter than any wrong centre. The cluster case is triply covered — dot count, badge count and place-pin count each redden if the dot goes into map.placeMarkMarkers — and leaflet-interactive/tabindex are set by independent branches of L.Marker._initIcon in the vendored Leaflet, so asserting both is not one assertion twice.

The tripwire hole is genuinely closed. I re-ran the stripping regex against the shipped file and four mutations: as shipped only three tokens survive, all code; plain swap, deletion, my decoy-comment evasion and a decoy string literal all go red. Their reproduction is the evasion I meant.

The ADR retraction is complete — the only surviving instance of the phrase in the whole branch is the sentence retracting it, which names what work the false claim was doing. Nothing regressed against the earlier verified list: secure check still the first statement of the only entry point, === false, Math.max, MinimumZoom = null still emitting the identical setView, ThrowingJSRuntime intact.

On the declined refusal path: the conclusion is right but the stated reason is the weaker half. An ungranted permission under Playwright is deterministic, not flaky — the defensible reason is marginal value, since the only thing such a test would add over bUnit is that a real GeolocationPositionError.PERMISSION_DENIED maps to "denied". The ADR already lists that code mapping as reasoned about rather than executed, so the gap is disclosed rather than hidden. Leave it.

Merge order: no reason #198 should not go second or third. Its map.js additions are appended at end of file, disjoint from #197's edit to the setView doc comment and from #199's changes, so textual conflict is unlikely — but two things for whoever rebases last. #198 now depends on MapZoomRequest and extends it with MinimumZoom, so confirm #197's final state retains the type and LeafletMap.ZoomRequest before merging either. And #197 rewrites setView's doc comment to say it is reached only from a Centre change; once #198 lands that is wrong, because centreAtLeast sits beside it as the other half of the same choice and setView still serves ADR-0132. Reconcile the prose in whichever goes second, and rebuild rather than trusting the earlier green.

Verdict: mergeable All three round-2 findings actioned, and I checked each rather than taking the account. The E2E pair discriminates, and neither test would do it alone — worth recording, because it is the argument for keeping both: the zoom-18 case fails on any regression to a fixed level (`Math.min`, `MinimumZoom` dropped, `setView(..., minimumZoom)`), and the zoom-2 case fails on the opposite regression, a pan that never touches zoom. Only `Math.max` passes both. Lat/lng transposition fails the centre assertion at a tolerance three orders of magnitude tighter than any wrong centre. The cluster case is triply covered — dot count, badge count and place-pin count each redden if the dot goes into `map.placeMarkMarkers` — and `leaflet-interactive`/`tabindex` are set by independent branches of `L.Marker._initIcon` in the vendored Leaflet, so asserting both is not one assertion twice. The tripwire hole is genuinely closed. I re-ran the stripping regex against the shipped file and four mutations: as shipped only three tokens survive, all code; plain swap, deletion, my decoy-comment evasion and a decoy string literal all go red. Their reproduction is the evasion I meant. The ADR retraction is complete — the only surviving instance of the phrase in the whole branch is the sentence retracting it, which names what work the false claim was doing. Nothing regressed against the earlier verified list: secure check still the first statement of the only entry point, `=== false`, `Math.max`, `MinimumZoom = null` still emitting the identical `setView`, `ThrowingJSRuntime` intact. On the declined refusal path: the conclusion is right but the stated reason is the weaker half. An ungranted permission under Playwright is deterministic, not flaky — the defensible reason is marginal value, since the only thing such a test would add over bUnit is that a real `GeolocationPositionError.PERMISSION_DENIED` maps to `"denied"`. The ADR already lists that code mapping as reasoned about rather than executed, so the gap is disclosed rather than hidden. Leave it. Merge order: no reason #198 should not go second or third. Its `map.js` additions are appended at end of file, disjoint from #197's edit to the `setView` doc comment and from #199's changes, so textual conflict is unlikely — but two things for whoever rebases last. #198 now *depends* on `MapZoomRequest` and extends it with `MinimumZoom`, so confirm #197's final state retains the type and `LeafletMap.ZoomRequest` before merging either. And #197 rewrites `setView`'s doc comment to say it is reached only from a `Centre` change; once #198 lands that is wrong, because `centreAtLeast` sits beside it as the other half of the same choice and `setView` still serves ADR-0132. Reconcile the prose in whichever goes second, and rebuild rather than trusting the earlier green.

src/PlaceMark.WebUI/wwwroot/js/map.js Outdated

					
				@ -927,0 +960,4 @@

						icon: currentLocationIcon(),

						interactive: false,

						keyboard: false,

						alt: "Your current location",

Non-blocking, and not yours to fix here. alt does nothing for a divIcon: the vendored Leaflet applies it only to an IMG ("IMG"===i.tagName&&(i.alt=t.alt||"")), and currentLocationIcon returns a divIcon, so this string never reaches the DOM. Harmless for the dot itself — it is decorative, non-interactive and out of the tab order by design, so having no accessible name is arguably correct — but the option reads as though it provides one.

The reason to mention it: line 798 does the same thing for place markers, alt: marker.label || "Marker", and colouredPinIcon is also a divIcon. So the group-and-place label ADR-0098 says is "read aloud by a screen reader" is not reaching assistive technology either. That is a pre-existing defect, out of scope for this PR, and worth a ticket.

Non-blocking, and not yours to fix here. `alt` does nothing for a `divIcon`: the vendored Leaflet applies it only to an `IMG` (`"IMG"===i.tagName&&(i.alt=t.alt||"")`), and `currentLocationIcon` returns a `divIcon`, so this string never reaches the DOM. Harmless for the dot itself — it is decorative, non-interactive and out of the tab order by design, so having no accessible name is arguably correct — but the option reads as though it provides one. The reason to mention it: line 798 does the same thing for *place* markers, `alt: marker.label || "Marker"`, and `colouredPinIcon` is also a `divIcon`. So the group-and-place label ADR-0098 says is "read aloud by a screen reader" is not reaching assistive technology either. That is a pre-existing defect, out of scope for this PR, and worth a ticket.

rob force-pushed current-location from 4ae93241cb

CI / build (pull_request) Successful in 7m54s

Details

CI / container-images (pull_request) Successful in 40s

Details

CI / e2e (pull_request) Successful in 5m33s

Details

to 146470034b

CI / build (pull_request) Failing after 3m16s

Details

CI / e2e (pull_request) Has been skipped

Details

CI / container-images (pull_request) Has been skipped

Details

2026-08-20 08:02:56 +00:00

Compare

rob commented

2026-08-20 08:03:24 +00:00

Rebased onto 10ce610 (#199) — head is now 1464700. Rebuilt and re-ran, not just rebased.

The rebase was not conflict-free. Three files conflicted: LeafletMap.razor.cs, LeafletMapTests.cs, HomeTests.cs. The two source-side ones and HomeTests were purely additive (a _renderedCurrentLocation field beside _renderedSelectedMarkerId, a first-render block beside #199's, a stub line) and were resolved by keeping both sides. LeafletMapTests.cs interleaved mid-method, because git aligned both branches' new test blocks on shared boilerplate ({, var module = SetUpMapModule();, .Add(p => p.Centre, _london)) — a naive "keep both" there would have produced two half-methods that happened to compile into nonsense. That file was instead rebuilt from origin/main's copy plus my two insertion hunks, extracted from the diff rather than retyped. Verified afterwards that every deletion in the branch diff against main is one of my own intended edits (the MapZoomRequest doc comment and the setView call replaced by the MinimumZoom branch) and nothing of #199's was lost.

My three commits were squashed into one first, so the interleaved region was resolved once rather than three times. The review trail lives in these comments.

Ring/dot interaction — confirmed, then proved rather than reasoned. Three independent things keep them apart: applySelectionRing only walks map.placeMarkMarkers.getLayers(), and the dot is added with .addTo(map) (map.js:1061), never placeMarkMarkers.addLayer; the dot carries no placeMarkId to match a selection id against; and the ring rule is keyed on .placemark-marker-icon--selected, which the dot's element (leaflet-marker-icon placemark-current-location-icon) never gets. Added a fourth E2E case, ShowMyLocation_WithAPlaceSelectedAndRinged_LeavesTheDotUnringed, which asserts the pin is ringed first so it cannot pass by nothing being ringed, then that the dot has neither the class nor the --placemark-selection-ring-colour inline property, then that the ring is still on the pin.

Coverage re-measured, not carried over. coverage-baseline.json on current main still reads 91.5 for PlaceMark.WebUI. Release, as CI runs it: main (10ce610) 91.5861%, this branch 91.6988%.

Local runs on the rebased head: WebUI 904 (+4 skipped), Architecture 135, Domain 39, Infrastructure 360, Api 622, E2E 56 (all skipped locally by design; 4 of them mine). Build clean, dotnet --version 10.0.100 matching global.json.

CI is running for 1464700.

Rebased onto `10ce610` (#199) — head is now `1464700`. Rebuilt and re-ran, not just rebased. **The rebase was not conflict-free.** Three files conflicted: `LeafletMap.razor.cs`, `LeafletMapTests.cs`, `HomeTests.cs`. The two source-side ones and `HomeTests` were purely additive (a `_renderedCurrentLocation` field beside `_renderedSelectedMarkerId`, a first-render block beside #199's, a stub line) and were resolved by keeping both sides. `LeafletMapTests.cs` interleaved mid-method, because git aligned both branches' new test blocks on shared boilerplate (`{`, `var module = SetUpMapModule();`, `.Add(p => p.Centre, _london)`) — a naive "keep both" there would have produced two half-methods that happened to compile into nonsense. That file was instead rebuilt from `origin/main`'s copy plus my two insertion hunks, extracted from the diff rather than retyped. Verified afterwards that every deletion in the branch diff against `main` is one of my own intended edits (the `MapZoomRequest` doc comment and the `setView` call replaced by the `MinimumZoom` branch) and nothing of #199's was lost. My three commits were squashed into one first, so the interleaved region was resolved once rather than three times. The review trail lives in these comments. **Ring/dot interaction — confirmed, then proved rather than reasoned.** Three independent things keep them apart: `applySelectionRing` only walks `map.placeMarkMarkers.getLayers()`, and the dot is added with `.addTo(map)` (map.js:1061), never `placeMarkMarkers.addLayer`; the dot carries no `placeMarkId` to match a selection id against; and the ring rule is keyed on `.placemark-marker-icon--selected`, which the dot's element (`leaflet-marker-icon placemark-current-location-icon`) never gets. Added a fourth E2E case, `ShowMyLocation_WithAPlaceSelectedAndRinged_LeavesTheDotUnringed`, which asserts the pin is ringed *first* so it cannot pass by nothing being ringed, then that the dot has neither the class nor the `--placemark-selection-ring-colour` inline property, then that the ring is still on the pin. **Coverage re-measured, not carried over.** `coverage-baseline.json` on current `main` still reads **91.5** for `PlaceMark.WebUI`. Release, as CI runs it: `main` (`10ce610`) **91.5861%**, this branch **91.6988%**. Local runs on the rebased head: WebUI 904 (+4 skipped), Architecture 135, Domain 39, Infrastructure 360, Api 622, E2E 56 (all skipped locally by design; 4 of them mine). Build clean, `dotnet --version` 10.0.100 matching `global.json`. CI is running for `1464700`.

rob referenced this pull request from a commit

2026-08-20 08:11:33 +00:00

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

rob commented

2026-08-20 08:28:32 +00:00

#820's two failures are pre-existing timing fragility, not this branch. No code change; head is still 1464700. Evidence, strongest first.

1. Neither failing test renders anything this branch adds — proved, not argued. I put a live tripwire in CurrentLocationButton.OnInitialized (throw) and ran both CI-failing tests: both still passed. The same tripwire reddens CurrentLocationButton_Rendered_… and PlaceForm_CurrentLocationFound_FillsBothCoordinateFields, so it discriminates.

GroupFormPanelTests renders GroupFormPanel directly; DrawerAuthenticatedTests renders Drawer, not Home — so the hypothesis that Home's new floating button is in that tree does not hold. Drawer can reach PlaceForm (and therefore my button) via .accordion-add-place, but this particular test only asserts that button exists and never clicks it — which is exactly what the tripwire confirms.

2. Both failures are one mechanism, and it is a real-time delay against a 1 s budget. ModalOverlay.RequestCloseAsync awaits Task.Delay(ClosingAnimationDurationMilliseconds) = 200 ms, and both test classes stub prefers-reduced-motion to false (GroupFormPanelTests:45, DrawerAuthenticatedTests:43), so the delay genuinely runs. Both tests then wait on bUnit's WaitForAssertion with its default 1 s budget — closed.ShouldBeTrue() after the close, and .modal-overlay-panel disappearing after the close. A 5× margin, and the first thing to go when the runner is starved. That is why these two failed together and nothing else did.

3. Cannot reproduce on either tree. 59 runs on this branch and 44 on main at 10ce610, zero failures each: idle; 22 spinners; 96 spinners on 24 cores; pinned to a single core against 3 and then 10 contending spinners; and the whole WebUI assembly with coverlet instrumentation confined to two cores against 4 spinners — the closest shape to CI available here.

4. This branch does not slow the assembly, it speeds it up. Same two-core + coverage condition, alternated to rule out ordering: branch 5–7 s for 908 tests, main 9–10 s for 873. So the one indirect mechanism worth suspecting — 35 extra tests adding parallel load — points the wrong way.

Recommendation: ticket the fragility rather than patch it here. The fix is not a longer timeout; it is that ModalOverlay's 200 ms is real wall-clock in a test that could inject it. Two tests carry a 5× margin against a wall-clock delay, and there may be more on the same path (PlaceFormPanelTests has 17 WaitFor* sites). Task 261's bistable coverage line in PlaceFormPanel.RequestCloseAsync is the same area. I have deliberately not touched either test.

Please re-run #820 — the MCP tooling exposes no re-run, and pushing the same tree would need a fresh SHA.

**#820's two failures are pre-existing timing fragility, not this branch.** No code change; head is still `1464700`. Evidence, strongest first. **1. Neither failing test renders anything this branch adds — proved, not argued.** I put a live tripwire in `CurrentLocationButton.OnInitialized` (`throw`) and ran both CI-failing tests: **both still passed**. The same tripwire reddens `CurrentLocationButton_Rendered_…` and `PlaceForm_CurrentLocationFound_FillsBothCoordinateFields`, so it discriminates. `GroupFormPanelTests` renders `GroupFormPanel` directly; `DrawerAuthenticatedTests` renders `Drawer`, not `Home` — so the hypothesis that `Home`'s new floating button is in that tree does not hold. `Drawer` *can* reach `PlaceForm` (and therefore my button) via `.accordion-add-place`, but this particular test only asserts that button exists and never clicks it — which is exactly what the tripwire confirms. **2. Both failures are one mechanism, and it is a real-time delay against a 1 s budget.** `ModalOverlay.RequestCloseAsync` awaits `Task.Delay(ClosingAnimationDurationMilliseconds)` = **200 ms**, and both test classes stub `prefers-reduced-motion` to `false` (`GroupFormPanelTests:45`, `DrawerAuthenticatedTests:43`), so the delay genuinely runs. Both tests then wait on bUnit's `WaitForAssertion` with its default **1 s** budget — `closed.ShouldBeTrue()` after the close, and `.modal-overlay-panel` disappearing after the close. A 5× margin, and the first thing to go when the runner is starved. That is why these two failed together and nothing else did. **3. Cannot reproduce on either tree.** 59 runs on this branch and 44 on `main` at `10ce610`, zero failures each: idle; 22 spinners; 96 spinners on 24 cores; pinned to a single core against 3 and then 10 contending spinners; and the whole WebUI assembly with coverlet instrumentation confined to two cores against 4 spinners — the closest shape to CI available here. **4. This branch does not slow the assembly, it speeds it up.** Same two-core + coverage condition, alternated to rule out ordering: branch **5–7 s** for 908 tests, `main` **9–10 s** for 873. So the one indirect mechanism worth suspecting — 35 extra tests adding parallel load — points the wrong way. **Recommendation:** ticket the fragility rather than patch it here. The fix is not a longer timeout; it is that `ModalOverlay`'s 200 ms is real wall-clock in a test that could inject it. Two tests carry a 5× margin against a wall-clock delay, and there may be more on the same path (`PlaceFormPanelTests` has 17 `WaitFor*` sites). Task 261's bistable coverage line in `PlaceFormPanel.RequestCloseAsync` is the same area. I have deliberately not touched either test. Please re-run #820 — the MCP tooling exposes no re-run, and pushing the same tree would need a fresh SHA.

rob force-pushed current-location from 146470034b

CI / build (pull_request) Failing after 3m16s

Details

CI / e2e (pull_request) Has been skipped

Details

CI / container-images (pull_request) Has been skipped

Details

to 2f081e36a3

CI / build (pull_request) Successful in 3m13s

Details

CI / container-images (pull_request) Successful in 4s

Details

CI / e2e (pull_request) Successful in 4m33s

Details

2026-08-20 11:36:11 +00:00

Compare

rob commented

2026-08-20 11:36:31 +00:00

Rebased onto 738f720 (#197). Head is now 2f081e3.

Rebasing alone would have shipped this feature silently broken. #197 removed ZoomRequest="_zoomRequest" from Home.razor's <LeafletMap> along with the zoom button that fed it. Git merged that removal cleanly, the solution compiled with zero warnings — and "Show my location" plotted the dot but never moved the map, because HandleCurrentLocation was still setting a field nothing was bound to. Caught by Home_CurrentLocationFound_TakesTheMapThereWithoutEverZoomingOut, which failed on the first post-rebase run. Binding restored, with a comment saying why it came back.

Four conflicts, resolved as follows. docs/adr/README.md — index rows reordered 0166/0167/0168. MapZoomRequest.cs — took #197's rewritten summary wholesale; it is better than mine and already names task 254. LeafletMap.ZoomRequest — merged both, keeping #197's "whichever control asked for one" framing and my description of what MinimumZoom does. Home.razor.cs — kept _zoomRequest and HandleZoomToPlace's removal, and rewrote _zoomRequest's doc comment, which named a handler that no longer exists.

setView's doc comment: checked, and it had gone stale under me. #197's version says it is reached "from a Centre parameter change and from ZoomRequest" — accurate when #197 was reviewed, false once this branch lands. The only surviving producer of a MapZoomRequest is "Show my location", which names a floor and therefore reaches centreAtLeast, not setView. Corrected to say so explicitly rather than leaving a live-looking path that nothing takes. Same correction applied to centreAtLeast's own comment (it cited the removed zoom button as the level-naming caller), and to three passages in ADR-0167.

One thing I did not do, and want ruled on. With both branches combined, MapZoomRequest.MinimumZoom == null and LeafletMap.ExplicitZoomLevel have no producer in the application. The honest minimal shape would be to make MinimumZoom required and delete ExplicitZoomLevel. I did not, because ExplicitZoomLevel is ADR-0132's judgement and ADR-0166 kept it deliberately — deleting it inside this PR would re-litigate someone else's record silently. Instead every comment that touches the dead branch now says plainly that nothing reaches it. Both branches stay covered at the component boundary (LeafletMap_ZoomRequestNamingNoMinimumZoom_NeverReachesCentreAtLeast and the pre-existing …CallsSetViewAtTheExplicitZoomLevel). If you would rather it went, it is a small follow-up.

Coverage, three samples, Release, all identical: PlaceMark.WebUI 91.6435% (2632/2872). New main (738f720), two samples: 91.5302%. Floor in coverage-baseline.json is 91.4.

Local runs on the rebased head: WebUI 915 (+4 skipped), Architecture 135, Domain 39, Infrastructure 360, Api 622, E2E 56 skipped. Build clean, dotnet format --verify-no-changes clean, SDK 10.0.100 matching global.json.

CI is running for 2f081e3. If GroupFormPanel_EscapePressed_… and Drawer_GroupCreated_… fail again on this head, I will say so rather than re-explain task 263.

Rebased onto `738f720` (#197). Head is now `2f081e3`. **Rebasing alone would have shipped this feature silently broken.** #197 removed `ZoomRequest="_zoomRequest"` from `Home.razor`'s `<LeafletMap>` along with the zoom button that fed it. Git merged that removal cleanly, the solution compiled with zero warnings — and "Show my location" plotted the dot but never moved the map, because `HandleCurrentLocation` was still setting a field nothing was bound to. Caught by `Home_CurrentLocationFound_TakesTheMapThereWithoutEverZoomingOut`, which failed on the first post-rebase run. Binding restored, with a comment saying why it came back. **Four conflicts, resolved as follows.** `docs/adr/README.md` — index rows reordered 0166/0167/0168. `MapZoomRequest.cs` — took #197's rewritten summary wholesale; it is better than mine and already names task 254. `LeafletMap.ZoomRequest` — merged both, keeping #197's "whichever control asked for one" framing and my description of what `MinimumZoom` does. `Home.razor.cs` — kept `_zoomRequest` and `HandleZoomToPlace`'s removal, and rewrote `_zoomRequest`'s doc comment, which named a handler that no longer exists. **`setView`'s doc comment: checked, and it had gone stale under me.** #197's version says it is reached "from a `Centre` parameter change and from `ZoomRequest`" — accurate when #197 was reviewed, false once this branch lands. The only surviving producer of a `MapZoomRequest` is "Show my location", which names a floor and therefore reaches `centreAtLeast`, not `setView`. Corrected to say so explicitly rather than leaving a live-looking path that nothing takes. Same correction applied to `centreAtLeast`'s own comment (it cited the removed zoom button as the level-naming caller), and to three passages in ADR-0167. **One thing I did not do, and want ruled on.** With both branches combined, `MapZoomRequest.MinimumZoom == null` and `LeafletMap.ExplicitZoomLevel` have **no producer in the application**. The honest minimal shape would be to make `MinimumZoom` required and delete `ExplicitZoomLevel`. I did not, because `ExplicitZoomLevel` is ADR-0132's judgement and ADR-0166 kept it deliberately — deleting it inside this PR would re-litigate someone else's record silently. Instead every comment that touches the dead branch now says plainly that nothing reaches it. Both branches stay covered at the component boundary (`LeafletMap_ZoomRequestNamingNoMinimumZoom_NeverReachesCentreAtLeast` and the pre-existing `…CallsSetViewAtTheExplicitZoomLevel`). If you would rather it went, it is a small follow-up. **Coverage**, three samples, Release, all identical: `PlaceMark.WebUI` **91.6435%** (2632/2872). New `main` (`738f720`), two samples: **91.5302%**. Floor in `coverage-baseline.json` is **91.4**. **Local runs on the rebased head:** WebUI 915 (+4 skipped), Architecture 135, Domain 39, Infrastructure 360, Api 622, E2E 56 skipped. Build clean, `dotnet format --verify-no-changes` clean, SDK 10.0.100 matching `global.json`. CI is running for `2f081e3`. If `GroupFormPanel_EscapePressed_…` and `Drawer_GroupCreated_…` fail again on this head, I will say so rather than re-explain task 263.

~~rob referenced this pull request 2026-08-20 11:44:48 +00:00~~

Prove the place marker's accessible name against the accessibility tree #200

rob commented

2026-08-20 11:45:03 +00:00

Drop the alt option on the current-location dot's marker: Leaflet copies it onto the icon element only when that element is an <img>, so on a divIcon it is silently dropped. Task 262 removes the identical dead option from the place markers and corrects ADR-0098, which had asserted behaviour that option never had — worth not landing a third copy. See PR #200 for the wording of the call-site comment.

Drop the `alt` option on the current-location dot's marker: Leaflet copies it onto the icon element only when that element is an `<img>`, so on a `divIcon` it is silently dropped. Task 262 removes the identical dead option from the place markers and corrects ADR-0098, which had asserted behaviour that option never had — worth not landing a third copy. See PR #200 for the wording of the call-site comment.

rob reviewed

2026-08-20 11:48:20 +00:00

rob left a comment

Verdict: mergeable

The restored binding is correct and complete. ZoomRequest="_zoomRequest" and CurrentLocation="_currentLocation" are both on the <LeafletMap>; both fields are declared, written only by HandleCurrentLocation, and read only by those bindings. I swept every private field in Home.razor.cs for the same orphan shape — all nine have a consumer, and nothing anywhere still names HandleZoomToPlace or OnZoomToPlace. Worth stating because it is the point of the exercise: the regression is now guarded, not just fixed. Home_CurrentLocationFound_TakesTheMapThereWithoutEverZoomingOut asserts centreAtLeast was invoked with the coordinate and the floor, which cannot happen with ZoomRequest unbound — which is exactly why it caught this.

Conflicts: nothing lost. #197's sharper MapZoomRequest summary is in, and this PR's MinimumZoom parameter doc, the no-caller disclosure and the per-press-identity remarks all survive alongside it. _zoomRequest's doc no longer names a handler that does not exist.

The four stale passages all read correctly now, including the two I predicted would go wrong: setView says outright that no ZoomRequest this application raises reaches it any more and that a Centre change is what calls it, and centreAtLeast names the sibling path as having no caller rather than describing it as live.

Coverage arithmetic holds — 2632/2872 is 91.6435%, above both the 91.4 floor and new main at 91.5302 — and #822's own ratchet step is the check that matters, which passed.

Two judgements you asked for.

The dead MinimumZoom == null / ExplicitZoomLevel branch: leave it, as you have. Deleting it would retire ADR-0132's judgement as a side effect of an unrelated ticket, which is the thing this project keeps ADRs to prevent. It is disclosed in three places — the type's remarks, the MinimumZoom parameter doc and LeafletMap.ZoomRequest — each saying plainly that nothing reaches it, so no future reader infers a caller. A separate ticket is the right home.

The dead alt: leave it here too, to PR #200's ticket. Removing it from your dot fixes nothing observable — the dot is deliberately unlabelled and correctly so — and it would leave this line gratuitously different from the identical pre-existing alt: marker.label || "Marker" on the place-pin icon, which is the one that actually matters and which that ticket has to change anyway. One change, one place, one piece of reasoning.

Nit, non-blocking: two comment blocks were edited in place without rewrapping and now have an over-long line — centreAtLeast's in map.js ("…is not recoverable. map.getZoom() is read here rather than in .NET because it is") and ADR-0167's around "A reader pressing this is asking directly". Sweep them if you touch either file again.

Verdict: mergeable **The restored binding is correct and complete.** `ZoomRequest="_zoomRequest"` and `CurrentLocation="_currentLocation"` are both on the `<LeafletMap>`; both fields are declared, written only by `HandleCurrentLocation`, and read only by those bindings. I swept every private field in `Home.razor.cs` for the same orphan shape — all nine have a consumer, and nothing anywhere still names `HandleZoomToPlace` or `OnZoomToPlace`. Worth stating because it is the point of the exercise: the regression is now guarded, not just fixed. `Home_CurrentLocationFound_TakesTheMapThereWithoutEverZoomingOut` asserts `centreAtLeast` was invoked with the coordinate and the floor, which cannot happen with `ZoomRequest` unbound — which is exactly why it caught this. Conflicts: nothing lost. #197's sharper `MapZoomRequest` summary is in, and this PR's `MinimumZoom` parameter doc, the no-caller disclosure and the per-press-identity remarks all survive alongside it. `_zoomRequest`'s doc no longer names a handler that does not exist. The four stale passages all read correctly now, including the two I predicted would go wrong: `setView` says outright that no `ZoomRequest` this application raises reaches it any more and that a `Centre` change is what calls it, and `centreAtLeast` names the sibling path as having no caller rather than describing it as live. Coverage arithmetic holds — 2632/2872 is 91.6435%, above both the 91.4 floor and new `main` at 91.5302 — and #822's own ratchet step is the check that matters, which passed. Two judgements you asked for. **The dead `MinimumZoom == null` / `ExplicitZoomLevel` branch: leave it, as you have.** Deleting it would retire ADR-0132's judgement as a side effect of an unrelated ticket, which is the thing this project keeps ADRs to prevent. It is disclosed in three places — the type's remarks, the `MinimumZoom` parameter doc and `LeafletMap.ZoomRequest` — each saying plainly that nothing reaches it, so no future reader infers a caller. A separate ticket is the right home. **The dead `alt`: leave it here too, to PR #200's ticket.** Removing it from your dot fixes nothing observable — the dot is deliberately unlabelled and correctly so — and it would leave this line gratuitously different from the identical pre-existing `alt: marker.label || "Marker"` on the place-pin icon, which is the one that actually matters and which that ticket has to change anyway. One change, one place, one piece of reasoning. Nit, non-blocking: two comment blocks were edited in place without rewrapping and now have an over-long line — `centreAtLeast`'s in `map.js` ("…is not recoverable. `map.getZoom()` is read here rather than in .NET because it is") and ADR-0167's around "A reader pressing this is asking directly". Sweep them if you touch either file again.

rob merged commit eec31e1906 into main

2026-08-20 11:48:46 +00:00

rob deleted branch current-location

2026-08-20 11:48:46 +00:00

rob referenced this pull request from a commit

2026-08-20 11:48:47 +00:00

Show the reader's current location on the map and in the add-place form (#198)

~~rob referenced this pull request 2026-08-20 12:30:56 +00:00~~

Take ModalOverlay's closing delay off the wall clock #201

rob referenced this pull request from a commit

2026-08-21 12:34:04 +00:00

Remove the null-MinimumZoom branch and ExplicitZoomLevel (#209)