Refuse a reseed once the reader has supplied their own value #211

Merged

rob merged 2 commits from reseed-preserves-typed-coordinates into main 2026-08-21 14:48:59 +00:00

Conversation 2 Commits 2 Files changed 3 +168 -20

rob commented 2026-08-21 14:26:57 +00:00

Owner

Copy link

Closes task 210. Defensive: neither hazard is reachable through the application today, and this lands before the change that would make them live rather than after.

The hazard. PlaceForm.OnParametersSet's reseed paths overwrote reader-editable fields and their IsDirty baseline together, atomically. A reseed arriving after the reader typed or picked a value would have discarded their input and left IsDirty reading false — no prompt, no signal. Worse than the behaviour task 209 fixed, which at least still reported dirty.

Both reseed paths are covered, after an audit rather than a spot fix. The ticket named one; review found the second; the audit established there is no third.

Path	Reader-editable fields written	Gate keeping it unreachable	Guard
SeedFrom (InitialCoordinates) — a second map click relocating an open add panel	_latitude, _longitude	Home.razor's LeafletMap.Inert="OverlayFocus.IsOpen"	CoordinateFieldsMatchLastSeed
SeedFromPartial (InitialValues) — a second import-repair seed (ADR-0135)	_name, _description, _latitude, _longitude	ImportReportBanner.razor's inert="@OverlayFocus.IsOpen"	InitialValueFieldsMatchLastSeed

Edit mode never re-seeds: OnParametersSet returns early whenever Place is not null, so a changed Place after mount is inert by construction. DefaultGroupId, Groups and CallerRole do not feed a seeded field.

Decision: refuse to reseed, rather than reseed-and-stay-dirty. The ticket's bar was that silently winning is not an option; refusing means the reader's value is never overwritten in the first place. Neither guard needed new state — each re-derives "has the reader supplied something" from the existing seed baseline, the same comparison IsDirty already makes, with NormalizeDescription on the description leg to match. Reusing it rather than adding a parallel "was this reader-supplied" flag follows ADR-0126's own reasoning for rejecting a duplicate field for the group selector. Both refusals are all-or-nothing across the fields the path writes, because both write them atomically.

Reachability verified, not inherited. Traced on current main: ModalOverlay calls OverlayFocus.EnterAsync synchronously — adding to _openCallers and raising Changed before any await — and ExitAsync on dispose; both consumers' inert bindings read OverlayFocus.IsOpen; no commit since ADR-0126 weakens any of it.

Why now. OverlayFocusTrap is the single shared mechanism behind both gates. Task 197 is an open, active investigation into exactly that trap's release timing — six documented failed attempts, ADR-0119 — and a fix there could move when IsOpen flips for every consumer. That is the change the ticket warned "would arrive looking unrelated". Neither guard depends on the gate, so both stay correct however task 197 resolves.

Watched to fail, per guard. Reverting the coordinate guard makes LatitudeFieldValue read 48.8566 instead of the picked 40.7128. Reverting the repair-seed guard makes the name read "Big Ben" instead of the typed value. Each restored, with 68/68 PlaceFormTests, 916/920 WebUI (4 pre-existing skips) and the architecture suite green.

What the tests do and do not prove. Both drive PlaceForm directly through bUnit and assert the reader's value survives a reseed with IsDirty still true — the repair-seed test also asserts a still-blank latitude survives, proving the refusal is atomic rather than per-field. They prove the guards work. They do not prove any browser path reaches these states, because none does.

ADR-0126 is annotated per ADR-0170's correction shape at the Consequences paragraph that said the hazard was "not this ticket's to close"; its Decision section is untouched, since IsDirty's formula did not change. ADR-0135 is deliberately not annotated: it explains the baseline reuse but never claimed the repair-seed path was safe against this hazard, so there is no false statement there to correct — the code and tests are the record.

