From 3e8aa7a114a452a4af8d8965cdc71ba8f96387ee Mon Sep 17 00:00:00 2001 From: m4air Date: Wed, 5 Aug 2026 04:08:42 -0700 Subject: [PATCH] Harden Linear view persistence against the failures review surfaced MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five issues, each found by a reviewer and reproduced before fixing: - The filter dropdown's prune effect only ran when the user opened the popover, because the filter was always empty at startup. Restoration makes it run on mount, where `availableTeams` may still be the issue-scraped fallback rather than the real fetch. Metadata complete for a *partial* team set passes every R12 guard, so it pruned facets belonging to teams it simply hadn't seen — and the write persisted, deleting them permanently. Gated on `teamsSettled`. - `canonicalize` dedupes but enforces none of the transport bounds; only the throwing parser does. So `serialize` could emit a 101-label filter that the strict `ui.set` schema rejects, which drops the WHOLE taskResumeState — github, jira and linear query included — on every subsequent write, since the renderer resends the merged object each time. Added `boundLinearIssueAttributeFilter` and a round-trip test built from serializer output rather than a literal, which is the only kind that can catch renderer/schema drift. - `linearIssueView` now carries `.catch(undefined)`: value tolerance stops at the top level, so any future instance of the above is a cosmetic reset of the view instead of silent loss of every other resume field. - A workspace switch forced an uncached list read in both directions. The switch is a later observation, so the null-baseline fix didn't cover it; the cache is already workspace-keyed, making the force pure cost. - Recency for the 20-workspace cap came from object key order, which is wrong twice: re-filtering an existing workspace left it at the head (first evicted, though just used), and an array-index-like key enumerates first regardless of insertion, so a write could evict the very entry it added. Recency is now an explicit ordered key list. Also adds the nested parity assertion — the top-level one compares only TaskResumeState's own keys, so a field added to LinearIssueViewResumeState stayed invisible to it, which is exactly what `.strict()` rejects. The wiring test was blind: deleting the hydration guard outright left all four assertions green. The gate is now `shouldPersistLinearIssueView`, unit-tested directly, and the file is renamed to the repo's `*-boundary.test.ts` convention with an assertion that fails on that mutation. --- .../client-ui-linear-issue-view.test.ts | 61 ++++++++++++++-- .../rpc/methods/task-resume-state-schema.ts | 13 +++- src/renderer/src/components/TaskPage.tsx | 27 ++++--- ...-issue-attribute-filter-dropdowns.test.tsx | 3 + ...inear-issue-attribute-filter-dropdowns.tsx | 10 +++ .../linear-issue-view-resume-wiring.test.ts | 58 --------------- .../task-page-linear-issue-request.test.ts | 45 +++++++++--- .../task-page-linear-issue-request.ts | 28 ++++++-- ...sk-page-linear-issue-view-boundary.test.ts | 63 ++++++++++++++++ src/shared/linear-issue-attribute-filter.ts | 25 +++++++ .../linear-issue-view-resume-state.test.ts | 71 +++++++++++++++++++ src/shared/linear-issue-view-resume-state.ts | 45 +++++++++--- 12 files changed, 350 insertions(+), 99 deletions(-) delete mode 100644 src/renderer/src/components/linear-issue-view-resume-wiring.test.ts create mode 100644 src/renderer/src/components/task-page-linear-issue-view-boundary.test.ts diff --git a/src/main/runtime/rpc/methods/client-ui-linear-issue-view.test.ts b/src/main/runtime/rpc/methods/client-ui-linear-issue-view.test.ts index 003ec9ee0d3..3ef65200a64 100644 --- a/src/main/runtime/rpc/methods/client-ui-linear-issue-view.test.ts +++ b/src/main/runtime/rpc/methods/client-ui-linear-issue-view.test.ts @@ -1,6 +1,13 @@ import { describe, expect, it, vi } from 'vitest' import { getDefaultUIState } from '../../../../shared/constants' -import { LINEAR_ISSUE_ATTRIBUTE_FILTER_ID_MAX_LENGTH } from '../../../../shared/linear-issue-attribute-filter' +import { + LINEAR_ISSUE_ATTRIBUTE_FILTER_ID_MAX_LENGTH, + LINEAR_ISSUE_ATTRIBUTE_FILTER_MAX_LABEL_IDS +} from '../../../../shared/linear-issue-attribute-filter' +import { + defaultLinearIssueViewResumeState, + serializeLinearIssueViewResumeState +} from '../../../../shared/linear-issue-view-resume-state' import type { OrcaRuntimeService } from '../../orca-runtime' import type { RpcRequest } from '../core' import { RpcDispatcher } from '../dispatcher' @@ -98,16 +105,58 @@ describe('ui.set Linear issue view resume state', () => { } } ] - ])('refuses to persist a view with %s', async (_label, linearIssueView) => { + ])('drops only the view when it has %s', async (_label, linearIssueView) => { const { dispatcher, updateUIState } = makeDispatcher() const response = await dispatcher.dispatch( - makeRequest({ sidebarWidth: 280, taskResumeState: { linearIssueView } }) + makeRequest({ + sidebarWidth: 280, + taskResumeState: { linearQuery: 'label:bug', jiraPreset: 'reported', linearIssueView } + }) ) - // Why: taskResumeState rides the value-tolerant wrapper, so an invalid view is - // dropped from the batch rather than written — the rest of the payload lands. + // Why: value tolerance stops at the top level, so without the schema's own `.catch` + // one bad view value would drop every other resume field too — and keep doing so on + // every subsequent write, since the renderer resends the whole merged object. expect(response).toMatchObject({ ok: true }) - expect(updateUIState).toHaveBeenCalledWith({ sidebarWidth: 280 }) + expect(updateUIState).toHaveBeenCalledWith({ + sidebarWidth: 280, + taskResumeState: { + linearQuery: 'label:bug', + jiraPreset: 'reported', + linearIssueView: undefined + } + }) + }) + + // Why: hand-written fixtures can't catch renderer/schema drift — only the renderer's + // own output can. An over-limit filter here stops ALL resume state from persisting. + it.each([ + [ + 'a filter at the facet-count limit', + Array.from( + { length: LINEAR_ISSUE_ATTRIBUTE_FILTER_MAX_LABEL_IDS + 1 }, + (_, index) => `label-${String(index).padStart(4, '0')}` + ) + ], + [ + 'a filter with an over-long facet id', + ['x'.repeat(LINEAR_ISSUE_ATTRIBUTE_FILTER_ID_MAX_LENGTH + 1)] + ] + ])('accepts serializer output for %s', async (_label, labelIds) => { + const { dispatcher, updateUIState } = makeDispatcher() + const linearIssueView = serializeLinearIssueViewResumeState({ + ...defaultLinearIssueViewResumeState(), + filtersByWorkspaceId: { + 'workspace-1': { stateIds: [], priorities: [], assignee: null, labelIds } + } + }) + + const response = await dispatcher.dispatch( + makeRequest({ taskResumeState: { linearIssueView } }) + ) + + expect(response).toMatchObject({ ok: true }) + expect(updateUIState).toHaveBeenCalledWith({ taskResumeState: { linearIssueView } }) }) }) diff --git a/src/main/runtime/rpc/methods/task-resume-state-schema.ts b/src/main/runtime/rpc/methods/task-resume-state-schema.ts index 5e7c9fb3710..6b167524578 100644 --- a/src/main/runtime/rpc/methods/task-resume-state-schema.ts +++ b/src/main/runtime/rpc/methods/task-resume-state-schema.ts @@ -53,7 +53,10 @@ export const TaskResumeState = z }) .strict() .optional(), - linearIssueView: LinearIssueViewResumeState.optional(), + // Why: value tolerance stops at the top level, so without `.catch` one bad view + // value drops the whole taskResumeState — github/jira/linear query included — + // for paired clients on every subsequent write, not just this one. + linearIssueView: LinearIssueViewResumeState.optional().catch(undefined), jiraPreset: z.enum(['assigned', 'reported', 'all', 'done']).optional(), jiraQuery: z.string().optional() }) @@ -64,3 +67,11 @@ const _taskResumeStateParity: AssertNoMissingKeys< z.infer > = true void _taskResumeStateParity + +// Why: the top-level assertion only compares TaskResumeState's own keys, so a field +// added one level down stays invisible to it — and `.strict()` rejects exactly that. +const _linearIssueViewParity: AssertNoMissingKeys< + NonNullable, + z.infer +> = true +void _linearIssueViewParity diff --git a/src/renderer/src/components/TaskPage.tsx b/src/renderer/src/components/TaskPage.tsx index 88a2df8bd84..5d5232a3bbd 100644 --- a/src/renderer/src/components/TaskPage.tsx +++ b/src/renderer/src/components/TaskPage.tsx @@ -231,7 +231,9 @@ import { isLinearIssueSearchActive, shouldClearTeamDerivedFacets, shouldForceLinearIssueListRead, + shouldPersistLinearIssueView, teamDerivedFacetsForPrimaryTeamChange, + type LinearIssueListFilterRead, type LinearPrimaryTeamObservation } from '@/components/task-page-linear-issue-request' import { @@ -4400,7 +4402,7 @@ export default function TaskPage(): React.JSX.Element { [linearAttributeFilterWorkspaceId, linearIssueFiltersByWorkspaceId] ) // Why: null until the first list read, so a restored filter isn't mistaken for a change. - const linearAttributeFilterSignatureRef = useRef(null) + const linearAttributeFilterReadRef = useRef(null) const linearPrimaryTeamRef = useRef(null) const [linearViewMode, setLinearViewMode] = useState(DEFAULT_LINEAR_VIEW_MODE) const [linearGroupBy, setLinearGroupBy] = useState(DEFAULT_LINEAR_GROUP_BY) @@ -7413,12 +7415,11 @@ export default function TaskPage(): React.JSX.Element { // primary-team facet reset) persist too. The payload is workspace-independent, // so selecting a workspace mid-startup can't write a half-restored state. useEffect(() => { - if (!taskResumeApplied) { - return - } - if (!linearViewPersistReadyRef.current) { - // Why: the first pass after hydration carries the restored values; echoing them back is a no-op at best and a default-state stomp at worst. + const persistReady = linearViewPersistReadyRef.current + if (taskResumeApplied) { linearViewPersistReadyRef.current = true + } + if (!shouldPersistLinearIssueView({ taskResumeApplied, persistReady })) { return } setTaskResumeState({ @@ -7500,12 +7501,15 @@ export default function TaskPage(): React.JSX.Element { ) } - const nextFilterSignature = linearIssueAttributeFilterSignature(linearAttributeFilter) - const previousFilterSignature = linearAttributeFilterSignatureRef.current - linearAttributeFilterSignatureRef.current = nextFilterSignature + const nextFilterRead: LinearIssueListFilterRead = { + workspaceId: linearAttributeFilterWorkspaceId, + signature: linearIssueAttributeFilterSignature(linearAttributeFilter) + } + const previousFilterRead = linearAttributeFilterReadRef.current + linearAttributeFilterReadRef.current = nextFilterRead const filterForce = shouldForceLinearIssueListRead({ - previousFilterSignature, - nextFilterSignature, + previousFilterRead, + nextFilterRead, refreshForced: false }) @@ -9010,6 +9014,7 @@ export default function TaskPage(): React.JSX.Element { primaryTeam={linearAttributePrimaryTeam} selectedTeamIds={[...linearTeamSelection]} availableTeams={linearTeamOptions} + teamsSettled={availableTeams.length > 0} settings={linearTaskSourceContext ?? settings} /> ) : null} diff --git a/src/renderer/src/components/linear-issue-attribute-filter-dropdowns.test.tsx b/src/renderer/src/components/linear-issue-attribute-filter-dropdowns.test.tsx index 7dfd80d7143..4bdd6228d7d 100644 --- a/src/renderer/src/components/linear-issue-attribute-filter-dropdowns.test.tsx +++ b/src/renderer/src/components/linear-issue-attribute-filter-dropdowns.test.tsx @@ -99,6 +99,7 @@ describe('LinearIssueAttributeFilterDropdowns', () => { primaryTeam={team} selectedTeamIds={[]} availableTeams={[team]} + teamsSettled /> ) }) @@ -130,6 +131,7 @@ describe('LinearIssueAttributeFilterDropdowns', () => { primaryTeam={team} selectedTeamIds={[]} availableTeams={[team]} + teamsSettled /> ) }) @@ -163,6 +165,7 @@ describe('LinearIssueAttributeFilterDropdowns', () => { primaryTeam={team} selectedTeamIds={[]} availableTeams={[team]} + teamsSettled /> ) }) diff --git a/src/renderer/src/components/linear-issue-attribute-filter-dropdowns.tsx b/src/renderer/src/components/linear-issue-attribute-filter-dropdowns.tsx index bc2c94866dc..85e6553ba36 100644 --- a/src/renderer/src/components/linear-issue-attribute-filter-dropdowns.tsx +++ b/src/renderer/src/components/linear-issue-attribute-filter-dropdowns.tsx @@ -32,6 +32,8 @@ type Props = { /** Selected Linear team ids (All teams / multi-select). Empty → primary fallback. */ selectedTeamIds: readonly string[] availableTeams: readonly LinearTeam[] + /** False while `availableTeams` is still the issue-scraped fallback, not the real fetch. */ + teamsSettled: boolean settings?: RuntimeLinearSettings } @@ -71,6 +73,7 @@ export default function LinearIssueAttributeFilterDropdowns({ primaryTeam, selectedTeamIds, availableTeams, + teamsSettled, settings }: Props): React.JSX.Element { const [popoverOpen, setPopoverOpen] = useState(false) @@ -111,6 +114,12 @@ export default function LinearIssueAttributeFilterDropdowns({ if (activeTeamIds.length === 0 || !concreteWorkspaceId) { return } + // Why: restored filters make this run at startup, when availableTeams may still be + // the issue-scraped subset. Metadata complete for a partial team set looks valid, + // so pruning there would permanently delete facets from another team (R12). + if (!teamsSettled) { + return + } if (states.loading || labels.loading || members.loading) { return } @@ -143,6 +152,7 @@ export default function LinearIssueAttributeFilterDropdowns({ }, [ activeTeamIds, concreteWorkspaceId, + teamsSettled, states.loading, states.error, states.data, diff --git a/src/renderer/src/components/linear-issue-view-resume-wiring.test.ts b/src/renderer/src/components/linear-issue-view-resume-wiring.test.ts deleted file mode 100644 index 483d9c9aec5..00000000000 --- a/src/renderer/src/components/linear-issue-view-resume-wiring.test.ts +++ /dev/null @@ -1,58 +0,0 @@ -import { readFileSync } from 'node:fs' -import { join } from 'node:path' -import { describe, expect, it } from 'vitest' - -// Why: TaskPage is too large to render in a unit test, but the persistence -// contract lives in how its effects are wired, not in the pure module they call. -const SOURCE = readFileSync(join(__dirname, 'TaskPage.tsx'), 'utf8') - -const PERSIST_WRITE = 'linearIssueView: serializeLinearIssueViewResumeState(' - -function persistEffectSource(): string { - const write = SOURCE.indexOf(PERSIST_WRITE) - expect(write).toBeGreaterThan(0) - const start = SOURCE.lastIndexOf('useEffect(() => {', write) - const end = SOURCE.indexOf('])', write) - expect(start).toBeGreaterThan(0) - expect(end).toBeGreaterThan(write) - return SOURCE.slice(start, end) -} - -function hydrationEffectSource(): string { - const start = SOURCE.indexOf('if (taskResumeAppliedRef.current || !persistedUIReady') - expect(start).toBeGreaterThan(0) - const end = SOURCE.indexOf('taskResumeAppliedRef.current = true', start) - expect(end).toBeGreaterThan(start) - return SOURCE.slice(start, end) -} - -describe('Linear issue view resume wiring', () => { - it('writes the persisted view from exactly one effect, gated until after hydration', () => { - expect(SOURCE.match(/linearIssueView:/g)).toHaveLength(1) - - const effect = persistEffectSource() - expect(effect).toContain('if (!taskResumeApplied)') - // Why: the first pass after hydration must be skipped or startup rewrites the restored state. - expect(effect).toContain('linearViewPersistReadyRef.current = true') - }) - - it('keeps the persisted payload independent of the selected workspace', () => { - // Why: the payload carries every workspace's filter, so a workspace resolving - // or switching mid-startup cannot make the write depend on effect ordering. - expect(persistEffectSource()).not.toContain('selectedLinearWorkspaceId') - }) - - it('restores every workspace filter during hydration', () => { - expect(hydrationEffectSource()).toContain( - 'setLinearIssueFiltersByWorkspaceId(linearIssueView.filtersByWorkspaceId)' - ) - }) - - it('derives the active filter from the selected workspace instead of resetting it', () => { - expect(SOURCE).toContain('selectLinearWorkspaceIssueFilter(') - // Why: a mutable previous-workspace ref plus a reset effect is exactly the - // race this model replaces — a cold start would clear the restored filter. - expect(SOURCE).not.toContain('previousLinearWorkspaceIdForFiltersRef') - expect(SOURCE).not.toContain('setLinearAttributeFilter(') - }) -}) diff --git a/src/renderer/src/components/task-page-linear-issue-request.test.ts b/src/renderer/src/components/task-page-linear-issue-request.test.ts index 7f5a9e77f31..09bac285c07 100644 --- a/src/renderer/src/components/task-page-linear-issue-request.test.ts +++ b/src/renderer/src/components/task-page-linear-issue-request.test.ts @@ -5,6 +5,7 @@ import { isLinearIssueSearchActive, shouldClearTeamDerivedFacets, shouldForceLinearIssueListRead, + shouldPersistLinearIssueView, teamDerivedFacetsForPrimaryTeamChange } from './task-page-linear-issue-request' import type { LinearIssueAttributeFilter } from '../../../shared/linear-issue-attribute-filter' @@ -50,18 +51,18 @@ describe('task-page-linear-issue-request', () => { expect(signature).toContain('"stateIds":["s1"]') }) - it('forces a read when the filter signature changes', () => { + it('forces a read when the filter signature changes within one workspace', () => { expect( shouldForceLinearIssueListRead({ - previousFilterSignature: 'a', - nextFilterSignature: 'b', + previousFilterRead: { workspaceId: 'ws-1', signature: 'a' }, + nextFilterRead: { workspaceId: 'ws-1', signature: 'b' }, refreshForced: false }) ).toBe(true) expect( shouldForceLinearIssueListRead({ - previousFilterSignature: 'a', - nextFilterSignature: 'a', + previousFilterRead: { workspaceId: 'ws-1', signature: 'a' }, + nextFilterRead: { workspaceId: 'ws-1', signature: 'a' }, refreshForced: false }) ).toBe(false) @@ -70,20 +71,46 @@ describe('task-page-linear-issue-request', () => { it('does not force the first read of a session, so a restored filter serves warm cache', () => { expect( shouldForceLinearIssueListRead({ - previousFilterSignature: null, - nextFilterSignature: 'restored', + previousFilterRead: null, + nextFilterRead: { workspaceId: 'ws-1', signature: 'restored' }, refreshForced: false }) ).toBe(false) expect( shouldForceLinearIssueListRead({ - previousFilterSignature: null, - nextFilterSignature: 'restored', + previousFilterRead: null, + nextFilterRead: { workspaceId: 'ws-1', signature: 'restored' }, refreshForced: true }) ).toBe(true) }) + // Why: the list cache is workspace-keyed, so B's warm entry is already correct — + // forcing would mean a round trip behind a blocking spinner on every switch. + it('does not force on a workspace switch, even when each workspace has its own filter', () => { + expect( + shouldForceLinearIssueListRead({ + previousFilterRead: { workspaceId: 'ws-1', signature: 'filter-a' }, + nextFilterRead: { workspaceId: 'ws-2', signature: 'filter-b' }, + refreshForced: false + }) + ).toBe(false) + }) + + it('persists the Linear view only after hydration has taken its first pass', () => { + expect(shouldPersistLinearIssueView({ taskResumeApplied: false, persistReady: false })).toBe( + false + ) + // Why: the first pass after hydration carries the restored values, not a user change. + expect(shouldPersistLinearIssueView({ taskResumeApplied: true, persistReady: false })).toBe( + false + ) + expect(shouldPersistLinearIssueView({ taskResumeApplied: true, persistReady: true })).toBe(true) + expect(shouldPersistLinearIssueView({ taskResumeApplied: false, persistReady: true })).toBe( + false + ) + }) + it('clears team-derived facets only when the primary team changes within one workspace', () => { expect( shouldClearTeamDerivedFacets({ diff --git a/src/renderer/src/components/task-page-linear-issue-request.ts b/src/renderer/src/components/task-page-linear-issue-request.ts index 411fb0f05eb..440cce70285 100644 --- a/src/renderer/src/components/task-page-linear-issue-request.ts +++ b/src/renderer/src/components/task-page-linear-issue-request.ts @@ -54,10 +54,13 @@ export function buildLinearIssueListRequestSignature(options: { return `${sourceScope}::${workspace}::list::${options.filter ?? 'all'}::${options.limit}::${signature}` } +/** The last list read's filter, scoped to the workspace it was read for. */ +export type LinearIssueListFilterRead = { workspaceId: string | null; signature: string } + export function shouldForceLinearIssueListRead(options: { /** `null` until this session's first list read; a restored filter is a baseline, not a change. */ - previousFilterSignature: string | null - nextFilterSignature: string + previousFilterRead: LinearIssueListFilterRead | null + nextFilterRead: LinearIssueListFilterRead refreshForced: boolean }): boolean { if (options.refreshForced) { @@ -65,12 +68,29 @@ export function shouldForceLinearIssueListRead(options: { } // Why: forcing the very first read would bypass warm cache and show the blocking // spinner on every cold start that restores a persisted filter. - if (options.previousFilterSignature === null) { + if (options.previousFilterRead === null) { + return false + } + // Why: the list cache is keyed by workspace, so each workspace's filter is its own + // baseline — switching restores a different filter, which is not a filter *change*. + if (options.previousFilterRead.workspaceId !== options.nextFilterRead.workspaceId) { return false } // Why: filter signature changes always need a current server read, even when // returning to a previously cached signature that is still warm. - return options.previousFilterSignature !== options.nextFilterSignature + return options.previousFilterRead.signature !== options.nextFilterRead.signature +} + +/** + * The persist effect must skip its first pass after hydration: that pass carries the + * values just restored, so writing them back is a no-op at best and, before the + * restore lands, a default-state stomp. + */ +export function shouldPersistLinearIssueView(options: { + taskResumeApplied: boolean + persistReady: boolean +}): boolean { + return options.taskResumeApplied && options.persistReady } /** Where a primary team was observed; the team catalog is Linear-workspace scoped. */ diff --git a/src/renderer/src/components/task-page-linear-issue-view-boundary.test.ts b/src/renderer/src/components/task-page-linear-issue-view-boundary.test.ts new file mode 100644 index 00000000000..bb2728002b9 --- /dev/null +++ b/src/renderer/src/components/task-page-linear-issue-view-boundary.test.ts @@ -0,0 +1,63 @@ +import { readFileSync } from 'node:fs' +import { join } from 'node:path' +import { describe, expect, it } from 'vitest' + +// Why: TaskPage is too large to render in a unit test, so the decisions live in +// task-page-linear-issue-request.ts where they are unit-tested directly. What is +// left to pin here is that the effects delegate to them rather than re-deriving. +const TASK_PAGE_SOURCE = readFileSync(join(__dirname, 'TaskPage.tsx'), 'utf8') + +function sourceBetween(source: string, startPattern: string, endPattern: string): string { + const start = source.indexOf(startPattern) + expect(start).toBeGreaterThanOrEqual(0) + const end = source.indexOf(endPattern, start + startPattern.length) + expect(end).toBeGreaterThan(start) + return source.slice(start, end) +} + +function persistEffectSource(): string { + const write = TASK_PAGE_SOURCE.indexOf('linearIssueView: serializeLinearIssueViewResumeState(') + expect(write).toBeGreaterThan(0) + const start = TASK_PAGE_SOURCE.lastIndexOf('useEffect(() => {', write) + expect(start).toBeGreaterThan(0) + return TASK_PAGE_SOURCE.slice(start, TASK_PAGE_SOURCE.indexOf('])', write)) +} + +describe('TaskPage Linear issue view persistence boundary', () => { + it('writes the persisted view from exactly one effect, gated by the tested predicate', () => { + expect(TASK_PAGE_SOURCE.match(/linearIssueView:/g)).toHaveLength(1) + + const effect = persistEffectSource() + // Why: the gate is `shouldPersistLinearIssueView` in task-page-linear-issue-request.ts, + // which owns the hydration-first-pass skip; inlining it here would leave it untested. + expect(effect).toContain('shouldPersistLinearIssueView({') + expect( + sourceBetween(effect, 'shouldPersistLinearIssueView({', 'setTaskResumeState(') + ).toContain('return') + }) + + it('keeps the persisted payload independent of the selected workspace', () => { + // Why: the payload carries every workspace's filter, so a workspace resolving + // or switching mid-startup cannot make the write depend on effect ordering. + expect(persistEffectSource()).not.toContain('selectedLinearWorkspaceId') + }) + + it('restores every workspace filter during hydration', () => { + const hydration = sourceBetween( + TASK_PAGE_SOURCE, + 'if (taskResumeAppliedRef.current || !persistedUIReady', + 'taskResumeAppliedRef.current = true' + ) + expect(hydration).toContain( + 'setLinearIssueFiltersByWorkspaceId(linearIssueView.filtersByWorkspaceId)' + ) + }) + + it('derives the active filter from the selected workspace instead of resetting it', () => { + expect(TASK_PAGE_SOURCE).toContain('selectLinearWorkspaceIssueFilter(') + // Why: a mutable previous-workspace ref plus a reset effect is exactly the + // race this model replaces — a cold start would clear the restored filter. + expect(TASK_PAGE_SOURCE).not.toContain('previousLinearWorkspaceIdForFiltersRef') + expect(TASK_PAGE_SOURCE).not.toContain('setLinearAttributeFilter(') + }) +}) diff --git a/src/shared/linear-issue-attribute-filter.ts b/src/shared/linear-issue-attribute-filter.ts index d856b4ef9cc..62673a2a7b6 100644 --- a/src/shared/linear-issue-attribute-filter.ts +++ b/src/shared/linear-issue-attribute-filter.ts @@ -98,6 +98,31 @@ export function canonicalizeLinearIssueAttributeFilter( } } +/** + * Enforces the transport bounds that `canonicalize` does not. Canonical form is + * deduped but unbounded, so a filter built in memory can exceed limits that only + * the throwing parser checks — and be rejected the moment it crosses a schema. + */ +export function boundLinearIssueAttributeFilter( + filter: LinearIssueAttributeFilter +): LinearIssueAttributeFilter { + const withinLength = (id: string): boolean => + id.length > 0 && id.length <= LINEAR_ISSUE_ATTRIBUTE_FILTER_ID_MAX_LENGTH + return { + stateIds: filter.stateIds + .filter(withinLength) + .slice(0, LINEAR_ISSUE_ATTRIBUTE_FILTER_MAX_STATE_IDS), + priorities: filter.priorities.slice(0, LINEAR_ISSUE_ATTRIBUTE_FILTER_MAX_PRIORITIES), + assignee: + filter.assignee?.kind === 'user' && !withinLength(filter.assignee.id) + ? null + : filter.assignee, + labelIds: filter.labelIds + .filter(withinLength) + .slice(0, LINEAR_ISSUE_ATTRIBUTE_FILTER_MAX_LABEL_IDS) + } +} + export function isEmptyLinearIssueAttributeFilter( filter: LinearIssueAttributeFilter | null | undefined ): boolean { diff --git a/src/shared/linear-issue-view-resume-state.test.ts b/src/shared/linear-issue-view-resume-state.test.ts index a143ffa8311..f98ce5c988e 100644 --- a/src/shared/linear-issue-view-resume-state.test.ts +++ b/src/shared/linear-issue-view-resume-state.test.ts @@ -133,6 +133,39 @@ describe('normalizeLinearIssueViewResumeState', () => { }) }) +describe('isDefaultLinearIssueViewResumeState', () => { + // Why: this is the sole reason normalize returns undefined, which is what actually + // clears a persisted view when the user puts every setting back to its default. + it('drops a serialized default view so reverting a setting clears what was stored', () => { + expect( + normalizeLinearIssueViewResumeState( + serializeLinearIssueViewResumeState(defaultLinearIssueViewResumeState()) + ) + ).toBeUndefined() + }) + + it.each([ + ['viewMode', { viewMode: 'board' as const }], + ['groupBy', { groupBy: 'status' as const }], + ['orderBy', { orderBy: 'updated' as const }], + ['teamPropertyTouched', { teamPropertyTouched: true }], + ['displayProperties', { displayProperties: ['state' as const] }], + [ + 'filtersByWorkspaceId', + { filtersByWorkspaceId: { 'workspace-1': filter({ priorities: [1] }) } } + ] + ])('keeps a view that differs from the default in %s', (_label, overrides) => { + expect( + normalizeLinearIssueViewResumeState( + serializeLinearIssueViewResumeState({ + ...defaultLinearIssueViewResumeState(), + ...overrides + }) + ) + ).toBeDefined() + }) +}) + describe('serializeLinearIssueViewResumeState', () => { it('round-trips a restored view unchanged', () => { const restored = resolveLinearIssueViewResumeState({ @@ -217,6 +250,44 @@ describe('setLinearWorkspaceIssueFilter', () => { expect(selectLinearWorkspaceIssueFilter(filters, 'workspace-b')).toEqual(FILTER_B) }) + // Why: re-filtering an existing workspace used to reassign the key in place, leaving + // the workspace you just used at the head — first in line to be evicted. + it('evicts the least recently used workspace, not the one just re-filtered', () => { + let filters: Record = {} + for (let index = 0; index < LINEAR_ISSUE_VIEW_MAX_PERSISTED_WORKSPACES; index += 1) { + filters = setLinearWorkspaceIssueFilter( + filters, + `workspace-${index}`, + filter({ stateIds: [`state-${index}`] }) + ) + } + filters = setLinearWorkspaceIssueFilter(filters, 'workspace-0', FILTER_A) + filters = setLinearWorkspaceIssueFilter(filters, 'workspace-fresh', FILTER_B) + + expect(Object.keys(filters)).toHaveLength(LINEAR_ISSUE_VIEW_MAX_PERSISTED_WORKSPACES) + expect(selectLinearWorkspaceIssueFilter(filters, 'workspace-0')).toEqual(FILTER_A) + expect(selectLinearWorkspaceIssueFilter(filters, 'workspace-1')).toEqual( + emptyLinearIssueAttributeFilter() + ) + }) + + // Why: array-index-like keys enumerate before string keys regardless of insertion + // order, so key order alone would evict the entry this very write just added. + it('keeps an integer-like workspace key that the write just added', () => { + let filters: Record = {} + for (let index = 0; index < LINEAR_ISSUE_VIEW_MAX_PERSISTED_WORKSPACES; index += 1) { + filters = setLinearWorkspaceIssueFilter( + filters, + `workspace-${index}`, + filter({ stateIds: [`state-${index}`] }) + ) + } + + filters = setLinearWorkspaceIssueFilter(filters, '42', FILTER_A) + + expect(selectLinearWorkspaceIssueFilter(filters, '42')).toEqual(FILTER_A) + }) + it('returns the same record when the canonical filter is unchanged', () => { const filters = setLinearWorkspaceIssueFilter({}, 'workspace-a', FILTER_A) diff --git a/src/shared/linear-issue-view-resume-state.ts b/src/shared/linear-issue-view-resume-state.ts index 8ea21d47aa4..71863e5e0ac 100644 --- a/src/shared/linear-issue-view-resume-state.ts +++ b/src/shared/linear-issue-view-resume-state.ts @@ -4,6 +4,7 @@ // Persisted values are untrusted input: normalize before use, never assume shape. import { + boundLinearIssueAttributeFilter, canonicalizeLinearIssueAttributeFilter, emptyLinearIssueAttributeFilter, isEmptyLinearIssueAttributeFilter, @@ -96,6 +97,7 @@ function normalizeFiltersByWorkspaceId(value: unknown): Record = {} + const order: string[] = [] for (const [workspaceId, filter] of Object.entries(value)) { if (!isSafeWorkspaceKey(workspaceId)) { continue @@ -111,19 +113,31 @@ function normalizeFiltersByWorkspaceId(value: unknown): Record + filters: Record, + order: readonly string[] ): Record { - const entries = Object.entries(filters) - if (entries.length <= LINEAR_ISSUE_VIEW_MAX_PERSISTED_WORKSPACES) { + const kept = order.filter((workspaceId) => + Object.prototype.hasOwnProperty.call(filters, workspaceId) + ) + if (kept.length <= LINEAR_ISSUE_VIEW_MAX_PERSISTED_WORKSPACES) { return filters } - return Object.fromEntries(entries.slice(-LINEAR_ISSUE_VIEW_MAX_PERSISTED_WORKSPACES)) + const next: Record = {} + for (const workspaceId of kept.slice(-LINEAR_ISSUE_VIEW_MAX_PERSISTED_WORKSPACES)) { + next[workspaceId] = filters[workspaceId] as LinearIssueAttributeFilter + } + return next } /** Validates untrusted persisted state; returns undefined when nothing worth restoring survives. */ @@ -177,11 +191,18 @@ export function serializeLinearIssueViewResumeState( view: LinearIssueViewSelection ): LinearIssueViewResumeState { const filtersByWorkspaceId: Record = {} + const order: string[] = [] for (const [workspaceId, filter] of Object.entries(view.filtersByWorkspaceId)) { if (!isSafeWorkspaceKey(workspaceId) || isEmptyLinearIssueAttributeFilter(filter)) { continue } - filtersByWorkspaceId[workspaceId] = canonicalizeLinearIssueAttributeFilter(filter) + // Why: canonicalize dedupes but does not bound; only the throwing parser does, and + // an over-length facet list here is rejected by `ui.set` and drops the WHOLE + // taskResumeState for paired clients on every subsequent write. + filtersByWorkspaceId[workspaceId] = boundLinearIssueAttributeFilter( + canonicalizeLinearIssueAttributeFilter(filter) + ) + order.push(workspaceId) } return { viewMode: view.viewMode, @@ -189,7 +210,7 @@ export function serializeLinearIssueViewResumeState( orderBy: view.orderBy, displayProperties: orderLinearDisplayProperties(view.displayProperties), teamPropertyTouched: view.teamPropertyTouched, - filtersByWorkspaceId: trimToMostRecentWorkspaces(filtersByWorkspaceId) + filtersByWorkspaceId: trimToMostRecentWorkspaces(filtersByWorkspaceId, order) } } @@ -228,10 +249,14 @@ export function setLinearWorkspaceIssueFilter( return filters } const next = { ...filters } + delete next[workspaceId] if (isEmptyLinearIssueAttributeFilter(filter)) { - delete next[workspaceId] return next } + // Why: re-touching a workspace makes it the most recent, so it must be the last + // to be evicted — not the first, which reassigning in place would leave it. next[workspaceId] = canonicalizeLinearIssueAttributeFilter(filter) - return trimToMostRecentWorkspaces(next) + const order = Object.keys(filters).filter((key) => key !== workspaceId) + order.push(workspaceId) + return trimToMostRecentWorkspaces(next, order) }