Draft
Conversation
|
@Kzoeps is attempting to deploy a commit to the Hypercerts Foundation Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds new end-to-end Cucumber features and step definitions (automatic account creation, consent screen, internal API tests), centralizes OAuth account-creation flow and utilities, expands test runner configuration and environment wiring, and increases timeouts and Mailpit wait/cleanup behavior. Changes
Sequence DiagramsequenceDiagram
actor Browser as Browser/Client
participant Demo as Demo OAuth Server
participant PDS as PDS Server
participant Mailpit as Mailpit (Email)
rect rgba(100, 150, 200, 0.5)
Note over Browser,Mailpit: OAuth Account Creation & Consent Flow
Browser->>Demo: Initiate OAuth login (demo client)
Demo->>Browser: Show email entry form
Browser->>Demo: Submit email
Demo->>Mailpit: Send OTP email
Mailpit->>Browser: Email available in trap
Browser->>Mailpit: Retrieve OTP
Browser->>Demo: Enter OTP
Demo->>Browser: Show consent screen
Browser->>Demo: Click "Approve"
Demo->>PDS: Exchange token / create session
PDS->>Demo: Return session + DID
Demo->>Browser: Redirect to /welcome (render DID & handle)
end
rect rgba(150, 200, 100, 0.5)
Note over Browser,PDS: Post-Creation Validation
Browser->>PDS: resolveHandle(handle)
PDS->>Browser: Return DID (verify matches stored DID)
Browser->>PDS: GET https://<handle>/.well-known/atproto-did
PDS->>Browser: Return DID text (verify format)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
Contributor
Author
|
@coderabbitai please review |
|
✅ Actions performedReview triggered.
|
1 similar comment
|
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (4)
e2e/step-definitions/pds.steps.ts (2)
147-150: Document why this step is a no-op.The empty function body is intentional per the comment, but a brief inline comment inside the function would make this clearer for future maintainers.
Proposed fix
Then( 'the only way to authenticate is through the ePDS OAuth flow', - function (this: EpdsWorld) {}, + function (this: EpdsWorld) { + // No-op: assertion is implicit from preceding createSession rejection + }, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/step-definitions/pds.steps.ts` around lines 147 - 150, Add a brief inline comment inside the empty step-definition function for the step "the only way to authenticate is through the ePDS OAuth flow" (the Then(...) handler using EpdsWorld) to document that the no-op is intentional and why it exists; update the anonymous function body to contain a one-line comment explaining it's intentionally empty (e.g., authentication is validated elsewhere or handled by shared fixtures) so future maintainers understand the purpose.
53-56: UsereplaceAllfor cleaner regex escaping.SonarCloud suggests using
replaceAllhere. While the current code works correctly,replaceAllis more idiomatic in modern JS.Proposed fix
const pdsDomain = new URL(testEnv.pdsUrl).hostname // Handle must be <short-alphanumeric>.<pds-domain> const pattern = new RegExp( - `^[a-z0-9]{4,20}\\.${pdsDomain.replace(/\./g, '\\.')}$`, + `^[a-z0-9]{4,20}\\.${pdsDomain.replaceAll('.', '\\.')}$`, )Note: The ast-grep ReDoS warning is a false positive here—
pdsDomaincomes from controlled test infrastructure (testEnv.pdsUrl), not user input.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/step-definitions/pds.steps.ts` around lines 53 - 56, The regex construction uses pdsDomain.replace(/\./g, '\\.') — replaceAll is preferred for readability; change the call to pdsDomain.replaceAll('.', '\\.') when building the pattern variable so the RegExp remains `new RegExp(\`^[a-z0-9]{4,20}\\.${escaped}\\$\`)` (use the escaped value of pdsDomain), keeping the same validation logic in the code that defines pattern and references pdsDomain.e2e/support/flows.ts (1)
32-33: Consider reusinggetPagefromutils.ts.The page null-check here duplicates the logic in
getPage(world). Using the shared helper improves consistency.Proposed fix
+import { getPage } from './utils.js' import { waitForEmail, extractOtp, clearMailpit } from './mailpit.js' ... export async function createAccountViaOAuth( world: EpdsWorld, email: string, ): Promise<{ did: string; handle: string | undefined }> { - const page = world.page - if (!page) throw new Error('page is not initialised') + const page = getPage(world)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/support/flows.ts` around lines 32 - 33, Replace the direct access and null-check of page in e2e/support/flows.ts with the shared helper getPage(world) from utils.ts: find where you do "const page = world.page" and "if (!page) throw ..." and instead call getPage(world) to obtain the Page instance (importing getPage if not already), removing the duplicated null-check and ensuring you use the returned Page object for subsequent operations.e2e/support/hooks.ts (1)
27-32: Consider adding error handling for the BeforeAll Mailpit cleanup.The fetch to wipe leftover emails doesn't check
res.ok. If Mailpit is unreachable or returns an error, the suite proceeds silently. This could lead to flaky tests from stale emails, with no indication of the root cause.Proposed fix
// Wipe any emails left over from previous runs if (testEnv.mailpitPass) { - await fetch(`${testEnv.mailpitUrl}/api/v1/messages`, { + const res = await fetch(`${testEnv.mailpitUrl}/api/v1/messages`, { method: 'DELETE', headers: { Authorization: mailpitAuthHeader() }, }) + if (!res.ok) { + console.warn(`Mailpit cleanup failed: ${res.status} — stale emails may cause flaky tests`) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/support/hooks.ts` around lines 27 - 32, The BeforeAll Mailpit cleanup fetch call (inside the block checking testEnv.mailpitPass) does not verify the response; update the code around the fetch to check the returned Response (res) from the DELETE to `${testEnv.mailpitUrl}/api/v1/messages` and handle non-OK responses by throwing or logging an error with the status and body so the test run fails or reports the failure; ensure you still include the Authorization header from mailpitAuthHeader() and propagate or surface the error (or retry if desired) so stale emails won't be ignored silently.
🤖 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/step-definitions/auth.steps.ts`:
- Around line 257-266: Replace the hardcoded '00000000' in the When step handler
with a dynamically generated wrong OTP using the active OTP settings: if
this.otpCode is available, mutate or derive a wrong value from it; otherwise
build a bogus code using OTP_LENGTH and OTP_CHARSET (ensure length matches
OTP_LENGTH and characters are drawn from OTP_CHARSET but differ from a valid
code) and use that for page.fill('#code', ...); apply the same fix to the
single-attempt helper referenced above so the test exercises the retry logic
regardless of numeric/alphanumeric or length configuration.
In `@e2e/step-definitions/consent.steps.ts`:
- Around line 24-34: The test currently asserts any non-empty text in the
.subtitle but should verify the actual client name; update the Then("it shows
the demo client's name", async function (this: EpdsWorld) { ... }) step to
target the client name element (e.g. use the locator '.subtitle strong' instead
of '.subtitle') and assert its trimmed innerText equals the expected client name
(load/compare against the value in client-metadata.json) using getPage(...) and
the subtitle locator so the test fails if the <strong> client name is missing or
incorrect.
In `@features/consent-screen.feature`:
- Around line 12-20: The step "Then the browser is redirected back to the demo
client with a valid session" only checks the URL (via the shared auth step that
waits for **/welcome) and thus duplicates "Then no consent screen is shown"
without asserting session validity; update the step (or the shared step
implementation used by it) to explicitly verify authentication state after
redirect by asserting a real session artifact — e.g., check for the demo
client's session cookie (name used by the app), an access/id token in
localStorage, or call the demo client's session endpoint (e.g., /api/session) to
confirm the user is authenticated — and apply the same stronger assertion to the
other consent-success scenarios and the shared step referenced by "Then no
consent screen is shown".
---
Nitpick comments:
In `@e2e/step-definitions/pds.steps.ts`:
- Around line 147-150: Add a brief inline comment inside the empty
step-definition function for the step "the only way to authenticate is through
the ePDS OAuth flow" (the Then(...) handler using EpdsWorld) to document that
the no-op is intentional and why it exists; update the anonymous function body
to contain a one-line comment explaining it's intentionally empty (e.g.,
authentication is validated elsewhere or handled by shared fixtures) so future
maintainers understand the purpose.
- Around line 53-56: The regex construction uses pdsDomain.replace(/\./g, '\\.')
— replaceAll is preferred for readability; change the call to
pdsDomain.replaceAll('.', '\\.') when building the pattern variable so the
RegExp remains `new RegExp(\`^[a-z0-9]{4,20}\\.${escaped}\\$\`)` (use the
escaped value of pdsDomain), keeping the same validation logic in the code that
defines pattern and references pdsDomain.
In `@e2e/support/flows.ts`:
- Around line 32-33: Replace the direct access and null-check of page in
e2e/support/flows.ts with the shared helper getPage(world) from utils.ts: find
where you do "const page = world.page" and "if (!page) throw ..." and instead
call getPage(world) to obtain the Page instance (importing getPage if not
already), removing the duplicated null-check and ensuring you use the returned
Page object for subsequent operations.
In `@e2e/support/hooks.ts`:
- Around line 27-32: The BeforeAll Mailpit cleanup fetch call (inside the block
checking testEnv.mailpitPass) does not verify the response; update the code
around the fetch to check the returned Response (res) from the DELETE to
`${testEnv.mailpitUrl}/api/v1/messages` and handle non-OK responses by throwing
or logging an error with the status and body so the test run fails or reports
the failure; ensure you still include the Authorization header from
mailpitAuthHeader() and propagate or surface the error (or retry if desired) so
stale emails won't be ignored silently.
🪄 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: 0181e006-a423-405f-a75a-daa1f669c937
📒 Files selected for processing (14)
e2e/cucumber.mjse2e/step-definitions/auth.steps.tse2e/step-definitions/common.steps.tse2e/step-definitions/consent.steps.tse2e/step-definitions/email.steps.tse2e/step-definitions/pds.steps.tse2e/support/flows.tse2e/support/hooks.tse2e/support/mailpit.tse2e/support/utils.tse2e/support/world.tsfeatures/automatic-account-creation.featurefeatures/consent-screen.featurefeatures/passwordless-authentication.feature
Comment on lines
+24
to
+34
| Then("it shows the demo client's name", async function (this: EpdsWorld) { | ||
| // The consent page renders "<client_name> wants to access your account" | ||
| // in the .subtitle element. Assert it contains non-trivial text (not blank). | ||
| const page = getPage(this) | ||
| const subtitle = page.locator('.subtitle') | ||
| await expect(subtitle).toBeVisible() | ||
| const text = await subtitle.innerText() | ||
| if (!text.trim()) { | ||
| throw new Error('Consent screen subtitle is empty — expected client name') | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Assert the rendered client name, not just any subtitle text.
This still passes if the <strong> containing the client name disappears and only the static wants to access your account copy remains. Please target .subtitle strong at minimum, or compare against the value from client-metadata.json.
🎯 Tighten the assertion
Then("it shows the demo client's name", async function (this: EpdsWorld) {
- // The consent page renders "<client_name> wants to access your account"
- // in the .subtitle element. Assert it contains non-trivial text (not blank).
const page = getPage(this)
- const subtitle = page.locator('.subtitle')
- await expect(subtitle).toBeVisible()
- const text = await subtitle.innerText()
- if (!text.trim()) {
- throw new Error('Consent screen subtitle is empty — expected client name')
- }
+ const clientName = page.locator('.subtitle strong')
+ await expect(clientName).toBeVisible()
+ await expect(clientName).not.toHaveText(/^\s*$/)
})📝 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.
Suggested change
| Then("it shows the demo client's name", async function (this: EpdsWorld) { | |
| // The consent page renders "<client_name> wants to access your account" | |
| // in the .subtitle element. Assert it contains non-trivial text (not blank). | |
| const page = getPage(this) | |
| const subtitle = page.locator('.subtitle') | |
| await expect(subtitle).toBeVisible() | |
| const text = await subtitle.innerText() | |
| if (!text.trim()) { | |
| throw new Error('Consent screen subtitle is empty — expected client name') | |
| } | |
| }) | |
| Then("it shows the demo client's name", async function (this: EpdsWorld) { | |
| const page = getPage(this) | |
| const clientName = page.locator('.subtitle strong') | |
| await expect(clientName).toBeVisible() | |
| await expect(clientName).not.toHaveText(/^\s*$/) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/step-definitions/consent.steps.ts` around lines 24 - 34, The test
currently asserts any non-empty text in the .subtitle but should verify the
actual client name; update the Then("it shows the demo client's name", async
function (this: EpdsWorld) { ... }) step to target the client name element (e.g.
use the locator '.subtitle strong' instead of '.subtitle') and assert its
trimmed innerText equals the expected client name (load/compare against the
value in client-metadata.json) using getPage(...) and the subtitle locator so
the test fails if the <strong> client name is missing or incorrect.
Comment on lines
+12
to
20
| Given a returning user has a PDS account | ||
| When the demo client initiates an OAuth login | ||
| And the user enters the test email on the login page | ||
| Then an OTP email arrives in the mail trap | ||
| When the user enters the OTP code | ||
| Then a consent screen is displayed | ||
| And it shows the demo client's name and requested permissions | ||
| And it shows the demo client's name | ||
| When the user clicks "Approve" | ||
| Then the browser is redirected back to the demo client with a valid session |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
The “valid session” assertion is weaker than the scenario text.
Then the browser is redirected back to the demo client with a valid session currently just waits for **/welcome in the shared auth step. On Lines 33-44, that makes it effectively the same check as Then no consent screen is shown, so the session/token part is never asserted explicitly.
🔍 Make the session check explicit
- Then the browser is redirected back to the demo client with a valid session
+ Then the browser is redirected back to the demo client
+ And the demo client has a valid OAuth access tokenApply the same change to the other consent-success scenarios here, or strengthen the shared step implementation instead.
Also applies to: 33-39, 42-44
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@features/consent-screen.feature` around lines 12 - 20, The step "Then the
browser is redirected back to the demo client with a valid session" only checks
the URL (via the shared auth step that waits for **/welcome) and thus duplicates
"Then no consent screen is shown" without asserting session validity; update the
step (or the shared step implementation used by it) to explicitly verify
authentication state after redirect by asserting a real session artifact — e.g.,
check for the demo client's session cookie (name used by the app), an access/id
token in localStorage, or call the demo client's session endpoint (e.g.,
/api/session) to confirm the user is authenticated — and apply the same stronger
assertion to the other consent-success scenarios and the shared step referenced
by "Then no consent screen is shown".
Kzoeps
force-pushed
the
e2e-test-additional
branch
from
149cb11 to
8b9d00b
Compare
Contributor
Author
|
@coderabbitai please review this |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/step-definitions/auth.steps.ts (1)
50-55: Consider extracting the nested ternary for readability.The charset detection logic works correctly but the nested ternary is dense. Extracting to a helper or using early returns would improve clarity.
♻️ Optional refactor to improve readability
- const otpCharset = - inputModeAttr === 'numeric' || patternAttr === `[0-9]{${otpLength}}` - ? 'numeric' - : patternAttr === `[A-Z0-9]{${otpLength}}` - ? 'alphanumeric' - : testEnv.otpCharset + let otpCharset: 'numeric' | 'alphanumeric' + if (inputModeAttr === 'numeric' || patternAttr === `[0-9]{${otpLength}}`) { + otpCharset = 'numeric' + } else if (patternAttr === `[A-Z0-9]{${otpLength}}`) { + otpCharset = 'alphanumeric' + } else { + otpCharset = testEnv.otpCharset + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/step-definitions/auth.steps.ts` around lines 50 - 55, The nested ternary that computes otpCharset is hard to read; extract the logic into a small helper like getOtpCharset(inputModeAttr, patternAttr, otpLength, testEnv) or replace the ternary with an explicit if/else block: check if inputModeAttr === 'numeric' or patternAttr matches `[0-9]{${otpLength}}` then return 'numeric', else if patternAttr matches `[A-Z0-9]{${otpLength}}` return 'alphanumeric', otherwise return testEnv.otpCharset; update the original assignment to call that helper or use the clearer control flow and keep the same variable name otpCharset.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/step-definitions/auth.steps.ts`:
- Around line 50-55: The nested ternary that computes otpCharset is hard to
read; extract the logic into a small helper like getOtpCharset(inputModeAttr,
patternAttr, otpLength, testEnv) or replace the ternary with an explicit if/else
block: check if inputModeAttr === 'numeric' or patternAttr matches
`[0-9]{${otpLength}}` then return 'numeric', else if patternAttr matches
`[A-Z0-9]{${otpLength}}` return 'alphanumeric', otherwise return
testEnv.otpCharset; update the original assignment to call that helper or use
the clearer control flow and keep the same variable name otpCharset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ab9c56de-0558-4235-a822-2eb6e4c37ff7
📒 Files selected for processing (9)
.github/workflows/e2e-pr.ymle2e/.env.examplee2e/cucumber.mjse2e/step-definitions/auth.steps.tse2e/step-definitions/internal-api.steps.tse2e/support/env.tse2e/support/utils.tse2e/support/world.tsfeatures/internal-api.feature
✅ Files skipped from review due to trivial changes (2)
- e2e/support/env.ts
- e2e/.env.example
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/cucumber.mjs
- e2e/support/utils.ts
Contributor
|
Oh this was a stacked PR and it gets auto-closed if the target branch is deleted. Restored and reopening. |
aspiers
reviewed
Apr 2, 2026
| Given "alice@example.com" has a PDS account with DID "did:plc:alice" | ||
| When GET /_internal/account-by-email?email=alice@example.com is called with valid x-internal-secret | ||
| Then the response is 200 with { "did": "did:plc:alice" } | ||
| Scenario: Account lookup by email returns the account's DID |
Contributor
There was a problem hiding this comment.
@Kzoeps I'm not sure it's a good idea to be testing internal stuff. These are not part of a feature but rather implementation details, and the point of E2E tests is to test functionality for users in a more black box way. Were there any particular reasons why we need this file?
aspiers
reviewed
Apr 2, 2026
e2e/cucumber.mjs
Outdated
| import: ['e2e/step-definitions/**/*.ts', 'e2e/support/**/*.ts'], | ||
| format: ['pretty', 'html:reports/e2e.html'], | ||
| tags: 'not @manual', | ||
| tags: 'not @manual and not @docker-only', |
Contributor
There was a problem hiding this comment.
We should have an @email tag for tests involving sending email.
aspiers
approved these changes
Apr 2, 2026
|
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.
Builds on #42
Summary by CodeRabbit