fix(a11y): finish the Notice conversion in the appraise form - #137
Conversation
The last `&&`-gated Notice in the app, and the one PR #131 missed: a grep for the pattern across server pages found six, but this one is in a client component and reads `{state && !state.ok && <Notice/>}` rather than `{message && <Notice/>}`. It is the worst instance rather than an afterthought. `useActionState` keeps this component mounted across a rejection — that is the whole reason it exists (see the docblock) — so the surrounding tree never moves and the `&&` inserted a fresh role="alert" node carrying its own text into an otherwise stable document. In the server-page cases a document-less re-render at least changed several things at once; here the region's birth IS the only change, which is precisely the announcement AT handles least reliably. The visible behaviour is unchanged. The comment's argument — that `state === null` covers both "hasn't submitted yet" and "still pending", neither of which is a rejection worth announcing — is preserved exactly by moving the condition into the children. Empty children render the reserved `.notice-slot`, which is positioned out of flow, so the form-stack row gap does not open up. Cost to a user: an operator on a screen reader who pastes a malformed loot list into the appraise form is not reliably told the submit was rejected. The form stays where it is and the paste survives, so nothing else on screen reports it either. Gates: node-version, typecheck, lint, format:check, build, npm test (76 files / 1085 tests) all pass. docker build and test:e2e not run.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Comment |
|
@coderabbitai review |
|
|
Carries the round-4 e2e fix by merge (c44517f → 93d69c7). The loot-paste label added in #136 read |
… it (#138) The login seal carried alt="<brand> emblem" two lines above <h1>{brand.name}</h1>. Every other decorative image in the app already uses alt="" — account/page.tsx:473, account/page.tsx:681, and, most directly, ui.tsx:117, which is the same brand mark rendered immediately before the wordmark that spells the brand out in text. /login was the one exception. Cost to a user: a screen-reader visitor to the only page an unauthenticated person can reach hears the brand name, then the brand name again, before reaching the motto, the scope disclosure, or the sign-in control. The emblem communicates nothing the h1 does not, which is the case alt="" exists for. The EVE SSO mark below keeps its alt: that image is the control's label, not decoration, and nothing else names it. The eslint-disable-next-line for @next/next/no-img-element now sits directly above the <img> again — the new comment goes above it, not between. Inserting it between broke the directive's adjacency and reintroduced the warning #123 cleared; lint is quoted below to show it is clean. Gates: node-version, typecheck, lint (0 warnings), format:check, build, npm test (76 files / 1085 tests) all pass. docker build and test:e2e not run.
1 participant
Round 5 of the autonomous
design-sweep-looprun. Completes the cause PR #131 opened.What this round resolved
The last
&&-gatedNoticein the app, and the one #131 missed. That round grepped the pattern across server pages and found six; this one is in a client component and reads{state && !state.ok && <Notice/>}rather than{message && <Notice/>}.It is not an afterthought — it is the worst instance of the pattern.
useActionStatekeepsAppraiseFormmounted across a rejection; that is the entire reason the component exists (appraise-form.tsx:15-25). So the tree around the notice never moves, and the&&inserted a freshrole="alert"node carrying its own text into an otherwise stable document. In the server-page cases a re-render at least changed several things at once. Here the region's birth is the only change on the page, which is exactly the announcement AT handles least reliably — the caseNotice's docblock (ui.tsx:255-270) was written about.Cost to a user: an operator on a screen reader who pastes a malformed loot list into the appraise form is not reliably told the submit was rejected. The form stays put and the paste survives (by design), so nothing else on screen reports it either — the notice is the only signal, and it is the one that does not land.
Visible behaviour is unchanged. The existing comment's argument is preserved verbatim by moving the condition into the children:
state === nullstill covers both "hasn't submitted yet" and "still pending", and neither still announces anything. Empty children render.notice-slot, whichglobals.css:2231-2243positions out of flow, so the.form-stackrow gap does not open up around an empty region.After this,
grep '<Notice'acrosssrc/appreturns no&&-gated call site.What was skipped
Nothing this round.
What needs a human
Unchanged from #136 and the rest of the stack. The one adjacent to this file: the payout forms still have no per-field error association (
aria-invalid/aria-describedby). This PR makes the form-level rejection announceable; it does not connect a message to the field that caused it, and a screen-reader user who tabs into the ten-row textarea after the announcement has nothing tying the two together.Gate results
scripts/check-node-version.shnpm run typechecknpm run lintnpm run format:checkAll matched files use Prettier code style!npm run buildnpm testdocker build .npm run test:e2eStack position
PR 5 of the stack, based on
design-sweep/2026-08-06-4(#136).Merge #131 → #132 → #134 → #136 → this, and prefer a merge commit or rebase over squash.