fix(review): localize the degraded-queue warning, announce recovery, keep it visible - #2630
Merged
Conversation
The warning was hardcoded English in all three render sites (Paper's active-proposal column, Paper's empty column, Legacy), so an it/es reviewer saw English at exactly the moment the surface was admitting it might be wrong. Move the sentence to review.queue.degraded.body with faithful Italian and Spanish, and add the matching review.queue.degraded.recovered key the recovery announcement will render. Legacy's warning becomes one paragraph carrying the shared sentence instead of two hardcoded ones, so both skins render the same string and cannot drift (ADR-0038 / #1124). The Legacy comment claiming this skin's copy is entirely hardcoded English was already wrong for the unavailable panel below it and would have become wrong here; it now names the states that share the catalog. The two view specs assert the catalog value rather than a copy fragment, so an edit to the sentence cannot pass silently. Refs #2214
…kins Clearing the degraded state only unmounted the warning. A reviewer who was not watching that corner of the surface, and any screen-reader user, was never told the queue is trustworthy again, so the disclosure only ever ran one way. useReviewProposals now exposes queueRefreshRecovered: it turns true only when a VISIBLE degraded state is ended by a successful read, and falls back to false at the next degraded onset so a second recovery is announced too (a live region only speaks when its text changes). Both exits from the degraded state raise it, because the explicit-load clear now runs the same recordQueueRefreshSuccess accounting as a successful poll. An ordinary success after a success changes nothing, and a non-transient failure that resets the run without ever reaching the threshold recovers nothing. Both skins render a mounted sr-only region that withholds its text rather than appearing with it (#2593), gated on !queueAccessRevoked exactly like the warning, so the composable's 403 path keeps its single owner. The rail's count region is untouched. The recovered sentence stays until the next degraded onset or until the surface unmounts. No timer: a timed clear would add teardown state to a composable whose polling seams are already fake-timer sensitive, and the sentence remains true for as long as it is shown. Refs #2214
…ng column The main column is the scroller and a reviewer acts from a scrolled position, so the warning that the queue may be out of date had scrolled away by the time the decision was made. A dedicated pinned modifier keeps it on screen for as long as it is true; the two sibling notes that share the base class stay in flow, because they describe the record on screen rather than the whole queue. The pinned band sits below ReviewDecisionRail's own sticky z-index on purpose. Winning that band would put an opaque paragraph over the Approve and Reject controls and swallow clicks aimed at them. The spec pins the modifier on the element and the declared rule in the stylesheet, and says plainly that neither happy-dom nor vitest can prove the element actually stays on screen: vitest does not process SFC styles and happy-dom lays nothing out. Refs #2214
…disclosure The two existing degraded cases cover an active proposal and an unfiltered empty queue. Neither reaches the branch a reviewer is most likely to be on when polls start failing: a filter that currently matches nothing, where the surface is already saying no matches and could be read as an authoritative answer about a queue it can no longer refresh. The new case combines a foreign-authored row, the Mine filter and three consecutive 500s, and asserts the warning and the filter answer co-render while the rail keeps speaking the real awaiting count. The language case flips to Italian with the queue degraded and then recovers it, so both new catalog keys are proven to reach the DOM. The parity guard only proves they exist. Refs #2214
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Owner
Author
|
Review record (alpha product-trust lane, review-and-ship round 1 at head 950a054). Reviewer: one fresh-context independent reviewer subagent (read-only), input = merge-base..head diff plus the worktree at the head. Verdict: SHIP, no CRITICAL or HIGH; two MEDIUMs that affect whether #2214 items 1 and 4 are fully delivered, taken in round 2. Confirmed clean: the recovery flag is set only when a visible degraded state ends (never on success after success; a non-transient reset does not touch it; both refs are written in one synchronous block so the warning and the recovered sentence can never co-render); the signal call sits after the request-id, abort and isCurrentRead guards and replaces exactly the two statements it supersedes, so the 403, supersession, deadline and pin re-authorization paths are untouched; every exit from the stale state goes through the signal; the Legacy region is unconditionally rendered at template root; no count region changed; the sticky modifier is on exactly one element with an opaque background in both themes and the node stays a direct child of the scroller; catalog parity holds in en, it and es with faithful copy; the repointed assertions compare rendered text to the catalog value, not to a key. Triage:
Merge gate: ci-required green at the round-2 head, aged three minutes, one verification pass scoped to the fix diff (the fixes touch template structure, a rail CSS rule and one composable branch), then merge commit. |
…entence Review round 2 on #2630. MEDIUM 1. The recovery region was duplicated in the active-proposal and empty arms, so a branch flip remounted it. The recovering poll assigns the queue and records the success in one synchronous block, so a recovery that puts a proposal back into an empty or filtered-empty queue flipped the branch in the same render the sentence appeared, and the region mounted already carrying its text: the exact insert-with-text case the construction exists to avoid. One node now sits above the branch pair and survives every flip. It is absolutely positioned by sr-only, so it is not a grid item and takes no column of the three-column track list. The two root-children pins name it. MEDIUM 2. ReviewDecisionRail is sticky at top 0 in the same scroller, opaque via card-lift and taller, so past the card header it covered the pinned warning completely and the pin bought nothing in the state it exists for. The view now measures the warning with a ResizeObserver and writes --paper-review-sticky-offset onto the scrolling column; the rail's one sticky rule reads it with a 0 fallback, so the rail sits below the warning and both stay visible. Without ResizeObserver the offset stays 0, which is the previous behaviour rather than a broken one. The warning stays at z-index 1 so an opaque paragraph can never cover the decision controls or swallow their clicks. LOW 3. The recovered sentence lived for the whole session. The following healthy success now retires it, so it lives for about one poll interval, with no timer to tear down. A live region going empty announces nothing. LOW 5. The two stylesheet source reads use Vite's ?raw directly, the form ReviewDecisionRail.spec.ts already uses under this tsconfig, and the comment no longer claims a constraint that does not exist. Refs #2214
Owner
Author
|
Round-2 verification record (scoped to the fix diff 950a054..18cc35b; read-only pass). Verdict: SHIP for the fix diff, no CRITICAL or HIGH. Confirmed: the hoisted recovery region is the first element child of the unconditional root, out of flow (sr-only is absolute) so the three-column track list and the narrow two-column variant are unaffected; no spec or selector indexes the root's first child except the two updated pins, which match template order; the custom property is written through the column's single :style binding and inherits down to the rail; the observer is created only for a live element, disconnected on every ref change and on unmount, and guarded where ResizeObserver is absent; the empty branch renders no rail and carries no property; the retire cannot fire in the same call that sets the flag, the explicit-load recovery path still fires when stale was true, and no path sets the flag outside the one site; the source-read pins are not tautological; the ?raw imports are type-checked under the vitest tsconfig with the existing precedent. Two MEDIUMs and one LOW, tracked as #2638 rather than a third round (the two-round ceiling is reached and neither is a merge blocker):
Merge gate unchanged: ci-required green at 18cc35b, aged, merge commit. The #2214 residual items 1, 4, 5 and 6 are recorded in STATUS as delivered with #2638's two caveats stated. |
This was referenced Sep 5, 2026
Chris0Jeky
added a commit
that referenced
this pull request
Sep 5, 2026
…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
This was referenced Sep 5, 2026
Merged
Chris0Jeky
added a commit
that referenced
this pull request
Sep 5, 2026
Round-2 review finding, MEDIUM: raiseQueueRecovery stamped every raise with the ordinal of the last STARTED background read. That is the running read for a poll raise, but for the #2630 explicit-clear path (loadProposalsWithOutcome -> recordQueueRefreshSuccess) it names a read that already finished, so the very next tick satisfied count > stamp and retired the sentence. A post-decision reload that ends the degraded state 14.9 s into a 15 s cycle was blanked 100 ms later: the defect this PR exists to close, with the roles swapped. An explicit raise is now stamped with the read that has not started yet, so it survives the next poll success and retires on the one after. Either raise therefore gets at least one full interval, and the comment states both halves instead of claiming one interval for all of them. The onsets still retire immediately for both, because there the sentence is false rather than old. 'source' is threaded through recordQueueListReadSucceeded so a refusal retraction is stamped by the same rule. Red first against the round-1 head: 'gives an EXPLICIT-load recovery a full interval before a poll can retire it' failed 'expected false to be true' at the first poll tick, 1 failed / 163. The companion onset pin was green before and after, which is the point of it. Also two comment-only round-2 LOWs: the queue-sentence upgrade is justified by the degraded state having ended rather than by the read completing, with the refusal-only case named as a deliberate under-claim; and the retirement comment now names the reachability-limited case where a pin leg failing every tick with 405/410/409 (which this route does not emit) leaves a sentence standing until an onset, now that an explicit success no longer bounds it. Refs #2638, #2214
Chris0Jeky
added a commit
that referenced
this pull request
Sep 5, 2026
…pointers, #2701 record, #2630 correction clause) Answers the docs review of PR #2704: the #2703 bullet no longer says only force guarantees a request (it gets past the throttle only; the in-flight share and demo mode still apply, the very overclaim round 2 removed from the code); the #2700 and #2703 bullets name every open residual the not-shipped line names; the #2701 pointer says the sixteenth block's subsection is its record; the #2700 bullet says its round 2 touched copy and specs only; the corrections line quotes the thirteenth block's clause that the pinned warning itself sticks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The review queue's degraded disclosure only ever ran one way. When three consecutive transient poll failures set
queueRefreshStale, both skins rendered a warning that the retained queue may be out of date. When a later read succeeded, the flag cleared and the warning was simply unmounted. Nothing was said. A reviewer who was not watching that corner of the surface, and any screen-reader user, was never told the queue is trustworthy again.The warning was also hardcoded English in all three render sites, so an Italian or Spanish reviewer saw English at exactly the moment the surface was admitting it might be wrong. And in the Paper skin the warning sat in the main column, which is the scroller, so it had scrolled out of view by the time the reviewer reached the decision rail and acted.
This change moves the sentence into the catalog, adds the missing recovery announcement in both skins through a single region that survives a branch flip, keeps the Paper warning on screen while the column scrolls without letting the sticky decision rail cover it, and covers the filtered-to-empty case that neither existing degraded spec reached.
Refs #2214. Residual items 1, 4, 5 and 6 of the 2026-09-03 reconciliation comment are closed here. Items 2 (persistent disclosure of repeated non-transient failures), 7 and 8 (the pin-leg 400) remain for the next slice; item 3 already shipped.
Changes
Five commits. The first four are round 1; the fifth is review round 2.
fix(review): localize the degraded-queue warning in both skins(item 5)review.queue.degraded.bodyandreview.queue.degraded.recoveredadded to theen,itandesreview catalogs.PaperReviewView.vueactive-proposal column,PaperReviewView.vueempty column,LegacyReviewView.vue) render$t('review.queue.degraded.body'). The English wording is unchanged, verbatim.LegacyReviewView.vuecomment claiming this skin's copy is entirely hardcoded English was already wrong for the unavailable panel below it and would have become wrong here. It now names the states that share the catalog.PaperReviewView.spec.ts(two assertions) andReviewView.spec.ts(one) assert the catalog value instead of the fragment'may be out of date'.fix(review): announce recovery from a degraded review queue in both skins(item 1)useReviewProposalsexposesqueueRefreshRecovered. It turns true only when a visible degraded state is ended by a successful read, and falls back to false at the next degraded onset, because a live region only speaks when its text changes.recordQueueRefreshSuccess()accounting as a successful poll instead of duplicating its two assignments.sr-onlyregion (role="status",aria-live="polite",aria-atomic="true") that withholds its text rather than appearing with it, the same construction PR fix(review): name an unavailable pinned proposal in Legacy and hold the loading announcement #2593 established for the count regions. The regions arepaper-review-queue-recoveredandreview-queue-recovered.!queueAccessRevoked, the same guard the warning already uses, so the composable's 403 branch keeps its single owner and is untouched.ReviewQueueRail's count region is untouched.fix(review): pin the Paper degraded warning to the top of the scrolling column(item 4)paper-review-deep__queue-stale--pinnedmodifier on the active-proposal warning only:position: sticky; top: 0; z-index: 1;withbackground: var(--paper)and a bottom border. The two sibling notes that share the base class (decision-refresh lock, evidence-unavailable) stay in flow, because they describe the record on screen rather than the whole queue.test(review): cover the filtered-empty degraded case and the Italian disclosure(item 6)PaperReviewView.spec.tscase with a foreign-authored row, the Mine filter and three consecutive 500s, asserting the warning's role, aria-live and aria-atomic, its parent being the empty element, the co-renderedNo matches in Mine.title, and thatpaper-review-queue-livestill carries the real awaiting count.PaperReviewView.language.spec.tscase that flips to Italian with the queue degraded and then recovers it, proving both new keys reach the DOM.fix(review): hoist one recovery region, offset the rail, retire the sentence(review round 2)v-if="activeProposal"andv-elsearms, so a branch flip remounted it. The recovering poll assignsproposals.valueand callsrecordQueueRefreshSuccess()in one synchronous block, so a recovery that puts a proposal back into an empty or filtered-empty queue flipped the branch in the same render the sentence appeared, and the region mounted already carrying its text. That is precisely the insert-with-text case the construction exists to avoid, and after any recovery a later flip remounted a region with content. One node now sits above the branch pair. It is absolutely positioned by.sr-only, so it is not a grid item and takes no column of.paper-review-deep's three-column track list. The two root-children pins now name it.ReviewDecisionRailis alsoposition: sticky; top: 0in the same scroller, opaque throughcard-liftand taller, so past the card header it covered the pinned warning completely and the pin bought nothing in exactly the scrolled state item 4 exists for.PaperReviewViewnow measures the warning with aResizeObserverand writes--paper-review-sticky-offsetonto the scrolling column while the warning is on screen; the rail's single sticky rule becametop: var(--paper-review-sticky-offset, 0), so the rail sits below the warning and both stay visible. With no warning the property is absent and the fallback restores the rail's original position. WhereResizeObserveris unavailable the offset stays 0, which is the previous behaviour rather than a broken one. The warning stays atz-index: 1, below the rail's2, so an opaque paragraph can never cover the Approve and Reject controls or swallow clicks aimed at them.ReviewDecisionRail.vuewas touched for that one rule and its comment only.?rawdirectly, the formReviewDecisionRail.spec.tsalready uses under this tsconfig. The previous comment implied?rawwas unavailable, which was wrong.Test plan
All commands run from
frontend/taskdeck-webin the worktree.Red first, round 1. The recovery specs were written before the implementation and recorded failing.
Red first, round 2. The round-2 specs were run against the round-1 sources (the three source files checked out at the round-1 head, specs left in place) before the fix landed.
Honest notes on which round-1 specs were and were not red. The three literal
toContain('may be out of date')assertions did not go red on the catalog swap, because the English wording moved into the catalog verbatim; they were repointed at the catalog so a future copy edit cannot pass silently, not because they were failing. The filtered-to-empty co-render spec was green the moment it was written: the empty branch already renders the warning above the wholev-ifchain, so that spec is a regression pin on working behaviour, not a fix.Verified (at the round-2 head).
Round 1 also ran the broader Review set at its head:
src/tests/views/paper/review,ReviewView.spec.ts,ReviewView.coverage.spec.ts,useReviewProposals.spec.ts,useReviewActions.spec.ts,src/tests/i18n,src/tests/guards—25 passed / 639 tests.Both new CSS rules were confirmed present in the production build output, not only in source:
dist/assets/ReviewView-*.csscontainstop:var(--paper-review-sticky-offset,0)for the rail and the fullqueue-stale--pinnedrule.NOT verified.
<style scoped>and happy-dom lays nothing out, so neither sticky rule is reachable throughgetComputedStyleand the warning measures 0. The specs assert the modifier class on the element, the two declared rules read from source through?raw, and that the view writes--paper-review-sticky-offsetwhile degraded and stops writing it after recovery. The property's numeric value is asserted only as present. That the rail visibly clears the warning, and that both stay readable while the column scrolls, is a browser fact this suite cannot reach.it/esreview catalogs carry aGH-1307 machine-translated, pending native reviewmarker for the same reason.dotnet testwas run.Boundaries and risks
!queueAccessRevoked, so it is withheld while access is gone and returns when access comes back, if the following healthy success has not yet retired it. Accepted rather than fixed, because clearing it would mean writing to the recovery signal from the composable's 403 branch, which this slice deliberately does not touch..paper-review-deep, which is a three-column grid..sr-onlymakes it absolutely positioned, so it is out of flow and not a grid item; the three columns keep their tracks. Two specs pin the root's children and name it explicitly.recordQueueRefreshSuccess()instead of repeating its two assignments. That is why a board-filter change or a post-action reload also announces recovery, and why such a reload can retire the sentence early. It changes no load semantics; the existing explicit-load and stale specs are unchanged and green.ReviewDecisionRail.vuewas edited for one CSS declaration and its explanatory comment. Nothing else in that component changed, and its own spec suite is green.ReviewQueueRail's count region;LegacyReviewView's unavailable panel; every Inbox and board file.docs/STATUS.mdmay want a line; that call is left to the coordinator who holds the write lease.