diff --git a/frontend/taskdeck-web/src/composables/useReviewProposals.ts b/frontend/taskdeck-web/src/composables/useReviewProposals.ts index 55ceea711..f8a4db190 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 @@ -225,6 +258,27 @@ export function useReviewProposals() { // can bind). A wrong-identity or cross-board answer fails closed into it too. // Explicit navigation is narrower on purpose and still marks only the 404. const unavailableProposalId = ref(null) + // WHY that pin is unavailable, because the id alone collapses two different + // truths (#2214). A 403 or a 404 is about a proposal that exists or existed; + // a 400 is the by-id route refusing to bind the id at all, so the link never + // named a proposal and no later tick makes it work. Rendering "it may have + // been applied, archived, or removed" for the second sends the reviewer to + // wait for a recovery that cannot arrive. + // + // Kept in lockstep with the id through `markProposalUnavailable` / + // `clearProposalUnavailable` rather than assigned at each of the write sites: + // a reason that outlived its id would label the NEXT unavailable pin. + const unavailableProposalMalformed = ref(false) + + function markProposalUnavailable(proposalId: string, reason: 'malformed' | 'refused') { + unavailableProposalId.value = proposalId + unavailableProposalMalformed.value = reason === 'malformed' + } + + function clearProposalUnavailable() { + unavailableProposalId.value = null + unavailableProposalMalformed.value = false + } // Set when a background read is refused with 403 (board access revoked // mid-session). The surfaces swap the ordinary "Nothing waiting" empty state // for an honest one -- clearing the queue without saying why would turn a @@ -234,6 +288,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 @@ -518,14 +580,14 @@ export function useReviewProposals() { if (proposalsLoading.value) return const proposalId = getProposalIdFromHash(route.hash) if (!proposalId) { - unavailableProposalId.value = null + clearProposalUnavailable() return } // A different hash starts a fresh lookup. Keep the current unavailable // state only while it still describes the route the user asked for. if (!proposalIdsEqual(unavailableProposalId.value, proposalId)) { - unavailableProposalId.value = null + clearProposalUnavailable() } const currentProposal = proposals.value.find((p) => proposalIdsEqual(p.id, proposalId)) @@ -533,7 +595,7 @@ export function useReviewProposals() { if (!matchesActiveBoardFilter(currentProposal.boardId)) { return } - unavailableProposalId.value = null + clearProposalUnavailable() await scrollToProposalFromHash() return } @@ -550,14 +612,16 @@ export function useReviewProposals() { // different record. Retain the hash as unavailable instead of upserting a // response whose identity does not match the requested proposal. if (!proposalIdsEqual(fetchedProposal.id, proposalId)) { - unavailableProposalId.value = proposalId + // A wrong record is not a broken address: the id bound fine, the + // server simply answered with something else. + markProposalUnavailable(proposalId, 'refused') return } if (!matchesActiveBoardFilter(fetchedProposal.boardId)) { return } upsertProposal(fetchedProposal) - unavailableProposalId.value = null + clearProposalUnavailable() await nextTick() await scrollToProposalFromHash() } catch (e: unknown) { @@ -565,10 +629,34 @@ export function useReviewProposals() { // 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 + // One outcome per status CLASS, using exactly the three predicates the + // background pin leg uses, so the two paths cannot answer the same fact + // two different ways (#2214). Before this, a 400 or a 403 on the + // reviewer's own deep-link read raised a generic "Failed to load + // proposal" toast and set no state at all: the surface fell back to the + // ordinary empty queue, and the very next background tick converted the + // identical refusal into the pin-unavailable panel. The toast named + // neither fact and was gone seconds later, contradicted by a panel that + // stayed. + // + // A settled fact about the target gets the panel and no toast, because + // the panel is the durable report and two reports for one fact is the + // asymmetry being removed. 404 already behaved this way. + if (isMalformedTargetError(e)) { + markProposalUnavailable(proposalId, 'malformed') + return + } + if (isForbiddenError(e) || isHttpNotFound(e)) { + // By-id authority over ONE target. The queue-level 403 and its + // `queueAccessRevoked` teardown live on the list leg and are untouched. + markProposalUnavailable(proposalId, 'refused') return } + // 405, 410, 5xx and no response are not facts about this target — 405 + // and 410 are the route misbehaving rather than the id being refused + // (#2658 draws the same line on the pin leg), and the rest may resolve on + // a later tick. Pinning the target unavailable would be a false negative, + // so the reviewer who asked keeps getting told the read failed. toast.error(getErrorDisplay(e, t('review.toast.loadProposalFailed')).message) } } @@ -628,6 +716,19 @@ export function useReviewProposals() { // 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' + // Only the POLL used to handle this, so a cold entry to a board whose + // access had been revoked fell through to the generic toast and left the + // authority state unset for a whole poll interval (round-2 review + // finding). Worse, the hash lookup below then 403'd on the by-id read + // and, since this slice made that a pin-level outcome, rendered "no + // longer available to review; it may have been applied, archived, or + // removed" about a proposal that was none of those things -- the board + // simply was not this reviewer's any more. + // + // The toast stays: this read is one the caller asked for, and every + // action composable that calls `loadProposals` still gets its failure + // signal and its 'failed' outcome unchanged. + if (isForbiddenError(e)) recordQueueAccessRevoked() toast.error(getErrorDisplay(e, t('review.toast.loadProposalsFailed')).message) outcome = 'failed' } finally { @@ -636,7 +737,13 @@ export function useReviewProposals() { } if (signal?.aborted) return 'aborted' - if (requestId === latestProposalLoadRequestId) { + // A revoked queue has one owner and one explanation. Re-authorising a + // hash-pinned row inside a board the server just refused wholesale can only + // produce a second, narrower and wrong account of the same fact, so the + // pin-level outcome is not even asked for. A successful load clears + // `queueAccessRevoked` above, so this can only be true when THIS read + // revoked it or an earlier one did and nothing has restored access. + if (requestId === latestProposalLoadRequestId && !queueAccessRevoked.value) { await openProposalFromHash(options) } if (requestId !== latestProposalLoadRequestId) return 'superseded' @@ -653,6 +760,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 +769,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 @@ -680,13 +814,67 @@ export function useReviewProposals() { } } - function recordQueueRefreshSuccess() { + /** + * A 403 on the QUEUE read is revoked board access, not a blip: stop polling + * an endpoint that will keep refusing, drop rows the server no longer + * authorises, and let the surface say exactly that. + * + * Shared by the poll's outer catch and the explicit load, because a 403 means + * the same thing whoever asked (round-2 review finding). Duplicating the + * three statements would be how the two legs drift into telling a reviewer + * two different stories about one revocation. + */ + function recordQueueAccessRevoked() { + queueAccessRevoked.value = true + proposals.value = [] + suspendQueueRefreshForPermission() + } + + /** + * The LIST leg answered, whatever the rest of the composite read goes on to + * do. Returns whether it raised the recovery sentence. + * + * The refusal disclosure's clear is LIST-SCOPED while the transient state's + * stays COMPOSITE-SCOPED, and the asymmetry is in what each one claims. The + * refusal says "the server is refusing the refresh rather than failing + * temporarily", which is a statement about the list REQUEST; one successful + * list read falsifies it outright, and leaving it up afterwards is simply a + * lie. The transient state says "the queue you are looking at may be out of + * date", which is a statement about the RENDERED QUEUE; a composite read that + * bailed at the pin leg never reached `proposals.value = next`, so the queue + * on screen really is still the old one and clearing that warning would + * 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. + */ + function recordQueueListReadSucceeded(): 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 + 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. + */ + function recordQueueRefreshSuccess(options?: { recoveryAlreadyRaised?: boolean }) { 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. if (queueRefreshStale.value) { queueRefreshRecovered.value = true - } else if (queueRefreshRecovered.value) { + } 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 // event; leaving its text standing indefinitely turns it into a claim @@ -851,8 +1039,14 @@ export function useReviewProposals() { () => { refreshTimedOut = true }, ) if (!isCurrentRead()) return + // The list leg answered and this read is still the current question, so + // 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 next = [...loadedProposals] let pinUnavailable = false + let pinMalformed = false if ( hashTargetId && !next.some((proposal) => proposalIdsEqual(proposal.id, hashTargetId)) @@ -915,11 +1109,15 @@ export function useReviewProposals() { // bind — into whole-queue revocation, and do not discard a queue // answer that already arrived. pinUnavailable = true + // Only the 400 says the ADDRESS is wrong. A 403 or a 404 is about a + // proposal that exists or existed, and the surfaces say different + // things about the two (#2214). + pinMalformed = isMalformedTargetError(e) } else { // 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 } @@ -929,13 +1127,13 @@ export function useReviewProposals() { if (!isCurrentRead()) return if (hashTargetId) { if (pinUnavailable) { - unavailableProposalId.value = hashTargetId + markProposalUnavailable(hashTargetId, pinMalformed ? 'malformed' : 'refused') } else if (proposalIdsEqual(unavailableProposalId.value, hashTargetId)) { - unavailableProposalId.value = null + clearProposalUnavailable() } } proposals.value = next - recordQueueRefreshSuccess() + recordQueueRefreshSuccess({ 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 @@ -955,15 +1153,13 @@ export function useReviewProposals() { // Board access was revoked. Stop polling rather than hammering an // endpoint that will keep refusing, drop rows the server no longer // authorises, and let the surface say so. - queueAccessRevoked.value = true - proposals.value = [] - suspendQueueRefreshForPermission() + recordQueueAccessRevoked() return } // 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 @@ -1177,8 +1373,10 @@ export function useReviewProposals() { proposals, proposalsLoading, unavailableProposalId, + unavailableProposalMalformed, 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..a815424d4 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: { @@ -635,10 +649,21 @@ export default { body: 'Someone else decided, withdrew, or deferred it while you were reviewing it. Nothing was decided here, and no other proposal was opened in its place. Reload the queue to check.', return: 'Reload the queue', }, + // Two different truths, deliberately not sharing a sentence (#2214). + // `title`/`body` describe a proposal that exists or existed and that this + // reviewer cannot see now (403), or that is gone (404) — a state a later + // read can legitimately reverse. `malformedTitle`/`malformedBody` describe + // an id the by-id route refuses to bind at all (400): the address never + // named a proposal, so there is nothing to wait for and retrying is + // pointless. Translators: keep that distinction. The malformed copy must + // not promise a recovery, and must point at the link rather than at the + // proposal. The eyebrow and the return control are shared by both. unavailable: { eyebrow: 'Requested proposal', title: 'This proposal is unavailable.', body: 'Proposal {id} is no longer available to review. It may have been applied, archived, or removed.', + malformedTitle: 'This link is not a valid proposal link.', + malformedBody: 'The address asks for {id}, which is not a proposal id, so there is nothing to open here. Retrying will not help. Go back to Review and pick a proposal from the queue.', return: 'Back to Review', }, }, diff --git a/frontend/taskdeck-web/src/locales/es/review.ts b/frontend/taskdeck-web/src/locales/es/review.ts index 3115e09fa..d4c9dae6d 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. @@ -530,6 +533,8 @@ export default { eyebrow: 'Propuesta solicitada', title: 'Esta propuesta no esta disponible.', body: 'La propuesta {id} ya no esta disponible para revisar. Puede haberse aplicado, archivado o eliminado.', + malformedTitle: 'Este enlace no es un enlace de propuesta valido.', + malformedBody: 'La direccion pide {id}, que no es un id de propuesta: no hay nada que abrir y reintentarlo no sirve. Vuelve a Revision y elige una propuesta de la cola.', return: 'Volver a Revision', }, }, diff --git a/frontend/taskdeck-web/src/locales/it/review.ts b/frontend/taskdeck-web/src/locales/it/review.ts index d61432e10..a13b51f88 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. @@ -532,6 +535,8 @@ export default { eyebrow: 'Proposta richiesta', title: 'Questa proposta non e disponibile.', body: 'La proposta {id} non e piu disponibile per la revisione. Potrebbe essere stata applicata, archiviata o rimossa.', + malformedTitle: 'Questo collegamento non e un collegamento valido a una proposta.', + malformedBody: 'La pagina richiede {id}, che non e un id di proposta: non esiste nulla da aprire e riprovare non serve. Torna alla revisione e scegli una proposta dalla coda.', return: 'Torna alla revisione', }, }, diff --git a/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts b/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts index 7ad9424a5..bb6302826 100644 --- a/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts +++ b/frontend/taskdeck-web/src/tests/composables/useReviewProposals.spec.ts @@ -941,6 +941,156 @@ describe('useReviewProposals', () => { await rp.loadProposals() expect(mockToast.error).toHaveBeenCalled() }) + + /** + * The explicit path used to answer one fact two ways depending on which + * leg observed it. A 400 or a 403 on the reviewer's own deep-link read + * raised a generic "Failed to load proposal" toast and left no state, so + * the surface showed the ordinary empty queue; the very next background + * tick turned the same 400 or 403 into the pin-unavailable panel. The + * toast said nothing true about which of the two happened, and it was gone + * a few seconds later while the panel it contradicted stayed. + * + * The explicit path now uses exactly the three predicates the background + * pin leg uses, and reports one outcome per status CLASS. 404 already + * behaved this way; 400 and 403 join it. Everything else — 405, 410, 5xx, + * no response — stays a toast, because those are not facts about the + * target and a later tick can still resolve the pin. + */ + describe('explicit-path outcome per status class (#2214)', () => { + async function openHash(hash: string, failure: unknown) { + mockRoute.hash = hash + mockAutomationApi.getProposals.mockResolvedValueOnce([]) + mockAutomationApi.getProposal.mockRejectedValueOnce(failure) + const rp = useReviewProposals() + await rp.loadProposals() + return rp + } + + it('turns a 400 into the malformed-link state without a toast', async () => { + const rp = await openHash('#proposal-not-a-guid', { response: { status: 400 } }) + expect(rp.unavailableProposalId.value).toBe('not-a-guid') + expect(rp.unavailableProposalMalformed.value).toBe(true) + expect(mockToast.error).not.toHaveBeenCalled() + }) + + it('turns a 403 into the unavailable-pin state without a toast', async () => { + const rp = await openHash('#proposal-p-forbidden', { response: { status: 403 } }) + // The by-id 403 is authority over ONE target. The queue-level 403 and + // its `queueAccessRevoked` teardown are a different leg and untouched. + expect(rp.unavailableProposalId.value).toBe('p-forbidden') + expect(rp.unavailableProposalMalformed.value).toBe(false) + expect(rp.queueAccessRevoked.value).toBe(false) + expect(mockToast.error).not.toHaveBeenCalled() + }) + + it('keeps the 404 outcome it already had', async () => { + const rp = await openHash('#proposal-p-gone', { response: { status: 404 } }) + expect(rp.unavailableProposalId.value).toBe('p-gone') + expect(rp.unavailableProposalMalformed.value).toBe(false) + expect(mockToast.error).not.toHaveBeenCalled() + }) + + it.each([ + ['405', { response: { status: 405 } }], + ['410', { response: { status: 410 } }], + ['500', { response: { status: 500 } }], + ['no response', new Error('network down')], + ])('keeps the toast and pins nothing for %s', async (_label, failure) => { + const rp = await openHash('#proposal-p-transient', failure) + // Nothing here is a settled fact about the target: 405 and 410 are the + // route misbehaving rather than the id being refused (#2658 draws the + // same line on the pin leg), and 5xx or no response may resolve next + // tick. Claiming the pin is unavailable would be a false negative. + expect(rp.unavailableProposalId.value).toBeNull() + expect(rp.unavailableProposalMalformed.value).toBe(false) + expect(mockToast.error).toHaveBeenCalled() + }) + + // Review finding, round 2 (LOW): asserting both flags after ONE explicit + // failure could not fail, since both runs need three. This drives the + // threshold count, and drives it through the ROUTE-HASH watcher, which + // reaches `openProposalFromHash` without a list read -- otherwise each + // `loadProposals` success would reset the runs and mask a leak anyway. + it.each([ + ['transient', 500], + ['refusal', 404], + ])('never feeds the background %s run from the explicit path', async (_class, status) => { + mockRoute.hash = '#proposal-p-flaky' + mockAutomationApi.getProposals.mockResolvedValueOnce([]) + mockAutomationApi.getProposal.mockRejectedValueOnce({ response: { status } }) + const rp = useReviewProposals() + await rp.loadProposals() + + const openHashAgain = watcherForCurrentSourceValue('#proposal-p-flaky')[1] + for (let attempt = 1; attempt < REVIEW_QUEUE_CONSECUTIVE_FAILURE_THRESHOLD; attempt += 1) { + mockAutomationApi.getProposal.mockRejectedValueOnce({ response: { status } }) + await openHashAgain() + } + + // A deep link the reviewer followed says nothing about whether the + // QUEUE poll is healthy, so neither run may advance on it. + expect(rp.queueRefreshStale.value).toBe(false) + expect(rp.queueRefreshRefused.value).toBe(false) + expect(rp.queueAccessRevoked.value).toBe(false) + }) + }) + }) + + describe('explicit list read refused with 403 (#2214, round 2)', () => { + /** + * Review finding, round 2. Only the POLL's outer catch handled a 403 on the + * list read. On a cold entry to a board whose access had been revoked, the + * explicit load 403'd into the generic failure toast, and then + * `openProposalFromHash` 403'd on the by-id read and -- since this slice + * made that a pin-level outcome -- rendered "no longer available to review; + * it may have been applied, archived, or removed" about a proposal that was + * neither applied nor archived nor removed. The board was simply not this + * reviewer's any more. It stood until the next tick set the authority state. + */ + it('sets the same authority state the poll would, and never marks the pin', async () => { + mockRoute.query = { boardId: 'board-revoked' } + mockRoute.hash = '#proposal-p-pinned' + mockAutomationApi.getProposals.mockRejectedValueOnce({ response: { status: 403 } }) + const rp = useReviewProposals() + await rp.loadProposals() + + expect(rp.queueAccessRevoked.value).toBe(true) + expect(rp.proposals.value).toEqual([]) + // The whole board is refused, so re-authorising one row inside it can + // only produce a second, wrong explanation of the same fact. + expect(rp.unavailableProposalId.value).toBeNull() + expect(rp.unavailableProposalMalformed.value).toBe(false) + expect(mockAutomationApi.getProposal).not.toHaveBeenCalled() + }) + + it('leaves an explicit non-403 list failure on the ordinary path', async () => { + mockRoute.hash = '#proposal-p-pinned' + mockAutomationApi.getProposals.mockRejectedValueOnce({ response: { status: 500 } }) + mockAutomationApi.getProposal.mockResolvedValueOnce(makeProposal({ id: 'p-pinned' })) + const rp = useReviewProposals() + await rp.loadProposals() + + // A 5xx is not an authority answer: it must not tear the queue down, and + // the hash lookup still runs. + expect(rp.queueAccessRevoked.value).toBe(false) + expect(mockAutomationApi.getProposal).toHaveBeenCalled() + expect(mockToast.error).toHaveBeenCalled() + }) + + it('keeps the pin-leg 403 as the single-proposal outcome #2593 shipped', async () => { + // A readable board with one proposal this reviewer may not open is the + // opposite case, and it must stay the unavailable pin rather than tearing + // down a queue the server just served. + mockRoute.hash = '#proposal-p-forbidden' + mockAutomationApi.getProposals.mockResolvedValueOnce([]) + mockAutomationApi.getProposal.mockRejectedValueOnce({ response: { status: 403 } }) + const rp = useReviewProposals() + await rp.loadProposals() + + expect(rp.queueAccessRevoked.value).toBe(false) + expect(rp.unavailableProposalId.value).toBe('p-forbidden') + }) }) describe('navigation helpers', () => { @@ -1325,10 +1475,13 @@ describe('useReviewProposals', () => { mockAutomationApi.getProposal.mockRejectedValue({ response: { status: 400 } }) const rp = useReviewProposals() await rp.loadProposals() - // The explicit deep-link path is deliberately unchanged by this: only a - // 404 marks the target there, so a 400 still surfaces as a failure the - // reviewer asked for. - expect(rp.unavailableProposalId.value).toBeNull() + // The explicit deep-link path reaches the SAME conclusion from the same + // 400, and does so first. It used to raise a generic toast and set no + // state, which was the asymmetry #2658 recorded and this slice removed: + // one fact, one outcome, whichever leg observed it. + expect(rp.unavailableProposalId.value).toBe('not-a-guid') + expect(rp.unavailableProposalMalformed.value).toBe(true) + expect(mockToast.error).not.toHaveBeenCalled() mockToast.error.mockClear() const queueBeforePoll = rp.proposals.value @@ -2281,4 +2434,309 @@ 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('retracts a risen refusal as soon as the LIST read succeeds, even when the pin leg fails', async () => { + // Review finding, round 2. `queueRefreshRefused` was cleared only by + // `recordQueueRefreshSuccess`, which needs the WHOLE composite read; a + // pin-leg failure returns before it. So once the API recovered but the + // by-id read for a hash-pinned row kept failing, the surface went on + // saying "the server is refusing the refresh rather than failing + // temporarily" every tick, which was no longer true, and because the + // refusal copy has precedence the honest degraded copy could never + // appear. It survived until an explicit load. + 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) + expect(rp.queueRefreshRecovered.value).toBe(false) + + // The list read answers again; only the pinned row's by-id read is down. + const pinLegDown = async () => { + mockAutomationApi.getProposals.mockResolvedValueOnce([]) + mockAutomationApi.getProposal.mockRejectedValueOnce({ response: { status: 500 } }) + await vi.advanceTimersByTimeAsync(REVIEW_QUEUE_REFRESH_MS) + } + await pinLegDown() + + // The claim is about the LIST read, and the list read demonstrably + // succeeded, so the claim is retracted on that evidence alone. + expect(rp.queueRefreshRefused.value).toBe(false) + // And retracting it silently is the #2630 defect, so it announces once. + expect(rp.queueRefreshRecovered.value).toBe(true) + // The composite read still bailed, so the queue was NOT replaced and the + // transient state is untouched by the retraction. + expect(rp.proposals.value.map((p: any) => p.id)).toEqual(['p-pinned']) + expect(rp.queueRefreshStale.value).toBe(false) + + // The transient counter keeps its #2445 composite semantics: this pin-leg + // 500 was its first, and two more take it to the threshold. + await pinLegDown() + expect(rp.queueRefreshStale.value).toBe(false) + await pinLegDown() + expect(rp.queueRefreshStale.value).toBe(true) + // The degraded onset retires the recovery sentence, as it always has. + expect(rp.queueRefreshRecovered.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() + }) + }) + + describe('malformed vs unavailable pin (#2214)', () => { + /** + * `unavailableProposalId` collapses two different truths. "This proposal is + * no longer available to review; it may have been applied, archived, or + * removed" is right for a 403 or a 404 and wrong for a 400: an id the by-id + * route cannot bind never named a proposal at all, and no amount of waiting + * or retrying will make the link work. The reason rides alongside the id so + * both skins can say which one happened. + */ + it('marks a pin the by-id route cannot bind as malformed', async () => { + vi.useFakeTimers() + mockRoute.hash = '#proposal-not-a-guid' + mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'not-a-guid' })]) + const rp = useReviewProposals() + await rp.loadProposals() + rp.startQueueRefresh() + expect(rp.unavailableProposalMalformed.value).toBe(false) + + mockAutomationApi.getProposals.mockResolvedValueOnce([]) + mockAutomationApi.getProposal.mockRejectedValueOnce({ response: { status: 400 } }) + await vi.advanceTimersByTimeAsync(REVIEW_QUEUE_REFRESH_MS) + + expect(rp.unavailableProposalId.value).toBe('not-a-guid') + expect(rp.unavailableProposalMalformed.value).toBe(true) + rp.stopQueueRefresh() + }) + + it.each([403, 404])('does not call a %s pin malformed', async (status) => { + vi.useFakeTimers() + mockRoute.hash = '#proposal-p-pinned' + mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'p-pinned' })]) + const rp = useReviewProposals() + await rp.loadProposals() + rp.startQueueRefresh() + + mockAutomationApi.getProposals.mockResolvedValueOnce([]) + mockAutomationApi.getProposal.mockRejectedValueOnce({ response: { status } }) + await vi.advanceTimersByTimeAsync(REVIEW_QUEUE_REFRESH_MS) + + // The link named a real proposal. Whether it is gone or forbidden, it is + // not a broken address, and telling the reviewer their link is malformed + // would send them to fix something that is correct. + expect(rp.unavailableProposalId.value).toBe('p-pinned') + expect(rp.unavailableProposalMalformed.value).toBe(false) + rp.stopQueueRefresh() + }) + + it('does not call a wrong-identity or cross-scope answer malformed', async () => { + vi.useFakeTimers() + mockRoute.hash = '#proposal-p-pinned' + mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'p-pinned' })]) + const rp = useReviewProposals() + await rp.loadProposals() + rp.startQueueRefresh() + + mockAutomationApi.getProposals.mockResolvedValueOnce([]) + mockAutomationApi.getProposal.mockResolvedValueOnce(makeProposal({ id: 'p-different' })) + await vi.advanceTimersByTimeAsync(REVIEW_QUEUE_REFRESH_MS) + + expect(rp.unavailableProposalId.value).toBe('p-pinned') + expect(rp.unavailableProposalMalformed.value).toBe(false) + rp.stopQueueRefresh() + }) + + it('retires the malformed reason with the pin it describes', async () => { + vi.useFakeTimers() + mockRoute.hash = '#proposal-not-a-guid' + mockAutomationApi.getProposals.mockResolvedValueOnce([makeProposal({ id: 'not-a-guid' })]) + const rp = useReviewProposals() + await rp.loadProposals() + rp.startQueueRefresh() + + mockAutomationApi.getProposals.mockResolvedValueOnce([]) + mockAutomationApi.getProposal.mockRejectedValueOnce({ response: { status: 400 } }) + await vi.advanceTimersByTimeAsync(REVIEW_QUEUE_REFRESH_MS) + expect(rp.unavailableProposalMalformed.value).toBe(true) + rp.stopQueueRefresh() + + // A reason that outlived its id would label the NEXT unavailable pin. + mockRoute.hash = '' + await watcherForCurrentSourceValue('')[1]() + expect(rp.unavailableProposalId.value).toBeNull() + expect(rp.unavailableProposalMalformed.value).toBe(false) + }) + }) }) diff --git a/frontend/taskdeck-web/src/tests/views/ReviewView.spec.ts b/frontend/taskdeck-web/src/tests/views/ReviewView.spec.ts index 69f060d46..383bab4e3 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') @@ -1644,6 +1753,95 @@ describe('ReviewView', () => { } }) + it('says a malformed pin is a broken link, not an unavailable proposal (#2214)', async () => { + // Two different truths shared one sentence. "It may have been applied, + // archived, or removed" describes a proposal that existed; a 400 says the + // id never named one, so there is nothing to wait for and nothing to + // retry. Sending a reviewer back to watch for a recovery that cannot + // arrive is the failure this copy exists to stop. + vi.useFakeTimers({ toFake: ['setInterval', 'clearInterval', 'Date'] }) + try { + mocks.getProposals.mockResolvedValue([buildProposal({ id: 'proposal-not-a-guid' })]) + const { wrapper } = await mountAt('/workspace/review#proposal-proposal-not-a-guid') + + mocks.getProposals.mockResolvedValue([]) + mocks.getProposal.mockRejectedValue({ response: { status: 400 } }) + vi.advanceTimersByTime(REVIEW_QUEUE_REFRESH_MS) + await flushPromises() + await wrapper.vm.$nextTick() + + const unavailable = wrapper.get('[data-testid="review-unavailable-target"]') + expect(unavailable.text()).toContain(enReview.empty.unavailable.malformedTitle) + expect(unavailable.text()).not.toContain(enReview.empty.unavailable.title) + expect(unavailable.text()).toContain('proposal-not-a-guid') + // The only offered action stays the way back to the unpinned queue. + expect(wrapper.find('[data-testid="review-unavailable-return"]').exists()).toBe(true) + } finally { + vi.useRealTimers() + } + }) + + it('shows the revoked-access panel, not the unavailable pin, on a cold entry to a revoked board (#2214)', async () => { + // Round-2 review finding. Only the poll used to recognise a 403 on the list + // read, so this entry produced a generic toast, no authority state, and + // then a by-id 403 that -- now that a by-id 403 is a pin-level outcome -- + // rendered "may have been applied, archived, or removed" about a proposal + // that was none of those. The board was simply not this reviewer's any + // more, and that stood for a whole poll interval. + mocks.getProposals.mockRejectedValue({ response: { status: 403 } }) + mocks.getProposal.mockRejectedValue({ response: { status: 403 } }) + + const { wrapper } = await mountAt('/workspace/review?boardId=board-revoked#proposal-p-pinned') + + expect(wrapper.find('[data-testid="review-access-revoked"]').exists()).toBe(true) + expect(wrapper.find('[data-testid="review-unavailable-target"]').exists()).toBe(false) + expect(wrapper.text()).not.toContain(enReview.empty.unavailable.title) + // The whole board is refused; there is nothing to re-authorise inside it. + expect(mocks.getProposal).not.toHaveBeenCalled() + }) + + it('gives the explicit deep-link path one outcome per status class (#2214)', async () => { + // The reviewer followed a link and the by-id read was refused. A 403 is a + // settled fact about that target and now reads as the panel the background + // tick would have produced a moment later, instead of a generic toast that + // said neither what happened nor that the panel was about to contradict it. + mocks.getProposals.mockResolvedValue([]) + mocks.getProposal.mockRejectedValue({ response: { status: 403 } }) + + const { wrapper } = await mountAt('/workspace/review#proposal-proposal-forbidden') + + const unavailable = wrapper.get('[data-testid="review-unavailable-target"]') + expect(unavailable.text()).toContain(enReview.empty.unavailable.title) + expect(unavailable.text()).toContain('proposal-forbidden') + expect(mocks.errorToast).not.toHaveBeenCalled() + // The queue-level 403 owns the access-revoked panel; a by-id refusal is not + // whole-queue revocation. + expect(wrapper.find('[data-testid="review-access-revoked"]').exists()).toBe(false) + }) + + it('keeps the explicit deep-link toast for a transient class (#2214)', async () => { + mocks.getProposals.mockResolvedValue([]) + mocks.getProposal.mockRejectedValue({ response: { status: 500 } }) + + const { wrapper } = await mountAt('/workspace/review#proposal-proposal-flaky') + + // A 5xx is not a fact about the target, so pinning it unavailable would be + // a false negative; a later tick can still resolve it. + expect(mocks.errorToast).toHaveBeenCalled() + expect(wrapper.find('[data-testid="review-unavailable-target"]').exists()).toBe(false) + }) + + it('keeps the ordinary unavailable copy for a pin that is gone or forbidden (#2214)', async () => { + mocks.getProposals.mockResolvedValue([]) + mocks.getProposal.mockRejectedValue({ response: { status: 404 } }) + + const { wrapper } = await mountAt('/workspace/review#proposal-proposal-gone') + + const unavailable = wrapper.get('[data-testid="review-unavailable-target"]') + expect(unavailable.text()).toContain(enReview.empty.unavailable.title) + expect(unavailable.text()).not.toContain(enReview.empty.unavailable.malformedTitle) + }) + it('announces nothing from the queue live region while the queue is loading (#2214)', async () => { // The live region sits above the skeleton, so an ungated one reads "0 // proposals awaiting review." under the loading state and then the real 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..0ccfc6cfe 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 @@ -203,9 +203,17 @@ async function mountView( columns: unknown[] = [], // `document.activeElement` only tracks elements that are in the document, so // the focus specs need a real attachment; everything else mounts detached. - options: { attachTo?: boolean } = {}, + options: { attachTo?: boolean; listReadRejectsWith?: unknown } = {}, ) { - mocks.getProposals.mockResolvedValueOnce(proposals) + // A test that needs the FIRST list read to fail must route it through here. + // Queueing a rejection at the call site instead would leave the + // `mockResolvedValueOnce` below unconsumed, and that leftover entry shifts + // the once-queue for every later test in this file. + if ('listReadRejectsWith' in options) { + mocks.getProposals.mockRejectedValueOnce(options.listReadRejectsWith) + } else { + mocks.getProposals.mockResolvedValueOnce(proposals) + } // The Review surface re-reads its queue on a bounded poll while it is open // (#2194), so a `...Once` fixture alone would leave any timer-advancing test // facing a drained mock and an empty queue. A real server keeps answering @@ -1274,6 +1282,108 @@ describe('PaperReviewView', () => { expect(wrapper.find('[data-testid="paper-review-main"]').text()).toContain('First proposal') }) + it('says a malformed pin is a broken link, not an unavailable proposal (#2214)', async () => { + // A 400 from the by-id route is model binding refusing the id, so the link + // never named a proposal: "it may have been applied, archived, or removed" + // describes a proposal that existed, and pointing a reviewer at a recovery + // that cannot arrive is worse than saying nothing. + vi.useFakeTimers({ toFake: ['setInterval', 'clearInterval', 'Date'] }) + try { + const wrapper = await mountView( + [makeProposal({ id: 'proposal-not-a-guid' })], + '/workspace/review#proposal-proposal-not-a-guid', + ) + + mocks.getProposals.mockResolvedValue([]) + mocks.getProposal.mockRejectedValue({ response: { status: 400 } }) + vi.advanceTimersByTime(REVIEW_QUEUE_REFRESH_MS) + await flushPromises() + await wrapper.vm.$nextTick() + + const empty = wrapper.get('[data-testid="paper-review-empty"]') + expect(empty.text()).toContain(enReview.empty.unavailable.malformedTitle) + expect(empty.text()).not.toContain(enReview.empty.unavailable.title) + expect(empty.text()).toContain('proposal-not-a-guid') + expect(wrapper.find('[data-testid="paper-review-unavailable-return"]').exists()).toBe(true) + wrapper.unmount() + } finally { + vi.useRealTimers() + } + }) + + it('shows the revoked-access panel, not the unavailable pin, on a cold entry to a revoked board (#2214)', async () => { + const wrapper = await mountView( + [makeProposal({ id: 'proposal-first' })], + '/workspace/review?boardId=board-revoked#proposal-p-pinned', + [], + [], + { listReadRejectsWith: { response: { status: 403 } } }, + ) + + expect(wrapper.find('[data-testid="paper-review-access-revoked"]').exists()).toBe(true) + const empty = wrapper.get('[data-testid="paper-review-empty"]') + expect(empty.text()).not.toContain(enReview.empty.unavailable.title) + expect(empty.text()).not.toContain(enReview.empty.unavailable.malformedTitle) + expect(mocks.getProposal).not.toHaveBeenCalled() + wrapper.unmount() + }) + + it('gives the explicit deep-link path one outcome per status class (#2214)', async () => { + mocks.getProposal.mockRejectedValueOnce({ response: { status: 403 } }) + const wrapper = await mountView( + [makeProposal({ id: 'proposal-first' })], + '/workspace/review#proposal-PROPOSAL-FORBIDDEN', + ) + + const empty = wrapper.get('[data-testid="paper-review-empty"]') + expect(empty.text()).toContain(enReview.empty.unavailable.title) + expect(empty.text()).toContain('PROPOSAL-FORBIDDEN') + expect(mocks.errorToast).not.toHaveBeenCalled() + expect(wrapper.find('[data-testid="paper-review-access-revoked"]').exists()).toBe(false) + wrapper.unmount() + }) + + it('says a malformed link is malformed on the explicit path too (#2214)', async () => { + mocks.getProposal.mockRejectedValueOnce({ response: { status: 400 } }) + const wrapper = await mountView( + [makeProposal({ id: 'proposal-first' })], + '/workspace/review#proposal-not-a-guid', + ) + + const empty = wrapper.get('[data-testid="paper-review-empty"]') + expect(empty.text()).toContain(enReview.empty.unavailable.malformedTitle) + expect(empty.text()).not.toContain(enReview.empty.unavailable.title) + expect(mocks.errorToast).not.toHaveBeenCalled() + wrapper.unmount() + }) + + it('keeps the explicit deep-link toast for a transient class (#2214)', async () => { + mocks.getProposal.mockRejectedValueOnce({ response: { status: 500 } }) + const wrapper = await mountView( + [makeProposal({ id: 'proposal-first' })], + '/workspace/review#proposal-PROPOSAL-FLAKY', + ) + + expect(mocks.errorToast).toHaveBeenCalled() + expect(wrapper.get('[data-testid="paper-review-empty"]').text()).not.toContain( + enReview.empty.unavailable.title, + ) + wrapper.unmount() + }) + + it('keeps the ordinary unavailable copy for a pin that is gone (#2214)', async () => { + mocks.getProposal.mockRejectedValueOnce({ response: { status: 404 } }) + const wrapper = await mountView( + [makeProposal({ id: 'proposal-first' })], + '/workspace/review#proposal-PROPOSAL-MISSING', + ) + + const empty = wrapper.get('[data-testid="paper-review-empty"]') + expect(empty.text()).toContain(enReview.empty.unavailable.title) + expect(empty.text()).not.toContain(enReview.empty.unavailable.malformedTitle) + wrapper.unmount() + }) + it('updates the hash when manual queue selection replaces a deep-link target', async () => { mocks.approveProposal.mockResolvedValueOnce(makeProposal({ id: 'proposal-first' })) const wrapper = await mountView( @@ -3496,12 +3606,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 +3647,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 +3727,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..eb33eb1d5 100644 --- a/frontend/taskdeck-web/src/views/LegacyReviewView.vue +++ b/frontend/taskdeck-web/src/views/LegacyReviewView.vue @@ -29,8 +29,10 @@ const { summaryCards, queueAccessRevoked, queueRefreshStale, + queueRefreshRefused, queueRefreshRecovered, unavailableProposalId, + unavailableProposalMalformed, dismissableProposalIds, isProposalExpired, clearProposalDeepLink, @@ -212,8 +214,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 +341,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') }}

{ data-testid="review-unavailable-target" >

{{ $t('review.empty.unavailable.eyebrow') }}

-

{{ $t('review.empty.unavailable.title') }}

-

{{ $t('review.empty.unavailable.body', { id: unavailableProposalId }) }}

+ +

{{ unavailableProposalMalformed ? $t('review.empty.unavailable.malformedTitle') : $t('review.empty.unavailable.title') }}

+

{{ unavailableProposalMalformed ? $t('review.empty.unavailable.malformedBody', { id: unavailableProposalId }) : $t('review.empty.unavailable.body', { id: unavailableProposalId }) }}