Closes task 210. **Defensive: neither hazard is reachable through the application today**, and this lands before the change that would make them live rather than after. **The hazard.** `PlaceForm.OnParametersSet`'s reseed paths overwrote reader-editable fields *and* their `IsDirty` baseline together, atomically. A reseed arriving after the reader typed or picked a value would have **discarded their input and left `IsDirty` reading `false`** — no prompt, no signal. Worse than the behaviour task 209 fixed, which at least still reported dirty. **Both reseed paths are covered, after an audit rather than a spot fix.** The ticket named one; review found the second; the audit established there is no third. | Path | Reader-editable fields written | Gate keeping it unreachable | Guard | |---|---|---|---| | `SeedFrom` (`InitialCoordinates`) — a second map click relocating an open add panel | `_latitude`, `_longitude` | `Home.razor`'s `LeafletMap.Inert="OverlayFocus.IsOpen"` | `CoordinateFieldsMatchLastSeed` | | `SeedFromPartial` (`InitialValues`) — a second import-repair seed (ADR-0135) | `_name`, `_description`, `_latitude`, `_longitude` | `ImportReportBanner.razor`'s `inert="@OverlayFocus.IsOpen"` | `InitialValueFieldsMatchLastSeed` | Edit mode never re-seeds: `OnParametersSet` returns early whenever `Place is not null`, so a changed `Place` after mount is inert by construction. `DefaultGroupId`, `Groups` and `CallerRole` do not feed a seeded field. **Decision: refuse to reseed, rather than reseed-and-stay-dirty.** The ticket's bar was that silently winning is not an option; refusing means the reader's value is never overwritten in the first place. Neither guard needed new state — each re-derives "has the reader supplied something" from the existing seed baseline, the same comparison `IsDirty` already makes, with `NormalizeDescription` on the description leg to match. Reusing it rather than adding a parallel "was this reader-supplied" flag follows ADR-0126's own reasoning for rejecting a duplicate field for the group selector. Both refusals are **all-or-nothing across the fields the path writes**, because both write them atomically. **Reachability verified, not inherited.** Traced on current `main`: `ModalOverlay` calls `OverlayFocus.EnterAsync` synchronously — adding to `_openCallers` and raising `Changed` before any `await` — and `ExitAsync` on dispose; both consumers' `inert` bindings read `OverlayFocus.IsOpen`; no commit since ADR-0126 weakens any of it. **Why now.** `OverlayFocusTrap` is the single shared mechanism behind both gates. **Task 197 is an open, active investigation into exactly that trap's release timing** — six documented failed attempts, ADR-0119 — and a fix there could move when `IsOpen` flips for every consumer. That is the change the ticket warned "would arrive looking unrelated". Neither guard depends on the gate, so both stay correct however task 197 resolves. **Watched to fail, per guard.** Reverting the coordinate guard makes `LatitudeFieldValue` read `48.8566` instead of the picked `40.7128`. Reverting the repair-seed guard makes the name read `"Big Ben"` instead of the typed value. Each restored, with 68/68 `PlaceFormTests`, 916/920 WebUI (4 pre-existing skips) and the architecture suite green. **What the tests do and do not prove.** Both drive `PlaceForm` directly through bUnit and assert the reader's value survives a reseed with `IsDirty` still true — the repair-seed test also asserts a still-blank latitude survives, proving the refusal is atomic rather than per-field. They prove the guards work. They do **not** prove any browser path reaches these states, because none does. ADR-0126 is annotated per ADR-0170's correction shape at the Consequences paragraph that said the hazard was "not this ticket's to close"; its Decision section is untouched, since `IsDirty`'s formula did not change. ADR-0135 is deliberately **not** annotated: it explains the baseline reuse but never claimed the repair-seed path was safe against this hazard, so there is no false statement there to correct — the code and tests are the record.

rob added 1 commit 2026-08-21 14:26:57 +00:00

Refuse a coordinate reseed once the reader has supplied their own 90868978b3

rob reviewed 2026-08-21 14:34:58 +00:00

rob left a comment

Copy link

