From 17653af557dd8007a511a228073a478ff8ecc933 Mon Sep 17 00:00:00 2001 From: ciregenz Date: Wed, 12 Aug 2026 16:05:49 -0700 Subject: [PATCH] [eric] state: a wrong-shaped API answer changes nothing instead of throwing inside immer (ENG-277) --- frontend/src/shared/focusGuestForKeys.test.ts | 17 ++++- frontend/src/shared/focusGuestForKeys.ts | 5 +- frontend/src/shared/state/agentsSlice.ts | 5 ++ .../shared/state/badPayloadIsHarmless.test.ts | 74 +++++++++++++++++++ frontend/src/shared/state/dashboardsSlice.ts | 3 + frontend/src/shared/state/workflowsSlice.ts | 1 + 6 files changed, 101 insertions(+), 4 deletions(-) create mode 100644 frontend/src/shared/state/badPayloadIsHarmless.test.ts diff --git a/frontend/src/shared/focusGuestForKeys.test.ts b/frontend/src/shared/focusGuestForKeys.test.ts index dfad90fc..5c276213 100644 --- a/frontend/src/shared/focusGuestForKeys.test.ts +++ b/frontend/src/shared/focusGuestForKeys.test.ts @@ -8,11 +8,22 @@ import { test } from 'node:test'; import assert from 'node:assert/strict'; import { focusGuestForKeys, guestKeyTakeoverCount, resetGuestKeyTakeoverCount } from './focusGuestForKeys.ts'; -function fakeWebview(): { focus: () => void; focused: number } { - const wv = { focused: 0, focus(): void { wv.focused += 1; } }; - return wv as { focus: () => void; focused: number }; +function fakeWebview(): { focus: (o?: FocusOptions) => void; focused: number; lastOpts?: FocusOptions } { + const wv = { + focused: 0, + lastOpts: undefined as FocusOptions | undefined, + focus(o?: FocusOptions): void { wv.focused += 1; wv.lastOpts = o; }, + }; + return wv; } +// A docked browser parks off-canvas, and focus() is allowed to scroll its target into view. +test('the guest is focused without scrolling it into view', () => { + const wv = fakeWebview(); + withActive(userInput, () => focusGuestForKeys(wv as never)); + assert.equal(wv.lastOpts?.preventScroll, true, 'a bare focus() can drag a parked card into view'); +}); + function withActive(el: unknown, fn: () => void): void { const doc = globalThis.document as unknown as { activeElement: unknown }; const prev = doc.activeElement; diff --git a/frontend/src/shared/focusGuestForKeys.ts b/frontend/src/shared/focusGuestForKeys.ts index 3890e0c7..bd0b3728 100644 --- a/frontend/src/shared/focusGuestForKeys.ts +++ b/frontend/src/shared/focusGuestForKeys.ts @@ -33,7 +33,10 @@ export function focusGuestForKeys(wv: BrowserWebview): void { const before = typeof document !== 'undefined' ? document.activeElement : null; if (before !== (wv as unknown as Element) && p_isUserTextSurface(before)) p_takeovers += 1; try { - wv.focus(); + // A docked browser is parked far off-canvas, and a bare focus() is allowed to scroll its target + // into view. Measured harmless at left:-100000 (negative clamps to 0), but not for a card parked + // the other way, and refusing the scroll costs nothing: keys still land in a parked guest. + wv.focus({ preventScroll: true }); } catch { // The card unmounted mid-command. The keystroke will miss, which beats it hitting the user. } diff --git a/frontend/src/shared/state/agentsSlice.ts b/frontend/src/shared/state/agentsSlice.ts index c113b79f..24574591 100644 --- a/frontend/src/shared/state/agentsSlice.ts +++ b/frontend/src/shared/state/agentsSlice.ts @@ -1215,6 +1215,10 @@ const agentsSlice = createSlice({ }) .addCase(fetchSessions.fulfilled, (state, action) => { state.loading = false; + // A wrong-shaped answer must change nothing. Coercing it to [] is precisely the board wipe + // (ENG-271); throwing here dies inside immer's produce, which is the "payload is not + // iterable" crash. Doing nothing is the only option that is safe in both directions. + if (!Array.isArray(action.payload)) return; const fetchedIds = new Set(action.payload.map((s) => s.id)); const activeStatuses = new Set(['running', 'waiting_approval']); @@ -1430,6 +1434,7 @@ const agentsSlice = createSlice({ state.trackedNotificationIds = state.trackedNotificationIds.filter((id) => id !== sessionId); }) .addCase(fetchHistory.fulfilled, (state, action) => { + if (!Array.isArray(action.payload)) return; const history: Record = {}; for (const s of action.payload) { history[s.id] = s; diff --git a/frontend/src/shared/state/badPayloadIsHarmless.test.ts b/frontend/src/shared/state/badPayloadIsHarmless.test.ts new file mode 100644 index 00000000..15d508f9 --- /dev/null +++ b/frontend/src/shared/state/badPayloadIsHarmless.test.ts @@ -0,0 +1,74 @@ +// Run: node --test (via frontend/scripts/run-tests.mjs) +// +// ENG-277. A failed request used to reach these reducers as an undefined payload and they iterated +// it, which threw INSIDE immer's produce: "t.payload is not iterable" / "Cannot read properties of +// undefined (reading 'map')", live in a real console. +// +// The fix is deliberately "change nothing", not "treat it as empty". Empty is the dangerous reading: +// it is exactly how one bad answer wiped a whole dashboard layout (ENG-271). So each test asserts +// BOTH that it does not throw AND that the good data already in the store survived. +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import agents, { fetchSessions, fetchHistory } from './agentsSlice.ts'; +import dashboards, { fetchDashboards } from './dashboardsSlice.ts'; + +const BAD: unknown[] = [undefined, null, {}, 'nope', 42]; + +function agentsWithOneSession(): any { + const s = agents(undefined, { type: '@@init' }) as any; + return agents(s, { + type: fetchSessions.fulfilled.type, + payload: [{ id: 'keep-me', name: 'Keep me', status: 'completed', messages: [] }], + meta: { arg: { dashboardId: 'dash-1' } }, + }) as any; +} + +test('a session list that is not a list changes nothing and does not throw', () => { + const before = agentsWithOneSession(); + assert.ok(before.sessions['keep-me'], 'fixture never armed'); + for (const bad of BAD) { + const after = agents(before, { + type: fetchSessions.fulfilled.type, payload: bad, meta: { arg: { dashboardId: 'dash-1' } }, + }) as any; + assert.ok(after.sessions['keep-me'], `payload ${JSON.stringify(bad)} lost a real session`); + } +}); + +test('a bad history payload keeps the history it already had', () => { + let s = agents(undefined, { type: '@@init' }) as any; + s = agents(s, { type: fetchHistory.fulfilled.type, payload: [{ id: 'h1', name: 'Old chat' }] }) as any; + assert.ok(s.history.h1, 'fixture never armed'); + for (const bad of BAD) { + s = agents(s, { type: fetchHistory.fulfilled.type, payload: bad }) as any; + assert.ok(s.history.h1, `payload ${JSON.stringify(bad)} wiped history`); + } +}); + +test('a bad dashboard list does not read as "you have no dashboards"', () => { + let s = dashboards(undefined, { type: '@@init' }) as any; + s = dashboards(s, { type: fetchDashboards.fulfilled.type, payload: [{ id: 'd1', name: 'Board' }] }) as any; + assert.ok(s.items.d1, 'fixture never armed'); + for (const bad of BAD) { + s = dashboards(s, { type: fetchDashboards.fulfilled.type, payload: bad }) as any; + assert.ok(s.items.d1, `payload ${JSON.stringify(bad)} emptied the dashboard list`); + } +}); + +// The negative half: a genuinely good answer must still be applied, or "it changes nothing" would +// pass on a reducer that had been broken into doing nothing at all. +test('a real payload still applies', () => { + const s = agents(agentsWithOneSession(), { + type: fetchSessions.fulfilled.type, + payload: [{ id: 'keep-me', name: 'Keep me', status: 'completed', messages: [] }, + { id: 'new-one', name: 'New', status: 'running', messages: [] }], + meta: { arg: { dashboardId: 'dash-1' } }, + }) as any; + assert.ok(s.sessions['new-one'], 'a valid payload stopped being applied'); +}); + +test('a real dashboard list still replaces the old one', () => { + let s = dashboards(undefined, { type: '@@init' }) as any; + s = dashboards(s, { type: fetchDashboards.fulfilled.type, payload: [{ id: 'd1', name: 'Board' }] }) as any; + s = dashboards(s, { type: fetchDashboards.fulfilled.type, payload: [{ id: 'd2', name: 'Other' }] }) as any; + assert.ok(s.items.d2 && !s.items.d1, 'a valid dashboard list no longer replaces'); +}); diff --git a/frontend/src/shared/state/dashboardsSlice.ts b/frontend/src/shared/state/dashboardsSlice.ts index 4fcfb24c..80d93068 100644 --- a/frontend/src/shared/state/dashboardsSlice.ts +++ b/frontend/src/shared/state/dashboardsSlice.ts @@ -116,6 +116,9 @@ const dashboardsSlice = createSlice({ }) .addCase(fetchDashboards.fulfilled, (state, action) => { state.loading = false; + // This replaces the whole list, so a wrong-shaped answer must not be read as "you have no + // dashboards" any more than it should throw inside immer. Keep what we had. + if (!Array.isArray(action.payload)) return; const items: Record = {}; for (const d of action.payload) { items[d.id] = d; diff --git a/frontend/src/shared/state/workflowsSlice.ts b/frontend/src/shared/state/workflowsSlice.ts index 5233c4f2..9dfb0d33 100644 --- a/frontend/src/shared/state/workflowsSlice.ts +++ b/frontend/src/shared/state/workflowsSlice.ts @@ -641,6 +641,7 @@ const slice = createSlice({ delete state.runControlPending[action.meta.arg.runId]; }) .addCase(fetchRuns.fulfilled, (state, action) => { + if (!action.payload || !Array.isArray(action.payload.runs)) return; state.runs[action.payload.id] = action.payload.runs; for (const r of action.payload.runs) { const pending = state.runControlPending[r.id];