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

Merged
rob merged 1 commit from place-detail-panel-controls into main 2026-08-20 08:11:32 +00:00
Owner

Tasks 251 (#75), 255 (#79) and 257 (#81) — all three touch PlaceDetailPanel, so one branch.

251 — "Zoom to place" removed. The button, PlaceDetailPanel.OnZoomToPlace, Home.HandleZoomToPlace and Home's ZoomRequest binding. MapZoomRequest, LeafletMap.ZoomRequest and ExplicitZoomLevel stay — task 254 (PR #198) is a second consumer and needs the same per-press identity. Their doc comments re-point to it. ADR-0166 partly supersedes ADR-0132: the control, not the mechanism.

255 — one sharing control, chosen by pointer type. Home reads (pointer: coarse) through layout.js in OnInitializedAsync and passes it down as bool? PointerIsCoarse; the panel does not read it itself, so it is never rendered before the answer exists and never replays the transition on remount. Unknown shows both, as does an exhausted Share on a desktop, whose own message names the anchor. ADR-0168 partly supersedes ADR-0142.

257 — content width fixed at 240px. width: calc(240px + 2 * var(--space-medium)) under global border-box, with the viewport clamp kept and overflow-wrap: break-word added. The padding is load-bearing for that arithmetic and is pinned alongside it, as is the agreement between the 240px content box and task 253's 240×135 placeholder.

The layout.js module reference is relocated, not fixed — it is still assigned after the await, so a disposal mid-import orphans it. What changed is frequency: once per page load rather than once per place selection.

Coverage floor 91.5 → 91.4, because the measurement is bistable, not because three lines are unreachable. PlaceFormPanel.RequestCloseAsync:587 is pre-existing and timing-dependent, and this branch's line count lands on the rounding boundary it straddles: 2571 or 2572 of 2810, reported as 91.4 or 91.5 (five samples, 91.4 four times). main has the same flake but reports 91.5 either way. Of the 17 new coverable lines, 14 are covered and the three that are not are all in Home.DisposeAsync — the "nothing was imported" return, the catch (JSDisconnectedException), and that clause's closing brace, which coverlet counts separately. Two are genuinely unreachable from bUnit; the third is the same clause counted twice. Task 261 tracks the flake and owns raising the floor back. ADR-0168 has the full derivation, including two earlier accounts of this that were wrong.

Rebased onto #199. LeafletMap carries both SelectedMarkerId and ZoomRequest; Home.razor binds both SelectedMarkerId and PointerIsCoarse. Rebuilt, not just rebased.

Whoever merges this and #198 second must rebase and rebuild — git reports no conflict on MapZoomRequest where the compiler would.

Tasks 251 (#75), 255 (#79) and 257 (#81) — all three touch `PlaceDetailPanel`, so one branch. **251 — "Zoom to place" removed.** The button, `PlaceDetailPanel.OnZoomToPlace`, `Home.HandleZoomToPlace` and `Home`'s `ZoomRequest` binding. `MapZoomRequest`, `LeafletMap.ZoomRequest` and `ExplicitZoomLevel` **stay** — task 254 (PR #198) is a second consumer and needs the same per-press identity. Their doc comments re-point to it. **ADR-0166** partly supersedes ADR-0132: the control, not the mechanism. **255 — one sharing control, chosen by pointer type.** `Home` reads `(pointer: coarse)` through `layout.js` in `OnInitializedAsync` and passes it down as `bool? PointerIsCoarse`; the panel does not read it itself, so it is never rendered before the answer exists and never replays the transition on remount. Unknown shows both, as does an exhausted Share on a desktop, whose own message names the anchor. **ADR-0168** partly supersedes ADR-0142. **257 — content width fixed at 240px.** `width: calc(240px + 2 * var(--space-medium))` under global `border-box`, with the viewport clamp kept and `overflow-wrap: break-word` added. The padding is load-bearing for that arithmetic and is pinned alongside it, as is the agreement between the 240px content box and task 253's 240×135 placeholder. **The `layout.js` module reference is relocated, not fixed** — it is still assigned after the `await`, so a disposal mid-import orphans it. What changed is frequency: once per page load rather than once per place selection. **Coverage floor 91.5 → 91.4, because the measurement is bistable, not because three lines are unreachable.** `PlaceFormPanel.RequestCloseAsync:587` is pre-existing and timing-dependent, and this branch's line count lands on the rounding boundary it straddles: 2571 or 2572 of 2810, reported as 91.4 or 91.5 (five samples, 91.4 four times). `main` has the same flake but reports 91.5 either way. Of the 17 new coverable lines, 14 are covered and the three that are not are all in `Home.DisposeAsync` — the "nothing was imported" `return`, the `catch (JSDisconnectedException)`, and that clause's closing brace, which coverlet counts separately. Two are genuinely unreachable from bUnit; the third is the same clause counted twice. **Task 261** tracks the flake and owns raising the floor back. ADR-0168 has the full derivation, including two earlier accounts of this that were wrong. Rebased onto #199. `LeafletMap` carries both `SelectedMarkerId` and `ZoomRequest`; `Home.razor` binds both `SelectedMarkerId` and `PointerIsCoarse`. Rebuilt, not just rebased. Whoever merges this and #198 second must rebase **and rebuild** — git reports no conflict on `MapZoomRequest` where the compiler would.
Rework the place detail panel's controls and fix its width
Some checks failed
CI / build (pull_request) Failing after 4m24s
CI / e2e (pull_request) Has been skipped
CI / container-images (pull_request) Has been skipped
fa237b055b
rob left a comment

Verdict: changes needed

  • map.js line 148 still names ExplicitZoomLevel's zoom button, a symbol this PR deletes — task 251's "any now-dead zoom plumbing in map.js" is not fully discharged.
  • The 240px arithmetic checks out (box-sizing: border-box from app.css lines 8/12, no border on the panel, padding: var(--space-medium) uniform), but nothing pins the padding it depends on. Change the padding and the panel silently stops being 240px of content with all three stylesheet tests still green.
  • ShowOpenInMapsApp's _shareFallbackLink is not null clause is only half guarded: swap it for _shareStatus is not null and every test still passes, while a successful desktop share would wrongly re-reveal the anchor with no message naming it.

Also, not blocking:

  • HomeTests.cs:69-73 attributes the layout.js matches setup solely to PlaceFormPanel's reduced-motion read. PlaceDetailPanel now rides the same catch-all _ => true handler and gets false from it; flipping that literal would silently move every Home test to the coarse branch. Worth a sentence.
  • ReadPointerTypeAsync assigns _layoutModule after an await, so a panel disposed mid-import leaks the reference — DisposeAsync has already run and seen null. Pre-existing shape for share.js, but layout.js is imported on every mount and the panel is @keyed by place id, so it is now reachable by clicking through markers quickly.
  • WithoutComments and the [CallerFilePath] path helper are now duplicated verbatim from NavMenuStylesheetTests. Second copy; worth a shared helper before a third.
  • Two comments left mid-rewrap after deletions: LeafletMap.razor.cs ("…listens on" / "Leaflet's own…") and PlaceDetailPanel.razor.css:72 ("…already belongs to) — display, background," / "border, cursor.").

ADR-0166 and ADR-0168 use the allocated numbers, record their supersessions correctly on both sides, and the index matches. pointer over any-pointer is the right reading of the ticket. Deletion of the zoom chain is complete elsewhere: the surviving setView path is still covered by LeafletMap_CentreParameterChanges_PansTheExistingMapRatherThanRecreatingIt, and first-render-makes-no-setView by LeafletMap_PanTargetParameterChangesToANewCoordinate_CallsPanIntoView, so none of the ten deleted cases takes an unrelated guard with it.

Verdict: changes needed - `map.js` line 148 still names `ExplicitZoomLevel`'s zoom button, a symbol this PR deletes — task 251's "any now-dead zoom plumbing in `map.js`" is not fully discharged. - The 240px arithmetic checks out (`box-sizing: border-box` from `app.css` lines 8/12, no border on the panel, `padding: var(--space-medium)` uniform), but nothing pins the padding it depends on. Change the padding and the panel silently stops being 240px of content with all three stylesheet tests still green. - `ShowOpenInMapsApp`'s `_shareFallbackLink is not null` clause is only half guarded: swap it for `_shareStatus is not null` and every test still passes, while a *successful* desktop share would wrongly re-reveal the anchor with no message naming it. Also, not blocking: - `HomeTests.cs:69-73` attributes the `layout.js` `matches` setup solely to `PlaceFormPanel`'s reduced-motion read. `PlaceDetailPanel` now rides the same catch-all `_ => true` handler and gets `false` from it; flipping that literal would silently move every `Home` test to the coarse branch. Worth a sentence. - `ReadPointerTypeAsync` assigns `_layoutModule` after an await, so a panel disposed mid-import leaks the reference — `DisposeAsync` has already run and seen null. Pre-existing shape for `share.js`, but `layout.js` is imported on every mount and the panel is `@key`ed by place id, so it is now reachable by clicking through markers quickly. - `WithoutComments` and the `[CallerFilePath]` path helper are now duplicated verbatim from `NavMenuStylesheetTests`. Second copy; worth a shared helper before a third. - Two comments left mid-rewrap after deletions: `LeafletMap.razor.cs` ("…listens on" / "Leaflet's own…") and `PlaceDetailPanel.razor.css:72` ("…already belongs to) — display, background," / "border, cursor."). ADR-0166 and ADR-0168 use the allocated numbers, record their supersessions correctly on both sides, and the index matches. `pointer` over `any-pointer` is the right reading of the ticket. Deletion of the zoom chain is complete elsewhere: the surviving `setView` path is still covered by `LeafletMap_CentreParameterChanges_PansTheExistingMapRatherThanRecreatingIt`, and first-render-makes-no-`setView` by `LeafletMap_PanTargetParameterChangesToANewCoordinate_CallsPanIntoView`, so none of the ten deleted cases takes an unrelated guard with it.
@ -178,6 +213,34 @@ public partial class PlaceDetailPanel : IAsyncDisposable
if (firstRender)
{
await TryFocusAsync(_closeButton);
await ReadPointerTypeAsync();
Author
Owner

The both-controls frame is real and lands on every place selection, not once per session: Home.razor keys this panel on selectedPlace.Id, so clicking a second marker remounts it, re-imports layout.js and replays the null → answered transition.

ADR-0168 rejects a caching service on lifetime/staleness grounds, which is fair, but it does not consider the cheaper option: read the query once in Home — long-lived, never re-keyed — and pass the answer down as a parameter. No registration, no lifetime, no staleness question the panel does not already have, and the read has landed long before a reader selects anything. Either take that, or say in the ADR's Alternatives why the parent is not the right owner; as written the record argues past the option it should have rejected explicitly.

The both-controls frame is real and lands on every place selection, not once per session: `Home.razor` keys this panel on `selectedPlace.Id`, so clicking a second marker remounts it, re-imports `layout.js` and replays the null → answered transition. ADR-0168 rejects a caching service on lifetime/staleness grounds, which is fair, but it does not consider the cheaper option: read the query once in `Home` — long-lived, never re-keyed — and pass the answer down as a parameter. No registration, no lifetime, no staleness question the panel does not already have, and the read has landed long before a reader selects anything. Either take that, or say in the ADR's Alternatives why the parent is not the right owner; as written the record argues past the option it should have rejected explicitly.
@ -662,3 +659,1 @@
* selecting or deselecting a place never zooms: a reader pressing this button is asking directly, which
* is not the same event as a selection changing. `panIntoView` below is what a mere selection reaches
* instead.
* Pans and zooms an existing map to a new centre. Reached only from a `Centre` parameter change
Author
Owner

VIEWPORT_SAVE_DEBOUNCE_MS's own comment at line 148 still reads "a programmatic setView (ExplicitZoomLevel's own zoom button, say)". That constant and that button are both gone as of this PR — reword to a Centre change, which is the only programmatic setView left.

`VIEWPORT_SAVE_DEBOUNCE_MS`'s own comment at line 148 still reads "a programmatic setView (ExplicitZoomLevel's own zoom button, say)". That constant and that button are both gone as of this PR — reword to a `Centre` change, which is the only programmatic `setView` left.
@ -0,0 +37,4 @@
{
var panelDeclarations = DeclarationsIn(PlaceDetailPanelStylesheet(), ".place-detail-panel");
panelDeclarations.ShouldContainKeyAndValue("width", FixedContentWidth);
Author
Owner

calc(240px + 2 * var(--space-medium)) is only 240px of content while the panel's horizontal padding is exactly --space-medium on both sides. That coupling is the whole of task 257 and nothing pins it: change padding to var(--space-medium) var(--space-large) and the panel is 208px of content with all three tests here green. Assert padding == var(--space-medium) in this class too — the comment above already explains why it is load-bearing.

`calc(240px + 2 * var(--space-medium))` is only 240px of *content* while the panel's horizontal padding is exactly `--space-medium` on both sides. That coupling is the whole of task 257 and nothing pins it: change `padding` to `var(--space-medium) var(--space-large)` and the panel is 208px of content with all three tests here green. Assert `padding` == `var(--space-medium)` in this class too — the comment above already explains why it is load-bearing.
@ -4,3 +4,4 @@
using Bunit;
using Microsoft.AspNetCore.Components.Web;
using Microsoft.JSInterop;
using Microsoft.Extensions.DependencyInjection;
Author
Owner

Sorts before Microsoft.Extensions.DependencyInjection, not after.

Sorts before `Microsoft.Extensions.DependencyInjection`, not after.
@ -329,0 +395,4 @@
/// the panel name a control it is not rendering — so the exhausted outcome brings it back.
/// </summary>
[Fact]
public async Task PlaceDetailPanel_PointerIsFineAndShareIsExhausted_RevealsTheOpenInMapsAppLinkItsOwnMessageNames()
Author
Owner

This is the only test of the _shareFallbackLink is not null clause, and it does not discriminate the half that matters. Widen the clause to _shareStatus is not null and this still passes (insecure sets both), as does PointerIsFine_ShowsNoOpenInMapsAppLink (no share attempted) — yet the behaviour is now wrong: a successful shared/copied on a desktop would re-reveal the anchor with no message naming it, which is exactly the incoherence the clause exists to prevent. Add PlaceDetailPanel_PointerIsFineAndShareSucceeds_StillShowsNoOpenInMapsAppLink.

This is the only test of the `_shareFallbackLink is not null` clause, and it does not discriminate the half that matters. Widen the clause to `_shareStatus is not null` and this still passes (`insecure` sets both), as does `PointerIsFine_ShowsNoOpenInMapsAppLink` (no share attempted) — yet the behaviour is now wrong: a successful `shared`/`copied` on a desktop would re-reveal the anchor with no message naming it, which is exactly the incoherence the clause exists to prevent. Add `PlaceDetailPanel_PointerIsFineAndShareSucceeds_StillShowsNoOpenInMapsAppLink`.
rob force-pushed place-detail-panel-controls from fa237b055b
Some checks failed
CI / build (pull_request) Failing after 4m24s
CI / e2e (pull_request) Has been skipped
CI / container-images (pull_request) Has been skipped
to 8ce2f24314
All checks were successful
CI / build (pull_request) Successful in 9m47s
CI / container-images (pull_request) Successful in 16s
CI / e2e (pull_request) Successful in 9m4s
2026-08-20 07:18:38 +00:00
Compare
Author
Owner

Rebased onto merged #196 and pushed 8ce2f24 (force, history rewritten by the rebase; the reviewed head was fa237b05).

map.js:148 — reworded, along with the setView comment, which was wrong in the other direction once ZoomRequest came back.

Padding couplingpadding: var(--space-medium) is now asserted alongside width, and padding: var(--space-medium) var(--space-large) reddens it.

_shareFallbackLink clause — kept, now discriminating: PlaceDetailPanel_PointerIsFineAndShareSucceeds_StillShowsNoOpenInMapsAppLink over shared and copied. Your _shareStatus is not null mutation reddens exactly that test and nothing else. Re-derived rather than assumed: the fallback link is set on precisely the two outcomes whose message names the anchor, so the clause is coextensive with "a message naming the anchor is on screen", which is the property wanted. cancelled sets neither and correctly leaves the anchor hidden.

Pointer read moved to Home (your comment on ReadPointerTypeAsync) — taken rather than argued around. It reads in OnInitializedAsync alongside the three existing loads, so the answer lands before MapPlacesState can hold a selectable place; the panel is never rendered without it, and there is no transition to replay per selection. The panel takes bool? PointerIsCoarse. This also deletes the _layoutModule-after-await leak from the panel outright. HomeTests covers the read end to end — coarse, fine, and both failure clauses passing null down — and its constructor comment now names both readers going through matches, with the pointer query registered by argument over the catch-all.

Home becomes IAsyncDisposable. WithoutComments and the path helper are extracted to TestSupport/Stylesheet.cs; NavMenuStylesheetTests uses it too. Two comments rewrapped.

MapZoomRequest restored per the #198 ruling — the type, LeafletMap.ZoomRequest, ExplicitZoomLevel, _renderedZoomRequestId and all six LeafletMapTests cases. Deletion is now just the button, OnZoomToPlace, HandleZoomToPlace and Home's binding. ADR-0166 is amended to a partial supersession and says what survived and why; ADR-0132's status follows it to Accepted (partly superseded by 0166). Doc comments on both re-point to task 254. Whoever merges second here needs a rebuild, not just a rebase.

Placeholder interaction verified, not assumed. ThumbnailCropCalculator.ShrinkWidth is 240 and TargetCropHeight 135, and the placeholder takes its width from the content box, so both it and the settled thumbnail are exactly 240×135 — and the image is no longer scaled up past its own pixels, which it was at the old up-to-320px width. Pinned by a new assertion, since the two numbers appear in no file together.

Coverage floor lowered 91.5 → 91.4. Home gained the same three-line dispose-if-imported block Drawer has, and all three lines are unreachable from bUnit — confirmed against Drawer's own identical three uncovered lines rather than assumed. Reshaping the method to carry one fewer branch would have held the number; that was rejected as gaming it. Reasoning is in ADR-0168's Consequences.

WebUI 871 passing, Architecture 134, Domain 39, Contracts 139.

Rebased onto merged #196 and pushed `8ce2f24` (force, history rewritten by the rebase; the reviewed head was `fa237b05`). **map.js:148** — reworded, along with the `setView` comment, which was wrong in the other direction once `ZoomRequest` came back. **Padding coupling** — `padding: var(--space-medium)` is now asserted alongside `width`, and `padding: var(--space-medium) var(--space-large)` reddens it. **`_shareFallbackLink` clause** — kept, now discriminating: `PlaceDetailPanel_PointerIsFineAndShareSucceeds_StillShowsNoOpenInMapsAppLink` over `shared` and `copied`. Your `_shareStatus is not null` mutation reddens exactly that test and nothing else. Re-derived rather than assumed: the fallback link is set on precisely the two outcomes whose message names the anchor, so the clause is coextensive with "a message naming the anchor is on screen", which is the property wanted. `cancelled` sets neither and correctly leaves the anchor hidden. **Pointer read moved to `Home`** (your comment on `ReadPointerTypeAsync`) — taken rather than argued around. It reads in `OnInitializedAsync` alongside the three existing loads, so the answer lands before `MapPlacesState` can hold a selectable place; the panel is never rendered without it, and there is no transition to replay per selection. The panel takes `bool? PointerIsCoarse`. This also deletes the `_layoutModule`-after-`await` leak from the panel outright. `HomeTests` covers the read end to end — coarse, fine, and both failure clauses passing null down — and its constructor comment now names both readers going through `matches`, with the pointer query registered by argument over the catch-all. `Home` becomes `IAsyncDisposable`. `WithoutComments` and the path helper are extracted to `TestSupport/Stylesheet.cs`; `NavMenuStylesheetTests` uses it too. Two comments rewrapped. **`MapZoomRequest` restored** per the #198 ruling — the type, `LeafletMap.ZoomRequest`, `ExplicitZoomLevel`, `_renderedZoomRequestId` and all six `LeafletMapTests` cases. Deletion is now just the button, `OnZoomToPlace`, `HandleZoomToPlace` and `Home`'s binding. ADR-0166 is amended to a partial supersession and says what survived and why; ADR-0132's status follows it to `Accepted (partly superseded by 0166)`. Doc comments on both re-point to task 254. Whoever merges second here needs a rebuild, not just a rebase. **Placeholder interaction verified, not assumed.** `ThumbnailCropCalculator.ShrinkWidth` is 240 and `TargetCropHeight` 135, and the placeholder takes its width from the content box, so both it and the settled thumbnail are exactly 240×135 — and the image is no longer scaled up past its own pixels, which it was at the old up-to-320px width. Pinned by a new assertion, since the two numbers appear in no file together. **Coverage floor lowered 91.5 → 91.4.** `Home` gained the same three-line dispose-if-imported block `Drawer` has, and all three lines are unreachable from bUnit — confirmed against `Drawer`'s own identical three uncovered lines rather than assumed. Reshaping the method to carry one fewer branch would have held the number; that was rejected as gaming it. Reasoning is in ADR-0168's Consequences. WebUI 871 passing, Architecture 134, Domain 39, Contracts 139.
rob left a comment

Verdict: changes needed

Reviewed at 8ce2f24. The three blocking findings from fa237b05 are closed and I verified the discrimination claim on each: the _shareStatus is not null mutation now fails PlaceDetailPanel_PointerIsFineAndShareSucceeds_StillShowsNoOpenInMapsAppLink on both InlineData rows; widening padding to two values fails ..._KeepsThePaddingTheFixedWidthAddsBack; both map.js comments read correctly against the restored ZoomRequest. The 240×135 agreement holds — ThumbnailCropCalculator.ShrinkWidth is 240, TargetCropHeight 135, panel content box 240, and the panorama exception is already named in the CSS.

Two things to fix.

The coverage floor is not justified. await _layoutModule.DisposeAsync(); is not unreachable from bUnit — this test project drives DisposeComponentsAsync() in four places already (LeafletMapTests 190 and 200, ModalOverlayTests 55, PlaceDetailPanelTests 146), and LeafletMap_DisposeInteropFails_DoesNotThrow is the exact shape needed. Drawer's identical lines are uncovered because no DrawerTests case ever disposes a Drawer, not because bUnit cannot; ADR-0168 reads that as evidence of unreachability when it is evidence of the same gap. Add a Home_Disposed_... case, re-run --update, and move the floor only if 91.5 genuinely still cannot hold. 91.4 itself is fine as a number — check-coverage-ratchet.cs writes Math.Round(coverage, 1), so it is the script's own output and not a hand-rounded figure with slack in it. The placement in ADR-0168's Consequences is also fine: the block exists because of that record's ownership decision, and ADR-0080 wants the movement reviewed next to its cause.

ADR-0166's Consequences contradicts its own Decision — see inline.

Also, none of these blocking on their own: the Supersedes field wording, a stale CSS comment, and a HomeTests doc claim that overstates what the test discriminates (all inline). The _layoutModule-after-await leak is relocated rather than fixed — the window is now one page load instead of one per place selection, which is a real reduction, but ADR-0168's Consequences implies the panel-side move disposed of the problem rather than shrinking it. Stylesheet.WithoutComments is public with no caller outside Read.

EditorRequired on PointerIsCoarse: the conclusion is right, the stated reason is not quite. EditorRequired would not stop a host saying null explicitly — it would only stop one saying nothing. The real argument is the one in the following sentence: the degraded state is the documented safe state, so loudness buys nothing. Worth trimming the first clause.

Verdict: changes needed Reviewed at `8ce2f24`. The three blocking findings from `fa237b05` are closed and I verified the discrimination claim on each: the `_shareStatus is not null` mutation now fails `PlaceDetailPanel_PointerIsFineAndShareSucceeds_StillShowsNoOpenInMapsAppLink` on both `InlineData` rows; widening `padding` to two values fails `..._KeepsThePaddingTheFixedWidthAddsBack`; both `map.js` comments read correctly against the restored `ZoomRequest`. The 240×135 agreement holds — `ThumbnailCropCalculator.ShrinkWidth` is 240, `TargetCropHeight` 135, panel content box 240, and the panorama exception is already named in the CSS. Two things to fix. **The coverage floor is not justified.** `await _layoutModule.DisposeAsync();` is not unreachable from bUnit — this test project drives `DisposeComponentsAsync()` in four places already (`LeafletMapTests` 190 and 200, `ModalOverlayTests` 55, `PlaceDetailPanelTests` 146), and `LeafletMap_DisposeInteropFails_DoesNotThrow` is the exact shape needed. `Drawer`'s identical lines are uncovered because no `DrawerTests` case ever disposes a `Drawer`, not because bUnit cannot; ADR-0168 reads that as evidence of unreachability when it is evidence of the same gap. Add a `Home_Disposed_...` case, re-run `--update`, and move the floor only if 91.5 genuinely still cannot hold. 91.4 itself is fine as a number — `check-coverage-ratchet.cs` writes `Math.Round(coverage, 1)`, so it is the script's own output and not a hand-rounded figure with slack in it. The placement in ADR-0168's Consequences is also fine: the block exists *because* of that record's ownership decision, and ADR-0080 wants the movement reviewed next to its cause. **ADR-0166's Consequences contradicts its own Decision** — see inline. Also, none of these blocking on their own: the `Supersedes` field wording, a stale CSS comment, and a `HomeTests` doc claim that overstates what the test discriminates (all inline). The `_layoutModule`-after-`await` leak is relocated rather than fixed — the window is now one page load instead of one per place selection, which is a real reduction, but ADR-0168's Consequences implies the panel-side move disposed of the problem rather than shrinking it. `Stylesheet.WithoutComments` is `public` with no caller outside `Read`. `EditorRequired` on `PointerIsCoarse`: the conclusion is right, the stated reason is not quite. `EditorRequired` would not stop a host saying `null` explicitly — it would only stop one saying nothing. The real argument is the one in the following sentence: the degraded state is the documented safe state, so loudness buys nothing. Worth trimming the first clause.
@ -5,3 +5,3 @@
"PlaceMark.Domain": 100.0,
"PlaceMark.Infrastructure": 99.1,
"PlaceMark.WebUI": 91.5
"PlaceMark.WebUI": 91.4
Author
Owner

See the review body. One of the three lines is coverable today by the pattern this project already uses four times; the floor should not move until that has been tried and re-measured.

See the review body. One of the three lines is coverable today by the pattern this project already uses four times; the floor should not move until that has been tried and re-measured.
@ -0,0 +4,4 @@
- **Date:** 2026-08-20
- **Source:** Decided here, working Vikunja task 251, on Rob's own instruction: "drop 'Zoom to place'
entirely."
- **Supersedes:** [ADR-0132](0132-zoom-to-the-selected-place-from-an-explicit-control.md) — its
Author
Owner

Supersedes: claims a full supersession that the rest of this line then denies, and ADR-0132's reciprocal field says Partially superseded by. Should be Partially supersedes:, matching the 0142/0168 pairing. AdrFiles_Supersession_AgreesWithTheNamedRecordInBothDirections only checks the two name each other, so nothing catches the asymmetry.

`Supersedes:` claims a full supersession that the rest of this line then denies, and ADR-0132's reciprocal field says `Partially superseded by`. Should be `Partially supersedes:`, matching the 0142/0168 pairing. `AdrFiles_Supersession_AgreesWithTheNamedRecordInBothDirections` only checks the two name each other, so nothing catches the asymmetry.
@ -0,0 +105,4 @@
this returns to — with the difference that ADR-0130's clustering has since made a badge click a
genuine second route, which it was not when ADR-0101 was written.
**`LeafletMap`'s parameter surface shrinks back to what ADR-0101 and ADR-0133 left it.** `Centre`,
Author
Owner

Leftover from the first draft and now false. ZoomRequest stays, so the parameter surface does not shrink back, there is still a fourth exception to explain, and LeafletMap's doc comment got longer rather than shorter. Directly contradicts this record's own Decision and the Consequence three paragraphs down ("LeafletMap.ZoomRequest has no caller on this branch alone").

Leftover from the first draft and now false. `ZoomRequest` stays, so the parameter surface does not shrink back, there *is* still a fourth exception to explain, and `LeafletMap`'s doc comment got longer rather than shorter. Directly contradicts this record's own Decision and the Consequence three paragraphs down ("`LeafletMap.ZoomRequest` has no caller on this branch alone").
@ -111,0 +131,4 @@
button on a fine one so both carry the medium margin that opens this group of controls, rather than
the anchor alone carrying it as it did while the pair was always rendered together (task 226,
ADR-0142). The adjacent-sibling rule below restores the small inter-peer gap for the one case where
both do render: the pointer read has not landed yet, or Share has exhausted itself and its own
Author
Owner

"the pointer read has not landed yet" is no longer a state this panel has — the read moved to Home and completes before a place can be selected, which is what ADR-0168's Consequences means by "Neither is a transient render". The two remaining cases are: the host could not find out, and Share exhausted itself.

"the pointer read has not landed yet" is no longer a state this panel has — the read moved to `Home` and completes before a place can be selected, which is what ADR-0168's Consequences means by "Neither is a transient render". The two remaining cases are: the host could not find out, and Share exhausted itself.
@ -337,0 +349,4 @@
/// Task 255, ADR-0168: <c>Home</c> is what reads <c>(pointer: coarse)</c>, not the panel, so this is
/// where the read is proved end to end — the query going out, and the answer reaching the control
/// the panel actually renders from it. Reddens if the read is dropped from
/// <c>OnInitializedAsync</c>'s own await group, or if <c>PointerIsCoarse</c> stops being passed
Author
Owner

This half of the claim is not true. bUnit's planned invocations complete synchronously, so ReadPointerTypeAsync() has already set _pointerIsCoarse by the time Task.WhenAll is reached — drop pointerTask from the await group and nothing here reddens. Only "or if PointerIsCoarse stops being passed down" is actually guarded; say that, or drop the first clause.

This half of the claim is not true. bUnit's planned invocations complete synchronously, so `ReadPointerTypeAsync()` has already set `_pointerIsCoarse` by the time `Task.WhenAll` is reached — drop `pointerTask` from the await group and nothing here reddens. Only "or if `PointerIsCoarse` stops being passed down" is actually guarded; say that, or drop the first clause.
Measure the coverage floor instead of arguing it
All checks were successful
CI / build (pull_request) Successful in 3m16s
CI / container-images (pull_request) Successful in 4s
CI / e2e (pull_request) Successful in 3m54s
cf44a34a0d
Author
Owner

Pushed cf44a34.

1. Coverage floor — you were right that the justification was junk, and the corrected answer is still 91.4. The Drawer argument is gone. What I found instead:

  • await _layoutModule.DisposeAsync() was already covered before this round — bUnit disposes every rendered component at teardown regardless. It was never one of the three uncovered lines; those are the return at 362 and the catch/brace at 369/372.
  • I added Home_Disposed_ReleasesTheLayoutModuleWithoutThrowing anyway. It contributes zero covered lines (measured with and without) and cannot fail: with DisposeAsync mutated to throw outright it still passes, because DisposeComponentsAsync does not surface the exception to the caller. I deleted it rather than ship a test that cannot go red.
  • The real cause is that the measurement is bistable. Four identical runs give 2564 or 2565 of 2803, because PlaceFormPanel.RequestCloseAsync:587 — pre-existing, untouched here — is timing-dependent. That is 91.4734% or 91.5091%, reported as 91.4 or 91.5. main has the same flaky line (confirmed by running it there) but sits at 2550–2551 of 2786, so it reports 91.5 either way. This branch adds 17 coverable lines, which is enough to land on the boundary the flake straddles. At 91.5 the ratchet would fail roughly half of CI runs.

My earlier "91.5 held" was one lucky sample — I reported it after a single run. The two genuinely unreachable lines are now established by enumerating bUnit's API rather than by analogy: it refuses a throwing import handler outright (ArgumentException: Use one of the SetupModule() methods instead), every SetupModule overload succeeds, it records no invocation for a module reference's own DisposeAsync, and JSRuntimeUnhandledInvocationException derives from Exception rather than JSException. Flagging RequestCloseAsync:587 as the thing actually worth fixing; it is not this PR's code and the ratchet can be raised once it is.

2. ADR-0166 Consequences — rewritten. It now says the parameter surface does not change, that ZoomRequest is still the fourth exception, that the doc comment is the same length and only re-pointed, and that the shrinkage is in PlaceDetailPanel and Home instead. It also says plainly that the earlier claim was true of the first draft and survived the rewrite by inattention.

Non-blocking, all done. Partially supersedes:. CSS comment no longer names a state the panel has. Stylesheet.WithoutComments is private.

The HomeTests falsification claim was wrong and I have corrected it, not restated it. I ran your exact mutation — still call ReadPointerTypeAsync(), drop it from the WhenAll — and nothing goes red, for the reason you gave. The comment now names the three mutations that do redden it (no read, not passed down, answer inverted) and states that the await-group membership is uncovered, that a browser would differ, and that nothing in this suite can see it.

The leak: relocated, not fixed. Both versions assign the module after awaiting the import. What changed is frequency — once per page load instead of once per place selection. ADR-0168 now says exactly that, including why the _disposed flag that would actually close it is not written: nothing here can suspend a module import to prove such a guard works.

WebUI 871, Architecture 134, Domain 39, Contracts 139.

Pushed `cf44a34`. **1. Coverage floor — you were right that the justification was junk, and the corrected answer is still 91.4.** The `Drawer` argument is gone. What I found instead: - `await _layoutModule.DisposeAsync()` was **already covered** before this round — bUnit disposes every rendered component at teardown regardless. It was never one of the three uncovered lines; those are the `return` at 362 and the `catch`/brace at 369/372. - I added `Home_Disposed_ReleasesTheLayoutModuleWithoutThrowing` anyway. It contributes **zero** covered lines (measured with and without) and **cannot fail**: with `DisposeAsync` mutated to `throw` outright it still passes, because `DisposeComponentsAsync` does not surface the exception to the caller. I deleted it rather than ship a test that cannot go red. - The real cause is that **the measurement is bistable**. Four identical runs give 2564 **or** 2565 of 2803, because `PlaceFormPanel.RequestCloseAsync:587` — pre-existing, untouched here — is timing-dependent. That is 91.4734% or 91.5091%, reported as 91.4 or 91.5. `main` has the same flaky line (confirmed by running it there) but sits at 2550–2551 of 2786, so it reports 91.5 either way. This branch adds 17 coverable lines, which is enough to land on the boundary the flake straddles. At 91.5 the ratchet would fail roughly half of CI runs. My earlier "91.5 held" was one lucky sample — I reported it after a single run. The two genuinely unreachable lines are now established by enumerating bUnit's API rather than by analogy: it refuses a throwing `import` handler outright (`ArgumentException: Use one of the SetupModule() methods instead`), every `SetupModule` overload succeeds, it records no invocation for a module reference's own `DisposeAsync`, and `JSRuntimeUnhandledInvocationException` derives from `Exception` rather than `JSException`. Flagging `RequestCloseAsync:587` as the thing actually worth fixing; it is not this PR's code and the ratchet can be raised once it is. **2. ADR-0166 Consequences** — rewritten. It now says the parameter surface does not change, that `ZoomRequest` is still the fourth exception, that the doc comment is the same length and only re-pointed, and that the shrinkage is in `PlaceDetailPanel` and `Home` instead. It also says plainly that the earlier claim was true of the first draft and survived the rewrite by inattention. **Non-blocking, all done.** `Partially supersedes:`. CSS comment no longer names a state the panel has. `Stylesheet.WithoutComments` is private. **The `HomeTests` falsification claim was wrong and I have corrected it, not restated it.** I ran your exact mutation — still call `ReadPointerTypeAsync()`, drop it from the `WhenAll` — and nothing goes red, for the reason you gave. The comment now names the three mutations that *do* redden it (no read, not passed down, answer inverted) and states that the await-group membership is uncovered, that a browser would differ, and that nothing in this suite can see it. **The leak: relocated, not fixed.** Both versions assign the module after `await`ing the import. What changed is frequency — once per page load instead of once per place selection. ADR-0168 now says exactly that, including why the `_disposed` flag that would actually close it is not written: nothing here can suspend a module import to prove such a guard works. WebUI 871, Architecture 134, Domain 39, Contracts 139.
rob left a comment

Verdict: changes needed

Two things, both documentation, both small. The code is done.

The PR description still carries the retracted claim. "Home gains the same three-line dispose-if-imported block Drawer has, all three lines unreachable from bUnit" is the account ADR-0168 now names as the first of two wrong ones. This is what the merger reads and what the merge commit quotes. Replace it with the bistability reason.

The line arithmetic does not close — see inline. 2803−2786 = 17 coverable, 2564−2550 = 14 covered (and 2565−2551 = 14), so three of the seventeen are uncovered, and two are accounted for.


On the two questions you asked me to judge rather than just check:

Deleting Home_Disposed_… was right, and my round-2 instruction to add it was wrong. I asserted reachability from the presence of DisposeComponentsAsync elsewhere in the suite without establishing that the line was uncovered in the first place, which is the same error I had just charged the Drawer argument with. A test that still passes with DisposeAsync mutated to throw is not a weak test, it is a false statement about what is guarded, and this project's standard is explicit that such a test does not earn its place. Deleting it and recording why is the better outcome than the change I asked for.

91.4 is the right floor. The flake is real: PlaceFormPanel.RequestCloseAsync:587 is the return inside if (_importing), reached only if a dismiss lands while a background-thread upload is still in flight — PlaceFormPanel_CsvImportInFlight_CloseButtonClickIsANoOp races exactly that, and PlaceFormPanel_DisposedMidImport_…'s own remarks already document scheduling gaps in the same area. It is not this PR's code; PlaceFormPanel.razor.cs is not in the diff. The figures are self-consistent and only work under reportgenerator's truncation to one decimal, which is what makes 2564 report as 91.4 while main's 2550–2551 both report 91.5 — a model I would not have expected the numbers to fit if they had been reverse-engineered to suit. The margin is nine thousandths of a point, one covered line. Holding 91.5 against that means a ratchet that fails about half its runs, which trains people to force-merge past it; that is a worse failure than a floor sitting 0.1 low with a ticket and an ADR paragraph saying when to raise it. The bUnit enumeration is concrete and falsifiable rather than argued by analogy, which is the improvement that matters most here.

Everything else from round 2 is closed and verified: ADR-0166's Consequences now agrees with its own Decision and names its own error; Partially supersedes:, the CSS comment and Stylesheet.WithoutComments are all done; the three mutations HomeTests now claims all genuinely redden Home_PlaceSelectedOnACoarsePointer_…; and ADR-0168 is straight about the orphaned-reference window being relocated rather than closed, and about why the _disposed flag is not written.

Verdict: changes needed Two things, both documentation, both small. The code is done. **The PR description still carries the retracted claim.** "`Home` gains the same three-line dispose-if-imported block `Drawer` has, all three lines unreachable from bUnit" is the account ADR-0168 now names as the first of two wrong ones. This is what the merger reads and what the merge commit quotes. Replace it with the bistability reason. **The line arithmetic does not close** — see inline. 2803−2786 = 17 coverable, 2564−2550 = 14 covered (and 2565−2551 = 14), so three of the seventeen are uncovered, and two are accounted for. --- On the two questions you asked me to judge rather than just check: **Deleting `Home_Disposed_…` was right, and my round-2 instruction to add it was wrong.** I asserted reachability from the presence of `DisposeComponentsAsync` elsewhere in the suite without establishing that the line was uncovered in the first place, which is the same error I had just charged the `Drawer` argument with. A test that still passes with `DisposeAsync` mutated to throw is not a weak test, it is a false statement about what is guarded, and this project's standard is explicit that such a test does not earn its place. Deleting it and recording why is the better outcome than the change I asked for. **91.4 is the right floor.** The flake is real: `PlaceFormPanel.RequestCloseAsync:587` is the `return` inside `if (_importing)`, reached only if a dismiss lands while a background-thread upload is still in flight — `PlaceFormPanel_CsvImportInFlight_CloseButtonClickIsANoOp` races exactly that, and `PlaceFormPanel_DisposedMidImport_…`'s own remarks already document scheduling gaps in the same area. It is not this PR's code; `PlaceFormPanel.razor.cs` is not in the diff. The figures are self-consistent and only work under reportgenerator's truncation to one decimal, which is what makes 2564 report as 91.4 while `main`'s 2550–2551 both report 91.5 — a model I would not have expected the numbers to fit if they had been reverse-engineered to suit. The margin is nine thousandths of a point, one covered line. Holding 91.5 against that means a ratchet that fails about half its runs, which trains people to force-merge past it; that is a worse failure than a floor sitting 0.1 low with a ticket and an ADR paragraph saying when to raise it. The bUnit enumeration is concrete and falsifiable rather than argued by analogy, which is the improvement that matters most here. Everything else from round 2 is closed and verified: ADR-0166's Consequences now agrees with its own Decision and names its own error; `Partially supersedes:`, the CSS comment and `Stylesheet.WithoutComments` are all done; the three mutations `HomeTests` now claims all genuinely redden `Home_PlaceSelectedOnACoarsePointer_…`; and ADR-0168 is straight about the orphaned-reference window being relocated rather than closed, and about why the `_disposed` flag is not written.
@ -0,0 +175,4 @@
pre-existing, untouched here, and timing-dependent — is covered on some runs and not others. That is
91.4734% or 91.5091%, which reportgenerator reports as 91.4 or 91.5. `main` has the same flaky line,
confirmed by running it there too, but sits at 25502551 of 2786 and so reports 91.5 either way. This
branch adds 17 coverable lines, two of them genuinely unreachable, which is enough to move the value
Author
Owner

The counts in this paragraph imply a third uncovered line that the next paragraph does not account for: 2803 − 2786 = 17 coverable, and 2564 − 2550 = 14 covered (2565 − 2551 likewise), so three of the seventeen are uncovered, not two. Everything deleted here — OnZoomToPlace, HandleZoomToPlace, _zoomRequest — was covered before, so the deletions do not absorb it.

Probably innocent (coverlet counts a catch clause line and its closing brace separately, which would make the dispose block three uncovered lines rather than two). But this figure has now been wrong twice, and the paragraph below sets the standard itself: checked rather than assumed. Name the third, or correct the count.

The counts in this paragraph imply a third uncovered line that the next paragraph does not account for: 2803 − 2786 = 17 coverable, and 2564 − 2550 = 14 covered (2565 − 2551 likewise), so three of the seventeen are uncovered, not two. Everything deleted here — `OnZoomToPlace`, `HandleZoomToPlace`, `_zoomRequest` — was covered before, so the deletions do not absorb it. Probably innocent (coverlet counts a `catch` clause line and its closing brace separately, which would make the dispose block three uncovered lines rather than two). But this figure has now been wrong twice, and the paragraph below sets the standard itself: checked rather than assumed. Name the third, or correct the count.
rob force-pushed place-detail-panel-controls from cf44a34a0d
All checks were successful
CI / build (pull_request) Successful in 3m16s
CI / container-images (pull_request) Successful in 4s
CI / e2e (pull_request) Successful in 3m54s
to 883ebc56f5
All checks were successful
CI / build (pull_request) Successful in 4m5s
CI / container-images (pull_request) Successful in 4s
CI / e2e (pull_request) Successful in 4m46s
2026-08-20 07:57:29 +00:00
Compare
Author
Owner

Pushed 883ebc5 (force — squashed to one commit so the #199 rebase replayed once against the final tree rather than three times against intermediate states). CI #819 green on this head, 8m53s. #817 was green on the previous head too.

1. PR description corrected. It no longer claims three unreachable lines; it says the floor moved because the measurement is bistable, names the three lines, and distinguishes the two that are unreachable from the third that is the same catch counted twice. Task 261 named as owning the way back to 91.5, in the description and in ADR-0168.

2. Line arithmetic closes, measured per file. Three runs a side, unioned so the flaky line counts as covered on both:

file main branch delta
PlaceDetailPanel.razor 17/17 19/19 +2 covered, +2 coverable
PlaceDetailPanel.razor.cs 54/67 56/69 +2, +2
Home.razor.cs 54/54 64/67 +10, +13
total 2558/2793 2572/2810 +14, +17

So three uncovered, all in Home.DisposeAsync: the "nothing was imported" return, the catch (JSDisconnectedException), and that clause's closing brace. Your guess was right — coverlet counts a catch and its brace separately, which is the line the ADR was missing. LeafletMap.razor.cs is byte-identical to main's and moves neither figure.

Rebase onto #199. Conflicted on LeafletMap.razor.cs when replayed commit-by-commit, purely because my first commit deleted ZoomRequest and a later one restored it, and #199 inserted SelectedMarkerId immediately after. Squashing first made it clean. Verified rather than assumed: LeafletMap carries both SelectedMarkerId and ZoomRequest with separate reconciliation and _rendered… fields; Home.razor binds SelectedMarkerId and PointerIsCoarse; map.js has both #199's marker add handler and my two corrected comments. Rebuilt from clean — 0 warnings, 0 errors.

Baseline re-derived against the new denominator, not carried over. #199 moved WebUI to 2793 coverable on main and 2810 on this branch. Five samples here: 2571 four times, 2572 once — 91.4947% or 91.5302%, reported 91.4 or 91.5. main is 2557–2558 of 2793 and reports 91.5 either way (watched line 587 flip across four main samples). 91.4 stands, now measured post-rebase.

WebUI 880, Architecture 134, Domain 39, Contracts 139.

Pushed `883ebc5` (force — squashed to one commit so the #199 rebase replayed once against the final tree rather than three times against intermediate states). CI **#819 green** on this head, 8m53s. #817 was green on the previous head too. **1. PR description corrected.** It no longer claims three unreachable lines; it says the floor moved because the measurement is bistable, names the three lines, and distinguishes the two that are unreachable from the third that is the same `catch` counted twice. Task 261 named as owning the way back to 91.5, in the description and in ADR-0168. **2. Line arithmetic closes, measured per file.** Three runs a side, unioned so the flaky line counts as covered on both: | file | main | branch | delta | | --- | --- | --- | --- | | `PlaceDetailPanel.razor` | 17/17 | 19/19 | +2 covered, +2 coverable | | `PlaceDetailPanel.razor.cs` | 54/67 | 56/69 | +2, +2 | | `Home.razor.cs` | 54/54 | 64/67 | +10, +13 | | **total** | **2558/2793** | **2572/2810** | **+14, +17** | So three uncovered, all in `Home.DisposeAsync`: the "nothing was imported" `return`, the `catch (JSDisconnectedException)`, and that clause's closing brace. Your guess was right — coverlet counts a `catch` and its brace separately, which is the line the ADR was missing. `LeafletMap.razor.cs` is byte-identical to main's and moves neither figure. **Rebase onto #199.** Conflicted on `LeafletMap.razor.cs` when replayed commit-by-commit, purely because my first commit deleted `ZoomRequest` and a later one restored it, and #199 inserted `SelectedMarkerId` immediately after. Squashing first made it clean. Verified rather than assumed: `LeafletMap` carries both `SelectedMarkerId` and `ZoomRequest` with separate reconciliation and `_rendered…` fields; `Home.razor` binds `SelectedMarkerId` and `PointerIsCoarse`; `map.js` has both #199's marker `add` handler and my two corrected comments. Rebuilt from clean — 0 warnings, 0 errors. **Baseline re-derived against the new denominator, not carried over.** #199 moved WebUI to 2793 coverable on main and 2810 on this branch. Five samples here: 2571 four times, 2572 once — 91.4947% or 91.5302%, reported 91.4 or 91.5. `main` is 2557–2558 of 2793 and reports 91.5 either way (watched line 587 flip across four main samples). 91.4 stands, now measured post-rebase. WebUI 880, Architecture 134, Domain 39, Contracts 139.
rob left a comment

Verdict: mergeable

Reviewed at 883ebc5 against main at 10ce610. Both round-3 items are closed and nothing new.

The squash-rebase preserved everything. Checked on the merged result, not the process. LeafletMap carries ZoomRequest and SelectedMarkerId as separate parameters, with separate _renderedZoomRequestId and _renderedSelectedMarkerId fields, separate reconciliation blocks in OnParametersSetAsync, and both seeded on the first-render path after createMap — including #199's "only call setSelectedMarker when something is already selected at mount" guard. Home.razor binds SelectedMarkerId and PointerIsCoarse, and #199's SelectedMarkerId paragraph in the comment block survives alongside the removal of the ZoomRequest one. map.js keeps setSelectedMarker, applySelectionRing and the marker add handler, and the diff against main touches only the two comments — their hunk headers having moved from 145/656 to 156/671 is itself the evidence that #199's insertions are sitting under them. The decisive check is that the whole diff against the new main contains no deletion of anything #199 added; it does not.

The arithmetic closes, third time. 2571/2810 = 91.4947% and 2572/2810 = 91.5302%, so the branch straddles; main at 2557 and 2558 of 2793 gives 91.5503% and 91.5861%, both 91.5, so it does not. Deltas are +17 coverable and +14 covered on either pairing, and the per-file table sums to exactly that (2+2+13 and 2+2+10). The three uncovered are named and the brace is the one the earlier draft missed. Against main these figures are +7/+7 on both sides versus the last measurement, which is what #199 landing fully covered should look like — the two measurements corroborate each other rather than merely agreeing.

91.4 is more clearly right than it was last round, not less: four of five samples were the low value and the good one clears the truncation boundary by 0.005 points, so holding 91.5 would redden most runs, not half.

On the setView comment and #198 — the concern is stale, and neither PR needs to own it. It described this branch at fa237b05, before ZoomRequest was restored. The comment here now reads "Reached from a Centre parameter change (LeafletMap.razor.cs) and from ZoomRequest", which is already true after #198 lands. The only remaining absolute is "the only thing a place selection reaches at all", and #198's control is not a place selection, so that stays true too. Don't hold #198 for this.

The rebuild warning in the description is the right thing to carry into the merge, and still applies to whichever of this and #198 goes second.

Verdict: mergeable Reviewed at `883ebc5` against `main` at `10ce610`. Both round-3 items are closed and nothing new. **The squash-rebase preserved everything.** Checked on the merged result, not the process. `LeafletMap` carries `ZoomRequest` and `SelectedMarkerId` as separate parameters, with separate `_renderedZoomRequestId` and `_renderedSelectedMarkerId` fields, separate reconciliation blocks in `OnParametersSetAsync`, and both seeded on the first-render path after `createMap` — including #199's "only call `setSelectedMarker` when something is already selected at mount" guard. `Home.razor` binds `SelectedMarkerId` and `PointerIsCoarse`, and #199's `SelectedMarkerId` paragraph in the comment block survives alongside the removal of the `ZoomRequest` one. `map.js` keeps `setSelectedMarker`, `applySelectionRing` and the marker `add` handler, and the diff against `main` touches only the two comments — their hunk headers having moved from 145/656 to 156/671 is itself the evidence that #199's insertions are sitting under them. The decisive check is that the whole diff against the new `main` contains no deletion of anything #199 added; it does not. **The arithmetic closes, third time.** 2571/2810 = 91.4947% and 2572/2810 = 91.5302%, so the branch straddles; `main` at 2557 and 2558 of 2793 gives 91.5503% and 91.5861%, both 91.5, so it does not. Deltas are +17 coverable and +14 covered on either pairing, and the per-file table sums to exactly that (2+2+13 and 2+2+10). The three uncovered are named and the brace is the one the earlier draft missed. Against `main` these figures are +7/+7 on both sides versus the last measurement, which is what #199 landing fully covered should look like — the two measurements corroborate each other rather than merely agreeing. 91.4 is more clearly right than it was last round, not less: four of five samples were the low value and the good one clears the truncation boundary by 0.005 points, so holding 91.5 would redden most runs, not half. **On the `setView` comment and #198 — the concern is stale, and neither PR needs to own it.** It described this branch at `fa237b05`, before `ZoomRequest` was restored. The comment here now reads "Reached from a `Centre` parameter change (`LeafletMap.razor.cs`) and from `ZoomRequest`", which is already true after #198 lands. The only remaining absolute is "the only thing a place *selection* reaches at all", and #198's control is not a place selection, so that stays true too. Don't hold #198 for this. The rebuild warning in the description is the right thing to carry into the merge, and still applies to whichever of this and #198 goes second.
rob merged commit 738f720415 into main 2026-08-20 08:11:32 +00:00
rob deleted branch place-detail-panel-controls 2026-08-20 08:11:32 +00:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
1 participant
Notifications
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!197
No description provided.