[Frontend][A11y][Review] Queue live-region residuals: no repeat announcement on a same-scope reload, focus after the unavailable-pin return - #2712
Conversation
`!proposalsLoading` was the wrong question for both skins' announcement gates. An explicit `loadProposals` raises that flag WITHOUT clearing `proposals`, so the gate withheld the live region's content for the length of every reload and restored it afterwards: the region wrote count -> '' -> count, and a node addition inside a live region is spoken. The reviewer heard the same figure read back after the header Refresh and after filing a settled proposal away, about a queue that had not moved. `queueScopeLoaded` asks the question the gate actually needs: has a read landed for the board scope on screen? It settles at both landing sites -- the explicit load and the background poll, each stamped with the scope it asked about rather than whichever board is current when it lands -- and is false in exactly the three cases where the rendered count is not a count of the queue on screen: before the first read, after a board-filter change until the new scope's read lands, and after a failed read (including a refusal, which clears the queue outright). Scope comparison is lower-cased: the scope is the board, not the casing the query string carried, which is how `matchesActiveBoardFilter` already reads it. The signal only lands here in this commit; the two skins still gate on loading. Refs #2599, #2214
Two residuals from PR #2593's review, fixed in both skins together so the Review queue cannot drift (#1124 / ADR-0038). Item 1, no repeat announcement on a reload that changed nothing. Both gates now read `queueScopeLoaded` instead of the loading term: `LegacyReviewView.countIsAnnounceable` directly, and `ReviewQueueRail.countIsAnnounceable` through a new `queueScopeLoaded` prop that `PaperReviewView` feeds from the same composable value. A byte-identical same-scope reload keeps the SAME announcement node, so nothing is added to the live region and nothing is spoken; a reload that changed the queue still re-keys once, as #2710 shipped it; the first read, a board-filter change and a failed read still withhold, and the revoked gate is untouched beside it. The rail's `loading` prop is removed rather than left in place: it was that gate's only consumer, and a prop documenting a gate it no longer takes part in is how the next reader is misled. The replacement is optional and defaults to WITHHOLDING -- a parent that cannot say a read has landed cannot have its count spoken, because 0 from a never-read queue is the defect #2593 removed. `PaperReviewView` is its only mount. Item 2, focus after the unavailable-pin return control. Activating it removed the element focus was in and moved focus nowhere, so it fell to `<body>`: nothing announced, and the next keystroke acting on nothing. Both skins now move focus after `nextTick`, following the `settledElsewhereReturnRef` pattern Paper already uses one branch over. The target is the queue the panel was standing in front of, at its first row -- Paper's first rail row button (the rail owns its rows, so it owns the handoff and exposes `focusFirstQueueRow`), and Legacy's queue list section, which carries the "Proposals awaiting review" label and the Arrow cursor that starts on row one. When the queue behind the panel is empty, focus goes to the empty state that replaces it instead (`tabindex="-1"`, programmatic only -- neither element enters the tab order). Refs #2599, #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; one fresh-context read-only reviewer subagent since Codex credits are exhausted, SC-9; round 1 at head Verdict at round 1: SHIP; no correctness defect the reviewer could construct. Triage:
Clean lenses corroborated by the reviewer against the tree at the head: the three writes to Unverified by the read-only reviewer, run by the worker at the head (six named specs 6 files / 508 tests; typecheck, eslint, diff-check) and by CI's frontend unit job; no screen reader was used, as the PR says. Round count: 2 (comments and one spec, no logic, so no further pass is owed). Merge gate remaining: the fix head's |
…il's reload node Round 2 on PR #2712. Four LOWs from the fresh-context review: three are comment truth, one is a vacuous spec. No mechanism changed. 1. The gate does not withhold "after a failed read". `landedQueueScope` is written only where a read replaced the queue and cleared only by `recordQueueAccessRevoked`; the catch arm leaves it alone. So an entry load that landed for board A followed by a header Refresh that 500s keeps the count announceable -- correctly, because the rows on screen are still that landed answer, and re-withholding there would put the count -> '' -> count flicker back for a read that changed nothing. Reworded in all three places that carried the claim (the composable, the rail prop, and the Legacy gate, which the review did not name but repeated it), each now saying what is withheld -- a scope no read has landed for, including one whose only read failed -- and why a later failure deliberately is not. The rail's `queueUnavailable` docblock stopped pointing at the removed `loading` prop while there. 2. Legacy's Arrow cursor does not start on the first row. `activeReviewIndex` is a persistent ref, and `scrollVirtualizerToHashProposal` rewrites it only when the hash target is found -- exactly what a dead pin fails to do -- so a reviewer who arrowed to row 4 returns with the cursor at 4. The comment now says what is true: focus lands on the section, the cursor keeps its place, and it only drives `scrollToIndex`. 3. The two skins' focus targets are not the same element, and calling them one was the drift claim to avoid. Paper lands on a row `<button>` (announces the row, Enter selects); Legacy lands on the queue `<section>` (announces the region label, Enter does nothing) because its rows are not focusable. Both comments now record the divergence and its cause; the shared rule is where focus goes, not the element type. 4. The rail's same-scope-reload spec re-set byte-identical props, which does not even re-render, and passed on the pre-fix rail. It now mounts with the RETIRED `loading: true` input -- the pre-fix gate's first term, an inert fallthrough attribute here -- and additionally changes an unrelated prop to force a real re-render. Verified RED on the unmodified rail source at 5487c26: `Unable to get [data-testid="paper-review-queue-announcement"]`. The spec comment says where the end-to-end reload evidence lives (the two view specs). Refs #2599, #2214
|
Checkpoint (alpha lane session ending 2026-09-05 ~21:50Z): round-1 review SHIP (record above); the round-2 commit answering its four comment-and-spec LOWs is pushed at |
|
Round 2 at |
1 participant
Summary
The two non-blocking residuals from PR #2593's review (#2599 items 1 and 2), fixed in both skins
together because a one-skin repair of the Review queue is the drift class itself (#1124 / ADR-0038).
Item 3 of the issue shipped in #2593 and is untouched.
Item 1: an explicit reload that changed nothing no longer repeats the count in the reviewer's ear.
Item 2: leaving the target-unavailable panel no longer drops focus to
<body>.No copy changed (the review locales are untouched), no test id changed, no docs changed.
Changes
Item 1 — the announcement gate asks whether a read has landed, not whether one is in flight
useReviewProposalsexposesqueueScopeLoaded. Its exact semantics: a queue read has landed forthe board scope currently on screen.
scope it asked about, snapshotted before the await, so a late answer can never claim the board
that happens to be current when it lands. The poll is a landing site in its own right: after a
failed entry load it is what makes the count speakable again without the reviewer reloading.
lands, after a board-filter change until the new scope's read lands, and for a scope no read has
landed for — including one whose only read failed. A refusal (
recordQueueAccessRevoked) clearsthe queue, so it unsettles there too; the revoked panel keeps its own separate gate beside it.
landedQueueScopeis written only where a readreplaced the queue, and the catch arm leaves it alone, so a header Refresh that 500s keeps the last
landed count announceable — correctly, since those rows are what is still on screen. Withholding
there would restore the count → '' → count flicker for a read that changed nothing; a failing
refresh is reported by the degraded/refused disclosures instead (Review queue refresh residuals: deep-link re-auth, poll restart after recovery, stale indicator (from PR #2208) #2214).
loadProposalsraisesproposalsLoadingwithout clearingproposals, so the count on screen is still the lastlanded read's count of that same board.
the same rule
matchesActiveBoardFilteralready applies.Both gates now read it:
LegacyReviewView.countIsAnnounceabledirectly, andReviewQueueRail.countIsAnnounceablethrough a new optionalqueueScopeLoadedprop thatPaperReviewViewfeeds from the same composable value. Result: a byte-identical same-scope reloadkeeps the same announcement node (nothing added to the live region, nothing spoken); a changed
reload still re-keys once, exactly as #2710 shipped it; the skeleton, board-filter and revoked gates
all still withhold.
The rail's
loadingprop is removed rather than left in place. It was that gate's only consumer,and a prop whose documentation describes a gate it no longer takes part in is how the next reader is
misled. The new prop is optional and defaults to withholding: a parent that cannot say a read has
landed cannot have its count spoken, because 0 from a never-read queue is the defect #2593 removed.
PaperReviewViewis the rail's only mount site in the app.Item 2 — focus after the unavailable-pin return control
Both skins now move focus after
nextTick, following thesettledElsewhereReturnRefpattern Paperalready uses one branch over.
The target: the queue the panel was standing in front of, at its first row; the empty state that
replaces the panel when there is no queue behind it.
ReviewQueueItemrenders a<button>, so it is alreadyfocusable and announces the row it lands on). The rail owns its rows, so it owns the handoff and
exposes
focusFirstQueueRow(), which reports whether there was a row; the view reaches for itsempty column only when that answer is
false. A parent querying the child's subtree for a rowelement would be the drift seam this repair exists to close.
<section>. This skin's rows areReviewProposalCards, which are notfocusable and are not this claim's files; the list itself already is —
tabindex="0", labelled"Proposals awaiting review", and carrying the Arrow cursor that starts on row one. So the same
kind of target in both skins (the queue, at its first row), expressed through each skin's own
queue widget.
ReviewEmptyStateroot and Paper'spaper-review-deep__emptycolumn, eachgiven
tabindex="-1"— programmatic focus targets only, neither enters the tab order.Test plan
Every new spec was run against the unmodified source first. 16 tests failed red; recorded texts:
TypeError: Cannot read properties of undefined (reading 'value')Error: Unable to get [data-testid="review-queue-announcement"] within: <div class="td-review" ...>AssertionError: expected <body>…(3)</body> to be <section data-v-c94e4f43 …(3)>…(1)</section>AssertionError: expected null to be '-1'Error: Unable to get [data-testid="paper-review-queue-announcement"] within: <div class="paper-review-deep" ...>AssertionError: expected <body>…(6)</body> to be <button data-v-bc719c4d …(5)>…(3)</button>AssertionError: expected null to be '-1'AssertionError: expected '0 proposals awaiting review.' to be ''AssertionError: expected '2 proposals awaiting review.' to be ''AssertionError: expected true to be falsefocusFirstQueueRow(2 tests)TypeError: wrapper.vm.focusFirstQueueRow is not a functionThe Paper reload spec initially passed red for the wrong reason —
handleDismissAppliedpatches thequeue locally when the server dismisses everything asked for, so no reload happened. It now drives
the partial-dismiss branch, which does re-read, and asserts
getProposalswas called a second timeso it can never go vacuous again.
Two of the new specs are guards rather than red tests and are called out as such: the board-filter
withholding cases pass under the old gate too (a filter change also raises
proposalsLoading), andthey are here to keep that behaviour when the loading term is gone.
Commands, green at this head:
npx vitest --run --maxWorkers=2 src/tests/composables/useReviewProposals.spec.ts src/tests/views/ReviewView.spec.ts src/tests/views/paper/review/PaperReviewView.spec.ts src/tests/views/paper/review/ReviewQueueRail.spec.ts src/tests/views/paper/review/ReviewQueueRailMembershipFilter.spec.ts src/tests/views/paper/review/PaperReviewMembershipFilter.spec.ts→ Test Files 6 passed, Tests 508 passed.npm run typecheck→ clean.npx eslinton the eight changed files → clean.git diff --check→ clean.Not verified
identity across a reload, node presence/absence, region never remounted — which is the mechanism
aria-live'saria-relevant="additions text"acts on, not a recording of what NVDA or VoiceOveractually said.
frontend/taskdeck-web/srcchanged.document.activeElementwithattachTo: document.body, not a realbrowser's focus ring or its announcement of the focused element.
wrapper.vm.focusFirstQueueRow()reaching a<script setup>+defineExposecomponent throughVTU, and
InstanceType<typeof ReviewQueueRail>exposing that method undervue-tsc, are provenonly by the spec run and the typecheck run in this PR — not by any other consumer.
Boundaries
Eight files, all inside the claim:
useReviewProposals.ts(the scope signal only), both reviewviews,
ReviewQueueRail.vue, and the four matching specs. Not touched: the review locales,usePaperReviewSelectors.tsand the rail cards, the receipt recorders, the diff pane, the stickyseam,
src/api/**, docs,OUTSTANDING_TASKS.md.Recorded divergence for the next ADR-0038 / #1124 drift audit: the two skins' focus targets are
deliberately not the same element. Paper focuses a queue row
<button>— a screen reader announcesthat row and Enter selects it. Legacy focuses the queue
<section>— it announces the region labeland Enter does nothing — because Legacy's rows (
ReviewProposalCard) expose no focusable control andthe section owns the roving Arrow cursor. What the skins share is the rule about where focus goes
(the queue that replaced the panel, else the empty state that did), not the element type, which each
skin's queue widget determines. Making them literally identical means making Legacy's rows focusable,
which is a different change in files this claim does not own.
Round 2 (head
80631e221)Fresh-context review verdict was SHIP with no correctness defect; four LOWs, three of them comment
truth and one a vacuous spec. Fixed in one commit,
80631e221— no mechanism change, so thebehaviour described above is unchanged.
landedQueueScopeis written only where a readreplaced the queue and cleared only by
recordQueueAccessRevoked; the catch arm leaves it alone.A load that landed for board A followed by a Refresh that 500s keeps the count announceable, which
is correct — the rows on screen are still that landed answer. All three places carrying the claim
now say what is withheld (a scope no read has landed for, including one whose only read failed)
and why a later failure deliberately is not, so nobody "restores" a withhold that would bring the
flicker back. The Legacy gate repeated the same wording and was corrected with them; the rail's
queueUnavailabledocblock stopped pointing at the removedloadingprop.activeReviewIndexis a persistent ref andscrollVirtualizerToHashProposalrewrites it only when the hash target resolves — exactly what adead pin fails to do — so a reviewer who arrowed to row 4 returns with the cursor at 4. The comment
now states what is true: focus lands on the section, the cursor keeps its place, and it only drives
scrollToIndex.Boundaries above rather than papered over.
re-render, and passed on the pre-fix rail. It now mounts with the retired
loading: trueinput(the pre-fix gate's first term; an inert fallthrough attribute here) and changes an unrelated prop
to force a real re-render. Verified red on the unmodified rail source at
5487c263b:Unable to get [data-testid="paper-review-queue-announcement"]. Its comment points at the twoview-level specs that carry the end-to-end reload evidence.
Re-proved at
80631e221: the same six specs → Test Files 6 passed, Tests 508 passed;npm run typecheckclean;npx eslinton the four touched files clean;git diff --checkclean.Refs #2599, #2214