P0: fix(a11y): announce authentication errors - #224
Merged
Conversation
The error banners on the OAuth login, account-login, and recovery forms were plain divs / paragraphs. When an error appeared (or updated) screen-reader users got no announcement — the error silently changed and the user kept thinking the form had accepted their input. Add ARIA live-region semantics so updates announce naturally: - Main login flash banner (#error-msg): role=status aria-live=polite (the polite intent is correct — errors should announce on update without interrupting whatever the user was doing) - Server-rendered <p class="error"> banners on account-login and recovery: role=alert (these are static at render time, so the default assertive announcement on first render is appropriate) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The demo's sign-in error banner was a plain <div>. When an error appeared screen-reader users got no announcement — visual users saw the red banner, screen-reader users had no idea anything had changed. role="alert" matches the auth-service error banners in the previous commits (which got role=status / role=alert) and tells assistive tech to announce the message immediately. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The shared renderError() helper produces every styled HTML fallback page (Sign-in session expired, internal failures, session not found, …). The error message <p> was a plain paragraph; screen-reader users got no announcement when these pages loaded. role="alert" tells assistive tech to announce immediately on page render. Matches the role=alert treatment we just gave the inline server-rendered error banners on account-login and recovery. One-character behaviour change; no test fixtures depend on the exact HTML. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: bd1d0d2 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAuthentication errors now use accessible alert or status semantics across auth-service, shared rendering, and the demo client. The login page atomically replaces live-region content so repeated errors are announced. Handle availability feedback is linked to the input and announced politely. ChangesAuthentication accessibility
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🚅 Deployed to the ePDS-pr-224 environment in ePDS
|
Coverage Report for CI Build 30649473082Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage remained the same at 57.281%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions4 previously-covered lines in 2 files lost coverage.
Coverage Stats
💛 - Coveralls |
aspiers
changed the title
aspiers
marked this pull request as ready for review
July 30, 2026 14:31
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Adding role=status/aria-live to #error-msg was not sufficient on its own — the way showFlash mutated the region defeated it in two ways. The region is display:none until an error fires, and showFlash wrote textContent *before* setting display:block. A hidden element is excluded from the accessibility tree, so the text landed while the live region had no "before" state to diff against; assistive tech could legitimately announce nothing. Unhide first, then mutate. Assigning textContent in place also meant a repeated identical message was not a DOM mutation at all. Re-submitting a bad OTP yields the same "Invalid code" string, so the second failure was silent — precisely the "user thinks the form accepted their input" case this branch set out to fix. Build content off-DOM and swap it in via replaceChildren so every failure is a real mutation. Both concerns are now funnelled through one setFlash() helper, which also fixes the double-announcement in showErrorWithAction and the aborted-flow notice: each appended its action button *after* showFlash had already made the region live, so the message and its button announced as two separate updates. Building the whole content in a fragment makes each of them a single announcement. textContent remains the only sink for caller-supplied strings, so reflected better-auth errors still cannot be interpolated as HTML. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The choose-handle page was missed by the earlier commits in this
branch. Two separate gaps:
The server-rendered error banner used the same <div class="error">
pattern as account-login and recovery, so it gets the same
role=alert. Only the populated branch needs it — the empty
placeholder is never written to by this page's script.
More significantly, #handle-status had no live-region semantics at
all. It is the sole feedback channel for whether a handle is free
("Checking…" / "Available!" / "Already taken." / format errors) and
it gates the Create button via submitBtn.disabled. Screen-reader
users therefore got no availability feedback whatsoever, and hit a
button that had been silently disabled with no stated reason.
aria-live=polite rather than assertive because setStatus() fires on
every debounced keystroke; assertive would interrupt the user
mid-type. aria-describedby also associates the status with the input
so it is read on focus, not only on change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Widens the existing changeset to match what the branch now does: handle selection is a newly covered surface, and repeated identical failures announcing each time is a user-visible behaviour change worth calling out separately from the initial role/aria additions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/auth-service/src/routes/login-page.ts (1)
657-657: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse CSS classes for flash-region visibility.
Line 657 sets initial visibility with inline
display:none. Lines 1015 and 1076 then change inline display values at runtime. Add a.hiddenclass and toggle that class instead.Proposed fix
- <div id="error-msg" class="flash-msg" style="display:none;" role="status" aria-live="polite"></div> + <div id="error-msg" class="flash-msg hidden" role="status" aria-live="polite"></div> + .flash-msg.hidden { display: none; } - errorEl.style.display = 'block'; + errorEl.classList.remove('hidden'); - errorEl.style.display = 'none'; + errorEl.classList.add('hidden');As per coding guidelines, “Use CSS classes to control visibility (hidden, active) rather than inline display style except for dynamic values set at render time.”
Also applies to: 1012-1015, 1070-1076
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/auth-service/src/routes/login-page.ts` at line 657, Replace the inline display styling on the error flash region with the existing or newly defined hidden CSS class, and update the visibility logic around the relevant error-message handling functions to add or remove that class instead of assigning inline display values. Preserve the current shown/hidden behavior and target the error region identified by error-msg.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/auth-service/src/routes/login-page.ts`:
- Line 657: Replace the inline display styling on the error flash region with
the existing or newly defined hidden CSS class, and update the visibility logic
around the relevant error-message handling functions to add or remove that class
instead of assigning inline display values. Preserve the current shown/hidden
behavior and target the error region identified by error-msg.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bcb68292-9cae-4331-934e-74542cafd25b
📒 Files selected for processing (7)
.changeset/announce-auth-errors.mdpackages/auth-service/src/routes/account-login.tspackages/auth-service/src/routes/choose-handle.tspackages/auth-service/src/routes/login-page.tspackages/auth-service/src/routes/recovery.tspackages/demo/src/app/components/LoginForm.tsxpackages/shared/src/render-error.ts
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
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
Give authentication errors standard alert and live-region semantics so screen-reader users hear failures when the page updates.
Changes
Testing
pnpm format:checkpnpm lintpnpm typecheckpnpm testpnpm test:coverageScreenshots
The visual error treatment is intentionally unchanged. DOM inspection on this deployed page confirmed that the banner is now exposed as
role=alertso assistive technology announces it.Notes
Summary by CodeRabbit