fix(review): disclose refused poll refreshes, name a malformed link, and make the explicit-path outcome consistent - #2694
Conversation
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
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
…able `unavailableProposalId` collapsed two different truths into one sentence. "It may have been applied, archived, or removed" describes a proposal that exists or existed and is right for the 403 and 404 cases, which a later read can legitimately reverse. It is wrong for the 400 that #2658 added: a model-binding refusal means the id never named a proposal, so there is nothing to wait for and retrying cannot help. The old copy sent the reviewer to watch for a recovery that cannot arrive. `unavailableProposalMalformed` carries the reason alongside the id, and both skins render `review.empty.unavailable.malformedTitle` / `.malformedBody` in place of `title` / `body` when it is set. The eyebrow and the return-to-queue control are unchanged, and so is every other outcome: a 403, a 404, a wrong-identity answer and a cross-scope answer all stay the ordinary unavailable state. The predicate is the 400 OUTCOME, the same `isMalformedTargetError` #2658 uses on the pin leg, not a client-side GUID test on the hash value. The `Guid` model binder accepts the N, D, B, P and X formats, so a regex tight enough to be worth writing would be narrower than the binder and would call a perfectly bindable id malformed. Deriving it from the server's answer also means the pin outcome and the panel copy cannot disagree, since they read one predicate. The cost is that a malformed link still spends one request finding out. Reason and id are written and cleared together through `markProposalUnavailable` / `clearProposalUnavailable` rather than at each of the seven write sites: a reason that outlived its id would label the next unavailable pin. Refs #2214
…class `openProposalFromHash` answered the same fact two different ways depending on which leg observed it. Before this commit the explicit path was: 400 generic "Failed to load proposal" toast, no state 403 generic "Failed to load proposal" toast, no state 404 unavailable pin, no toast 405/410/5xx/no response generic toast, no state while the background pin leg turned 400, 403 and 404 alike into the unavailable pin. So a reviewer who followed a bad link got a toast that named neither what happened nor which of the two it was, the surface fell back to the ordinary empty queue, and one tick later the pin panel appeared and contradicted the toast, which by then had gone. After, using exactly the three predicates the pin leg uses: 400 malformed-link panel, no toast 403 unavailable-pin panel, no toast 404 unavailable-pin panel, no toast (unchanged) 405/410/5xx/no response generic toast, no state (unchanged) A settled fact about the target gets the panel and nothing else, because the panel is the durable report and two reports for one fact is the asymmetry being removed. The split at 405/410 is #2658's line: those are the route misbehaving rather than the id being refused, and 5xx or no response may resolve on a later tick, so pinning the target unavailable would be a false negative. The 403 authority path is untouched beyond this. `queueAccessRevoked`, the queue teardown and the poll suspension all live on the LIST leg; a by-id 403 has always been authority over one target only, and the background leg has rendered it as the unavailable pin since #2357. The transient and refusal counters are untouched on every explicit outcome: a deep link the reviewer followed says nothing about whether the queue poll is healthy. A #2658 assertion that pinned the old asymmetry ("only a 404 marks the target there") is updated to record the new outcome; the background half of that test is unchanged. Refs #2214
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Review record (alpha product-trust lane, review-and-ship round 1 at head Reviewer: one fresh-context independent reviewer subagent (read-only; Codex credits exhausted, SC-9), given the worktree at the head and the merge-base diff. Verdict: SHIP, no CRITICAL or HIGH; two MEDIUMs are truth defects in the new disclosure copy and are fixed in round 2. Lenses that found nothing: the refusal predicate is sound (a list-leg 403 provably cannot reach it because the outer catch returns first as authority revocation, 401 is the interceptor's, 408/429/5xx/no-response are consumed by the transient predicate first, non-object errors short-circuit); a pin-leg 400/403/404 never reaches the accounting and a pin-leg 5xx can only reset the run; the issue's headline case is real end to end ( Findings, triaged once:
Round count: 2 after the fix push. Merge gate: |
…succeeds Answers round-2 review finding 1 (MEDIUM, truth defect). `queueRefreshRefused` was cleared only by `recordQueueRefreshSuccess`, which needs the whole composite read, and a pin-leg failure returns at `recordQueueRefreshFailure(e, 'pin')` before reaching it. So: three list 404s raise the refusal, the API recovers and the list read succeeds every tick, but the by-id read for the hash-pinned row keeps 500ing. The surface went on saying "the server is refusing the refresh rather than failing temporarily" on every tick, which was no longer true, and because the refusal copy takes precedence in the shared slot the honest degraded copy could never appear. It stood until an explicit load. `recordQueueListReadSucceeded` now runs the moment the list answer is known good and the read is still the current question, before the pin leg can strand the claim. It clears the refusal and raises the #2630 recovery signal once, because retracting a disclosure silently is the defect #2630 exists to stop. The refusal's clear is list-scoped while the transient state's stays composite-scoped, and the comment says why: the refusal is a claim about the list REQUEST, which one successful list read falsifies outright, whereas the transient state is a claim about the RENDERED QUEUE, and 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. Clearing that one would fabricate a freshness the surface does not have. #2445's composite semantics for the transient counter are therefore untouched. `recordQueueRefreshSuccess` takes `recoveryAlreadyRaised` so the poll's later composite success cannot retire a sentence its own read raised a few lines earlier. Explicit loads call it unchanged. Refs #2214
…it is Answers round-2 review finding 2 (MEDIUM, lifecycle falsehood). Only the POLL's outer catch handled a 403 on the list read. On a cold entry to /workspace/review?boardId=B#proposal-X after access to B was revoked, the explicit load 403'd into the generic failure toast and set no authority state. `openProposalFromHash` then 403'd on the by-id read and, because the previous commit in this branch made a by-id 403 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 simply was not this reviewer's any more. That stood until the next poll tick set queueAccessRevoked. The poll's three-statement teardown is extracted as `recordQueueAccessRevoked` and called from both legs, rather than duplicated: a 403 means the same thing whoever asked, and two copies are how the legs drift into telling a reviewer two different stories about one revocation. `startQueueRefresh` already latches `queueRefreshSuspendedForPermission` when `queueAccessRevoked` is set, so the helper is correct whichever order mount runs it in. The explicit load also stops asking for the pin when the queue itself was just refused. Re-authorising one row inside a board the server refused wholesale can only produce a second, narrower and wrong account of the same fact. A successful load clears `queueAccessRevoked` before that point, so the guard can only be true when this read revoked it or an earlier one did. The pin-leg 403 is unchanged: an inaccessible single proposal on an accessible board stays the unavailable pin exactly as #2593 shipped it, and a spec pins that. The toast and the 'failed' outcome are unchanged too, so no action composable that calls `loadProposals` loses its failure signal. `mountView` in the Paper spec gains `listReadRejectsWith`, because a test that needs the first list read to fail must route it through the helper that owns that fixture. Queueing the rejection at the call site left mountView's own `mockResolvedValueOnce` unconsumed, and that leftover entry shifted the once-queue for 35 later tests in the file. Refs #2214
|
Round-2 record (alpha product-trust lane; fix head Fixed:
Verified at the fix head: Not verified: Residual recorded on #2214 rather than fixed: on the list-success/pin-fail tick the retraction raises Merge gate remaining: |
|
Round-2 verification record (scoped to the fix diff Verdict: SHIP for the fix diff, no CRITICAL, HIGH or MEDIUM. What it checked: Findings, triaged once at the two-round ceiling:
Round count: 2. Merge gate remaining: |
|
CI at |
…ss retraction, #2696 headline, residual list, D-13 clause) Answers the docs review of PR #2699: the #2694 bullet no longer claims the explicit list read's 403 is toast-free (the toast stays beside the revoked panel, the recorded LOW) nor that a success leaves a risen refusal standing (a successful list read retracts it); the #2696 headline says the listed literals render from the catalogs rather than that the components render no hardcoded English; the not-shipped line names the hash-watcher residual; item 2 is described as the list leg plus the explicit path; the #2697 bullet says its round 2 touched specs and comments only; OUTSTANDING_TASKS.md D-13 no longer says item 3 waits on a merged PR.
1 participant
Summary
Three residuals recorded on #2214 after PR #2658, all frontend, all needing both review skins.
Repeated non-transient failures of the review queue's LIST read were only logged. The transient counter shipped by #2445 deliberately ignores a non-transient answer: it resets its run and returns. So a
?boardId=not-a-guidin the address bar, which reachesGetProposals([FromQuery] Guid? boardId)under[ApiController]becausenormalizeBoardIdQueryParamonly trims, produced a model-binding 400 on every tick with no disclosure at all. The poll kept running, the counter kept resetting, no degraded state ever rose, and the surface went on showing rows the server had not confirmed since the reviewer arrived. This PR adds a second, separate threshold for that case.The unavailable-pin panel said "no longer available to review; it may have been applied, archived, or removed" for a malformed id too. That describes a proposal that existed. A 400 from the by-id route means the id never named one, so the panel was pointing the reviewer at a recovery that cannot arrive.
openProposalFromHashanswered the same refusal two different ways depending on which leg observed it. A 400 or a 403 on the reviewer's own deep-link read raised a generic toast and set no state, while the background pin leg turned the identical status into the pin-unavailable panel one tick later.Refs #2214 (item 2 on both legs, the malformed-link copy, the explicit-path toast asymmetry). The issue stays open for its remaining residuals.
Refs PRs #2445, #2593, #2630, #2658.
Changes
The list-leg disclosure state (
useReviewProposals.ts, the three review catalogs).queueRefreshRefusedis 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; the next successful list read clears it and raises the #2630 recovery signal. It keeps the last trustworthy queue and keeps polling. It is deliberately notqueueRefreshStale, which belongs to the transient counter: "the network keeps blipping, we are retrying" and "the server is answering and refusing" are different facts with different remedies.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, raisesqueueAccessRevokedand suspends the poll; counting it here as well would put two disclosures on screen for one fact, one of which is wrong for revoked access. 401 belongs toapi/http.ts, 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.The skins' regions and copy (
LegacyReviewView.vue,PaperReviewView.vue). Each skin gains one always-mountedsr-onlylive region that withholds its text until the disclosure rises, the #2630 pattern: the visible warning is inside av-if, so it mounts already carrying its text, which a live region announces unreliably. Paper's is hoisted above theactiveProposal/v-elsepair 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. 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 means the #2630 sticky/offset seam is untouched:
queueStaleRefstays on the same node, its ResizeObserver keeps measuring the same warning, andmainColStickyStylejust widens its guard to match the widenedv-if.Copy in
review.queue.refused.bodyfor en, it and es, saying the three things that are true: the queue shown is the last one the server confirmed, the refresh is being refused rather than failing temporarily, and the reviewer can reload or check the board filter.The malformed-link panel (
useReviewProposals.ts, both views, the three catalogs).unavailableProposalMalformedcarries the reason alongside the id, and both skins renderreview.empty.unavailable.malformedTitle/.malformedBodyin place oftitle/bodywhen it is set. The eyebrow and the return-to-queue control are unchanged, and a 403, a 404, a wrong-identity answer and a cross-scope answer all stay the ordinary unavailable state.The predicate is the 400 outcome, the same
isMalformedTargetError#2658 uses on the pin leg, not a client-side GUID test on the hash value. TheGuidmodel binder accepts the N, D, B, P and X formats, so a regex tight enough to be worth writing would be narrower than the binder and would call a bindable id malformed. Deriving it from the server's answer also means the pin outcome and the panel copy read one predicate and cannot disagree. The cost is that a malformed link still spends one request finding out. Reason and id are written and cleared together throughmarkProposalUnavailable/clearProposalUnavailablerather than at each of the seven write sites, so a reason cannot outlive its id and label the next unavailable pin.The explicit-path matrix (
useReviewProposals.ts). Before:After:
A settled fact about the target gets the panel and nothing else, because the panel is the durable report and two reports for one fact is the asymmetry being removed. The split at 405 and 410 is #2658's line: those are the route misbehaving rather than the id being refused, and 5xx or no response may resolve on a later tick, so pinning the target unavailable would be a false negative. The transient and refusal counters are untouched on every explicit outcome.
The 403 authority path is untouched beyond this.
queueAccessRevoked, the queue teardown and the poll suspension all live on the LIST leg; a by-id 403 has always been authority over one target only, and the background leg has rendered it as the unavailable pin since #2357.Test plan
Verified, in the worktree at the exact head:
npx vitest --run --maxWorkers=2 src/tests/composables/useReviewProposals.spec.ts src/tests/views/paper/review/PaperReviewView.spec.ts src/tests/views/ReviewView.spec.ts src/tests/i18n/catalogs.spec.ts— Test Files 4, Tests 423 passed.npm run typecheck— clean.npm run build— clean.npx eslinton all nine changed files — exit 0.git diff --check origin/main..HEAD— exit 0.Red first, against the unmodified sources:
queueRefreshRefusedbeing undefined. They cover the rise only at three, no rise on two plus a success, an intervening transient failure resetting the run without raising it, an intervening transient failure not clearing a risen disclosure, the clear plus recovery on the next success, 404/405/410 counting as refusals, 403 leaving the authority path toqueueAccessRevoked, 401 excluded, and a pin-leg failure not counting.unavailableProposalMalformedbeing undefined and onmalformedTitlebeing undefined in the rendered panels.openProposalFromHashblock and re-running: 3 failed in one run and 1 in a second, then green again once restored.One inherited assertion changed rather than being added.
useReviewProposals.spec.tscarried a #2658 line pinning the old asymmetry ("only a 404 marks the target there, so a 400 still surfaces as a failure the reviewer asked for"). That is the behaviour this PR removes, so it now asserts the malformed pin and the absent toast. The background half of that test is untouched.NOT verified: Playwright; a real screen-reader pass; the real backend's 400 for a non-GUID
boardIdquery, which is reasoned from the controller attributes and never observed; the Italian and Spanish wording is this lane's, not native-reviewed; the sticky/offset behaviour in a real browser, unchanged here but only asserted at the class and property level as it was in #2630.Boundaries and risks
Changed:
composables/useReviewProposals.ts,views/LegacyReviewView.vue,views/paper/PaperReviewView.vue,locales/{en,it,es}/review.ts, and the three specs. Not changed:api/automationApi.ts(expectedStatusesalready exists and needed no new option),ReviewQueueRail.vue,usePaperReviewSelectors.ts, the Inbox files, and any doc. InPaperReviewView.vuethe receipt recorders from #2692 and thestoredBannerresidual from #1434 are untouched; the diff there is the destructure, the sticky-style guard, one new region, and the copy switches in the three existing warning and panel slots.Risks and accepted behaviour:
sr-onlyregion. The alternative was strippingaria-livefrom the visible warning, which fix(review): localize the degraded-queue warning, announce recovery, keep it visible #2630's specs pin.recordQueueRefreshSuccessnow raisesqueueRefreshRecoveredwhen a refused state clears, not only a degraded one. That widens when the fix(review): localize the degraded-queue warning, announce recovery, keep it visible #2630 recovery sentence fires, deliberately: the warning simply vanishing is silent either way.sr-onlyregion added to Paper is a second root child. It is absolutely positioned and takes no grid column, and the two root-children assertions in the Paper spec are extended to name it while keeping the three columns' order and parent.!queueAccessRevoked, so the access-revoked panel remains the single disclosure for revoked access.