Verdict: changes needed

Verified independently against the pinned SDK (10.0.100): the guard is a correct reuse of IsDirty's own coordinate comparison — traced through IsDirty, confirmed the two predicates cannot diverge while InitialCoordinates is the active seed (mutually exclusive with InitialValues, so the coalescing collapses to exactly this pair). Reverted the guard to unconditional SeedFrom and reran the new test: it reddens exactly as claimed (LatitudeFieldValue reads 48.8566 not 40.7128), restored and reconfirmed 67/67 PlaceFormTests green. AdrFiles_Corrected_HasAMatchingInlineMarkerInBothDirections passes; the correction sits in Consequences, Decision untouched, as claimed. Reachability trace checks out: OverlayFocusTrap.EnterAsync does add-to-set-and-fire-Changed before its first await, matching the PR body's claim. No AI attribution, no stray ADR number, no American spellings in the diff.

One real gap, inline: SeedFromPartial's own reseed path (the InitialValues/import-repair seed, ADR-0135) has the exact same silently-discards-and-clears-IsDirty shape, gated by the same shared OverlayFocusTrap this PR's own "why now" argument is about — and it's untouched and unrecorded. Given this project's own standard for not losing a found-but-unfixed hazard (task 210 exists for exactly this reason), that needs either the same guard or an explicit record before this merges.

Verdict: changes needed Verified independently against the pinned SDK (10.0.100): the guard is a correct reuse of `IsDirty`'s own coordinate comparison — traced through `IsDirty`, confirmed the two predicates cannot diverge while `InitialCoordinates` is the active seed (mutually exclusive with `InitialValues`, so the coalescing collapses to exactly this pair). Reverted the guard to unconditional `SeedFrom` and reran the new test: it reddens exactly as claimed (`LatitudeFieldValue` reads `48.8566` not `40.7128`), restored and reconfirmed 67/67 `PlaceFormTests` green. `AdrFiles_Corrected_HasAMatchingInlineMarkerInBothDirections` passes; the correction sits in Consequences, Decision untouched, as claimed. Reachability trace checks out: `OverlayFocusTrap.EnterAsync` does add-to-set-and-fire-`Changed` before its first `await`, matching the PR body's claim. No AI attribution, no stray ADR number, no American spellings in the diff. One real gap, inline: `SeedFromPartial`'s own reseed path (the `InitialValues`/import-repair seed, ADR-0135) has the exact same silently-discards-and-clears-`IsDirty` shape, gated by the same shared `OverlayFocusTrap` this PR's own \"why now\" argument is about — and it's untouched and unrecorded. Given this project's own standard for not losing a found-but-unfixed hazard (task 210 exists for exactly this reason), that needs either the same guard or an explicit record before this merges.

src/PlaceMark.WebUI/Places/PlaceForm.razor.cs

rob commented 2026-08-21 14:34:58 +00:00

Author

Owner

Copy link

This branch — SeedFromPartial for the InitialValues/repair-seed path — has the identical unconditional-overwrite shape task 210 just closed for InitialCoordinates: a second reseed here would still silently discard _name/_description/coordinates the reader had already typed and clear IsDirty in the same step. It's gated by the same OverlayFocusTrap: ImportReportBanner.razor puts .import-report-banner (all its own Repair buttons) inert="@OverlayFocus.IsOpen", so it's exactly as unreachable today, and exactly as exposed to task 197's timing change, as the coordinate path this PR fixes. Left unguarded here and not named anywhere (ADR-0135 has no note of it). Either apply the same CoordinateFieldsMatchLastSeed-style guard to this branch, or record it explicitly (a task-210-shaped ticket, plus an ADR-0135 consequence) so it isn't silently lost — the thing this whole ticket chain exists to avoid.

