diff --git a/frontend/taskdeck-web/src/composables/usePaperReviewSelectors.ts b/frontend/taskdeck-web/src/composables/usePaperReviewSelectors.ts index 75f123556..36e200840 100644 --- a/frontend/taskdeck-web/src/composables/usePaperReviewSelectors.ts +++ b/frontend/taskdeck-web/src/composables/usePaperReviewSelectors.ts @@ -127,7 +127,28 @@ export interface SimilarPastRow { date: string } -export type CoreSelectorBatchOutcome = 'settled' | 'failed' | 'superseded' | 'unavailable' +/** + * How one exact-key core evidence batch ended. + * + * - `settled` every one of the six proposal reads landed for the requested key. + * - `failed` at least one read rejected on its own; the snapshot is incomplete. + * - `superseded` the reviewer moved to another proposal or revision mid-flight. + * - `unavailable` the requested key is not the one this surface is rendering. + * - `aborted` the CALLER cancelled through its own signal. Kept distinct from + * `failed` because nothing failed: only the caller knows why it cancelled + * (a deadline, a teardown), so only the caller may name that reason. + */ +export type CoreSelectorBatchOutcome = + | 'settled' + | 'failed' + | 'superseded' + | 'unavailable' + | 'aborted' + +/** Caller-owned cancellation for one explicit core-batch wait. */ +export interface CoreSelectorBatchWaitOptions { + signal?: AbortSignal +} export interface PaperReviewSelectors { provenance: ComputedRef @@ -143,6 +164,7 @@ export interface PaperReviewSelectors { waitForCoreBatch: ( proposalId: string, revisionIdentity: string | null, + options?: CoreSelectorBatchWaitOptions, ) => Promise } @@ -533,6 +555,22 @@ export function usePaperReviewSelectors( similarPastData.value = EMPTY_SIMILAR } + /** + * Cancel the in-flight batch for ONE key without disturbing the generation + * bookkeeping. `invalidateCoreBatch` is the stronger neighbour: it also + * supersedes the batch's waiters. Here the waiter reports its own outcome, so + * the batch is left to reach its ordinary failure branch (which clears + * `loading`) instead of being declared superseded. + * + * The key check matters: a reviewer who moved to another proposal while the + * caller was waiting has a NEW batch in flight, and a late cancellation from + * the abandoned wait must not tear that one down. + */ + function abortInFlightCoreBatchForKey(key: SelectorKey) { + if (!activeCoreBatch || !selectorKeysEqual(activeCoreBatch.key, key)) return + abortController?.abort() + } + function invalidateCoreBatch() { fetchGeneration += 1 activeCoreBatch?.supersede() @@ -728,16 +766,56 @@ export function usePaperReviewSelectors( return promise } + /** + * Await the exact-key core batch on behalf of an explicit reviewer action. + * + * The optional `signal` belongs to the CALLER, not to this composable. When + * it fires the wait resolves `aborted` immediately and the in-flight batch is + * cancelled, so a caller holding a decision lock is released even when the + * transport (or a test double) ignores cancellation and never settles. The + * batch's own continuations still run behind their generation guard, so a + * late answer can neither publish evidence for a key the reviewer has left + * nor be mistaken for this wait's result. + */ function waitForCoreBatch( proposalId: string, revisionIdentity: string | null, + options?: CoreSelectorBatchWaitOptions, ): Promise { + const signal = options?.signal + if (signal?.aborted) return Promise.resolve('aborted') const key = selectorKeyForProposal(activeProposal.value) if ( !key || !proposalIdsEqual(key.proposalId, proposalId) || !nullableIdentifiersEqual(key.revisionIdentity, revisionIdentity) ) return Promise.resolve('unavailable') + + if (!signal) return runCoreBatchWithSameActionRetry(key) + + let onAbort: (() => void) | null = null + const aborted = new Promise((resolve) => { + onAbort = () => { + // Cancel the reads this wait is holding open before reporting, so the + // abandoned batch stops occupying the transport. + abortInFlightCoreBatchForKey(key) + resolve('aborted') + } + signal.addEventListener('abort', onAbort, { once: true }) + }) + + return Promise.race([runCoreBatchWithSameActionRetry(key), aborted]).finally(() => { + if (onAbort) signal.removeEventListener('abort', onAbort) + }) + } + + /** + * One explicit wait retries a failed batch once, inside the same action + * (#2528). A second failure is reported honestly rather than retried again. + */ + function runCoreBatchWithSameActionRetry( + key: SelectorKey, + ): Promise { const promise = ensureCoreBatch(key) return promise.then((outcome) => { if ( diff --git a/frontend/taskdeck-web/src/composables/useReviewProposals.ts b/frontend/taskdeck-web/src/composables/useReviewProposals.ts index 1134f50c8..ab7a2240e 100644 --- a/frontend/taskdeck-web/src/composables/useReviewProposals.ts +++ b/frontend/taskdeck-web/src/composables/useReviewProposals.ts @@ -65,7 +65,26 @@ export const REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD = 3 * intentionally keeps its historical `Promise` contract for action * composables that only need a best-effort refresh. */ -export type ProposalLoadOutcome = 'landed' | 'failed' | 'superseded' +export type ProposalLoadOutcome = 'landed' | 'failed' | 'superseded' | 'aborted' + +/** + * Cancellation and retry controls for an explicit queue read whose caller owns + * a deadline. + * + * `aborted` is reported instead of `failed` so a caller that cancelled its own + * read never mistakes it for a server or transport failure: only the caller + * knows why it aborted, and only the caller can tell a deadline apart from any + * other cancellation. The composable deliberately makes no such judgement. + * + * `skipRetry` matters for a deadline-bounded caller: the shared interceptor + * retries an idempotent read up to `MAX_RETRIES` times with doubling backoff, + * which can consume most of a caller's budget and turn a recoverable failure + * into a reported timeout. A bounded caller wants the first honest answer. + */ +export interface ProposalLoadOptions { + signal?: AbortSignal + skipRetry?: boolean +} /** * Decision rules shared by every review surface (Paper deep-review and the @@ -448,7 +467,7 @@ export function useReviewProposals() { } } - async function openProposalFromHash() { + async function openProposalFromHash(options?: ProposalLoadOptions) { if (proposalsLoading.value) return const proposalId = getProposalIdFromHash(route.hash) if (!proposalId) { @@ -473,7 +492,12 @@ export function useReviewProposals() { } try { - const fetchedProposal = await automationApi.getProposal(proposalId) + const fetchedProposal = options + ? await automationApi.getProposal(proposalId, { + signal: options.signal, + skipRetry: options.skipRetry, + }) + : await automationApi.getProposal(proposalId) if (!proposalIdsEqual(getProposalIdFromHash(route.hash), proposalId)) return // A route lookup may canonicalize GUID hex casing, but it may not return a // different record. Retain the hash as unavailable instead of upserting a @@ -490,6 +514,9 @@ export function useReviewProposals() { await nextTick() await scrollToProposalFromHash() } catch (e: unknown) { + // A caller-owned cancellation is not a lookup failure: the deep-link read + // was cut short deliberately and its caller reports the real outcome. + if (options?.signal?.aborted) return if (!proposalIdsEqual(getProposalIdFromHash(route.hash), proposalId)) return if (isHttpNotFound(e)) { unavailableProposalId.value = proposalId @@ -511,18 +538,33 @@ export function useReviewProposals() { await safeReplace({ name: 'workspace-review', query: route.query }) } - async function loadProposalsWithOutcome(): Promise { + async function loadProposalsWithOutcome( + options?: ProposalLoadOptions, + ): Promise { + const signal = options?.signal + if (signal?.aborted) return 'aborted' reviewLoadPerf.start() const requestId = ++latestProposalLoadRequestId let outcome: ProposalLoadOutcome = 'landed' try { proposalsLoading.value = true - const loadedProposals = await automationApi.getProposals({ + const filters = { limit: 200, boardId: activeBoardFilter.value || undefined, - }) + } + // The second argument is forwarded ONLY when a caller supplied options, + // so every existing call site keeps its exact single-argument shape. + const loadedProposals = options + ? await automationApi.getProposals(filters, { + signal, + skipRetry: options.skipRetry, + }) + : await automationApi.getProposals(filters) if (requestId !== latestProposalLoadRequestId) return 'superseded' + // An answer the caller stopped waiting for must not become the rendered + // authority behind its back, and proves nothing about queue freshness. + if (signal?.aborted) return 'aborted' proposals.value = loadedProposals // An explicit successful load is as trustworthy as a successful poll and // clears any older degraded indication without changing load semantics. @@ -534,6 +576,10 @@ export function useReviewProposals() { if (accessWasRevoked) resumeQueueRefreshAfterPermissionRecovery() } catch (e: unknown) { if (requestId !== latestProposalLoadRequestId) return 'superseded' + // Cancellation by the caller's own deadline is not a queue failure. It + // must not raise the failure toast, and it must not be reported as + // `failed`, or the caller would blame the server for its own timeout. + if (signal?.aborted) return 'aborted' toast.error(getErrorDisplay(e, t('review.toast.loadProposalsFailed')).message) outcome = 'failed' } finally { @@ -541,10 +587,12 @@ export function useReviewProposals() { reviewLoadPerf.end() } + if (signal?.aborted) return 'aborted' if (requestId === latestProposalLoadRequestId) { - await openProposalFromHash() + await openProposalFromHash(options) } if (requestId !== latestProposalLoadRequestId) return 'superseded' + if (signal?.aborted) return 'aborted' return outcome } diff --git a/frontend/taskdeck-web/src/locales/en/review.ts b/frontend/taskdeck-web/src/locales/en/review.ts index 05ff0b2e3..f093a6fb8 100644 --- a/frontend/taskdeck-web/src/locales/en/review.ts +++ b/frontend/taskdeck-web/src/locales/en/review.ts @@ -705,6 +705,8 @@ export default { 'Review refreshed after your save attempt. Check the current evidence, then choose the action again.', revisionReviewUnavailable: 'Review evidence could not be refreshed. No decision was made. Choose the current action again to retry.', + revisionReviewTimedOut: + 'Refreshing the review took too long, so it was stopped. No decision was made. Choose the current action again to retry.', notRejectable: 'This proposal can no longer be rejected. Refresh review to see current status.', notEditable: 'This proposal can no longer be edited.', notDeferrable: 'This proposal can no longer be deferred.', diff --git a/frontend/taskdeck-web/src/locales/es/review.ts b/frontend/taskdeck-web/src/locales/es/review.ts index 9f256d2f7..468b2719c 100644 --- a/frontend/taskdeck-web/src/locales/es/review.ts +++ b/frontend/taskdeck-web/src/locales/es/review.ts @@ -609,6 +609,8 @@ export default { 'La revisión se actualizó después del intento de guardado. Comprueba las pruebas actuales y vuelve a elegir la acción.', revisionReviewUnavailable: 'No se pudieron actualizar las pruebas de revisión. No se tomó ninguna decisión. Vuelve a elegir la acción actual para reintentarlo.', + revisionReviewTimedOut: + 'Actualizar la revisión tardó demasiado y se detuvo. No se tomó ninguna decisión. Vuelve a elegir la acción actual para reintentarlo.', notRejectable: 'Esta propuesta ya no se puede rechazar. Actualiza la revisión para ver el estado actual.', notEditable: 'Esta propuesta ya no se puede editar.', diff --git a/frontend/taskdeck-web/src/locales/it/review.ts b/frontend/taskdeck-web/src/locales/it/review.ts index 35e279fa7..fe4031455 100644 --- a/frontend/taskdeck-web/src/locales/it/review.ts +++ b/frontend/taskdeck-web/src/locales/it/review.ts @@ -612,6 +612,8 @@ export default { 'Revisione aggiornata dopo il tentativo di salvataggio. Controlla le prove correnti, quindi scegli di nuovo l’azione.', revisionReviewUnavailable: 'Non è stato possibile aggiornare le prove della revisione. Non è stata presa alcuna decisione. Scegli di nuovo l’azione corrente per riprovare.', + revisionReviewTimedOut: + 'L’aggiornamento della revisione ha richiesto troppo tempo ed è stato interrotto. Non è stata presa alcuna decisione. Scegli di nuovo l’azione corrente per riprovare.', notRejectable: 'Questa proposta non può più essere rifiutata. Aggiorna la revisione per vedere lo stato attuale.', notEditable: 'Questa proposta non può più essere modificata.', diff --git a/frontend/taskdeck-web/src/tests/composables/usePaperReviewSelectors.spec.ts b/frontend/taskdeck-web/src/tests/composables/usePaperReviewSelectors.spec.ts index fb8b030f0..1a81dac64 100644 --- a/frontend/taskdeck-web/src/tests/composables/usePaperReviewSelectors.spec.ts +++ b/frontend/taskdeck-web/src/tests/composables/usePaperReviewSelectors.spec.ts @@ -580,6 +580,100 @@ describe('usePaperReviewSelectors', () => { expect(selectors.conflicts.value).toEqual([]) }) + // #2460 -- a caller that holds the decision lock owns the deadline. A read + // that never answers must release that caller, and must not be reported as a + // server failure it never was. + it('reports a caller-cancelled wait as aborted and stops the reads it held open', async () => { + mockAllEndpointsEmpty() + let historySignal: AbortSignal | undefined + vi.mocked(proposalDeepReviewApi.getHistory).mockImplementationOnce( + (_id: string, options?: { signal?: AbortSignal }) => { + historySignal = options?.signal + // Never settles: the exact stall a signal alone cannot recover from. + return new Promise(() => {}) + }, + ) + const proposal = ref(makeProposal({ latestRevisionId: 'rev-1' })) + const selectors = usePaperReviewSelectors(computed(() => proposal.value)) + const controller = new AbortController() + + const wait = selectors.waitForCoreBatch('p-1', 'rev-1', { signal: controller.signal }) + await nextTick() + expect(historySignal?.aborted).toBe(false) + + controller.abort() + await expect(wait).resolves.toBe('aborted') + // The abandoned batch is cancelled rather than left occupying the transport. + expect(historySignal?.aborted).toBe(true) + }) + + it('reports a wait whose caller has already given up as aborted without reading', async () => { + mockAllEndpointsEmpty() + const proposal = ref(makeProposal({ latestRevisionId: 'rev-1' })) + const selectors = usePaperReviewSelectors(computed(() => proposal.value)) + await vi.waitFor(() => { + expect(proposalDeepReviewApi.getHistory).toHaveBeenCalled() + }) + const callsBefore = vi.mocked(proposalDeepReviewApi.getHistory).mock.calls.length + + const controller = new AbortController() + controller.abort() + + await expect( + selectors.waitForCoreBatch('p-1', 'rev-1', { signal: controller.signal }), + ).resolves.toBe('aborted') + expect(vi.mocked(proposalDeepReviewApi.getHistory).mock.calls.length).toBe(callsBefore) + }) + + it('does not cancel the batch the reviewer moved to when an abandoned wait is cancelled', async () => { + mockAllEndpointsEmpty() + const historySignals: Array = [] + vi.mocked(proposalDeepReviewApi.getHistory).mockImplementationOnce( + (_id: string, options?: { signal?: AbortSignal }) => { + historySignals.push(options?.signal) + return new Promise(() => {}) + }, + ) + const proposal = ref(makeProposal({ latestRevisionId: 'rev-1' })) + const selectors = usePaperReviewSelectors(computed(() => proposal.value)) + const controller = new AbortController() + const wait = selectors.waitForCoreBatch('p-1', 'rev-1', { signal: controller.signal }) + await nextTick() + + vi.mocked(proposalDeepReviewApi.getHistory).mockImplementationOnce( + (_id: string, options?: { signal?: AbortSignal }) => { + historySignals.push(options?.signal) + return Promise.resolve([]) + }, + ) + proposal.value = makeProposal({ latestRevisionId: 'rev-2' }) + await nextTick() + + controller.abort() + await expect(wait).resolves.toBe('aborted') + + // The cancellation belongs to the rev-1 wait. The rev-2 batch the reviewer + // is now looking at must survive it intact. + expect(historySignals).toHaveLength(2) + expect(historySignals[1]?.aborted).toBe(false) + await expect(selectors.waitForCoreBatch('p-1', 'rev-2')).resolves.toBe('settled') + }) + + it('still reports a genuine read failure as failed when a signal is supplied', async () => { + mockAllEndpointsEmpty() + vi.mocked(proposalDeepReviewApi.getHistory) + .mockRejectedValueOnce(new Error('fail')) + .mockRejectedValueOnce(new Error('retry fail')) + const proposal = ref(makeProposal({ latestRevisionId: 'rev-1' })) + const selectors = usePaperReviewSelectors(computed(() => proposal.value)) + const controller = new AbortController() + + await expect( + selectors.waitForCoreBatch('p-1', 'rev-1', { signal: controller.signal }), + ).resolves.toBe('failed') + expect(controller.signal.aborted).toBe(false) + }) + it('drops the previous key evidence when the next batch fails', async () => { mockAllEndpointsEmpty() vi.mocked(proposalDeepReviewApi.getConflicts).mockResolvedValueOnce([ diff --git a/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts b/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts index b5cc8f3b0..5dc5750b0 100644 --- a/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts +++ b/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts @@ -697,6 +697,89 @@ describe('useReviewProposals', () => { expect(rp.proposalsLoading.value).toBe(false) }) + // #2460 -- a caller that owns a deadline needs its own cancellation told + // apart from a server failure, or a timeout would be blamed on the backend. + it('reports a caller-aborted explicit load as aborted rather than failed', async () => { + const controller = new AbortController() + let rejectRead!: (error: Error) => void + mockAutomationApi.getProposals.mockReturnValueOnce( + new Promise((_resolve, reject) => { + rejectRead = reject + }), + ) + const rp = useReviewProposals() + + const load = rp.loadProposalsWithOutcome({ signal: controller.signal }) + controller.abort() + rejectRead(new Error('canceled')) + + await expect(load).resolves.toBe('aborted') + expect(mockToast.error).not.toHaveBeenCalled() + expect(mockAutomationApi.getProposals).toHaveBeenCalledWith( + expect.objectContaining({ limit: 200 }), + expect.objectContaining({ signal: controller.signal }), + ) + }) + + // A deadline-bounded caller must not spend its budget in the shared retry + // interceptor's doubling backoff and then be reported as a timeout. + it('forwards a caller opt-out of the shared retry interceptor', async () => { + const controller = new AbortController() + mockAutomationApi.getProposals.mockResolvedValueOnce([]) + const rp = useReviewProposals() + + await expect( + rp.loadProposalsWithOutcome({ signal: controller.signal, skipRetry: true }), + ).resolves.toBe('landed') + expect(mockAutomationApi.getProposals).toHaveBeenCalledWith( + expect.objectContaining({ limit: 200 }), + expect.objectContaining({ signal: controller.signal, skipRetry: true }), + ) + }) + + it('does not issue an explicit load whose caller has already given up', async () => { + const controller = new AbortController() + controller.abort() + const rp = useReviewProposals() + + await expect( + rp.loadProposalsWithOutcome({ signal: controller.signal }), + ).resolves.toBe('aborted') + expect(mockAutomationApi.getProposals).not.toHaveBeenCalled() + expect(mockToast.error).not.toHaveBeenCalled() + }) + + it('reports an aborted deep-link leg as aborted and raises no lookup error', async () => { + mockRoute.hash = '#proposal-p-remote' + const controller = new AbortController() + mockAutomationApi.getProposals.mockResolvedValueOnce([]) + let rejectLookup!: (error: Error) => void + mockAutomationApi.getProposal.mockReturnValueOnce( + new Promise((_resolve, reject) => { + rejectLookup = reject + }), + ) + const rp = useReviewProposals() + + const load = rp.loadProposalsWithOutcome({ + signal: controller.signal, + skipRetry: true, + }) + await Promise.resolve() + await Promise.resolve() + controller.abort() + rejectLookup(new Error('canceled')) + + await expect(load).resolves.toBe('aborted') + expect(mockToast.error).not.toHaveBeenCalled() + // The deep-link leg is part of the same explicit read, so it carries the + // same cancellation and retry contract rather than running unbounded. + expect(mockAutomationApi.getProposal).toHaveBeenCalledWith( + 'p-remote', + expect.objectContaining({ signal: controller.signal, skipRetry: true }), + ) + }) + it('does not report landed until its deep-link lookup completes', async () => { mockRoute.hash = '#proposal-p-remote' mockAutomationApi.getProposals.mockResolvedValueOnce([]) diff --git a/frontend/taskdeck-web/src/tests/views/paper/review/PaperReviewView.spec.ts b/frontend/taskdeck-web/src/tests/views/paper/review/PaperReviewView.spec.ts index b2611f2b4..a7bcbe4bc 100644 --- a/frontend/taskdeck-web/src/tests/views/paper/review/PaperReviewView.spec.ts +++ b/frontend/taskdeck-web/src/tests/views/paper/review/PaperReviewView.spec.ts @@ -5461,4 +5461,381 @@ describe('PaperReviewView', () => { expect(wrapper.get('[data-testid="decision-apply"]').attributes('disabled')).toBeUndefined() }) }) + + describe('post-revision truth refresh deadline (#2460)', () => { + // Mirrors POST_REVISION_REVIEW_DEADLINE_MS in PaperReviewView.vue. Kept as a + // literal so raising the view's cap has to be a deliberate change here too. + const DEADLINE_MS = 12_000 + const TIMED_OUT_COPY = + 'Refreshing the review took too long, so it was stopped. No decision was made. Choose the current action again to retry.' + const FAILED_COPY = + 'Review evidence could not be refreshed. No decision was made. Choose the current action again to retry.' + const REFRESHED_COPY = + 'Review refreshed after your save attempt. Check the current evidence, then choose the action again.' + const REVISED_PAYLOAD = '{"operations":[{"sequence":0,"actionType":"CreateCard"}]}' + + /** Arm the post-revision barrier the way the composer does. */ + async function armBarrier( + wrapper: Awaited>, + proposalId: string, + revisionId: string, + ) { + const now = new Date().toISOString() + mocks.createRevision.mockResolvedValueOnce({ + id: revisionId, + proposalId, + revisionNumber: 1, + editorUserId: 'u-1', + revisedPayload: REVISED_PAYLOAD, + revisedAt: now, + reason: 'Deadline coverage', + createdAt: now, + }) + await wrapper.get('[data-testid="decision-edit"]').trigger('click') + await flushPromises() + wrapper.findComponent(ReviewRevisionEditor).vm.$emit('save', { + revisedPayload: REVISED_PAYLOAD, + reason: 'Deadline coverage', + }) + await flushPromises() + } + + function rejectEveryCoreRead(times: number) { + const reads = [ + mocks.getProvenance, + mocks.getConfidence, + mocks.getSideEffects, + mocks.getConflicts, + mocks.getHistory, + mocks.getSimilarPast, + ] + for (const read of reads) { + for (let attempt = 0; attempt < times; attempt += 1) { + read.mockRejectedValueOnce(new Error('core evidence unavailable')) + } + } + } + + it('releases the rail on its deadline when the authoritative queue read never answers', async () => { + vi.useFakeTimers({ toFake: ['setInterval', 'clearInterval', 'setTimeout', 'clearTimeout', 'Date'] }) + try { + const original = makeProposal({ id: 'stalled-queue' }) + const refreshed = makeProposal({ + id: 'stalled-queue', + latestRevisionId: 'rev-stalled-1', + }) + mocks.approveProposal.mockResolvedValueOnce( + makeProposal({ + id: 'stalled-queue', + status: 'Approved', + approvedRevisionId: 'rev-stalled-1', + }), + ) + const wrapper = await mountView([original]) + await armBarrier(wrapper, 'stalled-queue', 'rev-stalled-1') + + let resolveStalled!: (proposals: Proposal[]) => void + mocks.getProposals.mockImplementationOnce( + () => new Promise((resolve) => { resolveStalled = resolve }), + ) + await wrapper.get('[data-testid="decision-apply"]').trigger('click') + await flushPromises() + + // The shared decision lock is held while the read is outstanding. + expect(wrapper.find('[data-testid="decision-lock-note"]').exists()).toBe(true) + expect(wrapper.get('[data-testid="decision-apply"]').attributes('disabled')).toBeDefined() + + await vi.advanceTimersByTimeAsync(DEADLINE_MS) + await flushPromises() + + // The rail is handed back with a truthful, retryable explanation. + expect(wrapper.find('[data-testid="decision-lock-note"]').exists()).toBe(false) + expect(wrapper.get('[data-testid="decision-apply"]').attributes('disabled')).toBeUndefined() + expect(wrapper.get('[data-testid="paper-review-evidence-unavailable"]').text()).toContain( + 'Refreshing the review took too long', + ) + expect(mocks.errorToast).toHaveBeenCalledWith(TIMED_OUT_COPY) + expect(mocks.errorToast).not.toHaveBeenCalledWith(FAILED_COPY) + expect(mocks.approveProposal).not.toHaveBeenCalled() + expect(mocks.executeProposal).not.toHaveBeenCalled() + + // The abandoned attempt answering late changes nothing: it neither + // becomes the rendered queue nor clears the barrier it gave up on. + resolveStalled([refreshed]) + await flushPromises() + expect(mocks.infoToast).not.toHaveBeenCalledWith(REFRESHED_COPY) + expect(wrapper.get('[data-testid="paper-review-evidence-unavailable"]').text()).toContain( + 'Refreshing the review took too long', + ) + expect(mocks.approveProposal).not.toHaveBeenCalled() + + // The barrier survived the timeout, so the next explicit action still + // refreshes rather than deciding on pre-revision evidence. + mocks.getProposals.mockResolvedValue([refreshed]) + await wrapper.get('[data-testid="decision-apply"]').trigger('click') + await flushPromises() + expect(mocks.infoToast).toHaveBeenCalledWith(REFRESHED_COPY) + expect(wrapper.find('[data-testid="paper-review-evidence-unavailable"]').exists()).toBe(false) + expect(mocks.approveProposal).not.toHaveBeenCalled() + + // Only the second explicit action after a clean refresh decides. + await wrapper.get('[data-testid="decision-apply"]').trigger('click') + await flushPromises() + expect(mocks.approveProposal).toHaveBeenCalledOnce() + expect(mocks.executeProposal).not.toHaveBeenCalled() + wrapper.unmount() + } finally { + vi.useRealTimers() + } + }) + + it('releases the rail on its deadline when a core evidence read never answers', async () => { + vi.useFakeTimers({ toFake: ['setInterval', 'clearInterval', 'setTimeout', 'clearTimeout', 'Date'] }) + try { + const original = makeProposal({ id: 'stalled-evidence' }) + const refreshed = makeProposal({ + id: 'stalled-evidence', + latestRevisionId: 'rev-evidence-1', + }) + mocks.approveProposal.mockResolvedValueOnce( + makeProposal({ + id: 'stalled-evidence', + status: 'Approved', + approvedRevisionId: 'rev-evidence-1', + }), + ) + const wrapper = await mountView([original]) + await armBarrier(wrapper, 'stalled-evidence', 'rev-evidence-1') + + mocks.getProposals.mockResolvedValue([refreshed]) + let resolveHistory!: (rows: unknown[]) => void + mocks.getHistory.mockImplementationOnce( + () => new Promise((resolve) => { resolveHistory = resolve }), + ) + await wrapper.get('[data-testid="decision-apply"]').trigger('click') + await flushPromises() + + expect(wrapper.find('[data-testid="decision-lock-note"]').exists()).toBe(true) + expect(wrapper.get('[data-testid="decision-apply"]').attributes('disabled')).toBeDefined() + + await vi.advanceTimersByTimeAsync(DEADLINE_MS) + await flushPromises() + + expect(wrapper.find('[data-testid="decision-lock-note"]').exists()).toBe(false) + expect(wrapper.get('[data-testid="decision-apply"]').attributes('disabled')).toBeUndefined() + expect(wrapper.get('[data-testid="paper-review-evidence-unavailable"]').text()).toContain( + 'Refreshing the review took too long', + ) + expect(mocks.errorToast).toHaveBeenCalledWith(TIMED_OUT_COPY) + expect(mocks.approveProposal).not.toHaveBeenCalled() + expect(mocks.executeProposal).not.toHaveBeenCalled() + + // A late evidence answer may repopulate the panels, but it must not + // clear a barrier the timed-out attempt no longer owns. + resolveHistory([]) + await flushPromises() + expect(mocks.infoToast).not.toHaveBeenCalledWith(REFRESHED_COPY) + expect(mocks.approveProposal).not.toHaveBeenCalled() + + await wrapper.get('[data-testid="decision-apply"]').trigger('click') + await flushPromises() + expect(mocks.infoToast).toHaveBeenCalledWith(REFRESHED_COPY) + expect(mocks.approveProposal).not.toHaveBeenCalled() + + await wrapper.get('[data-testid="decision-apply"]').trigger('click') + await flushPromises() + expect(mocks.approveProposal).toHaveBeenCalledOnce() + expect(mocks.executeProposal).not.toHaveBeenCalled() + wrapper.unmount() + } finally { + vi.useRealTimers() + } + }) + + it('names a partial core evidence failure as a failure, never as a timeout', async () => { + const original = makeProposal({ id: 'partial-failure' }) + const refreshed = makeProposal({ + id: 'partial-failure', + latestRevisionId: 'rev-partial-1', + }) + const wrapper = await mountView([original]) + await armBarrier(wrapper, 'partial-failure', 'rev-partial-1') + + mocks.getProposals.mockResolvedValue([refreshed]) + // One of the six rejects, through the automatic batch and its same-action retry. + mocks.getHistory + .mockRejectedValueOnce(new Error('history unavailable')) + .mockRejectedValueOnce(new Error('history still unavailable')) + await wrapper.get('[data-testid="decision-apply"]').trigger('click') + await flushPromises() + + expect(mocks.errorToast).toHaveBeenCalledWith(FAILED_COPY) + expect(mocks.errorToast).not.toHaveBeenCalledWith(TIMED_OUT_COPY) + expect(wrapper.get('[data-testid="paper-review-evidence-unavailable"]').text()).toContain( + 'Review evidence could not be refreshed', + ) + // The rail is unlocked for the retry even though the refresh failed. + expect(wrapper.find('[data-testid="decision-lock-note"]').exists()).toBe(false) + expect(wrapper.get('[data-testid="decision-apply"]').attributes('disabled')).toBeUndefined() + expect(mocks.approveProposal).not.toHaveBeenCalled() + expect(mocks.executeProposal).not.toHaveBeenCalled() + wrapper.unmount() + }) + + it('names a total core evidence failure as a failure and retains the barrier', async () => { + const original = makeProposal({ id: 'total-failure' }) + const refreshed = makeProposal({ + id: 'total-failure', + latestRevisionId: 'rev-total-1', + }) + mocks.approveProposal.mockResolvedValueOnce( + makeProposal({ + id: 'total-failure', + status: 'Approved', + approvedRevisionId: 'rev-total-1', + }), + ) + const wrapper = await mountView([original]) + await armBarrier(wrapper, 'total-failure', 'rev-total-1') + + mocks.getProposals.mockResolvedValue([refreshed]) + rejectEveryCoreRead(2) + await wrapper.get('[data-testid="decision-apply"]').trigger('click') + await flushPromises() + + expect(mocks.errorToast).toHaveBeenCalledWith(FAILED_COPY) + expect(mocks.errorToast).not.toHaveBeenCalledWith(TIMED_OUT_COPY) + expect(wrapper.find('[data-testid="paper-review-evidence-unavailable"]').exists()).toBe(true) + expect(wrapper.find('[data-testid="decision-lock-note"]').exists()).toBe(false) + expect(mocks.approveProposal).not.toHaveBeenCalled() + + // The barrier is retained: the next action refreshes, and only the one + // after a clean refresh may approve. + await wrapper.get('[data-testid="decision-apply"]').trigger('click') + await flushPromises() + expect(mocks.infoToast).toHaveBeenCalledWith(REFRESHED_COPY) + expect(mocks.approveProposal).not.toHaveBeenCalled() + + await wrapper.get('[data-testid="decision-apply"]').trigger('click') + await flushPromises() + expect(mocks.approveProposal).toHaveBeenCalledOnce() + expect(mocks.executeProposal).not.toHaveBeenCalled() + wrapper.unmount() + }) + + it('reports nothing when the reviewer moved to another proposal before the deadline', async () => { + vi.useFakeTimers({ toFake: ['setInterval', 'clearInterval', 'setTimeout', 'clearTimeout', 'Date'] }) + try { + const proposalA = makeProposal({ id: 'aaa-moved', summary: 'First proposal' }) + const proposalB = makeProposal({ id: 'bbb-moved', summary: 'Second proposal' }) + const wrapper = await mountView([proposalA, proposalB]) + await wrapper.find('[data-serial="#AAA-"]').trigger('click') + await flushPromises() + await armBarrier(wrapper, 'aaa-moved', 'rev-moved-1') + + mocks.getProposals.mockImplementationOnce(() => new Promise(() => {})) + await wrapper.get('[data-testid="decision-apply"]').trigger('click') + await flushPromises() + expect(wrapper.find('[data-testid="decision-lock-note"]').exists()).toBe(true) + + // The queue rail is not disabled by the decision lock, so the reviewer + // can be looking at B when A's attempt ends. + await wrapper.find('[data-serial="#BBB-"]').trigger('click') + await flushPromises() + + await vi.advanceTimersByTimeAsync(DEADLINE_MS) + await flushPromises() + + // Neither half of the report may land on B. "Choose the current action + // again" read on B would be an instruction to approve a proposal that + // has no barrier at all. + expect(mocks.errorToast).not.toHaveBeenCalledWith(TIMED_OUT_COPY) + expect(mocks.errorToast).not.toHaveBeenCalledWith(FAILED_COPY) + expect(mocks.infoToast).not.toHaveBeenCalledWith(REFRESHED_COPY) + expect(wrapper.find('[data-testid="paper-review-evidence-unavailable"]').exists()).toBe(false) + expect(wrapper.get('[data-testid="decision-apply"]').attributes('disabled')).toBeUndefined() + + // A's barrier survived, so returning to it still refreshes before deciding. + mocks.getProposals.mockResolvedValue([ + makeProposal({ id: 'aaa-moved', summary: 'First proposal', latestRevisionId: 'rev-moved-1' }), + proposalB, + ]) + await wrapper.find('[data-serial="#AAA-"]').trigger('click') + await flushPromises() + await wrapper.get('[data-testid="decision-apply"]').trigger('click') + await flushPromises() + expect(mocks.approveProposal).not.toHaveBeenCalled() + expect(mocks.infoToast).toHaveBeenCalledWith(REFRESHED_COPY) + wrapper.unmount() + } finally { + vi.useRealTimers() + } + }) + + it('cancels an in-flight attempt on unmount so its deadline never reports', async () => { + vi.useFakeTimers({ toFake: ['setInterval', 'clearInterval', 'setTimeout', 'clearTimeout', 'Date'] }) + try { + const original = makeProposal({ id: 'unmounted-attempt' }) + const wrapper = await mountView([original]) + await armBarrier(wrapper, 'unmounted-attempt', 'rev-unmounted-1') + + let barrierSignal: AbortSignal | undefined + mocks.getProposals.mockImplementationOnce( + (_filters?: unknown, options?: { signal?: AbortSignal }) => { + barrierSignal = options?.signal + return new Promise(() => {}) + }, + ) + await wrapper.get('[data-testid="decision-apply"]').trigger('click') + await flushPromises() + expect(wrapper.find('[data-testid="decision-lock-note"]').exists()).toBe(true) + expect(barrierSignal?.aborted).toBe(false) + + wrapper.unmount() + // The reads the attempt was holding open are cancelled with the route. + expect(barrierSignal?.aborted).toBe(true) + + mocks.errorToast.mockClear() + mocks.infoToast.mockClear() + await vi.advanceTimersByTimeAsync(DEADLINE_MS * 2) + await flushPromises() + + // The deadline timer went with the component: nothing reports onto + // whatever page the reviewer navigated to. + expect(mocks.errorToast).not.toHaveBeenCalled() + expect(mocks.infoToast).not.toHaveBeenCalled() + } finally { + vi.useRealTimers() + } + }) + + it('disarms its deadline once the refresh lands, so no late timeout note appears', async () => { + vi.useFakeTimers({ toFake: ['setInterval', 'clearInterval', 'setTimeout', 'clearTimeout', 'Date'] }) + try { + const original = makeProposal({ id: 'clean-refresh' }) + const refreshed = makeProposal({ + id: 'clean-refresh', + latestRevisionId: 'rev-clean-1', + }) + const wrapper = await mountView([original]) + await armBarrier(wrapper, 'clean-refresh', 'rev-clean-1') + + mocks.getProposals.mockResolvedValue([refreshed]) + await wrapper.get('[data-testid="decision-apply"]').trigger('click') + await flushPromises() + expect(mocks.infoToast).toHaveBeenCalledWith(REFRESHED_COPY) + + await vi.advanceTimersByTimeAsync(DEADLINE_MS) + await flushPromises() + + expect(mocks.errorToast).not.toHaveBeenCalledWith(TIMED_OUT_COPY) + expect(wrapper.find('[data-testid="paper-review-evidence-unavailable"]').exists()).toBe(false) + expect(wrapper.find('[data-testid="decision-lock-note"]').exists()).toBe(false) + expect(wrapper.get('[data-testid="decision-apply"]').attributes('disabled')).toBeUndefined() + wrapper.unmount() + } finally { + vi.useRealTimers() + } + }) + }) }) diff --git a/frontend/taskdeck-web/src/views/paper/PaperReviewView.vue b/frontend/taskdeck-web/src/views/paper/PaperReviewView.vue index d02f37c1f..233757957 100644 --- a/frontend/taskdeck-web/src/views/paper/PaperReviewView.vue +++ b/frontend/taskdeck-web/src/views/paper/PaperReviewView.vue @@ -1197,7 +1197,122 @@ let revisionEditEpoch = 0 let revisionReturnFocusEpoch: number | null = null let revisionReviewEpoch = 0 const revisionReviewRefreshEpochs = new Map() -const revisionReviewUnavailableKeys = ref>(new Set()) + +/** + * #2460 — the longest one post-revision truth refresh may hold the decision rail. + * + * The barrier is a COMPOSITE read: one authoritative queue read, then the six + * core evidence reads, plus at most one same-action retry of that batch. Every + * leg is bounded by the transport already, but a leg that never answers at all + * would hold the shared decision lock forever: the rail stays disabled, the + * keymap stays inert, and the reviewer has no way back to their own decision. + * So the CALLER caps the whole attempt, in the same spirit as + * `BOARD_REQUEST_TIMEOUT_MS` in `api/http.ts` caps one board read — long enough + * for a realistic composite round trip on a slow link, short enough that a + * locked rail still reads as work in progress rather than as broken. + * + * What this budget assumes about retries, because they are what make a cap of + * this size tight: the queue read (and its deep-link leg) is issued with + * `skipRetry`, so it costs one round trip and reports the first honest answer. + * The six core evidence reads are NOT, because they are served by the batch + * `usePaperReviewSelectors` shares with its own automatic refresh — a per-call + * opt-out there would apply only when the barrier happened to be the batch's + * creator, which is worse than retrying uniformly. So this budget must still + * cover one shared-interceptor retry pass on the evidence leg, and a genuinely + * flaky evidence read can exhaust it. + * + * That is an acceptable trade because timing out costs one more explicit action + * and nothing else: the per-proposal barrier is RETAINED, so the retry refreshes + * again before any decision can be made on pre-revision evidence. + */ +const POST_REVISION_REVIEW_DEADLINE_MS = 12_000 + +/** Race marker for {@link POST_REVISION_REVIEW_DEADLINE_MS}. */ +const REVISION_REVIEW_DEADLINE = 'post-revision-review-deadline' as const + +/** + * Why the barrier is telling the reviewer that evidence is not current. The two + * reasons need different copy: a failed read is a server or transport answer the + * reviewer cannot influence, a timed-out one is an attempt that may simply need + * longer. Collapsing them into one message would make the surface guess. + */ +type RevisionReviewUnavailableReason = 'failed' | 'timed-out' + +/** + * How one barrier attempt ended. Deliberately five distinct members rather than + * a boolean: only `refreshed` may clear the barrier, and the other four each + * call for different user-facing treatment. + */ +type RevisionReviewRefreshOutcome = + | 'refreshed' + | 'failed' + | 'timed-out' + | 'aborted' + | 'superseded' + +interface RevisionReviewAttempt { + /** Shared by the queue read and the six core selector reads. */ + signal: AbortSignal + /** Resolves with {@link REVISION_REVIEW_DEADLINE} when the attempt expires. */ + deadline: Promise + /** True once the deadline fired, which names an abort as a timeout. */ + readonly timedOut: boolean + dispose: () => void +} + +/** + * One cancellation contract per barrier attempt. + * + * Aborting is not enough on its own: a transport (or a test double) may ignore + * the signal and never settle, which is exactly the stall this guards against. + * So the deadline both aborts the shared controller AND resolves a race marker, + * and the caller releases its lock on whichever arrives first. + */ +function startRevisionReviewAttempt(): RevisionReviewAttempt { + const controller = new AbortController() + let timedOut = false + let resolveDeadline!: (value: typeof REVISION_REVIEW_DEADLINE) => void + const deadline = new Promise((resolve) => { + resolveDeadline = resolve + }) + const timer = setTimeout(() => { + timedOut = true + controller.abort() + resolveDeadline(REVISION_REVIEW_DEADLINE) + }, POST_REVISION_REVIEW_DEADLINE_MS) + return { + signal: controller.signal, + deadline, + get timedOut() { + return timedOut + }, + dispose() { + clearTimeout(timer) + // Cancel what this attempt still holds open. Clearing the timer alone + // would leave an early failed or superseded ending -- and an unmount -- + // with reads still running for a decision nobody is waiting on. + controller.abort() + }, + } +} + +/** + * The attempt currently holding the decision lock, so teardown can cancel it. + * A barrier attempt outlives its route otherwise: its deadline timer would fire + * after the reviewer has navigated away and report on whatever page they are on. + */ +let activeRevisionReviewAttempt: RevisionReviewAttempt | null = null + +/** + * Generation of the barrier attempt that currently owns the decision lock. A + * late attempt must not unlock a rail another attempt is holding, and must not + * write barrier state on behalf of a screen that has moved on. + */ +let revisionReviewAttemptGeneration = 0 + +const revisionReviewUnavailableKeys = ref>( + new Map(), +) function revisionReviewKey(proposalId: string): string { return proposalId.toLowerCase() @@ -1220,11 +1335,11 @@ function requireRevisionReviewRefresh(proposalId: string) { function setRevisionReviewUnavailable( proposalId: string, revisionIdentity: string | null, - unavailable: boolean, + reason: RevisionReviewUnavailableReason | null, ) { const key = revisionReviewUnavailableKey(proposalId, revisionIdentity) - const next = new Set(revisionReviewUnavailableKeys.value) - if (unavailable) next.add(key) + const next = new Map(revisionReviewUnavailableKeys.value) + if (reason) next.set(key, reason) else next.delete(key) revisionReviewUnavailableKeys.value = next } @@ -1240,15 +1355,33 @@ function isRevisionReviewUnavailableVisible(proposal: ApiProposal): boolean { ) } -const activeRevisionReviewUnavailable = computed(() => { - const proposal = activeProposal.value - return ( - !!proposal && - isRevisionReviewUnavailableVisible(proposal) && - revisionReviewUnavailableKeys.value.has( - revisionReviewUnavailableKey(proposal.id, proposalRevisionIdentity(proposal)), +const activeRevisionReviewUnavailableReason = computed( + () => { + const proposal = activeProposal.value + if (!proposal || !isRevisionReviewUnavailableVisible(proposal)) return null + return ( + revisionReviewUnavailableKeys.value.get( + revisionReviewUnavailableKey(proposal.id, proposalRevisionIdentity(proposal)), + ) ?? null ) - ) + }, +) + +const activeRevisionReviewUnavailable = computed( + () => activeRevisionReviewUnavailableReason.value !== null, +) + +/** + * The note the reviewer reads when the barrier could not confirm current truth. + * Both messages state the same non-negotiable fact — no decision was made — and + * differ only in why, and in whether waiting is worth another try. + */ +const revisionReviewUnavailableNote = computed(() => { + const reason = activeRevisionReviewUnavailableReason.value + if (!reason) return '' + return reason === 'timed-out' + ? t('review.toast.revisionReviewTimedOut') + : t('review.toast.revisionReviewUnavailable') }) watch( @@ -1268,7 +1401,7 @@ watch( // not inherit it. Clear it when that proposal leaves the actionable review // state, or when the active proposal itself is replaced. if (!sameProposal || !current?.eligible) { - setRevisionReviewUnavailable(previous.proposalId, previous.revisionIdentity, false) + setRevisionReviewUnavailable(previous.proposalId, previous.revisionIdentity, null) } }, ) @@ -1465,67 +1598,170 @@ function restoreApplyFocus(captured: HTMLElement | null) { }) } +/** + * Run one barrier attempt and report exactly how it ended. + * + * The barrier clears -- and the action becomes approvable on the NEXT explicit + * press -- only on `refreshed`: the authoritative proposal DTO and all six core + * evidence reads landed for one identity, status, expiry, defer state and + * effective revision. Every other ending leaves the epoch in place, so the next + * Approve or Apply refreshes again instead of deciding on pre-revision truth. + * + * This decides the outcome and writes nothing. `clearRevisionReviewBarrier` is + * what actually clears the epoch, once the caller has checked that this attempt + * still owns the barrier. + */ +async function runRevisionReviewRefresh( + proposal: ApiProposal, + attempt: RevisionReviewAttempt, +): Promise { + const queueOutcome = await Promise.race([ + // `skipRetry`: the interceptor's doubling backoff can spend most of this + // attempt's budget re-asking a question the barrier would rather answer + // honestly and let the reviewer retry deliberately. + loadProposalsWithOutcome({ signal: attempt.signal, skipRetry: true }), + attempt.deadline, + ]) + if (queueOutcome === REVISION_REVIEW_DEADLINE) return 'timed-out' + // Only this attempt aborts that signal, so an abort it did not schedule + // itself can only be a teardown -- never a deadline the reviewer should retry. + if (queueOutcome === 'aborted') return attempt.timedOut ? 'timed-out' : 'aborted' + if (queueOutcome === 'failed') return 'failed' + if (queueOutcome !== 'landed') return 'superseded' + + const refreshed = activeProposal.value + if ( + !refreshed || + !proposalIdsEqual(refreshed.id, proposal.id) || + !isApplyActionable(refreshed) || + isProposalDeferred(refreshed) + ) return 'superseded' + + const status = normalizeProposalStatus(refreshed.status) + const revisionIdentity = proposalRevisionIdentity(refreshed) + const expiresAt = refreshed.expiresAt ?? null + const deferredUntil = refreshed.deferredUntil ?? null + const selectorOutcome = await Promise.race([ + selectors.waitForCoreBatch(refreshed.id, revisionIdentity, { signal: attempt.signal }), + attempt.deadline, + ]) + if (selectorOutcome === REVISION_REVIEW_DEADLINE) return 'timed-out' + if (selectorOutcome === 'aborted') return attempt.timedOut ? 'timed-out' : 'aborted' + if (selectorOutcome === 'failed') return 'failed' + if (selectorOutcome !== 'settled') return 'superseded' + + const verified = activeProposal.value + if ( + !verified || + !proposalIdsEqual(verified.id, proposal.id) || + normalizeProposalStatus(verified.status) !== status || + !revisionIdentitiesEqual(proposalRevisionIdentity(verified), revisionIdentity) || + (verified.expiresAt ?? null) !== expiresAt || + (verified.deferredUntil ?? null) !== deferredUntil || + !isApplyActionable(verified) || + isProposalDeferred(verified) + ) return 'superseded' + + return 'refreshed' +} + +/** + * Clear the barrier for one attempt. This is the ONE piece of state whose loss + * matters -- everything else the attempt writes is a note or a toast -- so it is + * written last, behind the attempt-generation guard in the caller. + * + * The epoch is re-read here rather than trusted from the verification block: a + * save that landed while this attempt was resolving arms a NEWER epoch, and + * deleting that would drop a barrier this attempt never satisfied. + */ +function clearRevisionReviewBarrier( + proposal: ApiProposal, + requiredEpoch: number, +): RevisionReviewRefreshOutcome { + const key = revisionReviewKey(proposal.id) + if (revisionReviewRefreshEpochs.get(key) !== requiredEpoch) return 'superseded' + revisionReviewRefreshEpochs.delete(key) + return 'refreshed' +} + +/** + * Report one attempt's ending to the reviewer. + * + * The note and the toast are ONE report about ONE proposal, so they share ONE + * guard. The queue rail stays clickable while the barrier reads (only the + * decision rail and the keymap are locked), so a reviewer can be looking at B + * when A's attempt ends. Every message here says "choose the current action + * again" -- addressed to B, that is an instruction to press Apply on a proposal + * with no barrier, which falls straight through to a real decision. So a report + * that cannot be attributed to what is on screen is not shown at all. + * + * Withholding it costs nothing: the barrier state itself is written separately + * and independently of what is displayed, so returning to A still refreshes. + */ +function applyRevisionReviewOutcome( + proposal: ApiProposal, + outcome: RevisionReviewRefreshOutcome, +) { + // Nothing to tell the reviewer: the screen they were deciding on is gone or + // was replaced, and the barrier stays armed for whatever replaced it. + if (outcome === 'aborted' || outcome === 'superseded') return + + const current = activeProposal.value + if (!current || !proposalIdsEqual(current.id, proposal.id)) return + const revisionIdentity = proposalRevisionIdentity(current) + + switch (outcome) { + case 'refreshed': + setRevisionReviewUnavailable(current.id, revisionIdentity, null) + toast.info(t('review.toast.revisionReviewRefreshed')) + return + case 'failed': + setRevisionReviewUnavailable(current.id, revisionIdentity, 'failed') + toast.error(t('review.toast.revisionReviewUnavailable')) + return + case 'timed-out': + setRevisionReviewUnavailable(current.id, revisionIdentity, 'timed-out') + toast.error(t('review.toast.revisionReviewTimedOut')) + return + } +} + async function refreshRevisionReviewBeforeApply( proposal: ApiProposal, requiredEpoch: number, ) { const captured = applyReturnFocusEl + const generation = ++revisionReviewAttemptGeneration + const attempt = startRevisionReviewAttempt() + activeRevisionReviewAttempt = attempt applyGuardBusy.value = true revisionReviewRefreshBusy.value = true try { - const queueOutcome = await loadProposalsWithOutcome() - if (queueOutcome !== 'landed') return - - const refreshed = activeProposal.value - if ( - !refreshed || - !proposalIdsEqual(refreshed.id, proposal.id) || - !isApplyActionable(refreshed) || - isProposalDeferred(refreshed) - ) return - - const status = normalizeProposalStatus(refreshed.status) - const revisionIdentity = proposalRevisionIdentity(refreshed) - const expiresAt = refreshed.expiresAt ?? null - const deferredUntil = refreshed.deferredUntil ?? null - const selectorOutcome = await selectors.waitForCoreBatch( - refreshed.id, - revisionIdentity, + const outcome = await runRevisionReviewRefresh(proposal, attempt) + // A late attempt writes nothing: a newer attempt (or an unmount) owns the + // barrier, the rail and the note, and this one's answer is by definition + // older. The barrier clear sits behind this guard for that reason. + if (generation !== revisionReviewAttemptGeneration) return + applyRevisionReviewOutcome( + proposal, + outcome === 'refreshed' + ? clearRevisionReviewBarrier(proposal, requiredEpoch) + : outcome, ) - if (selectorOutcome === 'failed') { - setRevisionReviewUnavailable(proposal.id, revisionIdentity, true) - toast.error(t('review.toast.revisionReviewUnavailable')) - return - } - if (selectorOutcome !== 'settled') return - - const verified = activeProposal.value - if ( - !verified || - !proposalIdsEqual(verified.id, proposal.id) || - normalizeProposalStatus(verified.status) !== status || - !revisionIdentitiesEqual(proposalRevisionIdentity(verified), revisionIdentity) || - (verified.expiresAt ?? null) !== expiresAt || - (verified.deferredUntil ?? null) !== deferredUntil || - !isApplyActionable(verified) || - isProposalDeferred(verified) - ) return - - const key = revisionReviewKey(proposal.id) - if (revisionReviewRefreshEpochs.get(key) !== requiredEpoch) return - revisionReviewRefreshEpochs.delete(key) - setRevisionReviewUnavailable(proposal.id, revisionIdentity, false) - toast.info(t('review.toast.revisionReviewRefreshed')) } finally { - revisionReviewRefreshBusy.value = false - applyGuardBusy.value = false - applyReturnFocusEl = null - // The preflight consumes this action unconditionally. Return focus only - // after the shared lock releases, so a keyboard reviewer can inspect the - // refreshed evidence and deliberately invoke the current action again. - void nextTick(() => { - restoreApplyFocus(captured) - }) + attempt.dispose() + if (activeRevisionReviewAttempt === attempt) activeRevisionReviewAttempt = null + if (generation === revisionReviewAttemptGeneration) { + revisionReviewRefreshBusy.value = false + applyGuardBusy.value = false + applyReturnFocusEl = null + // The preflight consumes this action unconditionally. Return focus only + // after the shared lock releases, so a keyboard reviewer can inspect the + // refreshed evidence and deliberately invoke the current action again. + void nextTick(() => { + restoreApplyFocus(captured) + }) + } } } @@ -2220,6 +2456,14 @@ onMounted(() => { }) onUnmounted(() => { + // #2460: a barrier attempt outlives its route unless it is cancelled here. + // Bumping the generation first stops any outcome being applied to a surface + // that no longer exists; disposing then clears the deadline timer -- whose + // ending would otherwise toast over whatever page the reviewer moved to -- + // and aborts the reads it was holding open. + revisionReviewAttemptGeneration += 1 + activeRevisionReviewAttempt?.dispose() + activeRevisionReviewAttempt = null // Prevent an in-flight metadata open or save continuation from restoring // focus into a component that no longer owns the document. invalidateRevisionOpening() @@ -2354,7 +2598,7 @@ async function onClearBoardScope() { aria-atomic="true" data-testid="paper-review-evidence-unavailable" > - {{ $t('review.toast.revisionReviewUnavailable') }} + {{ revisionReviewUnavailableNote }}