PlaceForm.IsDirty misjudges two cases #125
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/place-form-is-dirty"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Task 186.
IsDirtydecides whether dismissing the add/edit panel confirms before discarding. Wrong in two directions:_seededSelectedGroupId, the auto-selected default, notnull— an untouched default still reads clean, only an actual change counts. Reverses ADR-0102's original exclusion; ADR-0110 records why and partially supersedes it._descriptionstartsnull; clearing a typed textarea binds"", and"" != nullread as dirty for an already-undone change. Both sides now go through a null/empty-normalizing helper.Decided deliberately, not left to fall out of the implementation: a coordinate reseed on a second map click (PR #120, ADR-0105's own follow-up) stays dirty, the same way the very first seed already does —
IsDirty's coordinate comparison is unchanged, and its baseline is alwaysnullin the only mode a reseed happens in, so there's nothing for a reseed to reset to. A test locks this in.Both fixes mutation-tested: reverting each independently reddens its own test, confirmed then restored.
Checked, not fixed, per the ticket's own scope limit: the same null-vs-
""question exists in the save path —SubmitAsyncsends_descriptionunnormalized, and nothing between there anddescription varchar(2000) NULLconverts""tonull, despite both contracts documentingnullas what clears it. Flagged as a comment on task 186; needs its own ticket.Full suite (Domain/Contracts/Infrastructure/Api/Architecture/WebUI, 1706 tests) and coverage ratchet green (WebUI 87.9% → 90.1%),
dotnet format --verify-no-changesclean, all on the pinned SDK.Verdict: mergeable
Reviewed substantive commit
45f361d, then re-fetched: head is nowcc396ce,mergeable: true. Confirmed the delta myself (git diff 45f361d cc396ce -- src/PlaceMark.WebUI/Places/PlaceForm.razor.cs tests/PlaceMark.WebUI.Tests/Places/PlaceFormTests.cs docs/adr/0102-*.md docs/adr/0110-*.mdis empty) — only main's own commits (#121, #123) plus the regenerated index landed. Rebuilt and re-ran the relevant suites atcc396ceitself, not just45f361d:PlaceFormTests39/39,AdrIndexTests119/119, both genuinely green. This review is againstcc396ce.Both fixes mutation-tested independently, not taken on trust. Removed the
(Place is null && _selectedGroupId != _seededSelectedGroupId)clause —PlaceForm_CreateModeGroupChanged_IsDirtyandPlaceForm_CreateModeGroupChangedThenRestored_IsNotDirtyAgainboth reddened, exactly as claimed. Restored, revertedNormalizeDescriptionback to a raw_description != Place?.Descriptioncomparison —PlaceForm_CreateModeDescriptionTypedThenCleared_IsNotDirtyandPlaceForm_EditModeDescriptionWasNullTypedThenCleared_IsNotDirtyboth reddened. Restored. Neither guard is vacuous.Reseed-stays-dirty reasoning verified by tracing the code, not by the test's existence.
OnParametersSetonly reseeds_latitude/_longitudeif (Place is null && ...);IsDirty's coordinate clause compares againstPlace?.Latitude/Place?.Longitude, which isnullthroughout add mode. Since a reseed only ever replaces one non-null coordinate with another (never resets tonull), and the comparison baseline never moves offnullin this mode, there is no code path by which a reseed could flip the clause back to false. The claim holds structurally, not just empirically.ADR-0110/ADR-0102 supersession mechanism, checked as the first real application of ADR-0107's lesson: both directions present (0102's
Partially superseded bynames 0110; 0110'sPartially supersedesnames 0102), Status text matches the established convention exactly (Accepted (partly superseded by 0110), same form as the pre-existingAccepted (partly superseded by 0017)). Didn't just read this — deleted 0102's ownPartially superseded byfield and Status annotation to simulate a one-sided edit:AdrFiles_Supersession_AgreesWithTheNamedRecordInBothDirectionsreddened immediately, naming both the missing reverse field and the missing Status text. Restored, green again.CrossRecordSupersessionGapsdoes what it's for.Save-path scope boundary confirmed genuinely untouched. Diffed
SubmitAsync's body against the pre-PR version directly — only a doc comment was added above the method; no line inside it changed. The unnormalized_descriptionsend is real (checked againstCreatePlaceRequest/UpdatePlaceRequest's own doc comments) and correctly left alone rather than half-fixed.Nothing else to flag.