From 0a554e8d26e8486f7617b344436f382b85d420f9 Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Sat, 5 Sep 2026 15:45:35 +0100 Subject: [PATCH 1/6] fix(review): disclose a list read the server keeps refusing The transient counter shipped by #2445 deliberately ignores a non-transient answer: a 400/404/405/410 on the LIST read resets its run and returns. That leaves the worst failure of all silent. A `?boardId=not-a-guid` query reaches `GetProposals([FromQuery] Guid? boardId)` under `[ApiController]` because `normalizeBoardIdQueryParam` only trims, so it is a model-binding 400 on every tick: the poll keeps running, the counter keeps resetting, no degraded state ever rises, and the surface goes on showing rows the server has not confirmed since the reviewer arrived. `queueRefreshRefused` is a second threshold with its own uninterrupted run. It reuses the #2445 threshold and ruling: three uninterrupted qualifying failures raise it, an interruption resets the run without taking a risen disclosure off the screen, and the next successful list read clears it and raises the #2630 recovery signal. It keeps the last trustworthy queue and keeps polling. The predicate is any 4xx the server answered on the LIST leg except 401 and 403. 403 is the authority path and is intercepted earlier in `refreshProposals`, which clears the queue, raises `queueAccessRevoked` and suspends the poll; 401 belongs to the HTTP interceptor, which clears the session and redirects to login. 408 and 429 need no exclusion because they are already transient. A pin-leg failure does not count toward the run and breaks it, since reaching the by-id request proves that tick's list read succeeded. Refs #2214 --- .../src/composables/useReviewProposals.ts | 83 ++++++++- .../taskdeck-web/src/locales/en/review.ts | 14 ++ .../taskdeck-web/src/locales/es/review.ts | 3 + .../taskdeck-web/src/locales/it/review.ts | 3 + .../composables/useReviewProposals.spec.ts | 167 ++++++++++++++++++ 5 files changed, 265 insertions(+), 5 deletions(-) diff --git a/frontend/taskdeck-web/src/composables/useReviewProposals.ts b/frontend/taskdeck-web/src/composables/useReviewProposals.ts index 55ceea711..d1ec69768 100644 --- a/frontend/taskdeck-web/src/composables/useReviewProposals.ts +++ b/frontend/taskdeck-web/src/composables/useReviewProposals.ts @@ -192,6 +192,39 @@ function isTransientQueueRefreshFailure(err: unknown): boolean { return status === undefined || status === 408 || status === 429 || status >= 500 } +/** + * A LIST read the server ANSWERED and refused (#2214 item 2). + * + * The transient predicate above deliberately excludes this family, and + * `recordQueueRefreshFailure` then resets its run and returns — which leaves + * the worst failure mode of all completely silent. `?boardId=not-a-guid` + * reaches `GetProposals([FromQuery] Guid? boardId)` under `[ApiController]` + * (`normalizeBoardIdQueryParam` only trims), so it is a model-binding 400 on + * EVERY tick: the poll keeps running, the counter keeps resetting, no degraded + * state ever rises, and the surface goes on showing rows the server has not + * confirmed since the reviewer arrived. A 404, 405 or 410 on the same read is + * the same class of fact — the query is being refused, and no later tick makes + * it acceptable. + * + * Excluded, both with an owner elsewhere: + * - 403 is the authority path. `refreshProposals` intercepts it before this + * accounting is reached: it clears the queue, raises `queueAccessRevoked` + * and suspends the poll. Counting it here as well would put two disclosures + * on screen for one fact, one of which ("reload, or check the board + * filter") is wrong for revoked access. + * - 401 is `api/http.ts`'s: it clears the session and redirects to login. + * Telling a reviewer on their way out to check the board filter is false. + * + * 408 and 429 need no exclusion — they are transient above, and this predicate + * is only consulted for failures that are not. + */ +function isRefusedQueueRefreshFailure(err: unknown): boolean { + if (isTransientQueueRefreshFailure(err)) return false + const status = (err as { response?: { status?: number } } | null)?.response?.status + if (status === undefined || status === 401 || status === 403) return false + return status >= 400 && status < 500 +} + export function isProposalStale(proposal: ApiProposal, nowMs: number): boolean { if (!proposal || normalizeProposalStatus(proposal.status) !== 'PendingReview') return false // Guard against missing/invalid createdAt: a falsy value (new Date(null) is @@ -234,6 +267,14 @@ export function useReviewProposals() { // Unlike `queueAccessRevoked`, this is not an authority result. It means the // last queue we could render is still shown while background reads retry. const queueRefreshStale = ref(false) + // The second, SEPARATE threshold (#2214 item 2). `queueRefreshStale` belongs + // to the transient counter and must keep belonging to it: "the network keeps + // blipping, we are retrying" and "the server is answering and refusing the + // query" are different facts with different remedies, and a reviewer who is + // told the first while the second is true will wait for a recovery that + // 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 // was not looking at that corner is never told the queue is trustworthy @@ -653,6 +694,7 @@ export function useReviewProposals() { let refreshInterval: ReturnType | null = null let refreshInFlight = false let consecutiveQueueRefreshFailures = 0 + let consecutiveQueueRefreshRefusals = 0 // A 403 pauses the configured poll without making it forget how the owning // surface asked it to behave. Permanent stop/disposal clears this state so a // late successful explicit load cannot resurrect a surface that has left. @@ -661,7 +703,33 @@ export function useReviewProposals() { let shouldRefreshNow: (() => boolean) | null = null let refreshAbort: AbortController | null = null - function recordQueueRefreshFailure(err: unknown) { + /** + * `leg` names which request of the composite background read failed, because + * the two thresholds ask different questions of it (#2214 item 2). + * + * The transient run counts both legs, exactly as it has since #2445. The + * REFUSAL run is a claim about the LIST read alone, so a pin-leg failure not + * only fails to count toward it, it breaks it: reaching the by-id request at + * all means that tick's list read had already succeeded. + */ + function recordQueueRefreshFailure(err: unknown, leg: 'list' | 'pin') { + if (leg === 'list' && isRefusedQueueRefreshFailure(err)) { + 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 + } + } else { + // Anything that is not another qualifying list refusal interrupts the + // uninterrupted run the disclosure claims. It resets the RUN only: a + // risen disclosure stays up, because an interruption is not evidence + // that the retained queue is current (the #2445 ruling, applied + // symmetrically to this second threshold). + consecutiveQueueRefreshRefusals = 0 + } if (!isTransientQueueRefreshFailure(err)) { // A non-transient failure breaks the uninterrupted transient run, but it // does not prove that the retained queue is fresh enough to clear a @@ -682,9 +750,12 @@ export function useReviewProposals() { function recordQueueRefreshSuccess() { consecutiveQueueRefreshFailures = 0 + consecutiveQueueRefreshRefusals = 0 // 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. - if (queueRefreshStale.value) { + // this on every success would make both skins announce every 15 s. A + // refused queue clearing is the same kind of transition and gets the same + // sentence: the warning simply vanishing is silent either way (#2630). + if (queueRefreshStale.value || queueRefreshRefused.value) { queueRefreshRecovered.value = true } else if (queueRefreshRecovered.value) { // The FOLLOWING success retires the sentence, so it lives for about one @@ -696,6 +767,7 @@ export function useReviewProposals() { queueRefreshRecovered.value = false } queueRefreshStale.value = false + queueRefreshRefused.value = false } /** @@ -919,7 +991,7 @@ export function useReviewProposals() { // The composite read is incomplete. Preserve the exact queue and // availability state currently rendered; a later tick can retry. if (isSupersededQueueRead()) return - recordQueueRefreshFailure(e) + recordQueueRefreshFailure(e, 'pin') logError('Review deep-link background refresh failed:', e) return } @@ -963,7 +1035,7 @@ export function useReviewProposals() { // A read for a board the reviewer has already left, or one superseded by // an explicit load, cannot make the current queue degraded. if (isSupersededQueueRead()) return - recordQueueRefreshFailure(e) + recordQueueRefreshFailure(e, 'list') logError('Review queue background refresh failed:', e) } finally { refreshInFlight = false @@ -1179,6 +1251,7 @@ export function useReviewProposals() { unavailableProposalId, queueAccessRevoked, queueRefreshStale, + queueRefreshRefused, queueRefreshRecovered, availableBoards, loadingBoards, diff --git a/frontend/taskdeck-web/src/locales/en/review.ts b/frontend/taskdeck-web/src/locales/en/review.ts index e53395351..70c58490c 100644 --- a/frontend/taskdeck-web/src/locales/en/review.ts +++ b/frontend/taskdeck-web/src/locales/en/review.ts @@ -85,11 +85,25 @@ export default { // successful read. Translators: keep it in the same register as `body`, and // keep it a statement of the CURRENT state — the surface does not promise that // any particular proposal arrived, only that the queue is trustworthy again. + // + // `refused.body` is the OTHER failure (#2214 item 2), and it deliberately + // says something `degraded.body` does not. Degraded means "retrying"; refused + // means the server keeps ANSWERING and rejecting the query — a + // `?boardId=not-a-guid` in the address bar 400s every tick — so waiting for a + // recovery is exactly the wrong thing to do. Translators: all three clauses + // are load-bearing. The queue shown is the last one the server CONFIRMED; + // the refusal is not a temporary failure; and the reviewer has two things to + // 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. queue: { degraded: { body: 'This review queue may be out of date. Showing the last available proposals while Taskdeck retries.', recovered: 'This review queue is up to date again. Showing current proposals.', }, + 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.', + }, }, batchExecute: { diff --git a/frontend/taskdeck-web/src/locales/es/review.ts b/frontend/taskdeck-web/src/locales/es/review.ts index 3115e09fa..2eee56063 100644 --- a/frontend/taskdeck-web/src/locales/es/review.ts +++ b/frontend/taskdeck-web/src/locales/es/review.ts @@ -46,6 +46,9 @@ export default { body: 'Es posible que esta cola de revisión no esté actualizada. Se muestran las últimas propuestas disponibles mientras Taskdeck lo vuelve a intentar.', recovered: 'Esta cola de revisión vuelve a estar actualizada. Se muestran las propuestas actuales.', }, + 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.', + }, }, // GH-1307 -- traducción automática (machine-translated), pendiente de revisión nativa. diff --git a/frontend/taskdeck-web/src/locales/it/review.ts b/frontend/taskdeck-web/src/locales/it/review.ts index d61432e10..c68cff790 100644 --- a/frontend/taskdeck-web/src/locales/it/review.ts +++ b/frontend/taskdeck-web/src/locales/it/review.ts @@ -47,6 +47,9 @@ export default { body: 'Questa coda di revisione potrebbe non essere aggiornata. Vengono mostrate le ultime proposte disponibili mentre Taskdeck riprova.', recovered: 'Questa coda di revisione è di nuovo aggiornata. Vengono mostrate le proposte correnti.', }, + 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.', + }, }, // GH-1307 -- traduzione automatica (machine-translated), in attesa di revisione madrelingua. diff --git a/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts b/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts index 7ad9424a5..dfb0fe873 100644 --- a/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts +++ b/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts @@ -2281,4 +2281,171 @@ describe('useReviewProposals', () => { rp.stopQueueRefresh() }) }) + + describe('refused list-read disclosure (#2214 item 2)', () => { + /** + * The transient counter (`queueRefreshStale`) deliberately IGNORES a + * non-transient answer: a 400/404/405/410 on the LIST read resets its run + * and returns. That leaves the worst case of all completely silent — a + * `?boardId=not-a-guid` query 400s every single tick, the poll keeps + * running, the counter keeps resetting, no degraded state ever rises, and + * the surface shows an ordinary queue (or an ordinary empty state) + * indefinitely while the server has not confirmed a single one of those + * rows since the reviewer arrived. + * + * `queueRefreshRefused` is that second, separate threshold. Same threshold + * and same #2445 ruling as the transient one, but its own uninterrupted + * run, because the two facts are different: "the network keeps blipping" + * versus "the server is answering and refusing". + */ + async function pollListFailures(count: number, failure: unknown) { + for (let attempt = 0; attempt < count; attempt += 1) { + mockAutomationApi.getProposals.mockRejectedValueOnce(failure) + await vi.advanceTimersByTimeAsync(REVIEW_QUEUE_REFRESH_MS) + } + } + + async function startedWithCurrentQueue() { + vi.useFakeTimers() + mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'current' })]) + const rp = useReviewProposals() + await rp.loadProposals() + rp.startQueueRefresh() + expect(rp.queueRefreshRefused.value).toBe(false) + return rp + } + + it('rises only at the threshold, keeps the last trustworthy queue, and keeps polling', async () => { + const rp = await startedWithCurrentQueue() + + for (let failure = 1; failure <= REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD; failure += 1) { + await pollListFailures(1, { response: { status: 400 } }) + expect(rp.queueRefreshRefused.value).toBe( + failure === REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD, + ) + } + // The retained queue is exactly what the server last confirmed. + expect(rp.proposals.value.map((p: any) => p.id)).toEqual(['current']) + // And this is a disclosure, not a stop: the next tick still asks. + const callsBefore = mockAutomationApi.getProposals.mock.calls.length + await pollListFailures(1, { response: { status: 400 } }) + expect(mockAutomationApi.getProposals.mock.calls.length).toBe(callsBefore + 1) + // The transient counter is a different fact and stays where it was. + expect(rp.queueRefreshStale.value).toBe(false) + rp.stopQueueRefresh() + }) + + it('counts 404, 405 and 410 as refusals too', async () => { + const rp = await startedWithCurrentQueue() + + await pollListFailures(1, { response: { status: 404 } }) + await pollListFailures(1, { response: { status: 405 } }) + expect(rp.queueRefreshRefused.value).toBe(false) + await pollListFailures(1, { response: { status: 410 } }) + expect(rp.queueRefreshRefused.value).toBe(true) + rp.stopQueueRefresh() + }) + + it('does not rise on two refusals plus a success', async () => { + const rp = await startedWithCurrentQueue() + + await pollListFailures(2, { response: { status: 400 } }) + mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'fresh' })]) + await vi.advanceTimersByTimeAsync(REVIEW_QUEUE_REFRESH_MS) + expect(rp.queueRefreshRefused.value).toBe(false) + + await pollListFailures(2, { response: { status: 400 } }) + expect(rp.queueRefreshRefused.value).toBe(false) + rp.stopQueueRefresh() + }) + + it('lets an intervening transient failure reset the run without raising it', async () => { + const rp = await startedWithCurrentQueue() + + await pollListFailures(2, { response: { status: 400 } }) + // A 500 is not a refusal. It breaks the uninterrupted run the disclosure + // claims, so the next two 400s cannot complete a three-long streak. + await pollListFailures(1, { response: { status: 500 } }) + await pollListFailures(2, { response: { status: 400 } }) + expect(rp.queueRefreshRefused.value).toBe(false) + + await pollListFailures(1, { response: { status: 400 } }) + expect(rp.queueRefreshRefused.value).toBe(true) + rp.stopQueueRefresh() + }) + + it('does not let an intervening transient failure clear a risen disclosure', async () => { + const rp = await startedWithCurrentQueue() + + await pollListFailures(REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD, { response: { status: 400 } }) + expect(rp.queueRefreshRefused.value).toBe(true) + + // Symmetric to the #2445 ruling on the transient side: an interruption + // resets the RUN, it does not prove the retained queue is current, so it + // cannot take a standing disclosure off the screen. + await pollListFailures(1, { response: { status: 500 } }) + expect(rp.queueRefreshRefused.value).toBe(true) + rp.stopQueueRefresh() + }) + + it('clears on the next successful list read and announces the recovery', async () => { + const rp = await startedWithCurrentQueue() + + await pollListFailures(REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD, { response: { status: 400 } }) + expect(rp.queueRefreshRefused.value).toBe(true) + expect(rp.queueRefreshRecovered.value).toBe(false) + + mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'recovered' })]) + await vi.advanceTimersByTimeAsync(REVIEW_QUEUE_REFRESH_MS) + expect(rp.queueRefreshRefused.value).toBe(false) + expect(rp.proposals.value.map((p: any) => p.id)).toEqual(['recovered']) + // Clearing the warning is silent on its own, exactly as it is for the + // transient state (#2630): the recovery sentence is the announcement. + expect(rp.queueRefreshRecovered.value).toBe(true) + rp.stopQueueRefresh() + }) + + it('leaves the 403 authority path to its own owner', async () => { + const rp = await startedWithCurrentQueue() + + await pollListFailures(REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD, { response: { status: 403 } }) + // 403 is revoked access, not a refused refresh: it clears the queue, says + // so through `queueAccessRevoked`, and suspends the poll. Two owners for + // one fact would render two contradictory panels. + expect(rp.queueRefreshRefused.value).toBe(false) + expect(rp.queueAccessRevoked.value).toBe(true) + rp.stopQueueRefresh() + }) + + it('excludes 401, which belongs to the HTTP interceptor', async () => { + const rp = await startedWithCurrentQueue() + + await pollListFailures(REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD, { response: { status: 401 } }) + // A 401 is the session ending; `api/http.ts` clears it and redirects to + // login. Telling the reviewer to check the board filter on the way out + // would be false. + expect(rp.queueRefreshRefused.value).toBe(false) + rp.stopQueueRefresh() + }) + + it('does not count a pin-leg failure, whose tick read the list successfully', async () => { + vi.useFakeTimers() + mockRoute.hash = '#proposal-p-pinned' + mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'p-pinned' })]) + const rp = useReviewProposals() + await rp.loadProposals() + rp.startQueueRefresh() + + // The list answer arrives every tick; only the by-id re-authorization + // read fails. The disclosure claims the LIST read is being refused, and + // it demonstrably is not. + for (let attempt = 0; attempt < REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD; attempt += 1) { + mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'other' })]) + mockAutomationApi.getProposal.mockRejectedValueOnce({ response: { status: 405 } }) + await vi.advanceTimersByTimeAsync(REVIEW_QUEUE_REFRESH_MS) + } + expect(rp.queueRefreshRefused.value).toBe(false) + rp.stopQueueRefresh() + }) + }) }) From 020bcdbec0c0513a0e086426aaab77cf4fe1ab74 Mon Sep 17 00:00:00 2001 From: Chris0Jeky Date: Sat, 5 Sep 2026 15:48:12 +0100 Subject: [PATCH 2/6] fix(review): render the refused-refresh disclosure in both skins Each skin gains one always-mounted sr-only live region that withholds its text until the disclosure rises, the #2630 pattern: the visible warning is rendered inside a v-if, so it mounts already carrying its text, and a live region inserted at the same moment its text appears is announced unreliably. Paper's is hoisted above the activeProposal/v-else pair for the same reason its recovery region is, and it is a separate region rather than a shared slot because the two say different things and can both need to speak inside one poll interval. The visible warning is the SAME slot the degraded warning already uses, with different copy, in all three places it renders (Legacy, Paper's active column, Paper's empty column). The two states are alternatives, not additions: "refreshes are being refused" subsumes "the queue may be out of date", and showing both would put "while Taskdeck retries" beside a sentence saying the retries are answered and refused. The refusal wins when both stand. Keeping it one element also means the #2630 sticky/offset seam is untouched: queueStaleRef stays on the same node, its ResizeObserver keeps measuring the same warning, and mainColStickyStyle just widens its guard to match the widened v-if. 403 keeps its own owner: both the region and the warning stay gated on !queueAccessRevoked, so the access-revoked panel remains the single disclosure for revoked access. Refs #2214 --- .../src/tests/views/ReviewView.spec.ts | 109 ++++++++++++++++++ .../paper/review/PaperReviewView.spec.ts | 105 ++++++++++++++++- .../src/views/LegacyReviewView.vue | 28 ++++- .../src/views/paper/PaperReviewView.vue | 39 ++++++- 4 files changed, 269 insertions(+), 12 deletions(-) diff --git a/frontend/taskdeck-web/src/tests/views/ReviewView.spec.ts b/frontend/taskdeck-web/src/tests/views/ReviewView.spec.ts index 69f060d46..dba941c69 100644 --- a/frontend/taskdeck-web/src/tests/views/ReviewView.spec.ts +++ b/frontend/taskdeck-web/src/tests/views/ReviewView.spec.ts @@ -459,6 +459,115 @@ describe('ReviewView', () => { } }) + it('discloses a list read the server keeps refusing, from a region that was already mounted (#2214)', async () => { + vi.useFakeTimers({ toFake: ['setInterval', 'clearInterval', 'setTimeout', 'clearTimeout', 'Date'] }) + try { + mocks.getProposals.mockResolvedValue([buildProposal({ id: 'retained-1' })]) + const { wrapper } = await mountAt('/workspace/review') + + // Mounted and silent before anything goes wrong, for the same reason as + // the recovery region beside it (#2593/#2630): a live region inserted at + // the same moment its text appears is unreliably announced. + const before = wrapper.find('[data-testid="review-queue-refused"]') + expect(before.exists()).toBe(true) + expect(before.attributes('role')).toBe('status') + expect(before.attributes('aria-live')).toBe('polite') + expect(before.attributes('aria-atomic')).toBe('true') + expect(before.text()).toBe('') + + // A `?boardId=not-a-guid` in the address bar answers every tick with a + // model-binding 400. + 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() + + const after = wrapper.find('[data-testid="review-queue-refused"]') + expect(after.text()).toBe(enReview.queue.refused.body) + expect(after.element).toBe(before.element) + + // Same visible slot as the degraded warning, carrying the copy that is + // actually true: this is a refusal, not a retry. + const visible = wrapper.find('[data-testid="review-queue-stale"]') + expect(visible.exists()).toBe(true) + expect(visible.text()).toBe(enReview.queue.refused.body) + expect(visible.text()).not.toBe(enReview.queue.degraded.body) + // The last queue the server confirmed is still on screen. + expect(wrapper.find('#proposal-retained-1').exists()).toBe(true) + + mocks.getProposals.mockResolvedValue([buildProposal({ id: 'retained-1' })]) + vi.advanceTimersByTime(REVIEW_QUEUE_REFRESH_MS) + await flushPromises() + await wrapper.vm.$nextTick() + + expect(wrapper.find('[data-testid="review-queue-stale"]').exists()).toBe(false) + expect(wrapper.find('[data-testid="review-queue-refused"]').text()).toBe('') + expect(wrapper.find('[data-testid="review-queue-recovered"]').text()).toBe( + enReview.queue.degraded.recovered, + ) + } finally { + vi.useRealTimers() + } + }) + + it('prefers the refusal copy over the degraded copy when both states stand (#2214)', async () => { + vi.useFakeTimers({ toFake: ['setInterval', 'clearInterval', 'setTimeout', 'clearTimeout', 'Date'] }) + try { + mocks.getProposals.mockResolvedValue([buildProposal({ id: 'retained-1' })]) + const { wrapper } = await mountAt('/workspace/review') + + mocks.getProposals.mockRejectedValue({ response: { status: 500 } }) + 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.find('[data-testid="review-queue-stale"]').text()).toBe( + enReview.queue.degraded.body, + ) + expect(wrapper.find('[data-testid="review-queue-refused"]').text()).toBe('') + + // The transient state stays raised (nothing has proved the queue fresh), + // so both are true at once. The refusal is the stronger and more + // actionable statement, and "Taskdeck retries" would now be false. + mocks.getProposals.mockRejectedValue({ response: { status: 404 } }) + 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.find('[data-testid="review-queue-stale"]').text()).toBe( + enReview.queue.refused.body, + ) + } finally { + vi.useRealTimers() + } + }) + + it('leaves the refusal disclosure to the access-revoked panel on a 403 (#2214)', async () => { + vi.useFakeTimers({ toFake: ['setInterval', 'clearInterval', 'setTimeout', 'clearTimeout', 'Date'] }) + try { + mocks.getProposals.mockResolvedValue([buildProposal({ id: 'retained-1' })]) + const { wrapper } = await mountAt('/workspace/review') + + mocks.getProposals.mockRejectedValue({ response: { status: 403 } }) + 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.find('[data-testid="review-access-revoked"]').exists()).toBe(true) + expect(wrapper.find('[data-testid="review-queue-stale"]').exists()).toBe(false) + expect(wrapper.find('[data-testid="review-queue-refused"]').text()).toBe('') + } finally { + vi.useRealTimers() + } + }) + it('shows guided empty-state actions when there are no proposals', async () => { const { wrapper } = await mountAt('/workspace/review') 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 9a0278d67..f89fd5862 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 @@ -3496,12 +3496,15 @@ describe('PaperReviewView', () => { expect(queue.element.parentElement).toBe(root.element) expect(main.element.parentElement).toBe(root.element) expect(right.element.parentElement).toBe(root.element) - // The hoisted recovery region is the first child so it survives every - // flip of the branch pair below it (#2214 round 2); the three columns - // keep their order and their parent. + // The hoisted disclosure regions are the first children so they survive + // every flip of the branch pair below them (#2214 round 2); the three + // columns keep their order and their parent. Both are `.sr-only` and + // therefore absolutely positioned, so neither takes a grid column. const recovered = wrapper.get('[data-testid="paper-review-queue-recovered"]') + const refused = wrapper.get('[data-testid="paper-review-queue-refused"]') expect(Array.from(root.element.children)).toEqual([ recovered.element, + refused.element, queue.element, main.element, right.element, @@ -3534,8 +3537,10 @@ describe('PaperReviewView', () => { const right = wrapper.get('.paper-review-deep__rail-empty') expect(stale.element.parentElement).toBe(empty.element) const recovered = wrapper.get('[data-testid="paper-review-queue-recovered"]') + const refused = wrapper.get('[data-testid="paper-review-queue-refused"]') expect(Array.from(root.element.children)).toEqual([ recovered.element, + refused.element, queue.element, empty.element, right.element, @@ -3612,6 +3617,100 @@ describe('PaperReviewView', () => { } }) + it('discloses a list read the server keeps refusing, from a region that was already mounted (#2214)', async () => { + vi.useFakeTimers({ toFake: ['setInterval', 'clearInterval', 'setTimeout', 'clearTimeout', 'Date'] }) + try { + const wrapper = await mountView([makeProposal({ id: 'retained-1' })]) + + const before = wrapper.find('[data-testid="paper-review-queue-refused"]') + expect(before.exists()).toBe(true) + expect(before.attributes('role')).toBe('status') + expect(before.attributes('aria-live')).toBe('polite') + expect(before.attributes('aria-atomic')).toBe('true') + expect(before.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() + + const after = wrapper.find('[data-testid="paper-review-queue-refused"]') + expect(after.text()).toBe(enReview.queue.refused.body) + expect(after.element).toBe(before.element) + + // The visible warning is the SAME pinned slot the degraded state uses, so + // the sticky/offset handshake from #2630 keeps working unchanged; only + // its copy differs. + const visible = wrapper.get('[data-testid="paper-review-queue-stale"]') + expect(visible.text()).toBe(enReview.queue.refused.body) + expect(visible.classes()).toContain('paper-review-deep__queue-stale--pinned') + const main = wrapper.get('.paper-review-deep__main-col') + expect(visible.element.parentElement).toBe(main.element) + expect(main.attributes('style') ?? '').toContain('--paper-review-sticky-offset') + expect(wrapper.text()).toContain('Split "dark mode" into 3 cards') + 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 { + const wrapper = await mountView([]) + mocks.getProposals.mockRejectedValue({ response: { status: 405 } }) + + for (let failure = 0; failure < REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD; failure += 1) { + vi.advanceTimersByTime(REVIEW_QUEUE_REFRESH_MS) + await flushPromises() + } + await wrapper.vm.$nextTick() + + const stale = wrapper.get('[data-testid="paper-review-queue-stale"]') + expect(stale.text()).toBe(enReview.queue.refused.body) + const empty = wrapper.get('[data-testid="paper-review-empty"]') + expect(stale.element.parentElement).toBe(empty.element) + wrapper.unmount() + } finally { + vi.useRealTimers() + } + }) + + it('prefers the refusal copy over the degraded copy when both states stand (#2214)', async () => { + vi.useFakeTimers({ toFake: ['setInterval', 'clearInterval', 'setTimeout', 'clearTimeout', 'Date'] }) + try { + const wrapper = await mountView([makeProposal({ id: 'retained-1' })]) + + mocks.getProposals.mockRejectedValue({ response: { status: 500 } }) + 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-stale"]').text()).toBe( + enReview.queue.degraded.body, + ) + + mocks.getProposals.mockRejectedValue({ response: { status: 410 } }) + for (let failure = 0; failure < REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD; failure += 1) { + vi.advanceTimersByTime(REVIEW_QUEUE_REFRESH_MS) + await flushPromises() + } + await wrapper.vm.$nextTick() + + // "Showing the last available proposals while Taskdeck retries" is no + // longer true: the retries are being answered and refused. + expect(wrapper.get('[data-testid="paper-review-queue-stale"]').text()).toBe( + enReview.queue.refused.body, + ) + wrapper.unmount() + } finally { + vi.useRealTimers() + } + }) + it('keeps one recovery region across an empty-to-active branch flip (#2214)', async () => { // The case a per-arm region could not serve. The recovering poll assigns diff --git a/frontend/taskdeck-web/src/views/LegacyReviewView.vue b/frontend/taskdeck-web/src/views/LegacyReviewView.vue index 73ab1f305..eb614558b 100644 --- a/frontend/taskdeck-web/src/views/LegacyReviewView.vue +++ b/frontend/taskdeck-web/src/views/LegacyReviewView.vue @@ -29,6 +29,7 @@ const { summaryCards, queueAccessRevoked, queueRefreshStale, + queueRefreshRefused, queueRefreshRecovered, unavailableProposalId, dismissableProposalIds, @@ -212,8 +213,8 @@ const workspace = useWorkspaceStore() // announcement below is plain text, matching the hardcoded copy around it. // The states this skin SHARES with Paper are the exception and go through the // catalog rather than being forked per skin: the refused-deep-link panel reuses -// `review.empty.unavailable.*`, and the degraded/recovered queue disclosure -// reuses `review.queue.degraded.*` (#2214). +// `review.empty.unavailable.*`, and the background-refresh disclosures reuse +// `review.queue.degraded.*` and `review.queue.refused.*` (#2214). const awaitingCount = computed( () => summaryCards.value.find((card) => card.id === 'pending-review')?.value ?? 0, ) @@ -339,15 +340,34 @@ onUnmounted(() => { data-testid="review-queue-recovered" >{{ queueRefreshRecovered && !queueAccessRevoked ? $t('review.queue.degraded.recovered') : '' }}

+ +

{{ queueRefreshRefused && !queueAccessRevoked ? $t('review.queue.refused.body') : '' }}

+ +
-

{{ $t('review.queue.degraded.body') }}

+

{{ queueRefreshRefused ? $t('review.queue.refused.body') : $t('review.queue.degraded.body') }}

- queueRefreshStale.value && !queueAccessRevoked.value + (queueRefreshStale.value || queueRefreshRefused.value) && !queueAccessRevoked.value ? { '--paper-review-sticky-offset': `${stickyOffsetPx.value}px` } : {}, ) @@ -2640,6 +2641,28 @@ async function onClearBoardScope() { data-testid="paper-review-queue-recovered" >{{ queueRefreshRecovered && !queueAccessRevoked ? $t('review.queue.degraded.recovered') : '' }}

+ +

{{ queueRefreshRefused && !queueAccessRevoked ? $t('review.queue.refused.body') : '' }}

+ +

- {{ $t('review.queue.degraded.body') }} + {{ queueRefreshRefused ? $t('review.queue.refused.body') : $t('review.queue.degraded.body') }}

- {{ $t('review.queue.degraded.body') }} + {{ queueRefreshRefused ? $t('review.queue.refused.body') : $t('review.queue.degraded.body') }}