Remember per-group map visibility between visits #189
Loading…
Reference in a new issue
No description provided.
Delete branch "feature/task-245-remember-group-visibility"
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?
Closes Vikunja task 245. Persists
MapPlacesState.HiddenGroupIdsinlocalStorage, followingViewportStore's own shape (task 217, ADR-0133): per-user key,schemaVersion, every read treated as untrusted.ExpandedGroupIdsis untouched — ADR-0131 keeps expansion and visibility independent on purpose.New
GroupVisibilityStorestores the hidden set, not the visible one, so a group the reader has never seen defaults to visible.MapPlacesStatedepends on it directly (unlikeViewportStore, which onlyHomesees) because eitherHome's map or the drawer'sPlaceAccordioncan be first to trigger a load, and the stored set has to be applied before either first renders.The stored set is pruned, on every load, against the groups the caller's just-loaded places actually belong to. This is deliberate, not merely tidy: it's what makes a group reappear visible after the reader is re-invited to it, rather than silently arriving hidden — reproducing task 243's "I accepted an invitation and nothing appeared" complaint by a different route. Full reasoning, including the alternatives considered (storing the visible set, never pruning, pruning against
CallerGroupsStateinstead), is in ADR-0157.ADR-0131 gains
Answered by: ADR-0157(metadata only, per ADR-0107) since it explicitly deferred the persistence question to a later record.Covered by
GroupVisibilityStoreTests,MapPlacesStateTests(including a round-trip test proving the save call itself matters, watched to redden with it removed), and a newRememberGroupVisibilityJourneyTestsE2E class — no fixed waits, since a toggle's save is awaited inside its own click handler.ce1c0c67bd2a3abe9110Rebased onto
main(now includes #188 and #190). Only conflict wasdocs/adr/README.md's generated table — resolved by keeping both rows, then confirmed viaAdrIndexTests(exact-match + prose-floor checks) that nothing else moved; diffedDrawer.razor.css/theme.cssagainstmaindirectly, both untouched by this branch. New head:2a3abe9110b566f0fd3a84309ff5306ddeb2745e, CI green (build/e2e/container-images all succeeded).Addressed both open points in ADR-0157 (Alternatives and Consequences):
GroupVisibilityStore: kept it required. Anulldefault has no meaning the running app can ever produce — DI always supplies both together — and the only way to makenullsafe would be a guard inApplyStoredHiddenGroupIdsAsync/ToggleGroupVisibilityAsyncthat moves the failure mode from "DI throws loudly at startup if mis-registered" to "persistence silently doesn't happen." Named the fourteen-file cost explicitly rather than eliding it, and noted a future dependency added here pays the same tax.ViewportStorehas the identical gap, just a quieter failure — its four fields are value types, so a same-schemaVersion-but-wrong-shape value binds toLatitude/Longitude/Zoom = 0instead ofnull, and(0,0,0)passesIsWithinRangeand gets silently trusted as a real saved viewport. Confirmed by hand-deserializing{"schemaVersion":1}against its own record shape. Documented plainly in ADR-0157's Consequences; not fixed here — out of this ticket's scope, named so it isn't rediscovered as a fresh gap.Verdict: changes needed
Re-invitation is not actually fixed (item 2 of the brief) — reproduced by test.
ApplyStoredHiddenGroupIdsAsyncprunes_hiddenGroupIdsin memory only; it never callsGroupVisibilityStore.SaveAsyncto write the pruned set back. Neither doesRemoveGroup. So the raw value inlocalStoragestill contains a group's id after access is lost, for as long as the reader makes no unrelated explicit toggle in between. On re-invitation, the next load reads that stale raw value straight back from storage and re-derives the group as hidden again — exactly task 243's "accepted and nothing appeared" complaint, by the exact route ADR-0157 claims to have closed.I confirmed this by adding a temporary test in the worktree (not part of this PR): save
{A}, load once with only group B's places (access lost, no toggle in between), then load again with group A's place present (re-invited). Result:HiddenGroupIdsstill contains A. It should be empty.ADR-0157's Decision section states plainly that "by the time access is regained, the id has already been pruned from what a load re-derives (it had nothing to survive on while access was lost)" — that's true of the in-memory set at the moment access was lost, but that derived value is discarded and never written anywhere; the next load re-derives afresh from the still-stale raw storage. The claim doesn't hold as stated, and needs either a fix (persist the pruned set back to storage — e.g. from
ApplyStoredHiddenGroupIdsAsyncwhen pruning actually drops something, or fromRemoveGroup) or the ADR's claim and the PR description corrected to match what actually ships.Everything else checked out:
UpsertPlace/ReloadAsync).ApplyStoredHiddenGroupIdsAsyncruns inside the privateLoadAsyncbeforeRebuildGroups/_isLoaded = true/Changed?.Invoke(), so no render can beat the apply.ViewportStoregap claimed in Consequences is accurate —StoredViewport's fields are all value types, so{"schemaVersion":1}alone deserialises toLatitude/Longitude/Zoom = 0, and0,0,0passesIsWithinRangeunchanged. Good citation.Answered byon ADR-0131 is correctly applied per ADR-0107: ADR-0131's own frozen Decision text ("Whether it should persist is Vikunja task 217's own question... this record takes no position on it beyond naming the boundary") is a genuine explicit deferral. ADR-0133 correctly left unannotated. README row is correctly inserted with the #0162 row intact.Two smaller points, non-blocking:
ToggleGroupVisibilityAsync'sSaveAsynccall has no try/catch, in eitherGroupVisibilityStoreorMapPlacesState. The in-memory toggle itself is safe (mutated synchronously before the await), but alocalStorage.setItemfailure (quota, private-mode) will throw out of anEventCallback-bound async handler and surface as an unhandled UI exception rather than degrading quietly. This mirrorsViewportStore.SaveAsync's existing behaviour, so it's not a new pattern, but it wasn't named as an accepted risk anywhere the way the other failure modes were — worth at least a one-line note if not a guard.docs/adr/0157-...mduses "deserialization"/"deserializing" a few times as ordinary prose nouns (not theDeserializeAPI name itself) — British spelling would be "deserialisation"/"deserialising" per the project's own convention.No genuine conflict with the parallel task 249 work beyond the ordinary merge risk of a widened
MapPlacesStateconstructor signature.2a3abe91103f0ca52b45Fixed. New head:
3f0ca52b4534dda92507e56227c99d5a336fc9ac, CI green (build/e2e/container-images all succeeded).The blocking finding:
ApplyStoredHiddenGroupIdsAsyncnow writes the pruned set back toGroupVisibilityStorewhenever pruning actually drops something, so a stale id is genuinely gone from storage, not merely absent from one load's own memory. Reproduced your exact repro shape first (EnsureLoadedAsync_AccessLostThenRegained_ThePreviouslyHiddenGroupArrivesVisible, watched red against the unpatched code), then fixed it.Race safety: added
_hiddenGroupIdsGeneration, bumped by every direct mutator ofHiddenGroupIds(ToggleGroupVisibilityAsync,RemoveGroup,EnsureGroupVisible). The load-time prune checks it immediately after its own storage read and backs off entirely — touching neither memory nor storage — if it's moved, rather than clobbering a fresher change with a stale computation. Proved by a test that simulates a toggle completing mid-read via a stale JSON snapshot; watched red with the guard removed. One residual gap named rather than closed: a toggle starting during the load's ownSaveAsynccall can still complete before that write lands — closing that fully would need a mutual-exclusion primitive around every write, judged disproportionate at hobby scale; documented in ADR-0157's own Consequences.The pruning-source question: kept place-derived pruning (not switched to
CallerGroupsState), now explicitly reasoned as a heavier trade than before — a hidden-but-placeless group's toggle reset is now durable, not session-scoped. Chosen over theCallerGroupsStatecoupling because the cost is bounded to a no-visible-effect toggle-state reset (hiding an empty group changes nothing on the map either way). Written up in both the Decision and Alternatives sections of ADR-0157.RemoveGroupandEnsureGroupVisiblealso now persist when they actually change the hidden set (fire-and-forget through a shared helper, kept synchronous) — the same class of gap, found while fixing the load path.EnsureGroupVisiblespecifically: without this, a place created into a persistently-hidden group would show correctly for the session but silently revert to hidden on the next load, since the group now has a place and so wouldn't be pruned.Non-blocking items:
GroupVisibilityStore.SaveAsyncnow catchesJSException/JSDisconnectedException;ViewportStore.SaveAsync's identical gap is named, not fixed, same treatment as its unrelated all-zero defect. British spelling fixed (deserialisation/deserialising).ADR-0157 rewritten throughout — Source, Decision, Alternatives and Consequences — to describe what actually ships, including a frank "caught in review" account of the original defect and the alternative (never writing back) named and rejected rather than silently corrected.
Rebased onto current
main(through #191); no conflicts this time.Verdict: mergeable
Re-reviewed at
3f0ca52b, independently of the ADR's own account.Re-invitation fix, verified by mutation, not just reading. I checked out the branch and reproduced my own round-1 failing scenario against the fix — it now passes. I then removed the write-back conditional from
ApplyStoredHiddenGroupIdsAsyncand confirmedEnsureLoadedAsync_AccessLostThenRegained_ThePreviouslyHiddenGroupArrivesVisiblereddens; restored it and reran the fullMapPlacesStateTests/GroupVisibilityStoreTestssuite (75/75) and the wholePlaceMark.WebUI.Testsproject (838/838, 4 pre-existing unrelated skips). The claim holds.Generation counter: sufficient for the primary race, correctly scoped as incomplete for the residual. Traced the ordering by hand (read → compare generation → mutate → conditional write) and confirmed independently, then also mutated: removed the
_hiddenGroupIdsGenerationcheck and watchedEnsureLoadedAsync_AToggleRacesTheStorageRead_TheToggleWinsRatherThanBeingClobberedredden. My own analysis of the remaining window matches what the ADR names as the residual risk almost exactly: because every writer (ToggleGroupVisibilityAsync, the load's own write-back) always serializes the live_hiddenGroupIdsreference at its own point of resumption rather than a captured snapshot, the only way to actually lose data is out-of-order completion at the JS interop boundary itself (a write whose serialization ran earlier landing inlocalStorageafter one whose serialization ran later) — which is exactly what's named as unclosed, narrow, and not worth a mutex at this scale. I did not find a second, unnamed hole beyond that one.Fire-and-forget writes.
RemoveGroup/EnsureGroupVisiblealways read the live_hiddenGroupIdsfield fresh atSaveAsync's own serialization point (never a value captured earlier), so a queued save can't clobber a later mutation with stale data — consistent with the concurrent-toggle reasoning above.JSException/JSDisconnectedExceptionare the two failure modes actually reachable fromjsRuntime.InvokeAsync("localStorage.setItem", ...)(quota, private browsing, a torn-down interop channel), and the doc comment scopes the claim narrowly to "the write itself," not blanket safety forSaveAsyncas a whole —StorageKeyAsync's ownGetAuthenticationStateAsync()call remains unguarded, but that's pre-existing, shared with the read path and withViewportStore, not a new gap. One real gap: unlikeDrawerTests/LeafletMapTests, which inject aJSExceptionvia the fakeIJSRuntimeto prove their own guards, nothing here does that forGroupVisibilityStore.SaveAsync— the guard reads correctly by inspection, but it's the one fix from round 1 that shipped without a "watch it fail" test of its own. Worth a follow-up, not blocking.Durable pruning cost — argued honestly, but the ADR's "no visible cost" framing understates the compound case. The Decision/Alternatives sections plainly reconsider
CallerGroupsState-based pruning now that the cost is real rather than theoretical, and are honest that the reset is now permanent. But the ADR frames the worst case as "an accordion toggle quietly reading shown," which is true only at the instant the reset happens (the group is empty then). Working through the coordinator's exact scenario: hide a placeless group → any reload while it's still empty durably drops the id from storage (write-back fires because pruning changed the count) → a place added to that group afterward, from anywhere, will then show a marker with no toggle interaction from the reader who explicitly hid it. That's more than a quiet toggle state — it's a marker appearing for a group the reader deliberately hid. It only happens if a reload lands in the empty window between hiding and the place arriving (add-then-reload doesn't trigger it, since the id survives pruning once the group has a place again), so it's genuinely narrow, but it's a real, visible surprise, not merely a UI-state inconsistency. Non-blocking — the trade itself is reasonable — but the record slightly undersells what a reader could actually see.Everything cleared in round 1 is undisturbed, confirmed directly against this diff: hidden-set/"not mentioned means visible" design and load-timing unchanged; the required-constructor-dependency reasoning unchanged; the
ViewportStoreall-zero gap claim unchanged and still accurate;Answered byon ADR-0131 unchanged and still correctly scoped per ADR-0107; ADR-0133 still correctly unannotated; README diff unchanged, #0162 row intact. The "deserialization" → "deserialisation" spelling is fixed throughout ADR-0157.The "caught in review" account is accurate. Cross-checked against my own round-1 finding and reproduction: the Alternatives section's "Pruning in memory... but never writing the pruned result back to storage" entry correctly describes what shipped and what I found, not a flattered version of it.
No conflict with the parallel task 249 work beyond the ordinary merge risk of the
MapPlacesStateconstructor signature.Both actioned. New head:
49b24e7fde0cfe46af913d098000a0487f72f1bf, CI green (build/e2e/container-images all succeeded).1. The phantom-marker trade, stated plainly. Rewrote the Decision, Alternatives and "What this does not verify" sections of ADR-0157 with the exact sequence you (via the reviewer) worked out: hide an empty group → an intervening load prunes and durably writes back the empty set → a place lands in that group afterwards →
EnsureGroupVisibleunconditionally unhides it → a marker appears with no toggle interaction since the original hide. No hedging — the "no visible cost" framing is gone, replaced with the five-step sequence and an honest "this is a real, visible surprise" statement.On reflection I kept the current pruning source rather than switching to
CallerGroupsState-based membership pruning. I considered the membership-based fix seriously — it would close this precisely, since a group the caller still belongs to is never dropped merely for being empty — but it needsMapPlacesStateto have real membership data at the exact moment place-pruning would need it, and the two loads run in parallel (Task.WhenAllin bothHomeandPlaceAccordion), so closing it properly means either serialising two loads that currently don't wait on each other, or a second explicit reconciliation step needing new plumbing at three call sites (Home,PlaceAccordion,Account's accept-invitation path) — real, ongoing complexity, at the tail of a PR already through two review rounds. Weighed against a failure mode that's narrow (needs a group hidden while genuinely empty), order-dependent, and corrected by the reader's own next toggle press. Recorded as a considered position with the sequence fully in view, not a default — a future record is free to reach it differently.2. Storage-failure guard, tested. Added
SaveAsync_LocalStorageSetItemThrowsAJsException_DoesNotThrowand itsJSDisconnectedExceptionsibling toGroupVisibilityStoreTests, following theDrawerTests/LeafletMapTestsprecedent for injecting interop failures. Watched both redden with thetry/catchremoved fromGroupVisibilityStore.SaveAsync, then restored it. (One incidental finding while writing these: NSubstitute's fluent.Returns(callInfo => throw ...)form false-positives CA2012 on aValueTask-returning member when the callback never returns a value — worked around with the explicitSubstituteExtensions.Returns<object?>(...)static call form, commented in place.)Full
PlaceMark.WebUI.Tests: 840/840 (2 net new). Job-log totals this run: build job 2050 non-E2E tests, e2e job 42/42.Verdict: mergeable
Re-reviewed at
49b24e7f.Phantom-marker sequence. ADR-0157's Decision section states it as the five numbered steps I worked out, and step 5 correctly names
EnsureGroupVisibleas the unconditional-unhide mechanism that turns the durably-pruned flag into an actual marker. Matches what I found; not softened.Place-derived pruning decision — genuinely reasoned. Checked the new claim rather than taking it on trust:
Home.razor.csandPlaceAccordion.razor.csdo both startPlacesState.EnsureLoadedAsyncandCallerGroups.EnsureLoadedAsyncinsideTask.WhenAll, so the parallel-load argument for whyCallerGroupsState-based pruning isn't a simple substitution is factually accurate, not asserted. TheAccount.razor.csaccept-invitation path is honestly scoped too — it reloads both sequentially, not in parallel, and the ADR only claims it "would need to learn the new step too," not that it shares the same parallel-load problem. This reads as an actual weighing, with the trade recorded as revisitable rather than closed.New failure-guard tests. Verified by mutation: removed the
try/catchfromGroupVisibilityStore.SaveAsyncand bothSaveAsync_LocalStorageSetItemThrowsAJsException_DoesNotThrowand itsJSDisconnectedExceptionsibling reddened with the exception propagating out ofSaveAsync, exactly as the doc comments claim. Restored and reran the full suite: 840/844 passing (4 pre-existing unrelated skips) — the two new tests are additive, nothing else moved.Nothing outstanding from prior rounds.