fix(auth): unify link affordances across the sign-in screens - #237
Merged
Conversation
🦋 Changeset detectedLatest commit: bfd481d 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.
|
|
🚅 Deployed to the ePDS-pr-237 environment in ePDS
|
|
Warning Review limit reached
Next review available in: 10 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThe PR normalizes known OTP verification errors while preserving raw backend errors for control flow. It also standardizes standalone and inline sign-in link styling across authentication routes, tests, theme CSS, and changesets. ChangesSign-in user experience
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant login-page
participant verifyOtp
participant better-auth
login-page->>verifyOtp: Submit OTP
verifyOtp->>better-auth: Verify OTP
better-auth-->>verifyOtp: Return raw verification error
verifyOtp-->>login-page: Return display text and rawError
login-page->>login-page: Detect expiry from rawError and render message
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 |
There was a problem hiding this comment.
Pull request overview
Unifies how “link-like” actions are styled across the auth sign-in/recovery surfaces, so equivalent actions look consistent regardless of whether they’re rendered as <button>s or <a>s, and improves keyboard focus visibility for inline/standalone actions.
Changes:
- Standardize underline/hover behavior across
.btn-secondary,.recovery-link,.flash-action, and.terms-link, including adding:focus-visibleoutlines where missing. - Align demo theme overrides so
.btn-secondaryand.recovery-linktheme identically (including hover), avoiding theme-driven divergence. - Add targeted CSS-pin tests in
login-page.test.tsto prevent regressions in the “standalone vs in-sentence” affordance convention, hover, focus ring, and muted-foreground token usage.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/auth-service/src/routes/login-page.ts | Implements the link-affordance convention in the sign-in page CSS (underline/hover/focus-visible/token usage). |
| packages/auth-service/src/routes/account-login.ts | Updates .btn-secondary styling to match the standalone-action convention (no underline, hover darken). |
| packages/auth-service/src/routes/recovery.ts | Updates .btn-secondary styling to match the standalone-action convention (no underline, hover darken). |
| packages/auth-service/src/tests/login-page.test.ts | Adds tests that pin the CSS convention, hover behavior, focus ring presence, and token usage. |
| packages/demo/src/lib/theme.ts | Ensures demo theming treats .btn-secondary and .recovery-link as a single visual class (including hover). |
| .changeset/unify-link-affordances.md | Adds a patch changeset describing user-visible styling consistency + theming impact for client developers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Coverage Report for CI Build 30948284199Coverage remained the same at 57.909%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
aspiers
force-pushed
the
fix/link-affordances
branch
from
August 4, 2026 18:55
315ec60 to
a839835
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (3)
packages/auth-service/src/tests/login-page.test.ts:857
- The test helper
ruleFor()is tightly coupled to the exact indentation/newline formatting of the rendered CSS (\n ${selector} {). That makes these tests prone to false failures from harmless formatting changes (e.g., template indentation tweaks), even when the CSS behavior is unchanged. Consider locating the rule with a regex that ignores whitespace and escaping selector metacharacters.
function ruleFor(html: string, selector: string): string {
const idx = html.indexOf(`\n ${selector} {`)
expect(idx, `no rule found for "${selector}"`).toBeGreaterThan(0)
const open = html.indexOf('{', idx)
const close = html.indexOf('}', open)
expect(close).toBeGreaterThan(open)
return html.slice(open + 1, close)
}
packages/auth-service/src/routes/recovery.ts:518
- This comment says "Resend code" here and on the sign-in page "must not render differently", but the two screens still differ in multiple styling details (e.g., hover color, font weight/padding). To avoid misleading future readers, narrow the claim to the specific affordance being unified (underline/hover behavior).
/* Standalone action in its own row — see the link-affordance convention
documented in login-page.ts. No underline, darkens on hover. "Resend
code" here and on the sign-in page must not render differently. */
packages/auth-service/src/routes/account-login.ts:254
- This comment says "Resend code" here and on the sign-in page "must not render differently", but the two screens still differ in several styling details. Narrow the statement to the specific affordance being unified (underline/hover behavior) so it doesn't become misleading documentation.
/* Standalone action in its own row — see the link-affordance convention
documented in login-page.ts. No underline, darkens on hover. "Resend
code" here and on the sign-in page must not render differently. */
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/auth-service/src/routes/login-page.ts:1136
- The separator between the error text and its inline action decides whether to insert a period by testing
/[.!?]$/againstmsg, but ifmsghas trailing whitespace/newlines the test fails and you can end up with double punctuation (e.g. "... . Send a new code"). Consider trimming trailing whitespace before both rendering and punctuation detection, and avoid adding any separator when the message is empty.
frag.appendChild(
document.createTextNode(/[.!?]$/.test(msg) ? ' ' : '. '),
);
packages/auth-service/src/routes/login-page.ts:1117
- PR description says the page renders better-auth's "Invalid OTP" message verbatim, but this change introduces otpErrorText() to rewrite several better-auth errors into end-user copy. Please update the PR description/screenshots text accordingly so reviewers and future readers aren't misled about the behavior being shipped.
function otpErrorText(raw) {
switch (raw) {
case 'Invalid OTP': return "That code didn't work.";
case 'OTP expired': return 'That code has expired.';
case 'Too many attempts':
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@packages/auth-service/src/routes/account-login.ts`:
- Around line 252-256: Update the route-local .btn-secondary rules in
packages/auth-service/src/routes/account-login.ts:252-256 and
packages/auth-service/src/routes/recovery.ts:516-520 to use the shared
--muted-foreground token with its existing fallback and the shared hover color,
preserving consistent themed styling across both routes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fe3666c3-b872-4f7a-9908-d26da69c7e3f
📒 Files selected for processing (7)
.changeset/human-sign-in-error-copy.md.changeset/unify-link-affordances.mdpackages/auth-service/src/__tests__/login-page.test.tspackages/auth-service/src/routes/account-login.tspackages/auth-service/src/routes/login-page.tspackages/auth-service/src/routes/recovery.tspackages/demo/src/lib/theme.ts
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (3)
packages/auth-service/src/routes/account-login.ts:256
- The hover color here is
#000, but the link-affordance convention inlogin-page.ts(and the PR description) says these standalone link-styled actions should darken to#1A130F. Also, the comment still references the old label "Resend code" even though the button now says "Send a new code".
/* Standalone action in its own row — see the link-affordance convention
documented in login-page.ts. No underline, darkens on hover. "Resend
code" here and on the sign-in page must not render differently. */
.btn-secondary { display: inline-block; color: #0f1828; background: none; border: none; font-size: 14px; cursor: pointer; text-decoration: none; border-radius: 4px; }
.btn-secondary:hover { color: #000; }
packages/auth-service/src/routes/recovery.ts:520
- The hover color here is
#000, but the link-affordance convention inlogin-page.ts(and the PR description) says these standalone link-styled actions should darken to#1A130F. Also, the comment still references the old label "Resend code" even though the UI now says "Send a new code".
/* Standalone action in its own row — see the link-affordance convention
documented in login-page.ts. No underline, darkens on hover. "Resend
code" here and on the sign-in page must not render differently. */
.btn-secondary { display: inline-block; margin-top: 12px; color: #0f1828; background: none; border: none; font-size: 14px; cursor: pointer; text-decoration: none; border-radius: 4px; }
.btn-secondary:hover { color: #000; }
packages/auth-service/src/routes/login-page.ts:656
- This comment refers to the "separate Resend button", but the UI label was changed to "Send a new code" (and this PR relies on the wording distinction). Updating the comment keeps it consistent with the current UI copy.
/* Inline action button rendered next to an OTP-expired error so
the user doesn't have to hunt for the separate Resend button.
Styled as a link rather than a button to make it visually
This was referenced Aug 4, 2026
The sign-in page styled clickable text four different ways, so the same
action rendered differently depending on where it appeared. "Resend
code" is underlined on the account-login and recovery pages but not on
the sign-in page; "Recover with backup email" is underlined while the
"Use different email" button beside it in the same cluster is not.
The anchor-vs-button split behind that inconsistency is real in the
markup but invisible to a user, since .btn-secondary elements are
buttons deliberately styled to look like links. The distinction that
actually carries meaning is positional:
STANDALONE (.btn-secondary, .recovery-link) sit in their own row,
where position and spacing already read as actionable, so
they need no underline.
IN-SENTENCE (.flash-action, .terms-link) are surrounded by prose and
inherit its colour, so the underline is their only marker
of being clickable and must stay.
That keeps intact the reasoning from 3d31876 for .flash-action, whose
underline is the whole of its affordance inside the error sentence.
Hover was also inverted between the two: .flash-action REMOVED its
underline on hover while .recovery-link kept its and darkened. An
affordance that appears or vanishes under the cursor is disorienting,
so every one of the four now darkens to #1A130F and no rule toggles
text-decoration in either direction.
Two accessibility gaps fixed along the way. .recovery-link,
.flash-action and .terms-link had no :focus-visible rule at all, so
keyboard users got only the UA default ring (none, for the buttons,
since the reset zeroes it); all three now match .btn-secondary's.
And .btn-secondary hardcoded #6b6b6b rather than reading
--muted-foreground — the same defect 6ed3d05 fixed for .divider, and
the last standalone action that ignored a branding.css override. It
now follows the token, which also lifts it from 5.02:1 to 5.41:1
against the card.
The demo theme overrode .recovery-link to textHint and .btn-secondary
to textMuted, which would have re-split the cluster under any theme;
both now resolve identically.
Tests pin each half of the convention, the hover rule and the focus
ring, so the four cannot silently drift apart again.
Implements beads issue atproto-n1s.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A rejected code rendered "Invalid OTP Send a new code" — better-auth's own wording pushed straight to the user, running into the recovery link beside it with only a space between them. "Invalid OTP" is a developer string: an unexplained acronym, no article, no indication of what to do next. It is also the single line a user actually reads when sign-in fails. Map the three known reasons to plain copy, and supply a full stop before the inline action when the message does not already end in one. Unrecognised errors pass through verbatim rather than collapsing into a generic apology: an unexpected failure that still names itself can be diagnosed from a screenshot, one that says "Something went wrong" cannot. verifyOtp() now returns the raw reason alongside the display text, and the expired-code branch tests the raw one. Matching the rewritten copy would have worked by luck today — "That code has expired." happens to contain "expir" — but would couple control flow to wording, so a later copy edit could silently change which recovery action is offered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The expiry scenario asserted the banner contained "OTP expired" — better-auth's raw string, which the previous commit stopped rendering. CI caught it: 74 scenarios passed, that one failed with Expected substring: "OTP expired" unexpected value "That code has expired. Send a new code" The unit tests missed it because they assert against the rendered page source, not against what a browser displays, so nothing local covered the gap between the two. Assert the user-facing string instead. Loosening the assertion to keep matching the old wording would have been worse: the scenario exists to prove the user is told something useful, and better-auth's wording is now an implementation detail behind otpErrorText(). The step pattern becomes a regex accepting "an" or "the" so the feature line reads as English with a message that starts with "That". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
aspiers
force-pushed
the
fix/link-affordances
branch
from
August 4, 2026 20:31
f8e950a to
bfd481d
Compare
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (3)
packages/auth-service/src/routes/login-page.ts:1152
- The punctuation separator between the error sentence and its inline action uses
/[.!?]$/againstmsgdirectly. Ifmsgever has trailing whitespace (e.g. a passed-through upstream message like "Invalid OTP "), the regex won’t match and the UI will append an extra period, producing awkward output like "Invalid OTP . Send a new code".
frag.appendChild(
document.createTextNode(/[.!?]$/.test(msg) ? ' ' : '. '),
);
packages/auth-service/src/routes/account-login.ts:258
- PR description says hover darkens to
#1A130Fconsistently across the sign-in screens, but this page’s.btn-secondary:hoveruses#000. If account-login is in-scope for the unified convention, this keeps hover behavior inconsistent.
.btn-secondary:hover { color: #000; }
packages/auth-service/src/routes/recovery.ts:521
- PR description calls out a consistent hover darkening (
#1A130F) for link-like actions across the sign-in screens, but recovery’s.btn-secondary:hoverstill uses#000. If recovery is in-scope for the unified convention, this remains inconsistent.
.btn-secondary:hover { color: #000; }
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.
Implements beads issue
atproto-n1s.Problem
The sign-in screens styled clickable text four different ways, so the same action rendered differently depending on where it appeared:
.btn-secondary(login-page)<button>.btn-secondary(account-login, recovery)<button>/<a>.recovery-link<a>.flash-action<button>.terms-link<a>As reported: "resend code sometimes appears as a clickable link with no underlining and then other times with underlining, and recover with backup mail is underlined, but Use different email isn't". Both halves of that report are covered — the first is the
.btn-secondarysplit across pages, the second is.recovery-linkversus its neighbours in the same cluster.Hover was inverted too:
.flash-actionremoved its underline on hover while.recovery-linkkept its and darkened.The convention
The anchor-vs-button split is real in the markup but invisible to a user, since
.btn-secondaryelements are buttons deliberately styled to look like links. The distinction that actually carries meaning is positional:.btn-secondary,.recovery-link) — in their own row, where position and spacing already read as actionable. No underline..flash-action,.terms-link) — surrounded by prose and inheriting its colour, so the underline is their only marker of being clickable. Underlined..recovery-linkwas the outlier and aligns to.btn-secondary, the more numerous treatment. This preserves the reasoning from 3d31876, where.flash-action's underline is the whole of its affordance inside the error sentence.Hover now darkens to
#1A130Fon all four, and no rule togglestext-decorationin either direction — an affordance that appears or vanishes under the cursor is disorienting.Screenshots
Both sides are live Railway previews — PR #223's, which still carries the old CSS, and this branch's own. The "before" shots predate #236, so they still show the button's old "Resend code" label; that rename is unrelated to this change.
Before: in one cluster, "Recover with backup email" is underlined while "Resend code" and "Use different email" directly above it are not — same position, same purpose, different affordance.
After: all three standalone actions match.
With a rejected code
Submitting a wrong code used to surface better-auth's own string verbatim, running straight into the recovery link: "Invalid OTP Send a new code". This PR also rewrites those messages for end users and separates them from the action.
The inline action stays underlined on both sides, and deliberately so: it sits inside the sentence and inherits its red, so the underline is the only thing marking it clickable. That is the distinction the convention preserves — position decides, not element type.
Before: the raw error runs into the action with only a space between them, and "Recover with backup email" is underlined while the buttons above it are not.
After: "That code didn't work." reads as a sentence, separated from the action, and the standalone cluster below is uniform.
Accessibility
.recovery-link,.flash-actionand.terms-linkhad no:focus-visiblerule, so keyboard users got only the UA default ring — none at all for the buttons, since the reset zeroes it. All three now match.btn-secondary's..btn-secondaryhardcoded#6b6b6binstead of reading--muted-foreground— the same defect 6ed3d05 fixed for.dividerin fix(auth): darken muted sign-in text to meet WCAG AA #232, and the last standalone action ignoring abranding.cssoverride. Following the token also lifts it from 5.02:1 to 5.41:1 against the card, so contrast improves rather than regresses.#666on#F8F8F8= 5.41:1 and on#E8E8E8= 4.69:1, both clearing WCAG AA.Demo theme
packages/demo/src/lib/theme.tsoverrode.recovery-linktotextHintand.btn-secondarytotextMuted, which would have re-split the cluster under any theme, and only.recovery-linkhad a hover override. Both selectors now resolve identically.Tests
13 new tests in
login-page.test.tspin each half of the convention plus the hover rule and focus ring, matching whole declaration blocks so a rule mentioning the property elsewhere cannot satisfy them. Each was verified to fail when its corresponding CSS change is reverted — no vacuous passes.Gates:
format:check,lint,typecheckall clean;pnpm test1102 passed across 73 files.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Style