diff --git a/src/workspaces/state/src/useDashboardState.test.tsx b/src/workspaces/state/src/useDashboardState.test.tsx new file mode 100644 index 00000000..91c3871b --- /dev/null +++ b/src/workspaces/state/src/useDashboardState.test.tsx @@ -0,0 +1,270 @@ +import type { ReactNode } from 'react'; +import { describe, expect, mock, test, beforeEach } from 'bun:test'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import { renderHook, act, waitFor } from '@testing-library/react'; + +/** + * The store every dashboard layout — and therefore every chat panel's `config.agentName` — is written + * through. It had no tests, and the two pieces of reasoning it carries are the ones that decide whether an + * unattended run is survivable: + * + * - `readValue` refuses a stored value whose *kind* disagrees with the default. One wrong-shaped jsonb + * row used to be `.children.map is not a function` during render, i.e. a white screen recoverable only + * by SQL. + * - `revert` rolls an optimistic write back **only if nothing has written that key since**. Writes to one + * key overlap freely (a window resize fires one per group), so an unconditional rollback would turn one + * failed write into two lost ones. + * + * Both are module-private, so everything here goes through the real hook. The only fakes are the transport + * (`useClient`), the identity (`useAuth`) and the toast — the logic under test is untouched. + * + * Writing these found a live defect on the second point: the guard was reference equality against the + * value written, and `setQueryData`'s structural sharing rebuilds objects, so it was false for every + * container — i.e. for every layout. Fixed by a per-key sequence number; the pin is the "refused OBJECT + * write" test below. + */ + +type Layout = { type: string; id: string; config?: { agentName: string } }; + +const patchCalls: Record[] = []; +let getResponse: () => Promise = async () => ({}); +let patchBehaviour: (body: Record) => Promise = async () => ({}); + +const toastErrors: { message: string; description?: string }[] = []; + +/** + * `mock.module` replaces a module *process-wide and permanently* — Bun has one module registry for the + * whole run, so a stub declared here is what every other test file gets too. Replacing the module wholesale + * is therefore a way to delete exports out from under files that never heard of this one: a bare + * `{ useClient }` stub took `getHeaders` away from `AppRegistry.test.ts`, which failed with a `SyntaxError` + * and no obvious connection to the change. + * + * So each stub spreads the real module and overrides exactly the one export it needs to fake. + */ +const realUseClient = await import('hooks/useClient'); +const realUseAuth = await import('hooks/useAuth'); +const realSonner = await import('sonner'); + +mock.module('hooks/useClient', () => ({ + ...realUseClient, + useClient: () => ({ + get: () => getResponse(), + patch: (_path: string, body: Record) => { + patchCalls.push(body); + return patchBehaviour(body); + }, + }), +})); + +mock.module('hooks/useAuth', () => ({ ...realUseAuth, useAuth: () => ({ isAuthenticated: true }) })); + +mock.module('sonner', () => ({ + ...realSonner, + toast: { + ...realSonner.toast, + error: (message: string, opts?: { description?: string }) => + toastErrors.push({ message, description: opts?.description }), + }, +})); + +const { useDashboardState, DASHBOARD_STATE_QUERY_KEY } = await import('./useDashboardState'); + +let queryClient: QueryClient; + +const wrapper = ({ children }: { children: ReactNode }) => ( + {children} +); + +const mount = (key: string, defaultValue: T) => + renderHook(() => useDashboardState(key, defaultValue), { wrapper }); + +const cache = () => queryClient.getQueryData>(DASHBOARD_STATE_QUERY_KEY) ?? {}; + +beforeEach(() => { + patchCalls.length = 0; + toastErrors.length = 0; + getResponse = async () => ({}); + patchBehaviour = async () => ({}); + queryClient = new QueryClient({ defaultOptions: { queries: { retry: false }, mutations: { retry: false } } }); +}); + +describe('readValue — a wrong-shaped row must not reach a consumer that traverses it', () => { + test('a stored array where an object is expected falls back to the default', async () => { + // The real case: a layout column that defaulted to `[]`. A LayoutNode consumer does `.children.map`. + getResponse = async () => ({ 'ws-layout-x': [] }); + const { result } = mount('ws-layout-x', { type: 'panel', id: 'root' }); + await waitFor(() => expect(result.current.isLoaded).toBe(true)); + expect(result.current.value).toEqual({ type: 'panel', id: 'root' }); + }); + + test('a stored object where an array is expected falls back to the default', async () => { + getResponse = async () => ({ 'panel-sessions': { a: 1 } }); + const { result } = mount('panel-sessions', []); + await waitFor(() => expect(result.current.isLoaded).toBe(true)); + expect(result.current.value).toEqual([]); + }); + + test('a container of the right kind is returned as stored, by reference', async () => { + const stored = { type: 'panel', id: 'stored', config: { agentName: 'scout' } }; + getResponse = async () => ({ 'ws-layout-x': stored }); + const { result } = mount('ws-layout-x', { type: 'panel', id: 'default' }); + await waitFor(() => expect(result.current.isLoaded).toBe(true)); + expect(result.current.value).toEqual(stored); + }); + + test('a wrong primitive is passed through — only containers are guarded', async () => { + // Deliberate: a wrong primitive is a cosmetic surprise, a wrong container is a crash. + getResponse = async () => ({ 'files/currentPath': 42 }); + const { result } = mount('files/currentPath', '/'); + await waitFor(() => expect(result.current.isLoaded).toBe(true)); + expect(result.current.value).toBe(42 as unknown as string); + }); + + test('a bad read is NOT written back — the server keeps the row until a real write repairs it', async () => { + getResponse = async () => ({ 'ws-layout-x': [] }); + const { result } = mount('ws-layout-x', { type: 'panel', id: 'root' }); + await waitFor(() => expect(result.current.isLoaded).toBe(true)); + expect(patchCalls).toEqual([]); + }); +}); + +describe('setValue — optimistic write, then persist', () => { + test('updates the cache immediately and PATCHes the same value', async () => { + getResponse = async () => ({ 'ws-layout-x': { type: 'panel', id: 'a' } }); + const { result } = mount('ws-layout-x', { type: 'panel', id: 'root' }); + await waitFor(() => expect(result.current.isLoaded).toBe(true)); + + act(() => result.current.setValue({ type: 'panel', id: 'b' })); + + expect(cache()['ws-layout-x']).toEqual({ type: 'panel', id: 'b' }); + expect(patchCalls).toEqual([{ 'ws-layout-x': { type: 'panel', id: 'b' } }]); + }); + + test('an updater composes on the cache, not on a captured render value', async () => { + // This is what stops two mutations inside one window from losing the first: every caller in + // WorkspaceView passes an updater, and the updater must see the previous write. + getResponse = async () => ({ counter: 0 }); + const { result } = mount('counter', 0); + await waitFor(() => expect(result.current.isLoaded).toBe(true)); + + act(() => { + result.current.setValue((n) => n + 1); + result.current.setValue((n) => n + 1); + result.current.setValue((n) => n + 1); + }); + + expect(cache().counter).toBe(3); + expect(patchCalls).toEqual([{ counter: 1 }, { counter: 2 }, { counter: 3 }]); + }); +}); + +describe('revert — a failed write must not roll back over a later successful one', () => { + test('a failed write is rolled back to the value it replaced, and says so', async () => { + getResponse = async () => ({ 'ws-layout-x': { type: 'panel', id: 'before' } }); + patchBehaviour = async () => { + throw new Error('boom'); + }; + const { result } = mount('ws-layout-x', { type: 'panel', id: 'root' }); + await waitFor(() => expect(result.current.isLoaded).toBe(true)); + + act(() => result.current.setValue({ type: 'panel', id: 'after' })); + await waitFor(() => expect(toastErrors.length).toBe(1)); + + expect(cache()['ws-layout-x']).toEqual({ type: 'panel', id: 'before' }); + expect(toastErrors[0]!.description).toContain('boom'); + }); + + test('a key that did not exist is deleted rather than reverted to undefined', async () => { + getResponse = async () => ({}); + patchBehaviour = async () => { + throw new Error('nope'); + }; + const { result } = mount('brand-new', 'seed'); + await waitFor(() => expect(result.current.isLoaded).toBe(true)); + // The seeding effect writes the default; let it fail and clear before the real assertion. + await waitFor(() => expect(toastErrors.length).toBeGreaterThan(0)); + toastErrors.length = 0; + queryClient.setQueryData(DASHBOARD_STATE_QUERY_KEY, {}); + + act(() => result.current.setValue('written')); + await waitFor(() => expect(toastErrors.length).toBe(1)); + + expect('brand-new' in cache()).toBe(false); + }); + + test('a LATER successful write survives an EARLIER failure — the whole point of the compare-and-swap', async () => { + getResponse = async () => ({ 'ws-layout-x': 'original' }); + let failNext = true; + patchBehaviour = async () => { + if (failNext) { + failNext = false; + throw new Error('first write refused'); + } + return {}; + }; + const { result } = mount('ws-layout-x', 'default'); + await waitFor(() => expect(result.current.isLoaded).toBe(true)); + + // Two writes in flight at once — a window resize fires one per group. The first is refused. + act(() => { + result.current.setValue('doomed'); + result.current.setValue('winner'); + }); + await waitFor(() => expect(toastErrors.length).toBe(1)); + + // Unconditional rollback would leave 'doomed' here, turning one lost write into two. + expect(cache()['ws-layout-x']).toBe('winner'); + }); + + test('a refused OBJECT write is rolled back too — the case reference equality could never catch', async () => { + // Regression pin. The guard used to be `cache[key] === attempted`, and `setQueryData` runs React + // Query's structural sharing, which rebuilds an object rather than storing the one it was handed. So + // the check was false for every container — every layout, every `config.agentName` — and a refused + // write kept its optimistic value while the toast claimed a rollback. Primitives passed, which is + // why it went unnoticed. If this ever fails again, check `writeSeq`, not the test. + getResponse = async () => ({ 'ws-layout-obj': { type: 'panel', id: 'before', config: { agentName: 'scout' } } }); + patchBehaviour = async () => { + throw new Error('refused'); + }; + const { result } = mount('ws-layout-obj', { type: 'panel', id: 'fallback' }); + await waitFor(() => expect(result.current.isLoaded).toBe(true)); + + act(() => result.current.setValue({ type: 'panel', id: 'after', config: { agentName: 'builder' } })); + await waitFor(() => expect(toastErrors.length).toBe(1)); + + expect(cache()['ws-layout-obj']).toEqual({ type: 'panel', id: 'before', config: { agentName: 'scout' } }); + }); + + test('the failure is surfaced, not swallowed — this store used to lose writes invisibly', async () => { + getResponse = async () => ({ 'ws-layout-x': 'original' }); + patchBehaviour = async () => { + throw new Error('server said no'); + }; + const { result } = mount('ws-layout-x', 'default'); + await waitFor(() => expect(result.current.isLoaded).toBe(true)); + + act(() => result.current.setValue('attempt')); + await waitFor(() => expect(toastErrors.length).toBe(1)); + + expect(toastErrors[0]!.message).toBe('Could not save your layout'); + expect(toastErrors[0]!.description).toContain('ws-layout-x'); + }); +}); + +describe('seeding — a missing key is written once, not on every mount', () => { + test('the default is seeded to the server when the key is absent', async () => { + getResponse = async () => ({}); + const { result } = mount('files/currentPath', '/'); + await waitFor(() => expect(patchCalls.length).toBe(1)); + expect(patchCalls[0]).toEqual({ 'files/currentPath': '/' }); + expect(result.current.value).toBe('/'); + }); + + test('an existing key is not re-seeded', async () => { + getResponse = async () => ({ 'files/currentPath': '/Pictures' }); + const { result } = mount('files/currentPath', '/'); + await waitFor(() => expect(result.current.isLoaded).toBe(true)); + expect(patchCalls).toEqual([]); + expect(result.current.value).toBe('/Pictures'); + }); +}); diff --git a/src/workspaces/state/src/useDashboardState.ts b/src/workspaces/state/src/useDashboardState.ts index 1e1b33d2..df8303fa 100644 --- a/src/workspaces/state/src/useDashboardState.ts +++ b/src/workspaces/state/src/useDashboardState.ts @@ -47,8 +47,9 @@ export function useDashboardState(key: string, defaultValue: T) { const currentState = queryClient.getQueryData(QUERY_KEY) ?? {}; if (!(key in currentState)) { queryClient.setQueryData(QUERY_KEY, { ...currentState, [key]: defaultValue }); + const seq = nextWriteSeq(key); persist(clientRef.current, key, defaultValue).catch((err: unknown) => - revert(queryClient, key, defaultValue, undefined, err), + revert(queryClient, key, seq, undefined, err), ); } }, [isSuccess, key, defaultValue, queryClient]); @@ -63,8 +64,9 @@ export function useDashboardState(key: string, defaultValue: T) { queryClient.setQueryData(QUERY_KEY, { ...currentState, [key]: newValue }); + const seq = nextWriteSeq(key); persist(clientRef.current, key, newValue).catch((err: unknown) => - revert(queryClient, key, newValue, key in currentState ? currentValue : undefined, err), + revert(queryClient, key, seq, key in currentState ? currentValue : undefined, err), ); }, [key, defaultValue, queryClient], @@ -81,6 +83,30 @@ export function useDashboardState(key: string, defaultValue: T) { let inFlightWrites = 0; let lastWriteAt = 0; +/** + * Per-key write sequence, and the reason `revert` cannot just look at the cache. + * + * It used to ask "does the cache still hold exactly what I wrote?" by reference — and for a container that + * question can never be true. `setQueryData` runs React Query's structural sharing, which rebuilds the + * object it stores rather than keeping the one it was handed (verified against @tanstack/react-query + * 5.101.4: an object value comes back `!==` what went in, a string comes back `===`). So the guard silently + * inverted for exactly the values this store exists to hold: every layout, every `config.agentName`. A + * refused write kept its optimistic value in the cache while the toast said it had been rolled back, and + * the change vanished at the next reload — the invisible loss the rollback was added to end. + * + * A sequence number asks the question the identity check meant to ask — "has anything written this key + * since?" — and does not depend on identity at all. Caveat, unchanged from before: if two writes to one key + * both fail, the later one reverts to what the earlier one put there. Recovering further would mean keeping + * a known-good value per key, and two consecutive refusals is a server that is telling you plainly. + */ +const writeSeq = new Map(); + +function nextWriteSeq(key: string): number { + const seq = (writeSeq.get(key) ?? 0) + 1; + writeSeq.set(key, seq); + return seq; +} + function persist(client: ReturnType, key: string, value: unknown): Promise { return patchState(client, { [key]: value }); } @@ -102,7 +128,7 @@ function patchState(client: ReturnType, patch: Record {})` in this family: a delete that failed left the dashboard * gone from the list and alive on the server, reappearing at the next reload with no hint why. * - * Recovery here is an invalidate rather than the hook's compare-and-swap revert. There is no single + * Recovery here is an invalidate rather than the hook's sequence-guarded revert. There is no single * previous value to swap back — the caller has already rewritten the roster and dropped keys from the * blob — and refetching is the only thing that makes the list agree with the server again. */ @@ -163,19 +189,19 @@ const warned = new Set(); * are not merely cosmetic — a chat panel's `config.agentName` is the address a peer agent is delivered * to, so a silently-dropped write leaves a panel that answers to a name it will not have tomorrow. * - * Reverts only if the cache still holds exactly what we wrote. Writes to one key overlap freely (a - * window resize fires one per group), and rolling back over a later successful write would turn one - * failure into two. + * Reverts only if nothing has written this key since. Writes to one key overlap freely (a window resize + * fires one per group), and rolling back over a later successful write would turn one failure into two. + * `seq` rather than the written value is deliberate and load-bearing — see `writeSeq` above. */ function revert( queryClient: ReturnType, key: string, - attempted: unknown, + seq: number, previous: unknown, err: unknown, ): void { - const state = queryClient.getQueryData(QUERY_KEY) ?? {}; - if (state[key] === attempted) { + if (writeSeq.get(key) === seq) { + const state = queryClient.getQueryData(QUERY_KEY) ?? {}; const next = { ...state }; if (previous === undefined) delete next[key]; else next[key] = previous; diff --git a/test-setup.ts b/test-setup.ts index 7fb6068e..edf81a9f 100644 --- a/test-setup.ts +++ b/test-setup.ts @@ -1,3 +1,4 @@ +import { afterEach } from 'bun:test'; import { GlobalWindow } from 'happy-dom'; const window = new GlobalWindow(); @@ -32,3 +33,21 @@ Object.assign(globalThis, { setInterval: window.setInterval.bind(window), clearInterval: window.clearInterval.bind(window), }); + +/** + * Unmount every rendered tree between tests — globally, because the library's own version of this is + * order-dependent in a way that produces a bewildering failure. + * + * `@testing-library/react` auto-registers `afterEach(cleanup)` at *module import* time. Bun evaluates a + * module once and attaches lifecycle hooks to whichever file is loading at that moment, so the file that + * happens to import the library first gets the cleanup and every later file silently gets none — its + * renders pile up in `document.body` and the next `screen.getAllByRole` sees the previous test's DOM. + * Adding one test file was enough to break fourteen assertions in a file it does not import, does not + * touch, and was passing on its own. + * + * Registering it here removes the ordering from the question: preload has no enclosing file scope, so this + * hook is global. The dynamic import is deliberate — `@testing-library/dom` builds `screen` from + * `document.body` while it initialises, so it must not load until the happy-dom globals above exist. + */ +const { cleanup } = await import('@testing-library/react'); +afterEach(cleanup);