This branch — `SeedFromPartial` for the `InitialValues`/repair-seed path — has the identical unconditional-overwrite shape task 210 just closed for `InitialCoordinates`: a second reseed here would still silently discard `_name`/`_description`/coordinates the reader had already typed and clear `IsDirty` in the same step. It's gated by the same `OverlayFocusTrap`: `ImportReportBanner.razor` puts `.import-report-banner` (all its own Repair buttons) `inert="@OverlayFocus.IsOpen"`, so it's exactly as unreachable today, and exactly as exposed to task 197's timing change, as the coordinate path this PR fixes. Left unguarded here and not named anywhere (ADR-0135 has no note of it). Either apply the same `CoordinateFieldsMatchLastSeed`-style guard to this branch, or record it explicitly (a task-210-shaped ticket, plus an ADR-0135 consequence) so it isn't silently lost — the thing this whole ticket chain exists to avoid.

rob added 1 commit 2026-08-21 14:40:48 +00:00

Guard the repair-seed reseed path the same way as the coordinate one 8247dea99f

rob changed title from Refuse a coordinate reseed once the reader has supplied their own to Refuse a reseed once the reader has supplied their own value 2026-08-21 14:41:09 +00:00

rob reviewed 2026-08-21 14:45:12 +00:00

rob left a comment

Copy link

Verdict: mergeable

Re-reviewed at 8247dea, superseding the changes needed verdict at 9086897.

Completeness of the audit verified directly, not accepted on account: every write site of _name/_description/_latitude/_longitude/_selectedGroupId in PlaceForm.razor.cs grepped and checked — the only parameter-driven writes are OnInitialized's once-only seed (edit mode and create mode alike) and the two OnParametersSet reseed branches, both now guarded; HandlePick/HandleCurrentLocation/HandleGroupChanged are all reader-driven, not parameter-driven. No @code block in PlaceForm.razor and no other lifecycle override exists, so there's no fourth place to check. "Edit mode never re-seeds" holds: OnParametersSet returns before either branch whenever Place is not null.

InitialValueFieldsMatchLastSeed re-derived against IsDirty's own coalescing for the InitialValues path (Place null, so Place?.X ?? _seededInitialCoordinates?.X ?? _seededInitialValues?.X collapses to _seededInitialValues?.X on every term) — exact match, including NormalizeDescription on the description leg and the _name == (_seededInitialValues?.Name ?? string.Empty) handling of a seed whose own Name was null. No divergence.

