Reload places and invitations on their own stale-state gaps (241, 242, 243) #185
Loading…
Reference in a new issue
No description provided.
Delete branch "feature/task-241-stale-group-state"
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?
Tasks 241, 242 and 243 turned out not to share one root cause.
241 (first SSO sign-in hides the personal group): not a stale-state bug. Added
OidcSignInJourneyTests, a real end-to-end SSO sign-in through a newStubOidcProviderKestrel host wired intoPlaceMarkAppFixture(this fixture never drove a genuine OIDC round trip before). It passes on unmodifiedmain:CallerGroupsStateloads the new personal group correctly and "Add a place" appears immediately, no navigation or reload needed. What Rob saw isPlaceAccordion.HasNothingToShow(ADR-0095, already covered byPlaceAccordionTests) — a brand-new account with no places and no shared group deliberately shows "You have no places yet." instead of a one-row accordion, identical for local and SSO sign-in. No code change; this is a UX expectation gap, not a defect. Worth a separate ticket if the personal group should be named explicitly on first sign-in.242 (received invitation needs a hard refresh): confirmed and fixed.
PendingInvitationsState.EnsureLoadedAsync's cache-once guard meant an invitation sent afterNavMenu's badge first loaded the state never reappeared, even navigating to/accountagain — a hard refresh was genuinely the only way to see it. AddedPendingInvitationsState.ReloadAsync(mirrorsCallerGroupsState.ReloadAsync) and call it fromAccount.OnInitializedAsyncinstead ofEnsureLoadedAsync. Scope is deliberately "re-fetch on navigation to/account", not live push — v1 has no realtime mechanism, and the comment says so. Reproduced first withReceivedInvitationVisibilityJourneyTests(E2E, in-app link navigation only, noGotoAsync) and a bUnit test renderingAccounttwice against one shared state; both went red against the oldEnsureLoadedAsynccall and green after the fix.243 (accepted invitation needs a hard refresh): partially already correct, one real gap fixed.
Account.AcceptInvitationAsyncalready calledCallerGroupsState.ReloadAsync(), so an empty freshly-joined group appears immediately (existingInviteAndAcceptJourneyTestscase, unmodified). The gap is a group that already has places before the invitee accepts:MapPlacesStatewas never reloaded, so the row appeared but empty ("No places in this group yet.") until something else reloaded places. AddedMapPlacesState.ReloadAsync(same shape) and call it alongside the groups reload. Reproduced with a newInviteThenAccept_GroupAlreadyHasAPlace_...E2E case (owner adds a place before inviting) and a bUnit assertion on theGET /api/placescount; both went red against the old code and green after.No ADR: both fixes apply the
ReloadAsyncpatternCallerGroupsStatealready established, to two more places that hadn't received it — not a new mechanism.All new/modified tests were watched red against the unfixed code before being restored green. Full suite: 2016 unit/integration tests (Domain, Contracts, Architecture, WebUI, Api, Infrastructure) plus 28 E2E journeys, all green.
Verdict: mergeable
242 and 243 are genuine, correctly-scoped fixes:
ReloadAsyncplacement is right, the bUnit tests plausibly discriminate the oldEnsureLoadedAsynccall (traced through by hand — reverting toEnsureLoadedAsyncwould leave the second/api/users/me/invitationsGET and the/api/placesGET un-made, failing both new assertions), the 243 E2E case asserts the place row with no navigation/reload between accept and check as task 243 requires, and 242's live-push limitation is stated in a durable code comment, not just the PR body.241's "not a bug" conclusion holds up.
HasNothingToShow(PlacesState.Groups.Count == 0 && !CallerGroups.Groups.Any(g => !g.IsPersonal)) is pre-existing and already has dedicated coverage —PlaceAccordion_AccountHasOnlyItsPersonalGroupAndNoPlaces_ShowsTheFlatEmptyMessageNotAnAccordionOfOneEmptyRowpredates this PR and proves a personal-group-only account renders zero accordion headers, by design (ADR-0095). Nothing in that gate or inCallerGroupsState/MapPlacesStatebranches on how the caller signed in, so the same UX applies to local registration too.OidcSignInJourneyTestsdoesn't merely pass vacuously:CallerGroupsStatehas never loaded before this first sign-in in a fresh browser context,AuthStateProvider.SignInAsyncis awaited fully before the client-sideNavigateTo, and PKCE/redirect mechanics are exercised for real, so a genuine stale-cache or broken-redirect regression would plausibly turn this red. Not proven, but well-supported, and no ADR is needed for either the 241 conclusion (already documented in ADR-0095) or the 242/243 fixes (both apply the existingReloadAsyncpattern).One gap worth closing, not blocking: the new test asserts "Add a place" is visible, but never asserts the flat "You have no places yet." message actually renders for this SSO account (or that zero accordion headers appear). That's the specific claim the PR body makes about what Rob saw, and right now it's inferred by reading
HasNothingToShow's pre-existing coverage rather than demonstrated by this test itself. Adding that assertion would make the test's connection to the 241 narrative airtight instead of merely consistent with it.@ -146,0 +158,4 @@/// itself, and for the same reason: nothing about the mutation that makes this stale is a signal/// this class watches on its own./// </summary>public async Task ReloadAsync(CancellationToken cancellationToken = default) => await LoadAsync(cancellationToken);ReloadAsync bypasses the
_isLoadingguard EnsureLoadedAsync has, by design — but that means a ReloadAsync call that lands while an EnsureLoadedAsync-triggered load is still in flight runs two concurrent LoadAsync invocations against the same_placeslist (Clear/AddRange interleaved across await points on the UI thread), which isn't safe. Narrow window in practice, and CallerGroupsState.ReloadAsync already has the same shape, so this isn't new — flagging for awareness rather than blocking on it.@ -0,0 +41,4 @@// CallerGroupsState were still showing whatever it held before sign-in (nothing, for a caller// who was anonymous a moment ago) rather than the freshly signed-in caller's own groups, this// button would not appear without a further navigation or reload.await Expect(page.GetByRole(AriaRole.Button, new PageGetByRoleOptions { Name = "Add a place", Exact = true })).ToBeVisibleAsync();This proves CallerGroupsState loaded the personal group (via the "Add a place" gate, which is independent of HasNothingToShow), but it doesn't assert the actual claim the PR body makes about this scenario: that the accordion shows the flat "You have no places yet." message with no group row, matching HasNothingToShow. Worth adding
await Expect(page.GetByText("You have no places yet.")).ToBeVisibleAsync();(or asserting zerobutton.accordion-header) here so this test demonstrates the 241 explanation directly rather than relying on a reader to cross-reference PlaceAccordionTests.