Skip to content

fix(review): associate the refresh-lock reason with every disabled decision - #2568

Merged
Chris0Jeky merged 2 commits into
mainfrom
issue-2461/decision-lock-reason
Sep 4, 2026
Merged

fix(review): associate the refresh-lock reason with every disabled decision#2568
Chris0Jeky merged 2 commits into
mainfrom
issue-2461/decision-lock-reason

Conversation

@Chris0Jeky

@Chris0Jeky Chris0Jeky commented Sep 4, 2026

Copy link
Copy Markdown
Owner

Summary

PR #2448 renders a live status while the post-revision truth refresh disables the Review decision
rail. The status is drawn by PaperReviewView above the main column, with its id passed down as
aria-describedby. That attribute lands on ReviewMain's root wrapper, which is not focusable, so
assistive technology inspecting the disabled Reject, Request edit, Defer or Approve control got no
relationship to the explanation. Those controls only ever carried the older edit-lock description.

ReviewDecisionRail now takes an explicit description-id contract for explanations rendered outside
the rail, and joins them with its own edit-lock note on every decision control. ReviewMain forwards
the aria-describedby it receives to the rail while keeping it on the wrapper, so no change is needed
in PaperReviewView.vue and the association it already asserts is preserved.

Closes #2461.

Changes

frontend/taskdeck-web/src/views/paper/review/ReviewDecisionRail.vue

  • New optional prop decisionDescriptionIds: space-separated DOM ids of explanations rendered outside
    the rail. The caller owns the existence of those ids, which is documented on the prop.
  • decisionDescribedBy now joins the external ids with the rail's own edit-lock note id when the edit
    lock is shown. External ids come first, matching the order the notes appear on screen. An empty or
    blank-only value produces no attribute at all, so a joined empty id list cannot dangle.
  • The existing edit-lock behaviour is unchanged: the note still renders only on a decision rail, the
    cancel button is still ungated by busy and still disabled while saving, and a settled filing rail
    still shows no lock.

frontend/taskdeck-web/src/views/paper/review/ReviewMain.vue

  • Reads the incoming aria-describedby fallthrough attribute and passes it to the rail as
    decision-description-ids. Attribute inheritance is left on, so the wrapper keeps the attribute.
  • The value is read through a function rather than a computed. Fallthrough attributes are not
    reactive in Vue, so a cached computed would keep the value the column had on its first render and
    would still point at a note after the lock cleared. There is a regression test for exactly that.
  • No prop was added or changed, so PaperReviewView.vue needs no edit.

frontend/taskdeck-web/src/tests/views/paper/review/ReviewDecisionRail.spec.ts

  • New attached-DOM tests: every disabled decision control carries the external explanation; the
    external explanation and the rail's own edit-lock note are carried together in that order; the only
    remaining control in apply-only mode is described; a blank id list produces no attribute. Each test
    that asserts an id also resolves it with document.getElementById, so a dangling reference fails.
  • The mount helper gained applyOnly, decisionDescriptionIds and an attachTo option.

frontend/taskdeck-web/src/tests/views/paper/review/ReviewMain.spec.ts

  • New attached-DOM tests: the column description id reaches all four disabled decision controls while
    the wrapper keeps it; nothing is described when the column has no explanation; the association is
    dropped from the controls when the lock clears, driven from a host component that re-renders.
  • The mount helper was split into mainProps plus mountMain so the reactivity test can render the
    component from a host, and gained busy and attrs options.

No new user-visible strings, so no locale catalog changed.

Test plan

