Restyle login page with white card layout + Certified branding#110
Open
s-adamantine wants to merge 8 commits intomainfrom
Open
Restyle login page with white card layout + Certified branding#110s-adamantine wants to merge 8 commits intomainfrom
s-adamantine wants to merge 8 commits intomainfrom
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:
📝 WalkthroughWalkthroughThis PR introduces a theme-driven CSS system for the login page with redesigned card and typography, replaces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🦋 Changeset detectedLatest commit: dd5faf4 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 |
|
🚅 Deployed to the ePDS-pr-110 environment in ePDS
|
Coverage Report for CI Build 25102591635Coverage increased (+0.1%) to 48.544%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions1 previously-covered line in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, reopen this pull request to trigger a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/auth-service/src/routes/preview.ts (1)
74-100:⚠️ Potential issue | 🟡 MinorValidate
AUTH_PREVIEW_DEFAULT_CLIENT_IDbefore echoing it back into the page.If this env var is typoed,
resolveClientMetadata()falls into the catch block but we still return the bad string asclientId. The preview login page then uses that value to derive the handle-sign-in redirect origin, so a misconfigured preview env breaks the toggle instead of cleanly falling back tohttps://preview.example/....Proposed fix
- const envDefault = process.env.AUTH_PREVIEW_DEFAULT_CLIENT_ID?.trim() + const rawEnvDefault = process.env.AUTH_PREVIEW_DEFAULT_CLIENT_ID?.trim() + let envDefault: string | undefined + if (rawEnvDefault) { + try { + envDefault = new URL(rawEnvDefault).toString() + } catch { + logger.warn( + { clientId: rawEnvDefault }, + 'Preview: ignoring invalid AUTH_PREVIEW_DEFAULT_CLIENT_ID', + ) + } + } const fakeDefault = 'https://preview.example/client-metadata.json'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/auth-service/src/routes/preview.ts` around lines 74 - 100, The code currently assigns envDefault to clientId without validating it, causing a bad string to be echoed if resolveClientMetadata fails; change the flow so you only use envDefault as the effective clientId after it successfully resolves. Specifically, when envDefault exists, attempt to resolveClientMetadata(envDefault, { noCache: true }) first (or validate it as a well-formed client metadata URL) and only set clientId = envDefault if resolution/validation succeeds; on failure fall back to the fakeDefault return path and avoid returning the original bad envDefault. Keep usage of getClientCss(clientId, metadata, trustedClients) and the existing logger.warn for real resolution errors.
🧹 Nitpick comments (1)
packages/auth-service/src/routes/login-page.ts (1)
71-71: Consider usingreplaceAll()for clarity.While
replace()with a global regex flag works correctly,replaceAll()is more explicit about the intent to replace all occurrences and doesn't require the/gflag.🔧 Suggested fix
- .replace(/fill="#726A60"/g, 'fill="currentColor"') + .replaceAll('fill="#726A60"', 'fill="currentColor"')🤖 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` at line 71, Replace the use of .replace(/fill="#726A60"/g, 'fill="currentColor"') with a clearer .replaceAll('fill="#726A60"', 'fill="currentColor"') call so it's explicit that all occurrences are replaced; locate the occurrence of .replace(/fill="#726A60"/g, 'fill="currentColor"') in the login-page code and swap to .replaceAll(...) ensuring the input is a plain string (not a RegExp) and behavior remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/preview-default-client-id.md:
- Around line 13-15: The fenced code block containing the environment string
AUTH_PREVIEW_DEFAULT_CLIENT_ID is missing a language tag (MD040); update the
fence by adding a language identifier (use "text") after the opening ``` so the
block becomes ```text and keep the existing content unchanged to satisfy
markdownlint; locate the block in the .changeset/preview-default-client-id.md
file where AUTH_PREVIEW_DEFAULT_CLIENT_ID=... appears and apply this small edit.
In `@packages/demo/.env.example`:
- Around line 44-48: Update the comment for EPDS_CLIENT_THEME to correctly
describe the unset behavior: mention that when EPDS_CLIENT_THEME is unset
getTheme() returns null, the demo pages use their own built-in light theme (not
the auth-service “Certified” styling), and the only auth-service effect is that
client-metadata.json stops shipping branding CSS; reference EPDS_CLIENT_THEME
and getTheme() in the comment for clarity.
In `@packages/demo/src/lib/theme.ts`:
- Around line 246-305: The auth-service CSS custom properties and .otp-box focus
override are only present in the cream preset; update the injectedCss for the
ocean and amber presets to include the same :root variable block (--page-bg,
--card-bg, --input-bg, --input-border, --muted-foreground, --card-border,
--btn-secondary-border, --focus-border) and the .otp-box:focus { border-color:
`#C8996C` !important; } rule so those themes apply the restyled login/OTP styling;
locate the injectedCss arrays for the ocean and amber theme entries (same
structure as the cream entry) and add the matching :root declaration and
.otp-box override alongside the existing auth-service selectors.
---
Outside diff comments:
In `@packages/auth-service/src/routes/preview.ts`:
- Around line 74-100: The code currently assigns envDefault to clientId without
validating it, causing a bad string to be echoed if resolveClientMetadata fails;
change the flow so you only use envDefault as the effective clientId after it
successfully resolves. Specifically, when envDefault exists, attempt to
resolveClientMetadata(envDefault, { noCache: true }) first (or validate it as a
well-formed client metadata URL) and only set clientId = envDefault if
resolution/validation succeeds; on failure fall back to the fakeDefault return
path and avoid returning the original bad envDefault. Keep usage of
getClientCss(clientId, metadata, trustedClients) and the existing logger.warn
for real resolution errors.
---
Nitpick comments:
In `@packages/auth-service/src/routes/login-page.ts`:
- Line 71: Replace the use of .replace(/fill="#726A60"/g, 'fill="currentColor"')
with a clearer .replaceAll('fill="#726A60"', 'fill="currentColor"') call so it's
explicit that all occurrences are replaced; locate the occurrence of
.replace(/fill="#726A60"/g, 'fill="currentColor"') in the login-page code and
swap to .replaceAll(...) ensuring the input is a plain string (not a RegExp) and
behavior remains identical.
🪄 Autofix (Beta)
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
Run ID: 245a3363-f6e9-46b7-8110-75080372b2c5
⛔ Files ignored due to path filters (2)
packages/auth-service/public/certified-brandmark.svgis excluded by!**/*.svgpackages/auth-service/public/certified-text-monochrome.svgis excluded by!**/*.svg
📒 Files selected for processing (9)
.changeset/preview-default-client-id.md.env.exampledocs/configuration.mdpackages/auth-service/.env.examplepackages/auth-service/src/routes/login-page.tspackages/auth-service/src/routes/preview.tspackages/demo/.env.examplepackages/demo/src/app/components/PageShell.tsxpackages/demo/src/lib/theme.ts
s-adamantine
force-pushed
the
restyle-login-page
branch
from
3dde305 to
746ff7e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/demo/src/lib/theme.ts (1)
227-306: Well-structured cream theme that follows established patterns.The warm cream palette is cohesive, and the
injectedCsscomprehensively covers both Provider-UI consent page and auth-service login/OTP/recovery markup. Good coverage of both.otp-box:focus(new multi-box OTP) and.otp-input:focus(legacy) selectors.Regarding the static analysis warnings about escaped backslashes (e.g.,
.md\\:bg-slate-100): this pattern is already used inoceanandamberthemes. If you'd like to address it, consider refactoring all three presets to useString.rawconsistently for the Tailwind selector strings—but this is optional and low-priority since the current escaping works correctly.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/demo/src/lib/theme.ts` around lines 227 - 306, Summary: optional static-analysis cleanup for escaped backslashes in cream.injectedCss. If you want to address the warnings, update the cream theme's injectedCss (the "cream" Theme object and its injectedCss property) to use raw string literals for the Tailwind selectors (e.g., replace escaped sequences like ".md\\:bg-slate-100" with String.raw templates) and apply the same change consistently to the other presets (ocean, amber) so selectors remain identical while removing backslash-escape warnings; otherwise no functional change is required.packages/auth-service/src/routes/login-page.ts (2)
71-71: Consider usingreplaceAll()for clarity.The regex with
gflag works correctly, butreplaceAll()expresses intent more clearly. Per SonarCloud hint.♻️ Suggested change
-.replace(/fill="#726A60"/g, 'fill="currentColor"') +.replaceAll('fill="#726A60"', 'fill="currentColor"')🤖 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` at line 71, Replace the regex-based call .replace(/fill="#726A60"/g, 'fill="currentColor"') with the clearer string-based .replaceAll('fill="#726A60"', 'fill="currentColor"') in the code that processes the SVG (the place where .replace is invoked) so intent is explicit and SonarCloud's suggestion is satisfied; ensure the runtime target supports replaceAll or add a small polyfill/alternative if needed.
24-26: Node built-in imports should be placed before external packages.Per coding guidelines, imports should be ordered: (1) Node built-ins with
node:prefix, (2) External packages. These new imports should be moved above theexpressimport on line 22.♻️ Suggested import reordering
+import { readFileSync } from 'node:fs' +import path from 'node:path' +import { fileURLToPath } from 'node:url' import { Router, type Request, type Response } from 'express' import { randomBytes } from 'node:crypto' -import { readFileSync } from 'node:fs' -import path from 'node:path' -import { fileURLToPath } from 'node:url'Note:
node:cryptoon line 23 should also move to the top group with the other Node built-ins.🤖 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 24 - 26, Reorder the imports so Node built-ins come before external packages: move the node:fs import (readFileSync), node:path import (path), node:url import (fileURLToPath) and node:crypto import to the top import group, placing them above the express import; keep external package imports (e.g., express) after the built-ins and maintain consistent grouping/ordering and spacing between groups.
🤖 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/login-page.ts`:
- Line 71: Replace the regex-based call .replace(/fill="#726A60"/g,
'fill="currentColor"') with the clearer string-based
.replaceAll('fill="#726A60"', 'fill="currentColor"') in the code that processes
the SVG (the place where .replace is invoked) so intent is explicit and
SonarCloud's suggestion is satisfied; ensure the runtime target supports
replaceAll or add a small polyfill/alternative if needed.
- Around line 24-26: Reorder the imports so Node built-ins come before external
packages: move the node:fs import (readFileSync), node:path import (path),
node:url import (fileURLToPath) and node:crypto import to the top import group,
placing them above the express import; keep external package imports (e.g.,
express) after the built-ins and maintain consistent grouping/ordering and
spacing between groups.
In `@packages/demo/src/lib/theme.ts`:
- Around line 227-306: Summary: optional static-analysis cleanup for escaped
backslashes in cream.injectedCss. If you want to address the warnings, update
the cream theme's injectedCss (the "cream" Theme object and its injectedCss
property) to use raw string literals for the Tailwind selectors (e.g., replace
escaped sequences like ".md\\:bg-slate-100" with String.raw templates) and apply
the same change consistently to the other presets (ocean, amber) so selectors
remain identical while removing backslash-escape warnings; otherwise no
functional change is required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0b58c27b-1134-437b-b6d2-ff553e3ab603
⛔ Files ignored due to path filters (2)
packages/auth-service/public/certified-brandmark.svgis excluded by!**/*.svgpackages/auth-service/public/certified-text-monochrome.svgis excluded by!**/*.svg
📒 Files selected for processing (9)
.changeset/preview-default-client-id.md.env.exampledocs/configuration.mdpackages/auth-service/.env.examplepackages/auth-service/src/routes/login-page.tspackages/auth-service/src/routes/preview.tspackages/demo/.env.examplepackages/demo/src/app/components/PageShell.tsxpackages/demo/src/lib/theme.ts
✅ Files skipped from review due to trivial changes (4)
- packages/demo/src/app/components/PageShell.tsx
- docs/configuration.md
- packages/demo/.env.example
- .env.example
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/auth-service/.env.example
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e/support/otp.ts (1)
2-2: Avoid hardcoding “6-box” in the helper docsThe helper logic is dynamic; the comment should be too, to avoid future confusion when OTP_LENGTH changes.
Based on learnings: "OTP codes are configurable via the OTP_LENGTH environment variable (range 4–12...)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/support/otp.ts` at line 2, Update the helper comment in e2e/support/otp.ts to remove the hardcoded “6-box” phrasing and instead describe that the helper fills the segmented OTP input dynamically based on the configured length (OTP_LENGTH env var); reference the helper that fills the auth-service login page’s segmented OTP input and mention the OTP_LENGTH environment variable (valid range 4–12) so the doc reflects the code’s dynamic behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e/support/otp.ts`:
- Around line 18-24: The fillOtp routine currently clears inputs by the rendered
box count (boxes.count()) but fills using otp.length, which can cause
out-of-range locator errors or missed auto-submit behavior; update the filling
logic in the fillOtp function to compute a fillCount = Math.min(count,
otp.length) and iterate only up to fillCount when calling
boxes.nth(i).fill(...), and optionally handle/log the case where otp.length !==
count so callers/maintainers know a mismatch occurred.
---
Nitpick comments:
In `@e2e/support/otp.ts`:
- Line 2: Update the helper comment in e2e/support/otp.ts to remove the
hardcoded “6-box” phrasing and instead describe that the helper fills the
segmented OTP input dynamically based on the configured length (OTP_LENGTH env
var); reference the helper that fills the auth-service login page’s segmented
OTP input and mention the OTP_LENGTH environment variable (valid range 4–12) so
the doc reflects the code’s dynamic behavior.
🪄 Autofix (Beta)
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
Run ID: 2271e248-6f8f-43c6-89ae-23f431512c3b
📒 Files selected for processing (6)
e2e/step-definitions/auth.steps.tse2e/step-definitions/client-branding.steps.tse2e/step-definitions/consent.steps.tse2e/step-definitions/session-reuse.steps.tse2e/support/flows.tse2e/support/otp.ts
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/auth-service/src/routes/login-page.ts (1)
735-748:⚠️ Potential issue | 🟡 MinorReset the message styling after a successful resend.
This branch turns the shared error box green, but the normal error path never restores the default red styles. Any later real error on the same page will still render as a success message.
Suggested fix
function showError(msg) { errorEl.textContent = msg; errorEl.style.display = 'block'; + errorEl.style.color = '#dc3545'; + errorEl.style.background = '#fdf0f0'; } ... - showError('Code resent!'); - errorEl.style.color = '#28a745'; - errorEl.style.background = '#f0fff4'; + errorEl.textContent = 'Code resent!'; + errorEl.style.display = 'block'; + errorEl.style.color = '#28a745'; + errorEl.style.background = '#f0fff4';🤖 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 735 - 748, The resend handler currently sets the shared error element (errorEl) to success styling on success but never restores default error styling for subsequent real errors; update the click listener attached to document.getElementById('btn-resend') (the async function that calls sendOtp and uses clearError/showError) so that after a success it sets a flag or attaches the original default styles back to errorEl (e.g., store original color/background before changing them) and ensure showError restores those default red error styles when called with an actual error; in short, capture and restore errorEl's original styles in the resend success branch and make showError reset to those defaults when showing an error.
🧹 Nitpick comments (1)
e2e/step-definitions/account-recovery.steps.ts (1)
189-193: Update the note about#recovery-link.The direct
page.goto()is reasonable here, but this comment now documents the wrong behavior:packages/auth-service/src/routes/login-page.tsstill rendersid="recovery-link"and shows it by default unless client CSS sets--recovery-link-display: none.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/step-definitions/account-recovery.steps.ts` around lines 189 - 193, Update the inline comment to reflect the true behavior: the login page still renders an element with id="recovery-link" (see packages/auth-service/src/routes/login-page.ts) and it is visible by default unless client CSS sets --recovery-link-display: none; keep the note that direct page.goto() is acceptable and that the auth_flow cookie carries the real request_uri for back-link resolution, but remove the incorrect statement that the recovery link was removed from the page and instead state that it is present but may be hidden by client CSS.
🤖 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 473-480: The OTP input template renders numeric-specific
labels/placeholders; update the rendering logic that builds each `<input
class="otp-box" data-slot="...">` (the loop using opts.otpLength and inputProps)
to make labels and placeholders charset-aware by checking opts.otpCharset (e.g.,
'numeric' vs 'alphanumeric') and: set aria-label to "Digit N" and placeholder
"0" for numeric, but "Character N" and placeholder "A" (or a generic letter) for
alphanumeric; ensure inputmode/autocapitalize remain driven by inputProps and
that the conditional autocomplete behavior stays the same.
- Around line 342-345: The client-supplied b.brand_color is being interpolated
into CSS without CSS-context validation (escapeHtml is insufficient); validate
b.brand_color against a safe color pattern (e.g. only accept hex colors like
/^#([A-Fa-f0-9]{3}|[A-Fa-f0-9]{6})$/) and if it fails, fall back to the default
'#1A130F'; update the assignment to brandColor (and any other uses in the later
block around the logo/branding code) to perform this check before interpolation,
leaving escapeHtml for HTML attributes like logoHtml but not as the CSS
sanitizer.
- Around line 506-507: The code currently assigns var clientId =
${JSON.stringify(opts.clientId)} and uses it to build a handle redirect target
client-side, which allows open-redirect if clientId came from a user-controlled
query; instead, stop using an unvalidated opts.clientId in the client bundle:
render a trusted, server-validated handle origin (e.g., a new server-side
variable like validatedHandleOrigin) into the template and use that value for
the redirect, and disable the handle-redirect path when no validatedHandleOrigin
is available; update the code that sets clientId/requestUri in
routes/login-page.ts to read from that server-side validated value and ensure
any branch that builds window.location.href for handle-login checks for its
presence before performing a redirect.
---
Outside diff comments:
In `@packages/auth-service/src/routes/login-page.ts`:
- Around line 735-748: The resend handler currently sets the shared error
element (errorEl) to success styling on success but never restores default error
styling for subsequent real errors; update the click listener attached to
document.getElementById('btn-resend') (the async function that calls sendOtp and
uses clearError/showError) so that after a success it sets a flag or attaches
the original default styles back to errorEl (e.g., store original
color/background before changing them) and ensure showError restores those
default red error styles when called with an actual error; in short, capture and
restore errorEl's original styles in the resend success branch and make
showError reset to those defaults when showing an error.
---
Nitpick comments:
In `@e2e/step-definitions/account-recovery.steps.ts`:
- Around line 189-193: Update the inline comment to reflect the true behavior:
the login page still renders an element with id="recovery-link" (see
packages/auth-service/src/routes/login-page.ts) and it is visible by default
unless client CSS sets --recovery-link-display: none; keep the note that direct
page.goto() is acceptable and that the auth_flow cookie carries the real
request_uri for back-link resolution, but remove the incorrect statement that
the recovery link was removed from the page and instead state that it is present
but may be hidden by client CSS.
🪄 Autofix (Beta)
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
Run ID: d49ec4f8-0f51-4a5f-9b55-f97425eee997
📒 Files selected for processing (4)
.changeset/login-page-theming.mde2e/step-definitions/account-recovery.steps.tse2e/step-definitions/client-branding.steps.tspackages/auth-service/src/routes/login-page.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/login-page-theming.md
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/step-definitions/client-branding.steps.ts
9 tasks
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
White card on muted grey, pill buttons, segmented 6-box OTP input with paste/arrow/backspace/auto-submit, "Powered by Certified" footer, Certified brandmark as fallback top logo. Adds an ATProto/Bluesky toggle that swaps the email form into handle-entry mode and submits to <client-origin>/api/oauth/login?handle=X — the client then resolves the handle to its PDS and starts a fresh PAR. Off-PDS handles end up on their own PDS's auth screen. Removes the "Recover with backup email" link from the page. Hardcodes the Terms/Privacy notice to Certified's terms (the auth server is always Certified's regardless of which client started the flow), with both links underlined and pointing at certified.app. Hides the Terms/Privacy notice on the OTP step. h1 reads "Sign in" on the email step, "Enter your code" on the OTP step. Exposes CSS custom properties for trusted-client theming: --page-bg, --card-bg, --input-bg, --input-border, --muted-foreground, --card-border, --btn-secondary-border, --focus-border Defaults stay neutral grey; trusted clients can override any of these via injected branding.css. Inlines the Certified wordmark SVG so the "Powered by" footer can tint via currentColor.
The login-page restyle replaced the single visible OTP input with a
hidden #code field plus six .otp-box slots. page.fill('#code', ...)
resolves the locator but Playwright refuses to fill a type=hidden
input, timing out every OTP-step scenario.
Add a fillOtp(page, otp) helper that clears any stale digits, then
types one per box. The page's input handler auto-submits on the last
digit, so the explicit verify-button click is dropped at every call
site. The 'incorrect OTP {int} times' loop registers waitForResponse
before fillOtp since the submit fires from inside the helper.
Also reworks buildIncorrectOtpCode's DOM-inference fallback: length
now comes from .otp-box count and charset from a single box's
inputmode, since the hidden #code carries no maxlength / pattern.
The recovery flow's #code input is unchanged and untouched here.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Restores the "Recover with backup email" link on the OTP step, shown by default, controlled by the new --recovery-link-display CSS var so clients can hide it via branding.css. E2E: recovery flows navigate to /auth/recover directly (more robust than depending on the link's visibility, which is now client-configurable). DEFAULT_LOGIN_BG_RGB updated to match the new --page-bg default (#E8E8E8). Adds a changeset documenting the login-page CSS theming surface. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Validate brand_color as a hex literal before interpolating into <style>; escapeHtml() doesn't sanitise CSS context, so a value containing `;` or `}` could break out of the declaration. - OTP slot placeholder/aria-label now switch to "A"/"Character" when otpCharset === 'alphanumeric', matching the configured charset. - Clarify EPDS_CLIENT_THEME unset behaviour in packages/demo/.env.example (getTheme() returns null; demo uses its own light look; no branding CSS shipped → auth-service renders default Certified styling). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The atproto-login-button feature was authored before PR #110's restyle changed the email label from "Email address" to "Enter your email address". Update the assertion to match the new copy. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Autonomous loop done — ready for review/mergeCI: 15/15 green (e2e finished in 8m8s). Branch tip: Changes since last user touch:
Unresolved review threads: 0. Ready for review/merge. |
Replace the hardcoded https://certified.app/terms and /privacy URLs in the login page with values read from PDS_TERMS_OF_SERVICE_URL / PDS_PRIVACY_POLICY_URL — the same env vars upstream PDS already consumes, so deployments configure them once. The terms line is omitted entirely when either var is missing (strict; avoids dead half-links). Optional PDS_LEGAL_ENTITY_NAME controls the possessive in the line: "By signing in, you agree to <name>'s Terms of Use and Privacy Policy." When unset, falls back to "By signing in, you agree to the Terms of Use and Privacy Policy." Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Upstream @atproto/oauth-provider-ui renders the consent + chooser pages with purple-on-white defaults that don't match the rest of an ePDS deployment. Inject a neutral default stylesheet (muted grey page bg, light card surface, dark primary button) into every /oauth/authorize response so unbranded deployments render coherently with auth-service's restyled login page. Trusted-client branding.css is now layered on top of the default — cascade order means client styles still win on overlapping selectors, no client opt-in or migration needed. CSP: each <style> block is hashed separately and appended to style-src. Also covers the /preview/consent and /preview/chooser routes so client-app developers iterate against the production look. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Justifies the absence of any preview-only override that synthesises `epds_handle_login_url`: the /preview/login button visibility is identical to the real /oauth/authorize flow, so iterating against real client metadata is the only way to see the button. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
aspiers
reviewed
Apr 29, 2026
| ${renderFaviconTag(opts.customFaviconUrl, opts.customFaviconUrlDark)} | ||
| <title>Sign in to ${escapeHtml(appName)}</title> | ||
| <style> | ||
| :root { --muted-foreground: #999; --input-bg: #ffffff; --input-border: #e5e5e5; --page-bg: #E8E8E8; --card-bg: #F8F8F8; --card-border: #E5E5E5; --btn-secondary-border: #e5e5e5; --focus-border: ${brandColor}; } |
Contributor
There was a problem hiding this comment.
Does it make sense to have this huge chunk of CSS embedded?
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
Restyle the OAuth login page (
packages/auth-service/src/routes/login-page.ts):Before:
After:
currentColor), Certified brandmark as fallback top logo when a client omitslogo_uri.LoginForm). Handle submission redirects to<client-origin>/api/oauth/login?handle=X, which resolves the handle dynamically and starts a fresh PAR against the correct PDS — so off-PDS handles end up on their own PDS's auth screen, just like the demo's flow.By signing in, you agree to Certified's Terms of Use and Privacy Policy.— both links underlined and pointing tohttps://certified.app/termsandhttps://certified.app/privacy. Copy does not vary per client (the auth server is always Certified's, regardless of which client initiated the flow).Email address→Enter your email address,Continue with email→Continue, removedto use <AppName>subtitle, removed the h1 subtitle treatment.Theming — CSS variables for trusted clients
Trusted clients can override these from their injected
branding.css:Defaults:
--page-bg: #E8E8E8,--card-bg: #F8F8F8,--input-bg: #ffffff,--input-border: #e5e5e5,--muted-foreground: #999. The email input and the ATProto button consume the same--input-bg/--input-borderso they stay visually matched.Hiding the "Recover with backup email" link
The recovery link in the OTP step is shown by default. Clients that don't want to surface backup-email recovery from the OAuth login flow can hide it from their
branding.css:The link itself stays in the DOM (so the recovery flow at
/auth/recoveris still reachable via direct navigation, e.g. from the/accountpage) — only its appearance on the login page is suppressed. Default isblock.Assets added
packages/auth-service/public/certified-text-monochrome.svg— wordmark (inlined into the page, themeable viacurrentColor)packages/auth-service/public/certified-brandmark.svg— square icon (fallback top logo, served at/static/certified-brandmark.svg)Before / After
Before:
After:
Preview URLs (PR-110 Railway environment)
Test plan
<client-origin>/api/oauth/login?handle=Xand (for off-PDS handles) ends up on the correct PDS's auth screen.logo_uri— Certified brandmark appears as top logo.logo_uri— its logo appears at top.branding.cssoverrides work: set--page-bg,--card-bg,--input-bg,--input-border,--muted-foregroundand confirm all surfaces retint in unison.:root { --recovery-link-display: none; }from a client'sbranding.csshides it.https://certified.app/termsandhttps://certified.app/privacyin new tabs.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Style
Tests