diff --git a/apps/web/src/App.tsx b/apps/web/src/App.tsx index 14c136a698..a0bd10472b 100644 --- a/apps/web/src/App.tsx +++ b/apps/web/src/App.tsx @@ -87,7 +87,11 @@ import { } from './providers/daemon'; import { AMR_LOGIN_STATUS_EVENT } from './components/amrLoginPolling'; import { CollabDemoView } from './collab/CollabDemoView'; -import { useWorkspaceBilling, useWorkspaceContext } from './collab/useWorkspaceContext'; +import { + beginWorkspaceScopedRead, + useWorkspaceBilling, + useWorkspaceContext, +} from './collab/useWorkspaceContext'; import { resolvePlanTier } from './collab/team-plan'; import { deriveTabIdentityScope, UNSET_ACCOUNT_BUCKET } from './collab/tab-scope'; import { CommunityView } from './components/CommunityView'; @@ -1763,7 +1767,13 @@ function AppInner() { // headerless read is not the "unfiltered" list — it is the list with every // workspace-claimed skill removed, including the ones claimed by the // workspace the user is actually in. - const list = await fetchSkills(workspaceContextRef.current); + const read = beginWorkspaceScopedRead(workspaceContextRef.current); + const list = await fetchSkills(read.context); + // A read for the workspace the user has since LEFT must not restore that + // workspace's catalog over the current one — see `beginWorkspaceScopedRead`. + // Skipping the gate too is deliberate: this response is not an answer about + // the current identity, and the newer read that replaced it will mark it. + if (!read.isStillCurrent(workspaceContextRef.current)) return; setSkills(list); markSkillRegistryReady('functional'); }, [markSkillRegistryReady]); @@ -2908,8 +2918,13 @@ function AppInner() { const handleSkillsChanged = useCallback( (affectedSkillId?: string) => { - // Scoped, like every other app-level skills read — see `refreshSkills`. - void fetchSkills(workspaceContextRef.current).then((list) => setSkills(list)); + // Scoped AND guarded on commit, like every other app-level skills read — + // see `refreshSkills` and `beginWorkspaceScopedRead`. + const skillsRead = beginWorkspaceScopedRead(workspaceContextRef.current); + void fetchSkills(skillsRead.context).then((list) => { + if (!skillsRead.isStillCurrent(workspaceContextRef.current)) return; + setSkills(list); + }); void fetchDesignTemplates().then((list) => setDesignTemplates(list)); iframeKeepAlivePool.evictMatching( (entry) => { diff --git a/apps/web/src/collab/useWorkspaceContext.ts b/apps/web/src/collab/useWorkspaceContext.ts index 81b50e9d0a..d5382feec5 100644 --- a/apps/web/src/collab/useWorkspaceContext.ts +++ b/apps/web/src/collab/useWorkspaceContext.ts @@ -114,6 +114,55 @@ export function workspaceIdentityCacheKey( ].join(':'); } +/** + * One workspace-scoped read: the identity it was issued for, plus the check that + * must pass before its response may be committed. + * + * Keying a request (or its `coalescedGet` entry) by identity stops the WRONG + * IDENTITY BEING SERVED an answer fetched for someone else. It does nothing + * about the other direction: a read issued for identity A resolves later, and by + * then the caller may be identity B. Committing that late answer restores A's + * data under B — the exact staleness the identity keys exist to prevent, + * arriving through the back door. Reverse-order completion is not exotic here; + * a workspace switch is precisely when one read is in flight and another starts. + * + * So every workspace-scoped read follows the same four steps: + * + * const read = beginWorkspaceScopedRead(contextRef.current); + * const data = await fetchSomething(read.context); + * if (!read.isStillCurrent(contextRef.current)) return; // ← the invariant + * commit(data); + * + * Two rules make it actually hold: + * + * 1. Request with `read.context`, never with the caller's own variable, so the + * request and the guard can never disagree about whose data was asked for. + * 2. Compare against a REF, never a closed-over prop or state value. A closure + * captures the identity the read was issued for, so comparing against it + * always succeeds and guards nothing. + * + * This is the cross-component form of the `requestEpochRef` ordering guard + * `useWorkspaceContext` already applies to its own read; identity is the right + * discriminator for reads that are scoped BY identity. + */ +export interface WorkspaceScopedRead { + /** The context to send with the request — see rule 1 above. */ + readonly context: WorkspaceCollabContext | null; + /** Whether `current` is still the identity this read was issued for. */ + isStillCurrent(current: WorkspaceCollabContext | null | undefined): boolean; +} + +export function beginWorkspaceScopedRead( + context: WorkspaceCollabContext | null | undefined, +): WorkspaceScopedRead { + const issuedFor = context ?? null; + const identity = workspaceIdentityCacheKey(issuedFor); + return { + context: issuedFor, + isStillCurrent: (current) => workspaceIdentityCacheKey(current ?? null) === identity, + }; +} + /** * `GET /api/workspace/context` is the read that ESTABLISHES the caller's * identity, so — unlike every other workspace read — it cannot be keyed on the diff --git a/apps/web/src/components/EntryNavRail.tsx b/apps/web/src/components/EntryNavRail.tsx index 6cfa752a72..6e55af698b 100644 --- a/apps/web/src/components/EntryNavRail.tsx +++ b/apps/web/src/components/EntryNavRail.tsx @@ -58,6 +58,7 @@ import type { EntrySettingsSection } from './EntrySettingsMenu'; import { useI18n } from '../i18n'; import { useDismissOnOutsideInteraction } from '../hooks/useDismissOnOutsideInteraction'; import { + beginWorkspaceScopedRead, notifyTeamProjectsChanged, notifyWorkspaceBillingRefresh, notifyWorkspaceContextRefresh, @@ -681,6 +682,11 @@ export function EntryNavRail({ setAccountOpen(false); }); const [teamOpen, setTeamOpen] = useState(false); + // The LATEST context, for async work to compare against. `loadWorkspaceDirectory` + // closes over the render's `context` prop, which is the identity its read was + // issued for — so only a ref can answer "has the identity moved since?". + const contextRef = useRef(context); + contextRef.current = context; const [workspaceItems, setWorkspaceItems] = useState( () => attributableWorkspaceDirectory(context) ?? [], ); @@ -748,28 +754,40 @@ export function EntryNavRail({ : []; async function loadWorkspaceDirectory() { + // Capture the identity this read is FOR, and compare against `contextRef` + // (not the closed-over `context`, which is by definition the identity we are + // reading for) before committing anything — see `beginWorkspaceScopedRead`. + const read = beginWorkspaceScopedRead(contextRef.current); // Only show the loading row when there is nothing to show yet. With a warm // cache the list is already on screen and this read just revalidates it — // but a cache belonging to another account counts as nothing to show. - if (attributableWorkspaceDirectory(context) === null) setWorkspaceDirectoryLoading(true); + if (attributableWorkspaceDirectory(read.context) === null) { + setWorkspaceDirectoryLoading(true); + } try { // The coalescing key carries the caller's identity for the same reason the // module cache does: `coalescedGet` shares a settled result for a second, // and this read's answer depends on WHO asked. - const cacheKey = `workspace-directory:${workspaceIdentityCacheKey(context)}`; + const cacheKey = `workspace-directory:${workspaceIdentityCacheKey(read.context)}`; const items = await coalescedGet(cacheKey, async () => { const response = await fetch('/api/workspace/directory', { cache: 'no-store' }); if (!response.ok) throw new Error(`directory ${response.status}`); const body = (await response.json()) as WorkspaceDirectoryResponse; return body.items ?? []; }); + // The account may have changed while this was in flight. Writing here + // would repopulate BOTH the module cache and the visible list with the + // previous account's names, after the identity-change effect below had + // already cleared them — so an abandoned read must leave no trace. + if (!read.isStillCurrent(contextRef.current)) return; cachedWorkspaceDirectory = items; setWorkspaceItems(items); } catch { // A failed revalidation must not blank a list the user is looking at — // keep the last known names and let the next open try again. A list this // caller has no claim to is not "a list the user is looking at". - if (attributableWorkspaceDirectory(context) === null) setWorkspaceItems([]); + if (!read.isStillCurrent(contextRef.current)) return; + if (attributableWorkspaceDirectory(read.context) === null) setWorkspaceItems([]); } finally { setWorkspaceDirectoryLoading(false); } diff --git a/apps/web/tests/components/App.skills-workspace-scope.test.tsx b/apps/web/tests/components/App.skills-workspace-scope.test.tsx index b1848e134f..b0a02fb47f 100644 --- a/apps/web/tests/components/App.skills-workspace-scope.test.tsx +++ b/apps/web/tests/components/App.skills-workspace-scope.test.tsx @@ -17,6 +17,7 @@ // so `route.kind` stays 'home' and no route change fires. Skills never did. import { act, cleanup, render, screen, waitFor } from '@testing-library/react'; +import type { SkillSummary } from '@open-design/contracts'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { App } from '../../src/App'; @@ -45,7 +46,14 @@ import { import { resetCoalescedGet } from '../../src/lib/coalesced-get'; vi.mock('../../src/components/EntryView', () => ({ - EntryView: () =>
, + EntryView: ({ skills }: { skills: Array<{ id: string }> }) => ( +
+
+ {skills.map((skill) => ( +
+ ))} +
+ ), })); vi.mock('../../src/components/ProjectView', () => ({ @@ -172,6 +180,25 @@ function skillsReadScopes(): Array { const projects: Project[] = []; +function skill(id: string): SkillSummary { + return { + id, + name: id, + description: id, + triggers: [], + mode: 'prototype', + source: 'user', + } as unknown as SkillSummary; +} + +function deferred() { + let resolve!: (value: T) => void; + const promise = new Promise((res) => { + resolve = res; + }); + return { promise, resolve }; +} + describe('App skills list — workspace scope', () => { beforeEach(() => { resetWorkspaceContextCache(); @@ -241,4 +268,59 @@ describe('App skills list — workspace scope', () => { await waitFor(() => expect(skillsReadScopes()).toContain('ws-b')); expect(skillsReadScopes()).toEqual(['ws-a', 'ws-b']); }); + + // Issuing the right request is only half the guarantee. Each read resolves + // later, and nothing stopped a read issued FOR the workspace the user has + // since left from committing when it finally landed — restoring that + // workspace's catalog over the current one, which is the very staleness this + // change exists to remove, arriving through the back door. + it("discards a slow read belonging to the workspace the user has left", async () => { + let activeWorkspaceId = 'ws-a'; + vi.stubGlobal( + 'fetch', + vi.fn(async (input: RequestInfo | URL) => { + const pathname = new URL(String(input), 'http://d.local').pathname; + return { + ok: true, + json: async () => + pathname.endsWith('/workspace/context') + ? workspaceContextPayload(activeWorkspaceId) + : {}, + } as Response; + }), + ); + + const readA = deferred(); + const readB = deferred(); + vi.mocked(fetchSkills).mockImplementation((context) => + context?.workspaceId === 'ws-b' ? readB.promise : readA.promise, + ); + + render(); + await waitFor(() => expect(skillsReadScopes()).toContain('ws-a')); + + // Switch while ws-a's read is still in flight. + activeWorkspaceId = 'ws-b'; + await act(async () => { + notifyWorkspaceContextRefresh(); + await Promise.resolve(); + }); + await waitFor(() => expect(skillsReadScopes()).toContain('ws-b')); + + // Reverse order: the workspace the user is actually IN answers first… + await act(async () => { + readB.resolve([skill('skill-from-b')]); + await Promise.resolve(); + }); + await waitFor(() => expect(screen.getByTestId('entry-skill-skill-from-b')).toBeTruthy()); + + // …and the abandoned workspace answers second. It must change nothing. + await act(async () => { + readA.resolve([skill('skill-from-a')]); + await Promise.resolve(); + }); + + expect(screen.getByTestId('entry-skill-skill-from-b')).toBeTruthy(); + expect(screen.queryByTestId('entry-skill-skill-from-a')).toBeNull(); + }); }); diff --git a/apps/web/tests/components/EntryNavRail.directory-account-scope.test.tsx b/apps/web/tests/components/EntryNavRail.directory-account-scope.test.tsx index 93f799099e..869a481573 100644 --- a/apps/web/tests/components/EntryNavRail.directory-account-scope.test.tsx +++ b/apps/web/tests/components/EntryNavRail.directory-account-scope.test.tsx @@ -16,7 +16,7 @@ // ACCOUNT see", so it is exactly the class of read `workspaceIdentityCacheKey` // warns about: a cache coarser than the identity of the request it holds. -import { cleanup, fireEvent, render, screen, waitFor, within } from '@testing-library/react'; +import { act, cleanup, fireEvent, render, screen, waitFor, within } from '@testing-library/react'; import type { WorkspaceCollabContext } from '@open-design/contracts'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; @@ -176,6 +176,51 @@ describe('workspace switcher directory — account scope', () => { expect(menu().queryByText('Ada private workspace')).toBeNull(); }); + // The identity-scoped cache key stops a cached answer being SERVED to the + // wrong identity. It does nothing about a request already in flight when the + // identity moves: that response lands afterwards and used to be written + // straight into both the module cache and component state, repopulating + // account A's names after the identity-change effect had cleared them. + it("discards an in-flight read that lands after the account changed", async () => { + const gate = installGatedFetch(() => ACCOUNT_A_DIRECTORY); + + const view = renderRail(contextFor('wm-a-team')); + fireEvent.click(screen.getByTestId('workspace-switcher')); + + // A's read is in flight and deliberately NOT released yet. Swap the account + // underneath the mounted rail (sign out, sign in as someone else) — no + // unmount, which is what leaves the pending request pointing at A. + view.rerender( + + {}} + onNewProject={() => {}} + open + context={contextFor('wm-b-team')} + /> + , + ); + + // Now let A's request answer, after the identity has already moved. + await act(async () => { + gate.releaseAll(); + await Promise.resolve(); + }); + + // Component state must not have taken A's names. + expect(menu().queryByText('Ada private workspace')).toBeNull(); + + // …and the module cache must not have taken them either. Observed by + // remounting as A with a read that never answers: an abandoned response + // leaves no trace, so there is no warm list to serve even to A. + view.unmount(); + installGatedFetch(() => ACCOUNT_A_DIRECTORY); + renderRail(contextFor('wm-a-team')); + fireEvent.click(screen.getByTestId('workspace-switcher')); + expect(menu().queryByText('Ada private workspace')).toBeNull(); + }); + it('still serves the warm list across a same-account workspace switch', async () => { const gate = installGatedFetch(() => ACCOUNT_A_DIRECTORY);