mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-10-26 06:19:49 +00:00
fix(web): the collection page's identity fence must be able to recover (BUG-3084) (#1374)
Repair for #1372, found by codex on the next surface in the family. `pageIdentityHeld()` compares against `identityEpochAtLoad`, and only `loadCollection` re-stamps that value. The load runs from an effect keyed on the ROUTE — `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 the page stays mounted with a stale epoch and every `pageIdentityHeld()` is false FOR EVER. The consequence is silent and permanent: the SSE and sync subscription callbacks on the collection page return at their first line, so the board stops receiving live updates and stops healing renames, with nothing on screen to say so. An anonymous viewer on a public collection who signs in is enough to reach it. The fix is that the stale-data condition the fence DETECTS has to be RESOLVED, not refused indefinitely. The page subscribes to `authStore.onIdentityChange` and re-runs its load, which re-stamps the epoch and replaces the previous session's view with this one's — the state the fence existed to stop being shown in the first place. The order is the store's contract rather than luck: the epoch is bumped BEFORE listeners run, and `notifyIdentityChange`'s own comment places the obligation on a listener to load for the identity signed in NOW rather than re-issue anything captured earlier. This listener reads the LIVE route values and captures nothing. The settings page (#1370) does NOT need this and was checked rather than assumed: its load effect is keyed on `${sessionUserId}\n${wsSlug}`, so an identity change re-runs it by itself. Two further defects found in the same review, both fixed here because both are in the code this change is about: - The listener CLEARS before it re-loads. `loadCollection` leaves the current collection, saved views, members and progress on screen for the whole round-trip, so re-loading alone would keep the previous user's private collection visible across a sign-out. The fences stop stale async COMMITS; they say nothing about state already rendered. `items` is not cleared here — it derives from `localIndex`, whose own identity listener drops the previous user's rows. - `leaveDiscard` replays `pendingNav` without an identity check, so a draft dialog answered after a sign-in navigates the new user to the previous user's destination. It uses `pageIdentityHeld()` rather than an entry capture, because the intent was formed when the previous user tried to leave and a capture taken at the dialog's answer is already the new epoch. The drafts are still discarded — they are the previous session's and clearing them is the safe direction — only the navigation is withheld. This is a commit point with NO AWAIT, which is a population neither the function-level enumeration nor the await-counting rules can reach. BUG-3084's body predicted this shape exists and said it would be found by reading; it was found by a reviewer instead, which is the same thing arriving later. It is asserted by name rather than enumerated, because "every synchronous function that replays captured intent" is not a set a grep can produce, and a rule that cannot enumerate its population should not pretend to. Instruments, each shown able to go red for the case it is named for: - The source guard gains three assertions — the subscription exists, it re-runs the load (a listener that merely cleared state would leave the epoch stale, which is the defect wearing a different shape), and the clear precedes the load. Surfaces 3-7 copy this guard, so the property is asserted where it will be inherited. - The behavioural suite gains a recovery case that drives the real sequence — bump the epoch, THEN notify, which is the order the store uses — and asserts the page re-loads, that the previous collection is not still rendered while it does, and that an SSE event afterwards is acted on again. Its fake auth store now dispatches listeners, since what a listener does is the whole subject. Against the page as it stands on main, exactly the two new tests fail and the other 21 pass. Each of the three fixes was mutated individually: all three detected (2, 1 and 3 failures respectively). Gates: `npx vitest run` from web/ — 221 files / 3035 tests, 0 failures; `npx svelte-check --threshold error` — 0 errors. Codex rounds 1-3: two P1s, CLEAN, CLEAN. Claude-Session: https://claude.ai/code/session_01PDYjP67KTAcqpFpo3ijAAA
This commit is contained in:
xarmian
@@ -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();
|
||||
}
|
||||
|
||||
|
||||
+69
-2
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user