Verified, from frontend/taskdeck-web:

  • Red first. With the two component files reverted to their main content and only the new specs in
    place, npx vitest --run --maxWorkers=2 src/tests/views/paper/review/ReviewDecisionRail.spec.ts src/tests/views/paper/review/ReviewMain.spec.ts
    reported 44 tests, 5 failed, 39 passed. The five failures were the three rail association tests and
    the two ReviewMain forwarding tests. The two negative tests, which prove no attribute is added when
    there is no explanation, passed before the change as coverage.
  • Green after the change, including the consumer view spec that was not edited.
    npx vitest --run --maxWorkers=2 src/tests/views/paper/review/ReviewDecisionRail.spec.ts src/tests/views/paper/review/ReviewMain.spec.ts src/tests/views/paper/review/PaperReviewView.spec.ts:
    3 files, 204 tests passed.
  • Neighbours.
    npx vitest --run --maxWorkers=2 src/tests/composables/useReviewProposals.spec.ts src/tests/composables/usePaperReviewSelectors.spec.ts:
    2 files, 159 tests passed.
  • npm run typecheck: clean.
  • npx eslint src/views/paper/review/ReviewDecisionRail.vue src/views/paper/review/ReviewMain.vue src/tests/views/paper/review/ReviewDecisionRail.spec.ts src/tests/views/paper/review/ReviewMain.spec.ts:
    no findings.
  • npm run build: 813 modules transformed, built in 2.28s.
  • git diff --check: clean.

NOT verified:

  • The browser focus-return proof from PR fix(review): require authoritative refresh after revisions #2448, tests/e2e/review-proposals.spec.ts, was NOT run. It
    needs a running stack. The file is untouched by this PR and its assertions do not depend on
    aria-describedby: it asserts the lock note is visible, that the four decision controls are
    disabled, and that Enter does not approve.
  • No screen reader was exercised. The association is proven structurally, by attribute value plus a
    document.getElementById resolution in an attached DOM, not by an assistive technology run.
  • The full frontend vitest suite was not run. A bare full run OOMs on this box, so the run was scoped
    to the two changed components, their specs, the view spec that renders them, and the two neighbour
    composables.

Boundaries and risks

  • Files touched: ReviewDecisionRail.vue, ReviewMain.vue and their two specs.
    PaperReviewView.vue, useProposalDisplayNames.ts, ReviewProposalCard.vue and their specs are
    owned by PR fix(review): enrich incomplete move-card headlines #2541 and were not edited. PaperReviewView.spec.ts was run, not modified, and its
    existing assertion that the column wrapper carries paper-review-revision-refresh-lock still holds.
  • Scope note: ReviewMain forwards whatever aria-describedby the Review view computes. Today that is
    the refresh lock and the evidence-unavailable note, so a reviewer on a decision control now also
    hears that evidence could not be refreshed when that note is on screen. That is decision-relevant
    context, but it is a wider association than the refresh lock alone, and it is the direct consequence
    of not editing PaperReviewView.vue to split the two ids apart.
  • Scope note: the filing rail's single File away button was deliberately left undescribed. The issue
    names the four decision controls, and the refresh lock cannot coexist with a settled proposal because
    the refresh runs only on the apply path of an actionable one.
  • The rail cannot verify that an id it is handed exists. That contract is documented on the prop, and
    the only caller derives its ids from the notes it actually renders.

Round 2 (head b608431)

The fresh-context review returned SHIP with one MEDIUM taken in scope: the evidence-unavailable note reached the four decision controls while they were enabled, so Reject, Request edit and Defer announced a description about a different action. ReviewMain now passes the description ids to the rail only while busy is true; the wrapper's own aria-describedby is unchanged and PaperReviewView.vue is still not edited. Regression: enabled controls never carry the external id even while a column note is on screen (red 1 failed / 18 passed before the gate, green after). The rail's prop doc now states both caller obligations, and both specs clean up injected notes and attached wrappers in afterEach so a failing assertion cannot leak ids into later tests.

Verified at this head: ReviewDecisionRail, ReviewMain and PaperReviewView specs 205 passed across 3 files; npm run typecheck; scoped ESLint; git diff --check. NOT verified: Playwright review-proposals.spec.ts (untouched), screen-reader run, npm run build on this branch (the earlier build on the same branch predates the one-line template change).

…cision

