diff --git a/frontend/taskdeck-web/src/components/shell/AppShell.vue b/frontend/taskdeck-web/src/components/shell/AppShell.vue index 395ea78d0..ea64b183c 100644 --- a/frontend/taskdeck-web/src/components/shell/AppShell.vue +++ b/frontend/taskdeck-web/src/components/shell/AppShell.vue @@ -3,6 +3,7 @@ import { computed, onMounted, onUnmounted, ref, watch } from 'vue' import { useRouter } from 'vue-router' import { useSessionStore } from '../../store/sessionStore' import { useWorkspaceStore } from '../../store/workspaceStore' +import { useCaptureStore } from '../../store/captureStore' import { usePaperThemeStore } from '../../store/paperThemeStore' import { useCaptureQueueSync } from '../../composables/useCaptureQueueSync' import { registerEscapeHandler } from '../../composables/useEscapeStack' @@ -42,6 +43,7 @@ type SidebarRef = { const router = useRouter() const session = useSessionStore() const workspace = useWorkspaceStore() +const capture = useCaptureStore() const paperTheme = usePaperThemeStore() const { mode: viewportMode } = useViewportMode() @@ -303,6 +305,9 @@ watch( (isAuthenticated) => { if (!isAuthenticated) { workspace.resetForLogout() + // The capture store's per-item generation guards are keyed by capture id + // and belong to the session that recorded them (#2571). + capture.resetForLogout() return } diff --git a/frontend/taskdeck-web/src/store/captureStore.ts b/frontend/taskdeck-web/src/store/captureStore.ts index 7f2151a3d..3d0e6c45e 100644 --- a/frontend/taskdeck-web/src/store/captureStore.ts +++ b/frontend/taskdeck-web/src/store/captureStore.ts @@ -119,6 +119,10 @@ export const useCaptureStore = defineStore('capture', () => { let latestListWriteGeneration = 0 const latestDetailWriteGenerationById = new Map() const latestSummaryGenerationById = new Map() + // Bumped by `resetForLogout`. Every detail read captures it when the request + // is issued, so a read that crosses a logout is dropped outright instead of + // being compared against generations the reset has already discarded. + let sessionEpoch = 0 const actionBusyItemId = ref(null) const listError = ref(null) const detailError = ref(null) @@ -272,6 +276,7 @@ export const useCaptureStore = defineStore('capture', () => { } const observedDetailWriteGeneration = detailWriteGeneration(itemId) + const observedSessionEpoch = sessionEpoch try { if (trackLoading) { loadingDetail.value = true @@ -283,6 +288,9 @@ export const useCaptureStore = defineStore('capture', () => { ? await captureApi.getItem(itemId, requestOptions) : await captureApi.getItem(itemId) if (!shouldCache()) return detail + // Before any generation compare: the reset discards the generations this + // read observed, so after a logout the compare is no longer meaningful. + if (observedSessionEpoch !== sessionEpoch) return detail if (observedDetailWriteGeneration !== detailWriteGeneration(itemId)) return detail cacheDetail(detail, syncSummary) return detail @@ -463,9 +471,15 @@ export const useCaptureStore = defineStore('capture', () => { try { const observedDetailWriteGeneration = detailWriteGeneration(itemId) + const observedSessionEpoch = sessionEpoch const detail = await captureApi.getItem(itemId) if (stopped) return - if (observedDetailWriteGeneration === detailWriteGeneration(itemId)) { + // The epoch is checked first: a logout discards the generations this + // read observed, so its response can no longer be reconciled. + if ( + observedSessionEpoch === sessionEpoch && + observedDetailWriteGeneration === detailWriteGeneration(itemId) + ) { cacheDetail(detail) if (isTriageTerminalStatus(detail.status)) { @@ -611,9 +625,25 @@ 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). + // Quiet in every respect, for BOTH callers: this reconciliation + // runs over the tracked batch, not over whatever detail is open, + // so it must not take the panel-wide loading flag away from that + // detail (#2304). + // + // `batchTriage` is a FOREGROUND caller and still reconciles + // quietly on purpose (#2571). `loadingDetail` is one store-wide + // boolean: raising it here would blank the panel and disable + // Refresh Detail for an open capture that is not in the batch + // selection, and the first of these parallel reads to settle would + // clear the flag under any genuine foreground detail load still in + // flight. The foreground feedback for a batch is `batchBusy`, + // which stays true for the whole `batchTriage` body and renders + // "Processing..." on the Legacy skin's batch buttons + // (`LegacyInboxView.vue` -> `InboxListPanel.vue`). That is the only + // skin either flag reaches: nothing under `views/paper/` binds + // `batchBusy` or `loadingDetail`, so the Paper inbox neither gains + // nor loses anything here. The `trackLoading` docstring above + // describes the same Legacy panel. trackLoading: false, requestOptions: options.requestOptions, shouldCache: isCurrent, @@ -923,6 +953,31 @@ export const useCaptureStore = defineStore('capture', () => { } } + /** + * End the session's per-item generation bookkeeping (#2571). + * + * EVICTION, not a bound. Both maps are keyed by capture id and take an entry + * for every summary write and every successful mutation, and nothing else + * ever removes one, so a session's entries would otherwise live as long as + * the store. This is the only eviction point. WITHIN a session both still + * grow by one entry per distinct capture id touched, exactly as before. + * + * INVALIDATION is what makes an in-flight read safe, and clearing the maps + * alone would not: it would INVERT the detail write guard. A read that + * starts with no recorded write for its id observes generation 0; if a write + * then lands (generation N) and the map is cleared, the read's compare + * becomes 0 !== 0, which is false, and the PRE-write body would be cached + * over the newer one and its status pushed back into the list. So the epoch + * moves here, after the clear: every detail read captures it when it issues + * its request and drops its response when it moved, before any generation + * compare. The shared clock itself stays monotonic. + */ + function resetForLogout() { + latestDetailWriteGenerationById.clear() + latestSummaryGenerationById.clear() + sessionEpoch += 1 + } + return { items, detailById, @@ -950,5 +1005,6 @@ export const useCaptureStore = defineStore('capture', () => { pollBatchTriageCompletion, batchTriage, updateSuggestion, + resetForLogout, } }) diff --git a/frontend/taskdeck-web/src/tests/components/AppShell.paperVariant.spec.ts b/frontend/taskdeck-web/src/tests/components/AppShell.paperVariant.spec.ts index b3b24e821..557547ee6 100644 --- a/frontend/taskdeck-web/src/tests/components/AppShell.paperVariant.spec.ts +++ b/frontend/taskdeck-web/src/tests/components/AppShell.paperVariant.spec.ts @@ -84,6 +84,10 @@ vi.mock('../../store/workspaceStore', () => ({ useWorkspaceStore: () => mockWorkspace, })) +vi.mock('../../store/captureStore', () => ({ + useCaptureStore: () => ({ resetForLogout: vi.fn() }), +})) + vi.mock('../../store/paperThemeStore', () => ({ usePaperThemeStore: () => mockPaperTheme, })) diff --git a/frontend/taskdeck-web/src/tests/components/AppShell.spec.ts b/frontend/taskdeck-web/src/tests/components/AppShell.spec.ts index 93b692705..7e722bb71 100644 --- a/frontend/taskdeck-web/src/tests/components/AppShell.spec.ts +++ b/frontend/taskdeck-web/src/tests/components/AppShell.spec.ts @@ -1,6 +1,6 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest' import { mount } from '@vue/test-utils' -import { defineComponent, h, reactive } from 'vue' +import { defineComponent, h, nextTick, reactive } from 'vue' import AppShell from '../../components/shell/AppShell.vue' import { useShellKeyboardHelp, @@ -30,11 +30,11 @@ const mockRoute = reactive({ path: '/workspace/home', }) -const mockSession = { +const mockSession = reactive({ isAuthenticated: true, username: 'test-user', logout: vi.fn(), -} +}) const mockFeatureFlags = { flags: { @@ -80,6 +80,14 @@ vi.mock('../../store/workspaceStore', () => ({ useWorkspaceStore: () => mockWorkspace, })) +const mockCapture = { + resetForLogout: vi.fn(), +} + +vi.mock('../../store/captureStore', () => ({ + useCaptureStore: () => mockCapture, +})) + const mockPaperTheme = reactive({ mode: 'off' as 'off' | 'paper' | 'paper-night' | 'auto', isOn: false, @@ -149,6 +157,7 @@ describe('AppShell workspace navigation and command palette', () => { mockWorkspace.preferencesHydrated = false mockFeatureFlags.isEnabled = vi.fn((_flag: keyof FeatureFlags) => true) mockPaperTheme.isOn = false + mockSession.isAuthenticated = true injectedShellHelp = null }) @@ -666,4 +675,17 @@ describe('AppShell workspace navigation and command palette', () => { expect(mockWorkspace.fetchHomeSummary).not.toHaveBeenCalled() }) + + it('resets the workspace and capture stores when the session ends', async () => { + mountedWrapper = mountShell() + expect(mockCapture.resetForLogout).not.toHaveBeenCalled() + + mockSession.isAuthenticated = false + await nextTick() + + // The capture store carries per-item generation guards keyed by capture + // id, so they belong to the session that recorded them (#2571). + expect(mockWorkspace.resetForLogout).toHaveBeenCalledOnce() + expect(mockCapture.resetForLogout).toHaveBeenCalledOnce() + }) }) diff --git a/frontend/taskdeck-web/src/tests/store/captureStore.spec.ts b/frontend/taskdeck-web/src/tests/store/captureStore.spec.ts index 0254176d0..4a2e1936d 100644 --- a/frontend/taskdeck-web/src/tests/store/captureStore.spec.ts +++ b/frontend/taskdeck-web/src/tests/store/captureStore.spec.ts @@ -2443,6 +2443,70 @@ describe('captureStore', () => { } }) + it('keeps a foreground list error when the poll is stopped mid-flight', async () => { + vi.useFakeTimers() + try { + const store = useCaptureStore() + let resolveList!: (value: unknown[]) => void + vi.mocked(captureApi.listItems) + .mockRejectedValueOnce(new Error('foreground-load-failed')) + .mockReturnValueOnce(new Promise((resolve) => { resolveList = resolve }) as never) + + await expect(store.fetchItems({ limit: 200 })).rejects.toThrow('foreground-load-failed') + expect(store.listError).toBe('Failed to load inbox items') + + const stop = store.pollBatchTriageCompletion(['c-1'], { limit: 200 }) + await vi.advanceTimersByTimeAsync(3_000) + expect(captureApi.listItems).toHaveBeenCalledTimes(2) + + // The orchestrator cancels this poll on a board or archived-history + // change and on unmount, which aborts the tick's read in flight. A + // response that lands after that is not proof the list the user is + // looking at became readable, so it must not clear their error (#2305). + stop() + resolveList([summaryRow('c-1', 'ProposalCreated')]) + await Promise.resolve() + await Promise.resolve() + + expect(store.listError).toBe('Failed to load inbox items') + expect(store.items).toEqual([]) + } finally { + vi.useRealTimers() + } + }) + + it('keeps a foreground list error when the deadline aborts a poll read mid-flight', async () => { + vi.useFakeTimers() + try { + const store = useCaptureStore() + let resolveList!: (value: unknown[]) => void + vi.mocked(captureApi.listItems) + .mockRejectedValueOnce(new Error('foreground-load-failed')) + .mockReturnValueOnce(new Promise((resolve) => { resolveList = resolve }) 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(BATCH_TRIAGE_POLL_MAX_DURATION_MS) + + // The 60 s deadline aborted the one read that was still in flight. + expect(captureApi.listItems).toHaveBeenCalledTimes(2) + expect(store.batchError).toBe( + 'Automatic checking stopped after 60 seconds. Triage may still be running. Use Refresh Detail to check the result.', + ) + + resolveList([summaryRow('c-1', 'ProposalCreated')]) + await Promise.resolve() + await Promise.resolve() + + expect(store.listError).toBe('Failed to load inbox items') + expect(store.items).toEqual([]) + } finally { + vi.useRealTimers() + } + }) + it('leaves the store-wide detail loading flag alone during batch reconciliation', async () => { vi.useFakeTimers() try { @@ -2459,8 +2523,10 @@ describe('captureStore', () => { 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. + // The reconciliation read is in flight. It is a quiet read, so + // whatever detail the panel has open keeps its body and its Refresh + // Detail control — including this row, which is the one being + // reconciled here. expect(captureApi.getItem).toHaveBeenCalledTimes(1) expect(store.loadingDetail).toBe(false) @@ -2473,6 +2539,59 @@ describe('captureStore', () => { } }) + it('leaves the store-wide detail loading flag alone through a foreground batch triage', async () => { + const store = useCaptureStore() + store.detailById['c-1'] = detailFor('c-1', 'Triaging') + store.detailById['c-2'] = detailFor('c-2', 'Triaging') + vi.mocked(captureApi.batchTriage).mockResolvedValue({ + total: 2, + succeeded: 2, + failed: 0, + results: [ + { itemId: 'c-1', success: true }, + { itemId: 'c-2', success: true }, + ], + }) + vi.mocked(captureApi.listItems).mockResolvedValue([ + summaryRow('c-1', 'ProposalCreated'), + summaryRow('c-2', 'ProposalCreated'), + ] as never) + const pendingDetails = new Map void>() + vi.mocked(captureApi.getItem).mockImplementation(((itemId: string) => + new Promise((resolve) => { pendingDetails.set(itemId, resolve) })) as never) + + // Two stale ids, so the reconciliation runs two reads in parallel. + const batch = store.batchTriage(['c-1', 'c-2'], 'triage') + await vi.waitFor(() => expect(captureApi.getItem).toHaveBeenCalledTimes(2)) + + // `batchTriage` is a foreground action and its reconciliation is still + // quiet (#2571). `batchBusy` is the foreground state for a batch and it + // is up for the whole body. + expect(store.batchBusy).toBe(true) + expect(store.loadingDetail).toBe(false) + + // A genuine foreground detail load starts while both reconciliation + // reads are still in flight. It owns the store-wide flag. + const foreground = store.fetchDetail('c-3', { forceRefresh: true }) + expect(store.loadingDetail).toBe(true) + + // The first reconciliation leg settles. Were those reads tracking the + // flag, this would clear it under the load that still owns it. + pendingDetails.get('c-1')!(detailFor('c-1', 'ProposalCreated')) + await Promise.resolve() + await Promise.resolve() + expect(store.loadingDetail).toBe(true) + + pendingDetails.get('c-2')!(detailFor('c-2', 'ProposalCreated')) + await batch + expect(store.batchBusy).toBe(false) + expect(store.loadingDetail).toBe(true) + + pendingDetails.get('c-3')!(detailFor('c-3', 'ProposalCreated')) + await foreground + expect(store.loadingDetail).toBe(false) + }) + it('still raises the detail loading flag for a foreground detail load', async () => { const store = useCaptureStore() let resolveDetail!: (value: unknown) => void @@ -2525,6 +2644,42 @@ describe('captureStore', () => { } }) + it('coalesces the workload count refresh to one call per poll tick', async () => { + vi.useFakeTimers() + try { + const store = useCaptureStore() + vi.mocked(captureApi.listItems).mockResolvedValue([ + summaryRow('c-a', 'ProposalCreated'), + summaryRow('c-b', 'ProposalCreated'), + summaryRow('c-c', 'ProposalCreated'), + summaryRow('c-d', 'Triaging'), + ] as never) + + store.pollBatchTriageCompletion(['c-a', 'c-b', 'c-c', 'c-d'], { limit: 200 }) + await vi.advanceTimersByTimeAsync(3_000) + + // Three tracked ids reached a terminal outcome in the SAME tick. + // `New + Failed` is one number read from one endpoint — the heaviest on + // the surface — so the tick refreshes it once, not once per id (#2571). + expect(workspaceMocks.refreshWorkloadCounts).toHaveBeenCalledTimes(1) + + vi.mocked(captureApi.listItems).mockResolvedValue([ + summaryRow('c-a', 'ProposalCreated'), + summaryRow('c-b', 'ProposalCreated'), + summaryRow('c-c', 'ProposalCreated'), + summaryRow('c-d', 'Failed'), + ] as never) + await vi.advanceTimersByTimeAsync(3_000) + + // A second tick with its own new outcome is a second refresh: the + // coalescing is per tick, which is what keeps the badge honest while + // a batch finishes over the poll window. + 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 { @@ -2603,6 +2758,117 @@ describe('captureStore', () => { } }, ) + + describe('resetForLogout', () => { + it('clears the per-item summary generation guard', 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 stop = store.pollBatchTriageCompletion(['c-1'], { limit: 200 }) + await vi.advanceTimersByTimeAsync(3_000) + expect(captureApi.listItems).toHaveBeenCalledTimes(1) + + // A newer explicit read stamps c-1's summary generation above the + // in-flight snapshot's, which is what pins the row (#2301). + 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') + + store.resetForLogout() + + resolveList([summaryRow('c-1', 'Triaging')]) + await Promise.resolve() + await Promise.resolve() + + // The entry is gone, so nothing stale-high is left to pin the row. + expect(store.items.find((item) => item.id === 'c-1')?.status).toBe('Triaging') + stop() + } finally { + vi.useRealTimers() + } + }) + + it('drops a detail read that a write and a logout crossed', async () => { + const store = useCaptureStore() + + // Nothing has been written for c-1 yet, so this read observes + // generation 0 — the value a cleared map also reads back. + 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 }) + + // A successful write lands while that read is in flight. It is the + // write generation that would normally reject the older response. + vi.mocked(captureApi.batchTriage).mockResolvedValue({ + total: 1, + succeeded: 1, + failed: 0, + results: [{ itemId: 'c-1', success: true }], + }) + vi.mocked(captureApi.listItems).mockResolvedValue([ + summaryRow('c-1', 'ProposalCreated'), + ] as never) + await store.batchTriage(['c-1'], 'triage') + expect(store.items.find((item) => item.id === 'c-1')?.status).toBe('ProposalCreated') + + store.resetForLogout() + + resolveDetail(detailFor('c-1', 'Triaging')) + await load + + // Clearing the maps alone would put the compare back at 0 !== 0 and + // cache this pre-write body. The session epoch drops it instead. + expect(store.detailById['c-1']).toBeUndefined() + expect(store.items.find((item) => item.id === 'c-1')?.status).toBe('ProposalCreated') + }) + + it('drops a single-item triage poll read that a write and a logout crossed', async () => { + vi.useFakeTimers() + try { + const store = useCaptureStore() + let resolveDetail!: (value: unknown) => void + vi.mocked(captureApi.getItem).mockReturnValueOnce( + new Promise((resolve) => { resolveDetail = resolve }) as never, + ) + + const stop = store.pollTriageCompletion('c-1') + await vi.advanceTimersByTimeAsync(2_000) + expect(captureApi.getItem).toHaveBeenCalledTimes(1) + + vi.mocked(captureApi.batchTriage).mockResolvedValue({ + total: 1, + succeeded: 1, + failed: 0, + results: [{ itemId: 'c-1', success: true }], + }) + vi.mocked(captureApi.listItems).mockResolvedValue([ + summaryRow('c-1', 'ProposalCreated'), + ] as never) + await store.batchTriage(['c-1'], 'triage') + + store.resetForLogout() + + resolveDetail(detailFor('c-1', 'Triaging')) + await Promise.resolve() + await Promise.resolve() + + expect(store.detailById['c-1']).toBeUndefined() + expect(store.items.find((item) => item.id === 'c-1')?.status).toBe('ProposalCreated') + stop() + } finally { + vi.useRealTimers() + } + }) + }) }) })