Mutated InitialValueFieldsMatchLastSeed's guard back to unconditional SeedFromPartial under the pinned SDK (10.0.100): reddens exactly as claimed, name reads "Big Ben" not the typed value; restored and confirmed clean. The new test's own second assertion (latitude stays blank rather than picking up the new seed's 51.5007) is what actually discriminates atomic-vs-per-field refusal, not just presence-of-a-guard — read the implementation to confirm it is a single &&-chain across all four fields, matching that assertion.

Full local run, pinned SDK, solution built first: dotnet build PlaceMark.slnx clean (0 warnings, 0 errors); 68/68 PlaceFormTests; 916/920 WebUI (4 pre-existing, unrelated skips — not E2EFactAttribute); 148/148 architecture tests, including AdrFiles_Corrected_HasAMatchingInlineMarkerInBothDirections. ADR-0126's own diff is unchanged from the previous round — Decision section untouched, correction still confined to Consequences.

Atomicity (point 3): right call. SeedFromPartial already writes all four fields as one construction-time unit; a per-field refusal would need to track provenance per field independently — exactly the second-tracking state the whole design (reuse IsDirty's comparison, add nothing) was chosen to avoid — and would leave _seededInitialValues describing no coherent single seed. The hypothetical "reader edited only the name, might want a fresh latitude" is a convenience nobody asked for, traded against new state and a real risk of the kind of drift task 210 exists to close.

ADR-0135 (point 4): read fully. Its only reseed-related claim (Consequences, "dismissing the repair form and clicking 'Repair' again reseeds identically") is about a full unmount/remount after dismissal, proved by a different test entirely (ImportReportBanner_RepairDismissedAfterTypingThenReopened...) — it says nothing about a second seed landing on an already-mounted, still-open instance. No false statement to correct; declining to annotate is correct.

CI: local build/test run is the load-bearing verification here; run #888 for 8247dea was still in progress at review time with nothing in #887 (the stale, superseded run) to suggest a discrepancy.

Verdict: mergeable Re-reviewed at `8247dea`, superseding the `changes needed` verdict at `9086897`. Completeness of the audit verified directly, not accepted on account: every write site of `_name`/`_description`/`_latitude`/`_longitude`/`_selectedGroupId` in `PlaceForm.razor.cs` grepped and checked — the only parameter-driven writes are `OnInitialized`'s once-only seed (edit mode and create mode alike) and the two `OnParametersSet` reseed branches, both now guarded; `HandlePick`/`HandleCurrentLocation`/`HandleGroupChanged` are all reader-driven, not parameter-driven. No `@code` block in `PlaceForm.razor` and no other lifecycle override exists, so there's no fourth place to check. "Edit mode never re-seeds" holds: `OnParametersSet` returns before either branch whenever `Place is not null`. `InitialValueFieldsMatchLastSeed` re-derived against `IsDirty`'s own coalescing for the `InitialValues` path (Place null, so `Place?.X ?? _seededInitialCoordinates?.X ?? _seededInitialValues?.X` collapses to `_seededInitialValues?.X` on every term) — exact match, including `NormalizeDescription` on the description leg and the `_name == (_seededInitialValues?.Name ?? string.Empty)` handling of a seed whose own `Name` was null. No divergence. Mutated `InitialValueFieldsMatchLastSeed`'s guard back to unconditional `SeedFromPartial` under the pinned SDK (10.0.100): reddens exactly as claimed, name reads `"Big Ben"` not the typed value; restored and confirmed clean. The new test's own second assertion (latitude stays blank rather than picking up the new seed's `51.5007`) is what actually discriminates atomic-vs-per-field refusal, not just presence-of-a-guard — read the implementation to confirm it is a single `&&`-chain across all four fields, matching that assertion. Full local run, pinned SDK, solution built first: `dotnet build PlaceMark.slnx` clean (0 warnings, 0 errors); 68/68 `PlaceFormTests`; 916/920 WebUI (4 pre-existing, unrelated skips — not `E2EFactAttribute`); 148/148 architecture tests, including `AdrFiles_Corrected_HasAMatchingInlineMarkerInBothDirections`. ADR-0126's own diff is unchanged from the previous round — Decision section untouched, correction still confined to Consequences. Atomicity (point 3): right call. `SeedFromPartial` already writes all four fields as one construction-time unit; a per-field refusal would need to track provenance per field independently — exactly the second-tracking state the whole design (reuse `IsDirty`'s comparison, add nothing) was chosen to avoid — and would leave `_seededInitialValues` describing no coherent single seed. The hypothetical "reader edited only the name, might want a fresh latitude" is a convenience nobody asked for, traded against new state and a real risk of the kind of drift task 210 exists to close. ADR-0135 (point 4): read fully. Its only reseed-related claim (Consequences, "dismissing the repair form and clicking 'Repair' again reseeds identically") is about a full unmount/remount after dismissal, proved by a different test entirely (`ImportReportBanner_RepairDismissedAfterTypingThenReopened...`) — it says nothing about a second seed landing on an already-mounted, still-open instance. No false statement to correct; declining to annotate is correct. CI: local build/test run is the load-bearing verification here; run #888 for `8247dea` was still in progress at review time with nothing in #887 (the stale, superseded run) to suggest a discrepancy.

rob merged commit 5cb644a53c into main 2026-08-21 14:48:59 +00:00

rob deleted branch reseed-preserves-typed-coordinates 2026-08-21 14:48:59 +00:00

rob referenced this pull request from a commit 2026-08-21 14:49:00 +00:00

Refuse a reseed once the reader has supplied their own value (#211)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

rob/PlaceMark!211

No description provided.