diff --git a/frontend/taskdeck-web/src/store/captureStore.ts b/frontend/taskdeck-web/src/store/captureStore.ts index 9828ffdba..63c30c8a3 100644 --- a/frontend/taskdeck-web/src/store/captureStore.ts +++ b/frontend/taskdeck-web/src/store/captureStore.ts @@ -31,6 +31,16 @@ type DetailLoadOptions = { syncSummary?: boolean requestOptions?: CaptureReadOptions shouldCache?: () => boolean + /** + * Whether this read owns the store-wide `loadingDetail` flag (default true). + * + * That flag is what the open detail panel renders from: while it is set the + * panel body is replaced by a "Refreshing detail..." spinner and its Refresh + * Detail button is disabled. A background reconciliation of some OTHER + * capture must not do that to the detail the user is reading (#2304), so the + * quiet reads pass `false` and leave the flag to foreground loads. + */ + trackLoading?: boolean } export const BATCH_TRIAGE_POLL_INTERVAL_MS = 3_000 @@ -102,9 +112,13 @@ export const useCaptureStore = defineStore('capture', () => { const loadingList = ref(false) const loadingDetail = ref(false) let latestListLoadRequestId = 0 - let nextCaptureWriteGeneration = 0 + // One monotonic clock for both guards below. A write records it to reject + // older reads; a summary records it so an older BACKGROUND list snapshot + // cannot regress a row that moved after that read began (#2301). + let nextCaptureGeneration = 0 let latestListWriteGeneration = 0 const latestDetailWriteGenerationById = new Map() + const latestSummaryGenerationById = new Map() const actionBusyItemId = ref(null) const listError = ref(null) const detailError = ref(null) @@ -113,6 +127,10 @@ export const useCaptureStore = defineStore('capture', () => { const hasItems = computed(() => items.value.length > 0) function upsertSummary(summary: CaptureItemSummary) { + // Every per-item summary write — an explicit detail load, the single-item + // triage poll, an optimistic mutation — stamps the shared clock so a list + // read that started earlier cannot overwrite it with older status. + latestSummaryGenerationById.set(summary.id, ++nextCaptureGeneration) const existingIndex = items.value.findIndex((item) => item.id === summary.id) if (existingIndex >= 0) { items.value[existingIndex] = summary @@ -135,7 +153,7 @@ export const useCaptureStore = defineStore('capture', () => { * callers, but they must not replace the newer mutation response. */ function recordCaptureWrite(itemId: string, syncSummary: boolean) { - const generation = ++nextCaptureWriteGeneration + const generation = ++nextCaptureGeneration latestDetailWriteGenerationById.set(itemId, generation) if (syncSummary) { latestListWriteGeneration = generation @@ -146,6 +164,30 @@ export const useCaptureStore = defineStore('capture', () => { return latestDetailWriteGenerationById.get(itemId) ?? 0 } + /** + * Apply a BACKGROUND list snapshot without regressing rows that moved after + * the read began (#2301). + * + * `latestListWriteGeneration` rejects a whole snapshot that a mutation has + * outdated, but the single-item triage poll and an explicit detail load are + * READS: they write a fresher summary through `upsertSummary` without + * recording a capture write, so a slower batch-list response landing after + * them used to put the row back to `Triaging`. The snapshot stays the + * authority for membership and order — the list still owns scope and the + * newest-first cap — and only a row whose own summary is newer than this + * read keeps its local value. + */ + function applyBackgroundListSnapshot( + loadedItems: CaptureItemSummary[], + observedGeneration: number, + ) { + const currentById = new Map(items.value.map((item) => [item.id, item])) + items.value = loadedItems.map((loaded) => { + if ((latestSummaryGenerationById.get(loaded.id) ?? 0) <= observedGeneration) return loaded + return currentById.get(loaded.id) ?? loaded + }) + } + async function fetchItems(query?: CaptureListQuery) { const requestId = ++latestListLoadRequestId if (isDemoMode) { @@ -190,6 +232,7 @@ export const useCaptureStore = defineStore('capture', () => { syncSummary = true, requestOptions, shouldCache = () => true, + trackLoading = true, } = options if (!forceRefresh && detailById.value[itemId]) { @@ -207,7 +250,9 @@ export const useCaptureStore = defineStore('capture', () => { const observedDetailWriteGeneration = detailWriteGeneration(itemId) try { - loadingDetail.value = true + if (trackLoading) { + loadingDetail.value = true + } if (recordError) { detailError.value = null } @@ -228,7 +273,9 @@ export const useCaptureStore = defineStore('capture', () => { } throw e } finally { - loadingDetail.value = false + if (trackLoading) { + loadingDetail.value = false + } } } @@ -541,6 +588,10 @@ export const useCaptureStore = defineStore('capture', () => { // endpoint lags it briefly, do not regress the row back to Triaging // or reinsert an item that fell beyond the visible list cap. syncSummary: false, + // Quiet in every respect: this reconciliation runs for items the + // user may not have open, so it must not take the panel-wide + // loading flag away from whatever detail IS open (#2304). + trackLoading: false, requestOptions: options.requestOptions, shouldCache: isCurrent, }) @@ -562,8 +613,50 @@ export const useCaptureStore = defineStore('capture', () => { let deadlineTimerId: ReturnType | null = null let activeRequest: AbortController | null = null const refreshedDetailIds = new Set() + const countedTerminalIds = new Set() let observedPostEnqueueList = false + /** + * A tracked item whose terminal outcome this poll has actually observed + * since the batch was enqueued — the same truth `isComplete` reads, one id + * at a time. Cached pre-batch state never qualifies. + */ + function isObservedTerminal(itemId: string): boolean { + const summary = items.value.find((item) => item.id === itemId) + if (summary) { + return observedPostEnqueueList && isTriageTerminalStatus(summary.status) + } + const detail = detailById.value[itemId] + return Boolean( + detail && + refreshedDetailIds.has(itemId) && + isTriageTerminalStatus(detail.status), + ) + } + + /** + * Move the badges for outcomes this poll observed but has not counted yet + * (#2303). + * + * The count is `New + Failed`, so a single item finishing changes it — + * waiting for the whole batch left the sidebar and Home stale for up to a + * minute whenever one item lagged, and stale forever when the deadline + * stopped the poll first. The counted set makes this idempotent: an + * unchanged snapshot notifies nobody. + */ + function refreshCountsForNewTerminalOutcomes() { + let observedNewOutcome = false + for (const id of trackedIds) { + if (countedTerminalIds.has(id)) continue + if (!isObservedTerminal(id)) continue + countedTerminalIds.add(id) + observedNewOutcome = true + } + if (observedNewOutcome) { + notifyTriageCountChanged() + } + } + function stop() { if (stopped) return stopped = true @@ -583,6 +676,9 @@ export const useCaptureStore = defineStore('capture', () => { function stopAtDeadline() { if (stopped) return + // An outcome this poll already observed still moved the workload count, + // even when the tick that saw it was aborted here before reconciling. + refreshCountsForNewTerminalOutcomes() // The batch write already succeeded. A deadline only means automatic // checking stopped; the server-side triage may still be running. if (!isComplete()) { @@ -632,6 +728,7 @@ export const useCaptureStore = defineStore('capture', () => { const observedListLoadRequestId = latestListLoadRequestId const observedListWriteGeneration = latestListWriteGeneration + const observedSummaryGeneration = nextCaptureGeneration const controller = new AbortController() activeRequest = controller const requestOptions = { signal: controller.signal, skipRetry: true } @@ -645,7 +742,16 @@ export const useCaptureStore = defineStore('capture', () => { try { const loadedItems = await captureApi.listItems(query, requestOptions) if (!isCurrent()) return - items.value = loadedItems + applyBackgroundListSnapshot(loadedItems, observedSummaryGeneration) + // A foreground read failure hides every row behind its message, so a + // batch whose immediate post-POST refresh exhausted its retries left + // the inbox looking empty-and-broken until the user pressed Retry + // (#2305). This accepted snapshot is proof the same list is readable + // again, and it is the rows now on screen. Only this success path + // clears the error: an aborted, superseded (newer explicit load or + // newer capture write), 401 or 403 response fails `isCurrent()` or + // lands in the catch below and leaves the foreground error standing. + listError.value = null observedPostEnqueueList = true await refreshTerminalDetails(trackedIds, { requestOptions, @@ -653,8 +759,8 @@ export const useCaptureStore = defineStore('capture', () => { onRefreshed: (id) => refreshedDetailIds.add(id), }) if (!isCurrent()) return + refreshCountsForNewTerminalOutcomes() if (isComplete()) { - notifyTriageCountChanged() stop() return } diff --git a/frontend/taskdeck-web/src/tests/store/captureStore.spec.ts b/frontend/taskdeck-web/src/tests/store/captureStore.spec.ts index e4f66e796..be3240381 100644 --- a/frontend/taskdeck-web/src/tests/store/captureStore.spec.ts +++ b/frontend/taskdeck-web/src/tests/store/captureStore.spec.ts @@ -2181,4 +2181,327 @@ describe('captureStore', () => { }) }) + describe('background batch poll list truth', () => { + const NOW = new Date().toISOString() + + function summaryRow(id: string, status: string, errorMessage: string | null = null) { + return { + id, + userId: 'u1', + boardId: null, + status, + source: 'Typed', + textExcerpt: id, + createdAt: NOW, + processedAt: status === 'Triaging' || status === 'New' ? null : NOW, + errorMessage, + disposition: null, + } as never + } + + function detailFor(id: string, status: string) { + return { + id, + userId: 'u1', + boardId: null, + status, + source: 'Typed', + textExcerpt: id, + rawText: id, + createdAt: NOW, + processedAt: status === 'Triaging' || status === 'New' ? null : NOW, + retryCount: 0, + errorMessage: null, + provenance: null, + disposition: null, + } as never + } + + it('does not regress a newer single-item poll summary with a delayed batch snapshot', async () => { + vi.useFakeTimers() + try { + const store = useCaptureStore() + store.items = [summaryRow('c-1', 'Triaging'), summaryRow('c-2', 'Triaging')] + + let resolveList!: (value: unknown[]) => void + vi.mocked(captureApi.listItems).mockReturnValueOnce( + new Promise((resolve) => { resolveList = resolve }) as never, + ) + + const stopBatch = store.pollBatchTriageCompletion(['c-1'], { limit: 200 }) + await vi.advanceTimersByTimeAsync(3_000) + expect(captureApi.listItems).toHaveBeenCalledTimes(1) + + // The single-item poll observes the terminal outcome while the batch + // list read for the same item is still in flight. + vi.mocked(captureApi.getItem).mockResolvedValue(detailFor('c-1', 'ProposalCreated')) + const stopSingle = store.pollTriageCompletion('c-1') + await vi.advanceTimersByTimeAsync(2_000) + expect(store.items.find((item) => item.id === 'c-1')?.status).toBe('ProposalCreated') + + resolveList([summaryRow('c-1', 'Triaging'), summaryRow('c-2', 'ProposalCreated')]) + await Promise.resolve() + await Promise.resolve() + + expect(store.items.find((item) => item.id === 'c-1')?.status).toBe('ProposalCreated') + // The rest of the snapshot still lands: this is a per-item merge, not a + // rejected response. + expect(store.items.find((item) => item.id === 'c-2')?.status).toBe('ProposalCreated') + stopSingle() + stopBatch() + } finally { + vi.useRealTimers() + } + }) + + it('does not regress a newer explicit detail load with a delayed batch snapshot', async () => { + vi.useFakeTimers() + try { + const store = useCaptureStore() + store.items = [summaryRow('c-1', 'Triaging')] + + let resolveList!: (value: unknown[]) => void + vi.mocked(captureApi.listItems).mockReturnValueOnce( + new Promise((resolve) => { resolveList = resolve }) as never, + ) + + const stopBatch = store.pollBatchTriageCompletion(['c-1'], { limit: 200 }) + await vi.advanceTimersByTimeAsync(3_000) + expect(captureApi.listItems).toHaveBeenCalledTimes(1) + + vi.mocked(captureApi.getItem).mockResolvedValue(detailFor('c-1', 'Failed')) + await store.fetchDetail('c-1', { forceRefresh: true }) + expect(store.items.find((item) => item.id === 'c-1')?.status).toBe('Failed') + + resolveList([summaryRow('c-1', 'Triaging')]) + await Promise.resolve() + await Promise.resolve() + + expect(store.items.find((item) => item.id === 'c-1')?.status).toBe('Failed') + stopBatch() + } finally { + vi.useRealTimers() + } + }) + + it('reveals rows without a manual retry when a later background poll succeeds', async () => { + vi.useFakeTimers() + try { + const store = useCaptureStore() + vi.mocked(captureApi.batchTriage).mockResolvedValue({ + total: 1, + succeeded: 1, + failed: 0, + results: [{ itemId: 'c-1', success: true }], + }) + vi.mocked(captureApi.listItems) + .mockRejectedValueOnce(new Error('post-write-refresh-exhausted')) + .mockResolvedValue([summaryRow('c-1', 'ProposalCreated')] as never) + + await store.batchTriage(['c-1'], 'triage') + expect(store.listError).toBe('Failed to load inbox items') + expect(store.items).toEqual([]) + + store.pollBatchTriageCompletion(['c-1'], { limit: 200 }) + await vi.advanceTimersByTimeAsync(3_000) + + expect(store.listError).toBeNull() + expect(store.items.map((item) => item.id)).toEqual(['c-1']) + } finally { + vi.useRealTimers() + } + }) + + it('keeps a foreground list error when a superseded poll response lands', async () => { + vi.useFakeTimers() + try { + const store = useCaptureStore() + let resolveList!: (value: unknown[]) => void + vi.mocked(captureApi.listItems) + .mockReturnValueOnce(new Promise((resolve) => { resolveList = resolve }) as never) + .mockRejectedValueOnce(new Error('scope-load-failed')) + + const stop = store.pollBatchTriageCompletion(['c-1'], { limit: 200 }) + await vi.advanceTimersByTimeAsync(3_000) + expect(captureApi.listItems).toHaveBeenCalledTimes(1) + + await expect( + store.fetchItems({ limit: 200, boardId: 'board-b' }), + ).rejects.toThrow('scope-load-failed') + expect(store.listError).toBe('Failed to load inbox items') + + resolveList([summaryRow('c-1', 'ProposalCreated')]) + await Promise.resolve() + await Promise.resolve() + + expect(store.listError).toBe('Failed to load inbox items') + expect(store.items).toEqual([]) + stop() + } finally { + vi.useRealTimers() + } + }) + + it('leaves the store-wide detail loading flag alone during batch reconciliation', async () => { + vi.useFakeTimers() + try { + const store = useCaptureStore() + store.detailById['c-1'] = detailFor('c-1', 'Triaging') + vi.mocked(captureApi.listItems).mockResolvedValue([ + summaryRow('c-1', 'ProposalCreated'), + ] as never) + let resolveDetail!: (value: unknown) => void + vi.mocked(captureApi.getItem).mockReturnValueOnce( + new Promise((resolve) => { resolveDetail = resolve }) as never, + ) + + store.pollBatchTriageCompletion(['c-1'], { limit: 200 }) + await vi.advanceTimersByTimeAsync(3_000) + + // The reconciliation read is in flight. An unrelated open detail must + // keep its panel and its refresh control. + expect(captureApi.getItem).toHaveBeenCalledTimes(1) + expect(store.loadingDetail).toBe(false) + + resolveDetail(detailFor('c-1', 'ProposalCreated')) + await Promise.resolve() + await Promise.resolve() + expect(store.loadingDetail).toBe(false) + } finally { + vi.useRealTimers() + } + }) + + it('still raises the detail loading flag for a foreground detail load', async () => { + const store = useCaptureStore() + let resolveDetail!: (value: unknown) => void + vi.mocked(captureApi.getItem).mockReturnValueOnce( + new Promise((resolve) => { resolveDetail = resolve }) as never, + ) + + const load = store.fetchDetail('c-1', { forceRefresh: true }) + expect(store.loadingDetail).toBe(true) + + resolveDetail(detailFor('c-1', 'ProposalCreated')) + await load + expect(store.loadingDetail).toBe(false) + expect(store.detailError).toBeNull() + }) + + it('refreshes workload counts for a partial batch outcome and not for unchanged snapshots', async () => { + vi.useFakeTimers() + try { + const store = useCaptureStore() + vi.mocked(captureApi.listItems).mockResolvedValue([ + summaryRow('c-a', 'ProposalCreated'), + summaryRow('c-b', 'Triaging'), + ] as never) + + store.pollBatchTriageCompletion(['c-a', 'c-b'], { limit: 200 }) + await vi.advanceTimersByTimeAsync(3_000) + + // One tracked item reached a terminal outcome while its sibling is + // still running: the workload count already moved server-side. + expect(workspaceMocks.refreshWorkloadCounts).toHaveBeenCalledTimes(1) + + await vi.advanceTimersByTimeAsync(9_000) + expect(vi.mocked(captureApi.listItems).mock.calls.length).toBeGreaterThan(1) + expect(workspaceMocks.refreshWorkloadCounts).toHaveBeenCalledTimes(1) + + vi.mocked(captureApi.listItems).mockResolvedValue([ + summaryRow('c-a', 'ProposalCreated'), + summaryRow('c-b', 'Failed'), + ] as never) + await vi.advanceTimersByTimeAsync(3_000) + expect(workspaceMocks.refreshWorkloadCounts).toHaveBeenCalledTimes(2) + + const callsAtCompletion = vi.mocked(captureApi.listItems).mock.calls.length + await vi.advanceTimersByTimeAsync(9_000) + expect(captureApi.listItems).toHaveBeenCalledTimes(callsAtCompletion) + expect(workspaceMocks.refreshWorkloadCounts).toHaveBeenCalledTimes(2) + } finally { + vi.useRealTimers() + } + }) + + it('does not refresh workload counts from cached state when every poll read fails', async () => { + vi.useFakeTimers() + try { + const store = useCaptureStore() + // Terminal rows cached before the batch: nothing this poll observed. + store.items = [summaryRow('c-1', 'ProposalCreated')] + vi.mocked(captureApi.listItems).mockRejectedValue(new Error('list unavailable')) + + store.pollBatchTriageCompletion(['c-1'], { limit: 200 }) + await vi.advanceTimersByTimeAsync(BATCH_TRIAGE_POLL_MAX_DURATION_MS) + + expect(vi.mocked(captureApi.listItems).mock.calls.length).toBeGreaterThan(0) + expect(workspaceMocks.refreshWorkloadCounts).not.toHaveBeenCalled() + expect(store.batchError).toBe( + 'Automatic checking stopped after 60 seconds. Triage may still be running. Use Refresh Detail to check the result.', + ) + } finally { + vi.useRealTimers() + } + }) + + it('refreshes workload counts once at the deadline for an outcome observed but not reconciled', async () => { + vi.useFakeTimers() + try { + const store = useCaptureStore() + store.detailById['c-1'] = detailFor('c-1', 'Triaging') + vi.mocked(captureApi.listItems).mockResolvedValue([ + summaryRow('c-1', 'ProposalCreated'), + ] as never) + // The detail reconciliation for the terminal row never returns, so the + // tick that observed the outcome cannot finish before the deadline. + vi.mocked(captureApi.getItem).mockReturnValueOnce(new Promise(() => {}) as never) + + store.pollBatchTriageCompletion(['c-1'], { limit: 200 }) + await vi.advanceTimersByTimeAsync(3_000) + expect(captureApi.getItem).toHaveBeenCalledTimes(1) + expect(workspaceMocks.refreshWorkloadCounts).not.toHaveBeenCalled() + + await vi.advanceTimersByTimeAsync(BATCH_TRIAGE_POLL_MAX_DURATION_MS - 3_000) + expect(workspaceMocks.refreshWorkloadCounts).toHaveBeenCalledTimes(1) + expect(store.batchError).toBe( + 'Automatic checking stopped after 60 seconds. Triage may still be running. Use Refresh Detail to check the result.', + ) + expect(toastMocks.warning).toHaveBeenCalledTimes(1) + + await vi.advanceTimersByTimeAsync(9_000) + expect(workspaceMocks.refreshWorkloadCounts).toHaveBeenCalledTimes(1) + } finally { + vi.useRealTimers() + } + }) + + it.each([401, 403])( + 'keeps a foreground list error when the background poll loses access (%s)', + async (status) => { + vi.useFakeTimers() + try { + const store = useCaptureStore() + vi.mocked(captureApi.listItems) + .mockRejectedValueOnce(new Error('foreground-load-failed')) + .mockRejectedValue({ response: { status } } as never) + + await expect(store.fetchItems({ limit: 200 })).rejects.toThrow('foreground-load-failed') + expect(store.listError).toBe('Failed to load inbox items') + + store.pollBatchTriageCompletion(['c-1'], { limit: 200 }) + await vi.advanceTimersByTimeAsync(3_000) + expect(captureApi.listItems).toHaveBeenCalledTimes(2) + expect(store.listError).toBe('Failed to load inbox items') + + await vi.advanceTimersByTimeAsync(9_000) + expect(captureApi.listItems).toHaveBeenCalledTimes(2) + expect(store.listError).toBe('Failed to load inbox items') + } finally { + vi.useRealTimers() + } + }, + ) + }) + })