feat(shared): preview validation flags missing epds_handle_login_url#176
feat(shared): preview validation flags missing epds_handle_login_url#176aspiers wants to merge 3 commits into
Conversation
Adds a `handle-login-url` row to `/preview/validate` so client devs get a warning when their metadata is missing the hand-off URL that gates the "Or sign in with ATProto/Bluesky" button — and an error when the URL is set but isn't a parseable http(s) URL (which would silently disable the button on real flows). Mirrors the runtime isSafeHttpUrl gate in auth-service's login-page, so http:// localhost dev clients still pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 1f54b7f 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-176 environment in ePDS
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds validation for the ChangesHandle login URL validation for ATProto/Bluesky integration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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)
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 |
Coverage Report for CI Build 26037634100Coverage increased (+0.2%) to 56.07%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
![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
🧹 Nitpick comments (1)
packages/shared/src/__tests__/preview-validation.test.ts (1)
149-193: ⚡ Quick winAdd a direct test for
epds_handle_login_url: ''→warn.You cover
undefined(missing) and invalid/non-http(s), but not the explicit empty-string branch in the validator. Adding this avoids regressions on that specific path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shared/src/__tests__/preview-validation.test.ts` around lines 149 - 193, Add a test case that explicitly covers epds_handle_login_url set to the empty string and asserts the validator returns a "warn" severity; specifically, in packages/shared/src/__tests__/preview-validation.test.ts add a new it(...) (or extend the existing cases) that calls mockFetchOnce with epds_handle_login_url: '' for a sample client_id/redirect_uris, invokes validateClientMetadataForPreview(url, null), finds the check with id 'handle-login-url', and expects check?.severity toBe('warn') so the empty-string branch in the validator is exercised.
🤖 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 @.changeset/preview-validate-handle-login-url.md:
- Line 7: Update the changeset's Affects line to list audiences in the mandated
order: "End users, Client app developers, Operators" (replace the current
"**Affects:** Client app developers" with the full ordered list) so the file
.changeset/preview-validate-handle-login-url.md conforms to the required
audience ordering.
---
Nitpick comments:
In `@packages/shared/src/__tests__/preview-validation.test.ts`:
- Around line 149-193: Add a test case that explicitly covers
epds_handle_login_url set to the empty string and asserts the validator returns
a "warn" severity; specifically, in
packages/shared/src/__tests__/preview-validation.test.ts add a new it(...) (or
extend the existing cases) that calls mockFetchOnce with epds_handle_login_url:
'' for a sample client_id/redirect_uris, invokes
validateClientMetadataForPreview(url, null), finds the check with id
'handle-login-url', and expects check?.severity toBe('warn') so the empty-string
branch in the validator is exercised.
🪄 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: 64118063-2cf0-46eb-883b-b9289cd85b7d
📒 Files selected for processing (3)
.changeset/preview-validate-handle-login-url.mdpackages/shared/src/__tests__/preview-validation.test.tspackages/shared/src/preview-validation.ts
|
|
||
| The `/preview/validate` page now checks the ATProto/Bluesky hand-off URL on your client metadata. | ||
|
|
||
| **Affects:** Client app developers |
There was a problem hiding this comment.
Fix Affects to use the required ordered audience list.
Please update the line to include audiences in the mandated order.
Suggested patch
-**Affects:** Client app developers
+**Affects:** End users, Client app developers, OperatorsAs per coding guidelines, "Every changeset must include a Affects: line listing audiences (End users, Client app developers, Operators in that order)".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| **Affects:** Client app developers | |
| **Affects:** End users, Client app developers, Operators |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.changeset/preview-validate-handle-login-url.md at line 7, Update the
changeset's Affects line to list audiences in the mandated order: "End users,
Client app developers, Operators" (replace the current "**Affects:** Client app
developers" with the full ordered list) so the file
.changeset/preview-validate-handle-login-url.md conforms to the required
audience ordering.
There was a problem hiding this comment.
Pushing back: the writing-changesets skill (.agents/skills/writing-changesets/SKILL.md) says "List only the audiences that actually need to adapt or will notice the change. Omit audiences that are not affected." This change only affects client app developers consuming /preview/validate — end users never see it, and operators have nothing to configure. Keeping **Affects:** Client app developers as-is.
There was a problem hiding this comment.
@aspiers, that's a fair correction — I misread the guideline as requiring all three audiences to always be listed, when in fact .agents/skills/writing-changesets/SKILL.md explicitly says to omit unaffected audiences. Since /preview/validate is a developer-facing tool that end users never see and operators have nothing to configure for, **Affects:** Client app developers alone is the right call. Sorry for the noise.
(ᵔ◡ᵔ)🐇
✏️ Learnings added
Learnt from: aspiers
Repo: hypercerts-org/ePDS PR: 176
File: .changeset/preview-validate-handle-login-url.md:7-7
Timestamp: 2026-05-18T13:44:58.247Z
Learning: In the ePDS repository, the writing-changesets skill (`.agents/skills/writing-changesets/SKILL.md`) requires listing only the audiences that actually need to adapt or will notice the change in the `**Affects:**` line of a changeset file. Audiences that are not affected should be omitted — do NOT default to listing all three audiences (End users, Client app developers, Operators) when some are not relevant to the change.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Pull request overview
This PR extends the shared /preview/validate client-metadata preflight validator to include a new row (handle-login-url) that validates epds_handle_login_url, matching the auth-service login-page gate (http/https allowed; anything else errors; missing warns).
Changes:
- Add
checkHandleLoginUrl()to validateepds_handle_login_urland include it invalidateClientMetadataForPreview()output. - Add unit tests covering
epds_handle_login_urlok/warn/error cases. - Add a changeset describing the additional
/preview/validaterow and its semantics.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/shared/src/preview-validation.ts | Implements the new epds_handle_login_url validation check and wires it into the preview validation pipeline. |
| packages/shared/src/tests/preview-validation.test.ts | Adds coverage for the new check (ok for http/https, error for invalid/unparseable, warn when missing). |
| .changeset/preview-validate-handle-login-url.md | Documents the new /preview/validate output row and expected severities. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This comment has been minimized.
This comment has been minimized.
ClientMetadata previously stated the field must be `https://`, but both the runtime `isSafeHttpUrl` gate in auth-service's login page and the new preview validation accept `http://` for localhost / dev clients. Bring the docstring in line with the actual behaviour. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
aspiers
force-pushed
the
preview-validate-handle-login-url
branch
from
2b84289 to
1f54b7f
Compare
|
| * Must be an absolute http(s):// URL on the client's own origin. | ||
| * `https://` is required in production; `http://` is accepted to | ||
| * support localhost / dev clients (this mirrors the runtime | ||
| * `isSafeHttpUrl` gate in auth-service's login page). If absent or |
| detail: `epds_handle_login_url="${value}". Login page will render the "Or sign in with ATProto/Bluesky" button and hand off to this URL with ?handle=<value>.`, | ||
| labelHtml, | ||
| detailHtml: `${code('epds_handle_login_url')}=${code('"' + value + '"')}. Login page will render the "Or sign in with ATProto/Bluesky" button and hand off to this URL with ${code('?handle=<value>')}.`, |
Summary
/preview/validatenow emits ahandle-login-urlrow coveringepds_handle_login_urlin client metadatawarn(field is optional; absence means the "Or sign in with ATProto/Bluesky" button just isn't rendered)http(s)://→ok(mirrors the realisSafeHttpUrlgate inauth-service/src/routes/login-page.ts, so localhost dev clients still pass)javascript:,file:, unparseable →error(the login page silently refuses to render the button; validator now points it out instead of letting OAuth round-trips fail visibly)Why
Up until now
/preview/validatecoveredtos_uri,policy_uri,brand_color,branding.css,redirect_uris, etc. — but notepds_handle_login_url, which is the gate for the multi-PDS hand-off path on the login page. Adding it puts that field on the same pre-flight checklist client devs already use.Test plan
pnpm test packages/shared/src/__tests__/preview-validation.test.ts— 13 passed (3 new cases for ok/error/error)pnpm test— 1024 passed (no regressions)pnpm format:check— cleanpnpm lint— cleanpnpm typecheck— cleanpnpm test:coverage—preview-validation.tsat 96%, new code covered/preview/validatepage against a metadata declaring + omittingepds_handle_login_url(left for human reviewer)🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes