feat: restyle login page to match Bluesky OAuth UI#10
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughLogin and recovery page renderers were rewritten into a token-based two-panel layout; Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant AuthService
participant Database
participant EmailService
Browser->>AuthService: GET /login (optional request_uri)
AuthService-->>Browser: 200 HTML (renderLoginPage includes requestUri in client JS)
Browser->>AuthService: POST /login/start (email, csrf, requestUri)
AuthService->>Database: create OTP/session record
AuthService->>EmailService: send OTP to user
EmailService-->>User: deliver OTP (out-of-band)
Browser->>AuthService: POST /login/verify (otp, csrf, requestUri)
AuthService->>Database: verify OTP, create auth session
AuthService-->>Browser: 302 Redirect (to requestUri or default)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/auth-service/src/routes/login-page.ts`:
- Around line 261-703: The file contains formatting issues flagged by Prettier
(CI failure); run your project's Prettier formatter against the login-page.ts
file (or run the repo-wide formatter) to reformat the CSS block including
selectors like :root, .input-container, .btn-primary, .title-panel, and
.otp-input, then stage and commit the formatted file so the CI Prettier check
passes; ensure you use the repo Prettier config (or npm/yarn script like
formatting or prettier --write) and push the commit.
- Around line 719-725: The markup and runtime code use inline style.display
toggles for visibility (e.g., the <div id="error-msg" class="admonition error"
style="display:none;"> and the sibling <span class="admonition-icon
admonition-icon-success" style="display:none;">, plus the runtime mutations that
set element.style.display around the login page logic), which violates the
project rule to use CSS classes; replace the static inline display attributes
with a default visibility class (e.g., add/remove a "hidden" or "inactive" class
on the `#error-msg` container and the .admonition-icon-success element) and change
any JS that does element.style.display = ... (the code that currently toggles
display at the login handling functions) to use element.classList.add/remove or
toggle with the chosen class so visibility is driven by CSS classes rather than
inline styles.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2bf36d5b-5f73-4de5-8680-acf2c50244a4
📒 Files selected for processing (2)
.beads/issues.jsonlpackages/auth-service/src/routes/login-page.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/auth-service/src/routes/recovery.ts (1)
362-730: Consider extractinggetBaseCSS()to a shared auth-page style module.The token/layout/admonition CSS here duplicates the same style block already present in
packages/auth-service/src/routes/login-page.ts(see context snippet lines 260-700). Centralizing this will prevent style drift between login and recovery pages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/recovery.ts` around lines 362 - 730, The getBaseCSS() block in recovery.ts duplicates the same CSS used in login-page.ts; extract it into a single shared style module (e.g., export a constant or function like BASE_AUTH_CSS or getBaseCSS from a new auth-page-styles module) and replace the local getBaseCSS() in both recovery.ts and login-page.ts with imports from that shared module (update any references to getBaseCSS to use the imported symbol and remove the duplicate definitions).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/auth-service/src/routes/recovery.ts`:
- Line 315: Replace the inline style on the form element in recovery.ts (the
<form method="POST" action="/auth/recover" style="display:inline;">) with a CSS
class (e.g., class="inline-form") and add the corresponding rule in the
component/site stylesheet (e.g., .inline-form { display: inline; }) or use an
existing utility class; update the form element to use that class and remove the
style attribute so styling is centralized in CSS.
- Around line 298-307: The OTP input (input id="code", name="code", class
"otp-input") lacks an explicit accessible label; add a visible or
visually-hidden <label for="code">One-time code</label> or an equivalent
aria-labelledby referencing a label element, ensuring the label text describes
the purpose (e.g., "One-time code" or "Verification code") and keep the input's
id "code" to maintain association; update the markup near the input-inner block
so screen readers receive a proper accessible name.
---
Nitpick comments:
In `@packages/auth-service/src/routes/recovery.ts`:
- Around line 362-730: The getBaseCSS() block in recovery.ts duplicates the same
CSS used in login-page.ts; extract it into a single shared style module (e.g.,
export a constant or function like BASE_AUTH_CSS or getBaseCSS from a new
auth-page-styles module) and replace the local getBaseCSS() in both recovery.ts
and login-page.ts with imports from that shared module (update any references to
getBaseCSS to use the imported symbol and remove the duplicate definitions).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 50b85954-654b-4a8e-a7c7-5981ac36da1f
📒 Files selected for processing (2)
.beads/issues.jsonlpackages/auth-service/src/routes/recovery.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/auth-service/src/routes/login-page.ts (1)
211-213: Hardenlogo_urihandling with protocol allowlisting.
escapeHtml()protects HTML context, butlogo_uriis still metadata-controlled. Consider validating URL protocol (https:; optionallyhttp:in dev) before rendering the<img>and dropping invalid values.🔒 Suggested hardening patch
- const logoHtml = b.logo_uri - ? `<img src="${escapeHtml(b.logo_uri)}" alt="${escapeHtml(appName)}" class="client-logo" aria-hidden="true">` - : '' + const logoUri = (() => { + if (!b.logo_uri) return null + try { + const u = new URL(b.logo_uri) + if (u.protocol === 'https:') return u.toString() + if (process.env.NODE_ENV === 'development' && u.protocol === 'http:') return u.toString() + return null + } catch { + return null + } + })() + + const logoHtml = logoUri + ? `<img src="${escapeHtml(logoUri)}" alt="${escapeHtml(appName)}" class="client-logo" aria-hidden="true">` + : ''🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/login-page.ts` around lines 211 - 213, The generated logoHtml currently uses b.logo_uri directly (escaped) which can still allow unsafe protocols; before constructing logoHtml, validate b.logo_uri using the URL constructor (or a safe URL-parse helper) and only allow protocols like "https:" (optionally "http:" behind a dev flag), otherwise treat it as missing/invalid and set logoHtml to ''. Update the code around logoHtml/escapeHtml to perform this protocol check (e.g., validate b.logo_uri, ensure it starts with an allowed protocol, then call escapeHtml(appName) and build the <img> string) so invalid or non-allowed-scheme URIs are dropped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/auth-service/src/routes/recovery.ts`:
- Around line 317-323: The resend-code form is nested inside the verify form
which produces invalid HTML; update the template in recovery.ts (where the
verify form is rendered) so the Resend Code form (the form with hidden csrf,
request_uri, email and the "Resend Code" submit button) is moved entirely
outside the verify/<verify form> block or converted to a separate button/link
that performs POST to /auth/recover (e.g., a standalone form sibling to the
verify form or an anchor that triggers a POST), ensuring no <form> tags are
nested and that the inputs (csrf, request_uri, email) remain included for the
resend action.
---
Nitpick comments:
In `@packages/auth-service/src/routes/login-page.ts`:
- Around line 211-213: The generated logoHtml currently uses b.logo_uri directly
(escaped) which can still allow unsafe protocols; before constructing logoHtml,
validate b.logo_uri using the URL constructor (or a safe URL-parse helper) and
only allow protocols like "https:" (optionally "http:" behind a dev flag),
otherwise treat it as missing/invalid and set logoHtml to ''. Update the code
around logoHtml/escapeHtml to perform this protocol check (e.g., validate
b.logo_uri, ensure it starts with an allowed protocol, then call
escapeHtml(appName) and build the <img> string) so invalid or non-allowed-scheme
URIs are dropped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 939a3e51-ef71-42ac-83ac-54e5dadce691
📒 Files selected for processing (3)
.beads/issues.jsonlpackages/auth-service/src/routes/login-page.tspackages/auth-service/src/routes/recovery.ts
Kzoeps
added a commit
that referenced
this pull request
Mar 4, 2026
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/auth-service/src/routes/recovery.ts (1)
361-731: Consider extracting shared CSS to a common utility.The
getBaseCSS()function contains ~370 lines of CSS that appears to be duplicated inlogin-page.ts. Both files define identical token systems, layout classes (.title-panel,.form-panel,.input-container), and component styles (.admonition,.btn-primary).Consider extracting this to a shared module (e.g.,
src/ui/base-css.ts) that both files can import. This would:
- Ensure visual consistency between login and recovery pages
- Reduce maintenance burden when updating styles
- Make it easier to catch accidental divergence
♻️ Example extraction
// src/ui/base-css.ts export function getBaseCSS(): string { return `/* ... shared CSS ... */` }// recovery.ts +import { getBaseCSS } from '../ui/base-css.js' -function getBaseCSS(): string { ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/recovery.ts` around lines 361 - 731, The getBaseCSS() function in recovery.ts duplicates ~370 lines of CSS already present in login-page.ts; extract the shared CSS into a new module (e.g., src/ui/base-css.ts) that exports function getBaseCSS(): string returning the common CSS, then replace the inline getBaseCSS implementations in both recovery.ts and login-page.ts with imports from that module (update both files to import { getBaseCSS } from 'src/ui/base-css' and remove the duplicated CSS blocks) so both pages share the same canonical stylesheet and avoid divergence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/auth-service/src/routes/recovery.ts`:
- Around line 361-731: The getBaseCSS() function in recovery.ts duplicates ~370
lines of CSS already present in login-page.ts; extract the shared CSS into a new
module (e.g., src/ui/base-css.ts) that exports function getBaseCSS(): string
returning the common CSS, then replace the inline getBaseCSS implementations in
both recovery.ts and login-page.ts with imports from that module (update both
files to import { getBaseCSS } from 'src/ui/base-css' and remove the duplicated
CSS blocks) so both pages share the same canonical stylesheet and avoid
divergence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e8680d0-7cc3-41be-903c-062c1984dc8c
📒 Files selected for processing (2)
.beads/issues.jsonlpackages/auth-service/src/routes/recovery.ts
Contributor
Author
|
@aspiers done. realized also that right now its mostly hardcoded. but with the css injection trusted clients should be able to change it. but it would also be nice to have some of it be configurable through the env vars like how bsky pds does it. but that would requires a lot more code and maybe will do it as another pr ( will create an issue for it as well ). and also another issue for the authorization page. I guess theres also css repetition will fix that i the morning |
aspiers
force-pushed
the
bsky-signin-page
branch
from
35e1074 to
2c08d68
Compare
Pull Request Test Coverage Report for Build 23422469672Details
💛 - Coveralls |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/auth-service/src/routes/login-page.ts`:
- Around line 216-218: The logoHtml construction suppresses assistive tech by
setting aria-hidden="true"; update the logic in the logoHtml/ b.logo_uri block
to either remove aria-hidden from the <img> so the alt (escapeHtml(appName)) is
read, or render the client name as visible text adjacent to the image (e.g.,
include a visually visible span with escapeHtml(appName) next to the <img>).
Ensure the alt remains set to escapeHtml(appName) and apply the same change to
the other occurrence around lines 719-721.
In `@packages/auth-service/src/routes/recovery.ts`:
- Line 257: The recovery flow drops client branding because the recovery links
rebuild "/oauth/authorize" with only request_uri; update the links generation in
recovery.ts (the anchor that currently outputs
/oauth/authorize?request_uri=${encodedUri}) to preserve client_id (e.g., append
&client_id=${encodeURIComponent(req.query.client_id || existingFlow?.clientId)})
or change the authorize handler to prefer existingFlow.clientId when request_uri
is used; locate the link construction and the authorize route logic
(createLoginPageRouter and any usage of existingFlow.clientId) and ensure either
the client_id query param is propagated through recovery or the authorize route
resolves client id from the existingFlow object so branding is preserved after
recovery.
- Around line 308-335: The "Resend Code" submit button currently triggers
browser validation on the verify form (blocking POST to /auth/recover if `#code`
is empty); update the button element (the submit button with text "Resend Code"
/ attributes class="link-btn" formaction="/auth/recover" formmethod="POST") to
include the formnovalidate attribute so clicking it bypasses form validation and
allows the resend POST to proceed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9c6a97f2-0fc6-4251-9fb5-f647d9e6abd4
📒 Files selected for processing (3)
.beads/issues.jsonlpackages/auth-service/src/routes/login-page.tspackages/auth-service/src/routes/recovery.ts
aspiers
force-pushed
the
bsky-signin-page
branch
from
2c08d68 to
6aa4413
Compare
aspiers
force-pushed
the
bsky-signin-page
branch
from
6aa4413 to
4de6764
Compare
aspiers
force-pushed
the
bsky-signin-page
branch
from
4de6764 to
00a638c
Compare
keep generate otp code as is for now fix: linting issues
- Split-panel layout: left title panel with 'Sign in with Certified', right form panel - CSS custom property token system with 15-stop contrast scale and dark mode - atproto-style input components (InputContainer pattern with icons and focus rings) - Admonition error/success messages with proper contrast (WCAG AA) - Logo conditional on client metadata logo_uri (no hardcoded static assets) - Recovery link passes actual PAR request_uri - Conditional autofocus (email step vs OTP step) - Resend button disabled during auto-send to prevent race condition - Responsive: stacks vertically on mobile (<768px)
- Split-panel layout matching login page design - Same CSS token system, input components, and admonition errors - Form-actions row: 'Back to sign in' left, submit button right (auto-width) - OTP label, autocomplete='email', proper field labels - Dark mode support with inverted contrast scale - Class-based visibility (.inline-form) instead of inline styles
- Add .hidden { display: none !important } utility class
- Replace all style="display:none" attributes with class="hidden"
- Replace all element.style.display JS mutations with classList.add/remove
- Per AGENTS.md: use CSS classes for visibility, not inline styles
aspiers
force-pushed
the
bsky-signin-page
branch
from
00a638c to
4a11e5b
Compare
Contributor
Author
|
@aspiers i think it would be better to redo this based on the existing code. rather than fix the merge?. i think this might also impact the css injection as well since it was done before that and the ui is hardcoded |
|
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
2 participants
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
GET /oauth/authorize) from a basic centered-card layout to the Bluesky@atproto/oauth-provider-uisplit-panel designChanges
Single file changed:
packages/auth-service/src/routes/login-page.ts(onlyrenderLoginPage()— route handler untouched)Layout
Branding
<h1>text (no static SVG/PNG assets)b.logo_urifrom OAuth client metadata (no hardcoded/static/references)Bug fixes included
request_uriinstead ofpdsPublicUrlvar(--color-text-light)) instead of hardcoded graymin-width: 100%instead of100vw(fixes horizontal scrollbar on Windows/Linux)autofocus— email input on email step, OTP input on OTP stepWhat's preserved
renderLoginPagesignature unchanged (onlyrequestUriadded for recovery link fix)Summary by CodeRabbit
New Features
Style
Bug Fixes