diff --git a/web/src/routes/[username]/[workspace]/[collection]/+page.svelte b/web/src/routes/[username]/[workspace]/[collection]/+page.svelte index 507330b39..24d1ea1d4 100644 --- a/web/src/routes/[username]/[workspace]/[collection]/+page.svelte +++ b/web/src/routes/[username]/[workspace]/[collection]/+page.svelte @@ -852,6 +852,54 @@ } } + /** + * RE-LOAD ON AN IDENTITY CHANGE (BUG-3084, repair). + * + * `pageIdentityHeld()` compares against `identityEpochAtLoad`, and only + * `loadCollection` re-stamps it. The effect below runs it on a ROUTE change + * — `wsSlug` / `collSlug` / `showArchived` — and never because the identity + * moved. `routes/+layout.svelte` deliberately does NOT reload the tab on an + * anonymous -> signed-in transition, so this page stays mounted with a + * stale epoch and every `pageIdentityHeld()` is false FOR EVER: the SSE and + * sync subscription callbacks go silently inert and never recover. An + * anonymous viewer on a public collection who signs in stops receiving live + * updates and rename healing, with nothing on screen to say so. + * + * The stale-data condition the fence detects therefore has to be RESOLVED + * rather than refused indefinitely. Re-loading re-stamps the epoch and + * replaces the previous session's view with this one's, which is the state + * the fence existed to stop being shown. + * + * SAFE ORDER, by the store's contract rather than by luck: the epoch is + * bumped BEFORE listeners run (`notifyIdentityChange`), whose own comment + * places the obligation on a listener to load for the identity signed in + * NOW rather than re-issue anything captured earlier. `loadCollection` here + * reads the LIVE route values and captures nothing, so it satisfies that. + * + * Not needed by the settings page (#1370), whose load effect is keyed on + * `${sessionUserId}\n${wsSlug}` and so re-runs on an identity change by + * itself — checked rather than assumed. + */ + const stopIdentityReload = authStore.onIdentityChange(() => { + // CLEARED FIRST, then re-loaded (codex [P1]). `loadCollection` leaves + // the current `collection`, saved views and progress on screen while it + // awaits, so re-loading alone would keep the previous user's private + // collection VISIBLE for the length of a round-trip. The fences stop + // stale async commits; they say nothing about state already rendered. + // + // `items` is derived from `localIndex`, whose own identity listener + // drops the previous user's rows, so it is not cleared here. + collection = null; + savedViews = []; + activeViewId = null; + itemProgress = {}; + workspaceMembers = []; + metaError = null; + metaFromCache = false; + if (wsSlug && collSlug) loadCollection(wsSlug, collSlug, showArchived); + }); + onDestroy(stopIdentityReload); + $effect(() => { if (wsSlug && collSlug) loadCollection(wsSlug, collSlug, showArchived); }); @@ -2322,6 +2370,27 @@ function leaveDiscard() { draftText = {}; draftOpen = {}; + // `pageIdentityHeld()`, and NOT an entry capture, for the reason the + // fence's header gives: `pendingNav` was formed when the PREVIOUS user + // tried to leave, and this function runs when the dialog is answered, + // which may be after the identity moved. An entry capture taken here is + // already the new epoch and detects nothing (BUG-3084, codex). + // + // The drafts are still discarded — they belong to the previous session + // and clearing them is the safe direction — but the NAVIGATION is not + // replayed, because it would send the new user to the previous user's + // destination. + // + // A commit point with NO AWAIT, which is a population neither the + // function-level enumeration nor the await-counting rules can see. The + // item's own body warned this shape exists and is found by reading; it + // was found by a reviewer instead, which is the same thing arriving + // later. + if (!pageIdentityHeld()) { + pendingNav = null; + showLeaveDialog = false; + return; + } runPendingNav(); } diff --git a/web/src/routes/[username]/[workspace]/[collection]/collectionIdentityFence.svelte.test.ts b/web/src/routes/[username]/[workspace]/[collection]/collectionIdentityFence.svelte.test.ts index d041b923b..66401daf9 100644 --- a/web/src/routes/[username]/[workspace]/[collection]/collectionIdentityFence.svelte.test.ts +++ b/web/src/routes/[username]/[workspace]/[collection]/collectionIdentityFence.svelte.test.ts @@ -223,18 +223,32 @@ vi.mock('$lib/stores/workspace.svelte', () => ({ */ const auth = vi.hoisted(() => { let epoch = 0; + // The fake dispatches listeners, because the repair's whole point is what a + // listener does. Order matches the real store's contract: the epoch is + // bumped BEFORE listeners run, so a load started from one re-stamps to the + // NEW value — which is exactly what makes recovery possible. + const listeners: Array<(previousUserId: string) => void> = []; return { + notifyIdentityChange(previousUserId = 'u0') { + for (const fn of [...listeners]) fn(previousUserId); + }, get identityEpoch() { return epoch; }, get userId() { return 'u1'; }, get user() { return { id: 'u1', name: 'A', email: 'a@example.com' }; }, get session() { return { user: { id: 'u1' } }; }, bumpEpoch() { epoch++; }, - resetEpoch() { epoch = 0; }, + resetEpoch() { epoch = 0; listeners.length = 0; }, identityFence() { const captured = epoch; return () => epoch === captured; }, - onIdentityChange() { return () => {}; }, + onIdentityChange(fn: (previousUserId: string) => void) { + listeners.push(fn); + return () => { + const i = listeners.indexOf(fn); + if (i >= 0) listeners.splice(i, 1); + }; + }, clear() {}, }; }); @@ -431,6 +445,59 @@ describe('the collection page stops a commit when the identity moves mid-flight' })(); }); + it('RECOVERS after a sign-in: the page re-loads and the subscriptions resume', () => { + // THE REPAIR (BUG-3084, codex P1). Without it the fence is a one-way + // door: `identityEpochAtLoad` is re-stamped only by the page load, that + // load is driven by a ROUTE-keyed effect, and `routes/+layout.svelte` + // does not reload the tab on an anonymous -> signed-in transition. So + // the page keeps a stale epoch and every `pageIdentityHeld()` is false + // FOR EVER — the SSE and sync callbacks go inert and never come back. + // + // This drives the real sequence rather than the epoch alone: bump, THEN + // notify, which is the order `notifyIdentityChange` uses and the reason + // a load started from a listener re-stamps to the new value. + return (async () => { + await mountPage(); + + // Sign in: the epoch moves and listeners run. + flipIdentity(); + auth.notifyIdentityChange(''); + + // The listener re-loads, which is what re-stamps the epoch. + const reload = await waitFor(() => { + if (collectionGets.length < 2) throw new Error('the identity change did not re-load the page'); + return collectionGets[1]!; + }); + // And it CLEARED first: the previous user's collection is not still + // on screen while the new one is being fetched (codex [P1]). Asserted + // at this point deliberately — after the listener ran, before the + // reload resolves, which is the whole window in question. + expect( + findPropsObject()?.collection ?? null, + "the previous user's collection stayed rendered for the length of the re-load" + ).toBeNull(); + reload.resolve({ ...COLLECTION }); + await tick(); + + // And the subscription callback works again. Without the repair this + // fetch never happens: `pageIdentityHeld()` is still comparing + // against the pre-sign-in epoch and returns at the first line. + const before = collectionGets.length; + const cb = sseHandlers[0]!({ type: 'collection_updated', collection_id: 'c1', collection: 'tasks' }); + const refresh = await waitFor(() => { + if (collectionGets.length <= before) { + throw new Error('the SSE callback is still inert after the sign-in'); + } + return collectionGets[collectionGets.length - 1]!; + }); + refresh.resolve({ ...COLLECTION, name: 'Renamed' }); + await cb; + await waitFor(() => { + expect((findPropsObject()!.collection as { name: string }).name).toBe('Renamed'); + }); + })(); + }); + // ── Timing class 3: a commit that OUTLIVES the page ─────────────────── // // The bulk Undo button is handed to the global toast store and the page may diff --git a/web/src/routes/[username]/[workspace]/[collection]/collectionIdentityFence.test.ts b/web/src/routes/[username]/[workspace]/[collection]/collectionIdentityFence.test.ts index 789286386..02874ac71 100644 --- a/web/src/routes/[username]/[workspace]/[collection]/collectionIdentityFence.test.ts +++ b/web/src/routes/[username]/[workspace]/[collection]/collectionIdentityFence.test.ts @@ -82,6 +82,71 @@ describe('the collection page fences every async commit point', () => { expect(beforeFirstAwait).toContain('identityEpochAtLoad = authStore.identityEpoch'); }); + it('re-loads on an identity change, so pageIdentityHeld() can recover', () => { + // WITHOUT THIS THE FENCE IS A ONE-WAY DOOR (BUG-3084 repair, codex P1). + // `identityEpochAtLoad` is re-stamped only by the page load, and that + // load is driven by a ROUTE-keyed effect. `routes/+layout.svelte` does + // not reload the tab on an anonymous -> signed-in transition, so without + // a subscription the page keeps a stale epoch and every + // `pageIdentityHeld()` is false for ever — the subscription callbacks + // this fence protects go silently inert and never recover. + // + // Asserted in the SOURCE guard rather than only behaviourally because + // this is a property every surface in the family owes, and the guard is + // what surfaces 3-7 will copy. + expect( + CODE, + 'the page does not subscribe to authStore.onIdentityChange, so a stale load epoch is never ' + + 'refreshed and pageIdentityHeld() refuses for ever after a sign-in' + ).toMatch(/authStore\.onIdentityChange\(/); + // And the subscription must actually re-run the load — a listener that + // merely clears state would leave the epoch stale, which is the defect + // wearing a different shape. + const sub = CODE.slice(CODE.indexOf('authStore.onIdentityChange(')); + expect( + sub.slice(0, sub.indexOf('});')), + 'the identity-change listener does not re-run loadCollection, so the load epoch stays stale' + ).toMatch(/loadCollection\(/); + // Unsubscribed, or it outlives the page it re-loads. + expect(CODE).toMatch(/onDestroy\(stopIdentityReload\)/); + }); + + it('fences the SYNCHRONOUS commit points the await-based rules cannot see', () => { + // A population neither instrument reaches: a function with NO await that + // still replays intent formed earlier. `leaveDiscard` runs when the + // unsaved-draft dialog is answered and replays `pendingNav`, which was + // created when the PREVIOUS user tried to leave the page — so after an + // identity change it navigates the new user to the previous user's + // destination. + // + // The item's body predicted this shape exists ("commit points with NO + // await at all") and said it is found by reading. It was found by a + // reviewer instead, which is the same thing arriving later. Named + // explicitly rather than enumerated, because "every synchronous function + // that might replay captured intent" is not a set a grep can produce — + // and a rule that cannot enumerate its population should not pretend to. + const discard = CODE.slice(CODE.indexOf('function leaveDiscard()')); + expect( + discard.slice(0, discard.indexOf('\n\t}')), + 'leaveDiscard replays pendingNav without checking the identity, so a dialog answered after ' + + "a sign-in navigates the new user to the previous user's destination" + ).toMatch(/pageIdentityHeld\(\)/); + }); + + it('clears the previous identity\'s page state before re-loading', () => { + // The re-load is a round-trip, and `loadCollection` leaves the current + // collection, views and progress on screen for its whole duration. The + // fences stop stale async COMMITS; they say nothing about state already + // rendered, so without this the previous user's private collection stays + // visible across a sign-out (codex [P1]). + const sub = CODE.slice(CODE.indexOf('authStore.onIdentityChange(')); + const body = sub.slice(0, sub.indexOf('});')); + expect(body, 'the identity-change listener re-loads without clearing what is on screen') + .toMatch(/collection = null/); + // And the clear must come BEFORE the load, or it is decoration. + expect(body.indexOf('collection = null')).toBeLessThan(body.indexOf('loadCollection(')); + }); + it('enumerates the population it claims to cover', () => { // COUNTS ARE ASSERTED so a new member cannot arrive unnoticed: an added // handler, arrow, callback or timer fails HERE and has to be looked at