diff --git a/frontend/taskdeck-web/src/composables/useReviewProposals.ts b/frontend/taskdeck-web/src/composables/useReviewProposals.ts index f8a4db190..ee82e86c7 100644 --- a/frontend/taskdeck-web/src/composables/useReviewProposals.ts +++ b/frontend/taskdeck-web/src/composables/useReviewProposals.ts @@ -236,6 +236,23 @@ export function isProposalStale(proposal: ApiProposal, nowMs: number): boolean { return nowMs - createdMs >= STALE_PROPOSAL_MS } +/** + * WHICH disclosure a standing recovery sentence retracts (#2638 item 2). + * + * The two are not interchangeable, because they are retracted on different + * evidence. 'degraded' ends the transient state, which is a claim about the + * RENDERED QUEUE, so its sentence may say the rows on screen are current: it is + * only raised by a read that completed and assigned `proposals.value`. + * 'refused' ends the refusal disclosure, which is a claim about the LIST + * REQUEST alone: it is raised the moment the list leg answers, on a tick whose + * composite read may still bail at the pin leg without replacing the queue, so + * its sentence must say the server is accepting refreshes again and NOTHING + * about the contents (#2214, from PR #2694's round-2 verification: the shared + * sentence's second clause overclaimed for up to two poll intervals on a + * list-success/pin-fail loop). + */ +export type QueueRecoveryKind = 'degraded' | 'refused' + export function useReviewProposals() { const route = useRoute() const router = useRouter() @@ -296,21 +313,77 @@ export function useReviewProposals() { // cannot arrive. Both can stand at once; the surfaces show the refusal, // which is the stronger and more actionable statement. const queueRefreshRefused = ref(false) - // The EVENT that pairs with the state above (#2214). Clearing - // `queueRefreshStale` unmounts the warning, which is silent: a reviewer who + // The EVENT that pairs with the two states above (#2214), and WHICH of them + // it retracts (#2638 item 2). Clearing `queueRefreshStale` or + // `queueRefreshRefused` unmounts the warning, which is silent: a reviewer who // was not looking at that corner is never told the queue is trustworthy - // again, and a screen-reader user is told nothing at all. This is true only - // after a degraded state was actually cleared by a successful read, so an - // ordinary success never announces anything, and it falls back to false at - // the next degraded onset so a second recovery is announced too (a live - // region only speaks when its TEXT changes). The FOLLOWING success also - // retires it, so the sentence lives for about one poll interval rather than - // the rest of the session. + // again, and a screen-reader user is told nothing at all. A kind is set only + // after a disclosure was actually retracted by a successful read, so an + // ordinary success never announces anything, and it falls back to null at the + // next degraded or refusal onset so a second recovery is announced too (a + // live region only speaks when its TEXT changes). // // It is deliberately NOT cleared on a 403: the surfaces already gate this // sentence on `!queueAccessRevoked`, the same guard the warning uses, so the // permission path keeps its single owner. - const queueRefreshRecovered = ref(false) + const queueRefreshRecoveredKind = ref(null) + const queueRefreshRecovered = computed(() => queueRefreshRecoveredKind.value !== null) + // WHEN the sentence was raised, counted in BACKGROUND reads (#2638 item 2). + // + // THE RETIREMENT RULE: a standing recovery sentence is retired by the next + // degraded or refusal onset, or by a BACKGROUND poll success belonging to a + // read LATER than the one the sentence is stamped with — never by an explicit + // load. The two stamps are set out below; the onsets are immediate for both, + // because there the sentence is false rather than merely old. + // + // Explicit loads take the same `recordQueueRefreshSuccess` path and are + // common within one poll interval of a recovery: the post-decision reloads in + // useReviewActions, the batch composables, the board-filter watcher, + // `dismissSettledElsewhereNotice` and the pre-decision refresh barrier all + // call `loadProposals`. A reviewer's already-clicked Approve can therefore + // land ~150 ms after the recovering poll and empty the region, and a polite + // live region whose text is reverted that fast may never be spoken at all + // (#2638 item 2, from PR #2630's verification pass). An explicit load that + // succeeds while the sentence stands leaves it standing; it still RAISES one + // when it is the read that ends a degraded state, which is the #2630 + // behaviour and is unchanged. + // + // Counting background reads rather than wall-clock keeps the composable free + // of teardown state and still gives the sentence at least one full poll + // interval of life, which is what #2630 intended: + // + // - a POLL-raised sentence is stamped with the read it was raised in, so it + // survives that read and retires on the next later poll success; + // - an EXPLICIT-raised sentence is stamped with the read that has not + // started yet, so it survives the next poll success and retires on the one + // after. + // + // The asymmetry is the whole point (round-2 review finding). An explicit load + // lands BETWEEN background reads, so the ordinal sitting on the counter names + // a read that already finished: stamping that would let the very next tick + // retire the sentence, and a post-decision reload that recovers the queue + // 14.9 s into a 15 s cycle would be blanked 100 ms later — the same defect + // this rule exists to close, with the roles swapped. + // + // One reachability-limited gap is accepted: if the poll never records another + // COMPOSITE success — a hash-pinned by-id read failing every tick with a + // status that is neither transient nor a pin-level outcome, so 405/410/409, + // which this route is documented not to emit — nothing retires the sentence + // for the rest of the session except a degraded or refusal onset, because an + // explicit success no longer bounds it either. + let backgroundQueueReadCount = 0 + let queueRecoveryRaisedAtBackgroundRead: number | null = null + + function raiseQueueRecovery(kind: QueueRecoveryKind, source: 'poll' | 'explicit') { + queueRefreshRecoveredKind.value = kind + queueRecoveryRaisedAtBackgroundRead = + source === 'poll' ? backgroundQueueReadCount : backgroundQueueReadCount + 1 + } + + function retireQueueRecovery() { + queueRefreshRecoveredKind.value = null + queueRecoveryRaisedAtBackgroundRead = null + } let latestProposalLoadRequestId = 0 const availableBoards = ref([]) const loadingBoards = ref(false) @@ -704,7 +777,11 @@ export function useReviewProposals() { // An explicit successful load is as trustworthy as a successful poll and // clears any older degraded indication without changing load semantics. // It goes through the same accounting as a successful poll so both exits - // from the degraded state raise the recovery signal (#2214). + // from the degraded state raise the recovery signal (#2214) — but it + // never RETIRES a standing one, because it can land a few hundred + // milliseconds after the poll that raised it and blank the live region + // before it is spoken (#2638 item 2). No `source` is passed: the default + // is 'explicit'. recordQueueRefreshSuccess() // An explicit load that succeeded is proof access is back. const accessWasRevoked = queueAccessRevoked.value @@ -783,10 +860,10 @@ export function useReviewProposals() { consecutiveQueueRefreshRefusals += 1 if (consecutiveQueueRefreshRefusals >= REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD) { queueRefreshRefused.value = true - // Same reason as the degraded onset below: a standing "up to date - // again" contradicts the warning beside it, and its unchanged text - // would silence the next real recovery. - queueRefreshRecovered.value = false + // Same reason as the degraded onset below: a standing recovery + // sentence of EITHER kind contradicts the warning beside it, and its + // unchanged text would silence the next real recovery. + retireQueueRecovery() } } else { // Anything that is not another qualifying list refusal interrupts the @@ -806,11 +883,14 @@ export function useReviewProposals() { consecutiveQueueRefreshFailures += 1 if (consecutiveQueueRefreshFailures >= REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD) { queueRefreshStale.value = true - // A degraded onset retires the previous recovery announcement. Leaving it - // standing would both contradict the warning beside it and stop the NEXT - // recovery from being announced, because the live region's text would - // never change. - queueRefreshRecovered.value = false + // A degraded onset retires the previous recovery announcement, whichever + // disclosure it retracted. Leaving it standing would both contradict the + // warning beside it and stop the NEXT recovery from being announced, + // because the live region's text would never change. This is the one + // retirement that is deliberately not gated on the read kind (#2638): it + // is not "the sentence has been up long enough", it is "the sentence is + // now false". + retireQueueRecovery() } } @@ -846,42 +926,81 @@ export function useReviewProposals() { * fabricate a freshness the surface does not have. Same tick, two different * claims, two different pieces of evidence — so #2445's composite semantics * for the transient counter are deliberately untouched here. + * + * `source` is the read this list leg belongs to, and it only decides how long + * the sentence raised here lives (the retirement rule at + * `backgroundQueueReadCount`); the retraction itself is the same either way. */ - function recordQueueListReadSucceeded(): boolean { + function recordQueueListReadSucceeded(source: 'poll' | 'explicit'): boolean { consecutiveQueueRefreshRefusals = 0 if (!queueRefreshRefused.value) return false queueRefreshRefused.value = false // Retracting a disclosure silently is exactly the #2630 defect: the warning // is simply gone on the next render and a reviewer who was not watching // that corner is never told the refusal claim no longer holds. - queueRefreshRecovered.value = true + // + // Its OWN sentence, not the queue one (#2638 item 2). All this evidence + // supports is that the list request is being answered again: the pin leg + // can still fail below and return before `proposals.value = next`, so + // "showing current proposals" would be false for up to two more poll + // intervals (#2214). + raiseQueueRecovery('refused', source) return true } /** - * The WHOLE composite read landed. `recoveryAlreadyRaised` is passed by the - * poll when `recordQueueListReadSucceeded` already announced a recovery - * earlier in this same read, so the retirement branch below cannot retire the - * sentence its own read just raised. + * The WHOLE composite read landed. + * + * `source` names the read that landed, because only a BACKGROUND one may + * retire a standing recovery sentence, and because a sentence raised HERE is + * stamped differently depending on it (#2638 item 2 — see the retirement rule + * at `backgroundQueueReadCount`). It defaults to 'explicit' so every caller + * that is not the poll is safe by construction. + * + * `recoveryAlreadyRaised` is passed by the poll when + * `recordQueueListReadSucceeded` already announced a recovery earlier in this + * same read, so the retirement branch below cannot retire the sentence its + * own read just raised (#2694). The read counter states the same fact for + * every kind of read; both are kept because they answer different questions — + * "did THIS read raise it" and "was it raised by an EARLIER read". */ - function recordQueueRefreshSuccess(options?: { recoveryAlreadyRaised?: boolean }) { + function recordQueueRefreshSuccess(options?: { + source?: 'poll' | 'explicit' + recoveryAlreadyRaised?: boolean + }) { + const source = options?.source ?? 'explicit' consecutiveQueueRefreshFailures = 0 // Idempotent: a no-op when the poll already ran it at the list-success // point, and the whole clear when an explicit load lands. const recoveryRaisedThisRead = - recordQueueListReadSucceeded() || options?.recoveryAlreadyRaised === true - // Only a success that ends a VISIBLE degraded state is a recovery. Setting - // this on every success would make both skins announce every 15 s. + recordQueueListReadSucceeded(source) || options?.recoveryAlreadyRaised === true + // The QUEUE sentence is spoken only when a read that COMPLETED ended a + // visible degraded state: `proposals.value` was replaced just above, so + // "showing current proposals" is provable. Raising it on every success + // would make both skins announce every 15 s, and raising it without the + // degraded state having been up would announce a recovery from nothing. + // + // It therefore also overwrites a 'refused' kind raised moments earlier in + // this same read, which is the stronger and now-provable statement. When + // NO degraded state was up, a refusal cleared by this same completed read + // keeps the refusal sentence instead: an under-claim, never a false one — + // it says the server is accepting refreshes again and stays silent about + // rows the reviewer can see for themselves. if (queueRefreshStale.value) { - queueRefreshRecovered.value = true - } else if (queueRefreshRecovered.value && !recoveryRaisedThisRead) { - // The FOLLOWING success retires the sentence, so it lives for about one - // poll interval instead of the whole session. An announcement is an + raiseQueueRecovery('degraded', source) + } else if ( + source === 'poll' && + !recoveryRaisedThisRead && + queueRecoveryRaisedAtBackgroundRead !== null && + backgroundQueueReadCount > queueRecoveryRaisedAtBackgroundRead + ) { + // A LATER background success retires the sentence, so it lives for about + // one poll interval instead of the whole session. An announcement is an // event; leaving its text standing indefinitely turns it into a claim // about the present that nothing is re-checking. Clearing it here rather // than on a timer keeps the composable free of teardown state, and the // clear is silent: a live region going empty announces nothing. - queueRefreshRecovered.value = false + retireQueueRecovery() } queueRefreshStale.value = false } @@ -978,6 +1097,11 @@ export function useReviewProposals() { async function refreshProposals(): Promise { // An explicit load is authoritative and about to replace the list wholesale. if (proposalsLoading.value || refreshInFlight) return + // This read's ordinal among background reads (#2638 item 2). Counted at the + // top, before any await, so a recovery raised anywhere inside this read + // belongs to THIS number and only a later read can retire it. Reads that + // return here never started, so they cannot age a standing sentence. + backgroundQueueReadCount += 1 // Snapshot the load counter rather than incrementing it: bumping it here // would make an in-flight `loadProposals` discard its own result AND skip // the `proposalsLoading = false` reset in its finally block, wedging the @@ -1043,7 +1167,7 @@ export function useReviewProposals() { // the refusal claim is falsified NOW -- before the pin leg gets a chance // to return early and strand it (round-2 review finding). The transient // accounting deliberately stays below, on the composite outcome. - const listRecoveryRaised = recordQueueListReadSucceeded() + const listRecoveryRaised = recordQueueListReadSucceeded('poll') const next = [...loadedProposals] let pinUnavailable = false let pinMalformed = false @@ -1133,7 +1257,7 @@ export function useReviewProposals() { } } proposals.value = next - recordQueueRefreshSuccess({ recoveryAlreadyRaised: listRecoveryRaised }) + recordQueueRefreshSuccess({ source: 'poll', recoveryAlreadyRaised: listRecoveryRaised }) // The queue moved under a reviewer who did not ask for it. Surfaces use // this to notice that the row they were rendering has just been dropped // or reordered away, instead of silently sliding onto another one @@ -1378,6 +1502,7 @@ export function useReviewProposals() { queueRefreshStale, queueRefreshRefused, queueRefreshRecovered, + queueRefreshRecoveredKind, availableBoards, loadingBoards, boardFilterInput, diff --git a/frontend/taskdeck-web/src/locales/en/review.ts b/frontend/taskdeck-web/src/locales/en/review.ts index a7be01a83..065fef8b9 100644 --- a/frontend/taskdeck-web/src/locales/en/review.ts +++ b/frontend/taskdeck-web/src/locales/en/review.ts @@ -96,6 +96,15 @@ export default { // try. It shares the visible slot with `degraded.body` and takes precedence // over it, and it is also the text of a mounted sr-only live region in each // skin, so keep it speakable as one sentence run. + // + // `refused.recovered` is that failure's own recovery sentence, and it is + // deliberately NARROWER than `degraded.recovered` (#2638 item 2). The refusal + // is retracted the moment the LIST read answers, on a tick whose composite + // read can still fail at the deep-link leg and leave the rendered rows + // untouched, so this sentence must say only that the server is accepting the + // refresh again. Translators: do NOT add a clause about the proposals being + // current or up to date — that is `degraded.recovered`'s job, and saying it + // here overclaimed for up to two poll intervals before this key existed. queue: { degraded: { body: 'This review queue may be out of date. Showing the last available proposals while Taskdeck retries.', @@ -103,6 +112,7 @@ export default { }, refused: { body: 'This review queue has stopped updating. The server is refusing the refresh rather than failing temporarily, so these are the last proposals it confirmed. Reload the page, or check the board filter in the address bar.', + recovered: 'The server is accepting refreshes for this review queue again.', }, }, diff --git a/frontend/taskdeck-web/src/locales/es/review.ts b/frontend/taskdeck-web/src/locales/es/review.ts index 01c040819..397a5b89f 100644 --- a/frontend/taskdeck-web/src/locales/es/review.ts +++ b/frontend/taskdeck-web/src/locales/es/review.ts @@ -48,6 +48,7 @@ export default { }, refused: { body: 'Esta cola de revisión ha dejado de actualizarse. El servidor está rechazando la actualización en lugar de fallar temporalmente, así que estas son las últimas propuestas que confirmó. Recarga la página o revisa el filtro de tablero en la barra de direcciones.', + recovered: 'El servidor vuelve a aceptar las actualizaciones de esta cola de revisión.', }, }, diff --git a/frontend/taskdeck-web/src/locales/it/review.ts b/frontend/taskdeck-web/src/locales/it/review.ts index b42bcb76a..4504a6d23 100644 --- a/frontend/taskdeck-web/src/locales/it/review.ts +++ b/frontend/taskdeck-web/src/locales/it/review.ts @@ -49,6 +49,7 @@ export default { }, refused: { body: 'Questa coda di revisione ha smesso di aggiornarsi. Il server sta rifiutando la richiesta di aggiornamento invece di fallire temporaneamente, quindi queste sono le ultime proposte che ha confermato. Ricarica la pagina oppure controlla il filtro bacheca nella barra degli indirizzi.', + recovered: 'Il server accetta di nuovo gli aggiornamenti di questa coda di revisione.', }, }, diff --git a/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts b/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts index bb6302826..b2b152092 100644 --- a/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts +++ b/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts @@ -2409,6 +2409,126 @@ describe('useReviewProposals', () => { rp.stopQueueRefresh() }) + it('is not retired by an explicit load inside the poll interval (#2638 item 2)', async () => { + vi.useFakeTimers() + mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'current' })]) + const rp = useReviewProposals() + await rp.loadProposals() + rp.startQueueRefresh() + + await pollTransientFailures(REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD) + mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'recovered' })]) + await vi.advanceTimersByTimeAsync(REVIEW_QUEUE_REFRESH_MS) + expect(rp.queueRefreshRecovered.value).toBe(true) + + // The reviewer's already-clicked Approve completes a few hundred + // milliseconds after the recovering poll and reloads the queue. Every + // explicit reload took the same success path, so it emptied the region + // before a polite live region had any chance to speak the sentence -- + // the #2638 defect. The load itself is unchanged: the fresh queue lands. + mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'after-decision' })]) + await rp.loadProposals() + expect(rp.proposals.value.map((p: any) => p.id)).toEqual(['after-decision']) + expect(rp.queueRefreshRecovered.value).toBe(true) + expect(rp.queueRefreshRecoveredKind.value).toBe('degraded') + + // Nor does a second one age it: explicit loads never retire, however many + // of them land inside the interval. + mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'and-again' })]) + await rp.loadProposals() + expect(rp.queueRefreshRecovered.value).toBe(true) + + // The next BACKGROUND success is what retires it, exactly as #2630 + // intended -- about one poll interval of life. + mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'still-fine' })]) + await vi.advanceTimersByTimeAsync(REVIEW_QUEUE_REFRESH_MS) + expect(rp.queueRefreshRecovered.value).toBe(false) + expect(rp.queueRefreshRecoveredKind.value).toBe(null) + rp.stopQueueRefresh() + }) + + it('is not retired by an explicit load that follows a FAILED background tick (#2638 item 2)', async () => { + vi.useFakeTimers() + mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'current' })]) + const rp = useReviewProposals() + await rp.loadProposals() + rp.startQueueRefresh() + + await pollTransientFailures(REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD) + mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'recovered' })]) + await vi.advanceTimersByTimeAsync(REVIEW_QUEUE_REFRESH_MS) + expect(rp.queueRefreshRecovered.value).toBe(true) + + // A later background read alone is not the rule: that read has to SUCCEED + // and be the one recording the success. This tick fails (below the + // threshold, so nothing is disclosed), and the explicit load that follows + // is still an explicit load. + mockAutomationApi.getProposals.mockRejectedValueOnce({ response: { status: 500 } }) + await vi.advanceTimersByTimeAsync(REVIEW_QUEUE_REFRESH_MS) + mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'explicit' })]) + await rp.loadProposals() + expect(rp.queueRefreshStale.value).toBe(false) + expect(rp.queueRefreshRecovered.value).toBe(true) + rp.stopQueueRefresh() + }) + + it('gives an EXPLICIT-load recovery a full interval before a poll can retire it (#2638 round 2)', async () => { + vi.useFakeTimers() + mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'current' })]) + const rp = useReviewProposals() + await rp.loadProposals() + rp.startQueueRefresh() + + await pollTransientFailures(REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD) + expect(rp.queueRefreshStale.value).toBe(true) + + // The post-decision reload is the read that ends the degraded state here, + // and it lands BETWEEN ticks -- 14.9 s into a 15 s cycle in the worst + // case. Stamping that raise with the ordinal already on the counter names + // a read that has finished, so the tick 100 ms later would retire the + // sentence: the same defect this rule exists to close, with the roles + // swapped (round-2 review finding). + mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'explicit' })]) + await rp.loadProposals() + expect(rp.queueRefreshStale.value).toBe(false) + expect(rp.queueRefreshRecovered.value).toBe(true) + expect(rp.queueRefreshRecoveredKind.value).toBe('degraded') + + // The next poll success is the one the sentence lives THROUGH. + mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'first-poll' })]) + await vi.advanceTimersByTimeAsync(REVIEW_QUEUE_REFRESH_MS) + expect(rp.queueRefreshRecovered.value).toBe(true) + + // The one after retires it, so it is bounded exactly as a poll-raised + // sentence is -- at least one full interval, never the session. + mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'second-poll' })]) + await vi.advanceTimersByTimeAsync(REVIEW_QUEUE_REFRESH_MS) + expect(rp.queueRefreshRecovered.value).toBe(false) + expect(rp.queueRefreshRecoveredKind.value).toBe(null) + rp.stopQueueRefresh() + }) + + it('still retires an explicit-load recovery immediately at a degraded onset (#2638 round 2)', async () => { + vi.useFakeTimers() + mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'current' })]) + const rp = useReviewProposals() + await rp.loadProposals() + rp.startQueueRefresh() + + await pollTransientFailures(REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD) + mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'explicit' })]) + await rp.loadProposals() + expect(rp.queueRefreshRecovered.value).toBe(true) + + // The extra interval of life is about the sentence being OLD. An onset + // makes it FALSE, and that retirement stays immediate for either stamp, + // or the next real recovery would be silent. + await pollTransientFailures(REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD) + expect(rp.queueRefreshStale.value).toBe(true) + expect(rp.queueRefreshRecovered.value).toBe(false) + rp.stopQueueRefresh() + }) + it('clears at the next degraded onset so a second recovery announces again', async () => { vi.useFakeTimers() mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'current' })]) @@ -2558,6 +2678,24 @@ describe('useReviewProposals', () => { rp.stopQueueRefresh() }) + it('announces the retraction with the refusal sentence, not the queue sentence (#2638 item 2)', async () => { + const rp = await startedWithCurrentQueue() + + await pollListFailures(REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD, { response: { status: 400 } }) + expect(rp.queueRefreshRefused.value).toBe(true) + + mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'recovered' })]) + await vi.advanceTimersByTimeAsync(REVIEW_QUEUE_REFRESH_MS) + + // The retraction is raised by the LIST leg, and the composite success + // that follows it in the same read must not swap the sentence for the + // queue one: this signal's job is to retract the refusal claim, and the + // surfaces say only that refreshes are being accepted again. + expect(rp.queueRefreshRecovered.value).toBe(true) + expect(rp.queueRefreshRecoveredKind.value).toBe('refused') + rp.stopQueueRefresh() + }) + it('leaves the 403 authority path to its own owner', async () => { const rp = await startedWithCurrentQueue() @@ -2633,6 +2771,50 @@ describe('useReviewProposals', () => { rp.stopQueueRefresh() }) + it('says nothing about the queue when the pin leg strands the composite read (#2638 item 2)', async () => { + // The copy defect PR #2694's round-2 verification recorded on #2214. On a + // list-success/pin-fail tick the composite read returns before + // `proposals.value = next`, so the rows on screen are exactly the ones + // that were there before -- and the shared #2630 sentence's second clause + // ("Showing current proposals") stood for up to two further poll + // intervals, because the next tick's list success returns early too and + // only the degraded onset after it retires the sentence. + vi.useFakeTimers() + mockRoute.hash = '#proposal-p-pinned' + mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'p-pinned' })]) + const rp = useReviewProposals() + await rp.loadProposals() + rp.startQueueRefresh() + + for (let failure = 0; failure < REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD; failure += 1) { + mockAutomationApi.getProposals.mockRejectedValueOnce({ response: { status: 404 } }) + await vi.advanceTimersByTimeAsync(REVIEW_QUEUE_REFRESH_MS) + } + expect(rp.queueRefreshRefused.value).toBe(true) + + // The list read answers again; only the pinned row's by-id read is down. + mockAutomationApi.getProposals.mockResolvedValueOnce([]) + mockAutomationApi.getProposal.mockRejectedValueOnce({ response: { status: 500 } }) + await vi.advanceTimersByTimeAsync(REVIEW_QUEUE_REFRESH_MS) + + expect(rp.queueRefreshRefused.value).toBe(false) + expect(rp.queueRefreshRecovered.value).toBe(true) + // The kind is what the surfaces read to pick the sentence, and this is + // the tick that proves why the two cannot share one: the queue was NOT + // replaced. + expect(rp.queueRefreshRecoveredKind.value).toBe('refused') + expect(rp.proposals.value.map((p: any) => p.id)).toEqual(['p-pinned']) + + // A LATER background success retires it, the same rule the queue sentence + // follows. This tick's list carries the pinned row, so there is no by-id + // leg and the composite read completes. + mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'p-pinned' })]) + await vi.advanceTimersByTimeAsync(REVIEW_QUEUE_REFRESH_MS) + expect(rp.queueRefreshRecovered.value).toBe(false) + expect(rp.queueRefreshRecoveredKind.value).toBe(null) + rp.stopQueueRefresh() + }) + it('does not count a pin-leg failure, whose tick read the list successfully', async () => { vi.useFakeTimers() mockRoute.hash = '#proposal-p-pinned' diff --git a/frontend/taskdeck-web/src/tests/views/ReviewView.spec.ts b/frontend/taskdeck-web/src/tests/views/ReviewView.spec.ts index 383bab4e3..fa2d087a2 100644 --- a/frontend/taskdeck-web/src/tests/views/ReviewView.spec.ts +++ b/frontend/taskdeck-web/src/tests/views/ReviewView.spec.ts @@ -504,7 +504,14 @@ describe('ReviewView', () => { expect(wrapper.find('[data-testid="review-queue-stale"]').exists()).toBe(false) expect(wrapper.find('[data-testid="review-queue-refused"]').text()).toBe('') + // The refusal's OWN retraction sentence (#2638 item 2). The queue + // sentence would add "Showing current proposals", which this signal does + // not prove: it is raised by the list leg, on a tick whose composite read + // can still bail before the rows are replaced. expect(wrapper.find('[data-testid="review-queue-recovered"]').text()).toBe( + enReview.queue.refused.recovered, + ) + expect(wrapper.find('[data-testid="review-queue-recovered"]').text()).not.toBe( enReview.queue.degraded.recovered, ) } finally { 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 385297676..baf68e533 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 @@ -4015,6 +4015,45 @@ describe('PaperReviewView', () => { } }) + it('announces a refusal retraction with the refusal sentence, not the queue one (#2638)', async () => { + vi.useFakeTimers({ toFake: ['setInterval', 'clearInterval', 'setTimeout', 'clearTimeout', 'Date'] }) + try { + const wrapper = await mountView([makeProposal({ id: 'retained-1' })]) + const region = wrapper.get('[data-testid="paper-review-queue-recovered"]') + expect(region.text()).toBe('') + + mocks.getProposals.mockRejectedValue({ response: { status: 400 } }) + for (let failure = 0; failure < REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD; failure += 1) { + vi.advanceTimersByTime(REVIEW_QUEUE_REFRESH_MS) + await flushPromises() + } + await wrapper.vm.$nextTick() + expect(wrapper.get('[data-testid="paper-review-queue-refused"]').text()).toBe( + enReview.queue.refused.body, + ) + + mocks.getProposals.mockResolvedValue([makeProposal({ id: 'retained-1' })]) + vi.advanceTimersByTime(REVIEW_QUEUE_REFRESH_MS) + await flushPromises() + await wrapper.vm.$nextTick() + + // One region, two sentences, picked by the kind the composable reports. + // The refusal retraction is raised by the LIST leg on a tick that may + // never replace the rendered rows, so "Showing current proposals" would + // overclaim (#2214, PR #2694 round 2); the degraded recovery beside it + // keeps saying exactly that, and its own test asserts so. + const after = wrapper.get('[data-testid="paper-review-queue-recovered"]') + expect(after.text()).toBe(enReview.queue.refused.recovered) + expect(after.text()).not.toBe(enReview.queue.degraded.recovered) + // Still the region that was mounted before anything went wrong (#2630). + expect(after.element).toBe(region.element) + expect(wrapper.find('[data-testid="paper-review-queue-refused"]').text()).toBe('') + wrapper.unmount() + } finally { + vi.useRealTimers() + } + }) + it('renders the refusal warning in the empty column too (#2214)', async () => { vi.useFakeTimers({ toFake: ['setInterval', 'clearInterval', 'setTimeout', 'clearTimeout', 'Date'] }) try { diff --git a/frontend/taskdeck-web/src/views/LegacyReviewView.vue b/frontend/taskdeck-web/src/views/LegacyReviewView.vue index eb33eb1d5..6ee586fe6 100644 --- a/frontend/taskdeck-web/src/views/LegacyReviewView.vue +++ b/frontend/taskdeck-web/src/views/LegacyReviewView.vue @@ -31,6 +31,7 @@ const { queueRefreshStale, queueRefreshRefused, queueRefreshRecovered, + queueRefreshRecoveredKind, unavailableProposalId, unavailableProposalMalformed, dismissableProposalIds, @@ -332,14 +333,29 @@ onUnmounted(() => { throughout, withholding its text, because a live region inserted at the same moment its text appears is unreliably announced (#2593). The signal comes from the shared composable, so both skins announce the - same transition with the same sentence (ADR-0038 / #1124). --> + same transition with the same sentence (ADR-0038 / #1124). + + TWO sentences through this one region (#2638 item 2), chosen by the + kind the composable reports: a 'degraded' recovery follows a completed + read and may say the rows are current, while a 'refused' one is raised + as soon as the LIST read answers — a tick that may never replace the + queue — so it says only that the server is accepting refreshes again. + Paper picks the key the same way. -->

{{ queueRefreshRecovered && !queueAccessRevoked ? $t('review.queue.degraded.recovered') : '' }}

+ >{{ + queueRefreshRecovered && !queueAccessRevoked + ? $t( + queueRefreshRecoveredKind === 'refused' + ? 'review.queue.refused.recovered' + : 'review.queue.degraded.recovered', + ) + : '' + }}