feat: restyle login page to match Bluesky OAuth UI#10
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughLogin and recovery page renderers were migrated to a token-based two-panel layout; Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser
participant Auth as Auth Service
participant DB as Database
participant Email as Email/OTP Service
Browser->>Auth: GET /login (optional request_uri)
Auth-->>Browser: 200 HTML (renderLoginPage includes requestUri in client payload)
Browser->>Auth: POST /login/start (email, csrf, requestUri)
Auth->>DB: create OTP/session record
Auth->>Email: request OTP delivery
Email-->>User: deliver OTP (out-of-band)
Browser->>Auth: POST /login/verify (otp, csrf, requestUri)
Auth->>DB: verify OTP, create auth session
Auth-->>Browser: 302 Redirect (to requestUri or client redirect)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
- 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
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
- 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
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 |
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