ReviewDecisionRail takes an explicit decisionDescriptionIds contract for
explanations rendered outside the rail, and joins them with its own edit-lock
note on Reject, Request edit, Defer and Approve. The attribute is omitted
entirely when there is nothing to point at, so a blank id list cannot become a
dangling aria-describedby.

ReviewMain forwards the aria-describedby it receives from the Review view to
the rail while keeping it on the column wrapper, which the view already
asserts. The value is read through a function rather than a computed because
fallthrough attributes are not reactive, so a cached read would keep pointing
at a note that has already been removed.

The prop surface of ReviewMain is unchanged, so PaperReviewView needs no edit.

Refs #2461
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Review gate (Codex credits exhausted, SC-9): one fresh-context adversarial reviewer on head bbf2ba995 (merge base 61e94f672). Verdict: SHIP. No merge-blocking defect: the useAttrs()-plus-function read updates on every host re-render, the joined aria-describedby has no empty, duplicate or whitespace-only tokens, no dangling id is possible in the shipped caller (notes and ReviewMain share one v-if block and one set of flags), and focus, disabled state, the keymap gate and cancel behaviour are byte-identical.

Findings by bin:

  • Fixed in the same round (one line, in scope, because this PR is what makes the note reach the controls): MEDIUM, reviewMainDescriptionIds unions the refresh-lock id with the evidence-unavailable id and only the former is coupled to busy, so after a failed refresh all four enabled decision buttons announced "Review evidence could not be refreshed. No decision was made. Choose the current action again to retry.", which is wrong for Reject, Request edit and Defer. The rail now receives the ids only while busy is true. Two LOWs folded into the same commit: the rail's prop doc understated what arrives; the injected external ids and attachTo wrappers in the two specs were cleaned only on the happy path.
  • Declined: the filing rail's File away stays undescribed while the refresh note is on screen (outside [Frontend][A11y] Associate the post-revision Review lock reason with disabled decisions #2461's four controls; narrow window during the lock reload).
  • Coordinator-owned: the STATUS entry lands in the next alpha-lane STATUS block.

Evidence at the reviewed head (from the implementation run): red-first 5 failed then 44/44 on the two component specs; 204 across ReviewDecisionRail, ReviewMain and PaperReviewView specs; 159 neighbours; typecheck, scoped ESLint, build and diff check clean. Not verified: tests/e2e/review-proposals.spec.ts (needs a running stack; untouched), screen-reader run. Merge after the fix commit ages and ci-required is green at the new head.

The ids the Review view passes down are a union of the refresh-lock note and
the evidence-unavailable note, and only the first is coupled to busy. After a
failed refresh the second note stays on screen while the controls are enabled,
so Reject, Request edit and Defer announced a retry instruction for an action
that had not been attempted. ReviewMain now forwards the ids only while busy
holds the controls disabled; the wrapper keeps its own aria-describedby.

The rail's prop doc states both caller obligations: the ids must resolve, and
they must describe the current state of the controls.

Spec cleanup moved to afterEach in both files so a failing assertion cannot
leak an injected note id or an attached wrapper into a later test.

Refs #2461
@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Round 2: scoped second-pass fresh-context review of the fix diff (bbf2ba9..b608431), verdict SHIP. Confirmed: every control that receives the ids is disabled by busy and nothing else; revisionReviewRefreshBusy implies busy at every render (written only as a synchronous pair with applyGuardBusy), so the refresh-lock note is never on screen with the rail enabled; the gated read is a template expression re-evaluated on every render; the new spec fails at the first button without the gate. Four LOWs tracked as #2578 (per-note split so the Apply unavailable note keeps its association; a throwing cleanup would drop later cleanups; prop doc wording; a view-level union case). Nothing fixed in this round. Round count: 2. Merge after ci-required is green at b608431; the Smart CI / Required Gate red on this head is the #2327 merge-ref race (main moved at 22:15Z when #2567 landed; the 22:11Z Smart CI run on the same head passed) and is advisory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[Frontend][A11y] Associate the post-revision Review lock reason with disabled decisions

1 participant