test(e2e): expand e2e coverage to account-settings, consent and auto-account-creation#43
test(e2e): expand e2e coverage to account-settings, consent and auto-account-creation#43Kzoeps wants to merge 9 commits intohypercerts-org:mainfrom
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. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 22 seconds. ⌛ 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)
📝 WalkthroughWalkthroughExpanded the E2E test suite: added/updated feature files and step definitions, introduced new support helpers and world fields, reworked Mailpit/OTP handling and timeouts, added internal-api tests guarded by an internal secret, and implemented multi-session and account-settings flows. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Playwright User
participant Demo as Demo Client
participant EPDS as ePDS (Auth)
participant Mailpit as Mailpit (Email Trap)
participant PDS as PDS (Identity)
User->>Demo: Start OAuth login
Demo->>EPDS: Redirect to /oauth/authorize
EPDS->>User: Render login form
User->>EPDS: Submit email
EPDS->>Mailpit: Send OTP email
Mailpit-->>User: OTP email arrives
User->>Mailpit: Poll /api/v1/search & fetch message
User->>EPDS: Submit OTP
EPDS->>PDS: Create account/session
EPDS->>User: Redirect to consent screen
User->>EPDS: Click Approve
EPDS->>Demo: Redirect with auth code
Demo->>EPDS: Exchange code for token
EPDS-->>Demo: Return token
Demo->>User: Redirect to /welcome
rect rgba(100,150,200,0.5)
Note over PDS,User: Post-signup validations
User->>PDS: com.atproto.identity.resolveHandle
PDS-->>User: Return DID
User->>PDS: GET /.well-known/atproto-did
PDS-->>User: Return DID text
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Suggested reviewers
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 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
| 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".
There was a problem hiding this comment.
What the finding claims:
'the browser is redirected back to the demo client with a valid session' only checks the URL (waits for **/welcome) and duplicates 'no consent screen is shown' without asserting session validity.
What the code actually does:
'the browser is redirected back to the demo client with a valid session' calls assertDemoClientSession() (auth.steps.ts:65-80), which does four distinct checks:
- waitForURL('**/welcome') — URL redirect
- Finds the session_id cookie and asserts it has a non-empty value — real session artifact
- toContainText('You are signed in.') — UI confirmation
- toContainText('Sign out') — button presence
- toContainText(/@[\w.-]+/) — handle is rendered (proves the session was decoded server-side)
And 'no consent screen is shown' (consent.steps.ts:55-60) only does the URL check — waitForURL('**/welcome'). These two steps are deliberately different: no consent screen is shown is a routing assertion, with a valid session is a session validity assertion. They're not duplicates.
The finding describes a problem that was already fixed. The current code is correct, the session check is substantive, and there's nothing to change.
There was a problem hiding this comment.
@Kzoeps, you're absolutely right — my finding was incorrect. The current code is correct and the two steps are meaningfully distinct:
'the browser is redirected back to the demo client with a valid session'delegates toassertDemoClientSession()which performs substantive session-validity checks (cookie presence, UI confirmation, handle rendering).'no consent screen is shown'is intentionally a lighter routing-only assertion (waitForURL('**/welcome')).
There is nothing to change here. I apologize for the noise.
✏️ Learnings added
Learnt from: Kzoeps
Repo: hypercerts-org/ePDS PR: 43
File: features/consent-screen.feature:12-20
Timestamp: 2026-04-06T07:00:00.197Z
Learning: In `e2e/step-definitions/auth.steps.ts` (hypercerts-org/ePDS), the `assertDemoClientSession()` helper (around lines 65–80) performs multiple distinct session-validity checks: `waitForURL('**/welcome')`, asserting the `session_id` cookie has a non-empty value, `toContainText('You are signed in.')`, `toContainText('Sign out')`, and `toContainText(/@[\w.-]+/)` to verify the handle is rendered server-side. The Gherkin step `'the browser is redirected back to the demo client with a valid session'` uses this helper and is substantively different from `'no consent screen is shown'` in `e2e/step-definitions/consent.steps.ts`, which only does a `waitForURL('**/welcome')` routing check. Do not flag these two steps as duplicates.
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.
🧠 Learnings used
Learnt from: Kzoeps
Repo: hypercerts-org/ePDS PR: 12
File: packages/auth-service/src/routes/choose-handle.ts:279-306
Timestamp: 2026-03-12T07:21:30.286Z
Learning: In `packages/auth-service/src/routes/choose-handle.ts` and `packages/auth-service/src/routes/otp-verification.ts` (hypercerts-org/ePDS): Do NOT call `ctx.db.recordClientLogin(email, clientId)` here. `recordClientLogin` must be called only on the consent page (after the user grants consent), not on the choose-handle or OTP-verification step. There is also a known bug where new users are not being shown the consent page — this is tracked in PR `#11`.
Learnt from: Kzoeps
Repo: hypercerts-org/ePDS PR: 43
File: e2e/step-definitions/consent.steps.ts:25-42
Timestamp: 2026-04-06T06:55:53.275Z
Learning: In `e2e/step-definitions/consent.steps.ts` (hypercerts-org/ePDS), the "it shows the demo client's name" step uses `await expect(page.getByText(clientName, { exact: true })).toBeVisible()`. The consent page renders `<p class="subtitle"><strong>${clientName}</strong> wants to access your account</p>`. Playwright's `getByText` with `exact: true` matches child text nodes, so it correctly finds the `<strong>` element whose full text is exactly `clientName` — not the `<p>` which has additional text. This is intentional and idiomatic; do not suggest changing it to a `.subtitle strong` locator.
Kzoeps
force-pushed
the
e2e-test-additional
branch
from
149cb11 to
8b9d00b
Compare
|
@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
|
Oh this was a stacked PR and it gets auto-closed if the target branch is deleted. Restored and reopening. |
| Given "[email protected]" has a PDS account with DID "did:plc:alice" | ||
| When GET /_internal/[email protected] 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 |
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?
There was a problem hiding this comment.
@aspiers this was already in the list of feature files and isnt a new addition. Just decided to implement it since i thought it was required but we could remove the whole feature file then?
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', |
There was a problem hiding this comment.
We should have an @email tag for tests involving sending email.
Kzoeps
force-pushed
the
e2e-test-additional
branch
from
745d436 to
169a040
Compare
Coverage Report for CI Build 24075231286Coverage remained the same at 29.583%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
e2e/step-definitions/consent.steps.ts (1)
1-7: Tracked TODO for future automation.The TODO comment documents a known gap (CSS injection for consent branding). Consider creating a GitHub issue to track this work if not already tracked.
Would you like me to help create an issue to track the automation of Scenario 5 (consent page client branding)?
🤖 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 1 - 7, Add a tracked issue for automating Scenario 5 (consent page shows client branding) by creating a GitHub issue that references the TODO in e2e/step-definitions/consent.steps.ts, names the missing work as "inject clientBrandingCss into renderConsent()" and describes that renderConsent() must accept and apply clientBrandingCss from client metadata (include scenario id/name and test file path), assign appropriate labels (e2e, enhancement) and link back to this PR so the TODO is tracked.features/account-settings.feature (1)
26-32: Consider uncommenting or removing the handle assertion.Line 31 has a commented-out step
# And the page displays their current handle. TherenderSettingsPageinaccount-settings.tsnow includes the "Current Handle" row, and the step definitionthe page displays their current handleexists inaccount-settings.steps.ts.Is this intentionally commented out, or should it be enabled now that the feature is implemented?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/account-settings.feature` around lines 26 - 32, The commented-out step "# And the page displays their current handle" in the feature should be re-enabled (or removed if intentionally deprecated) to match the implemented UI and existing step definition: update features/account-settings.feature to uncomment that line so the scenario includes "And the page displays their current handle", ensuring the step definition in account-settings.steps.ts is used by the test and that renderSettingsPage (which renders the "Current Handle" row) is validated; run the feature tests and remove the step instead only if the handle assertion is no longer desired.e2e/step-definitions/pds.steps.ts (1)
53-62: Consider usingreplaceAllfor consistency (optional).Static analysis suggests using
String.prototype.replaceAll()instead ofreplace()with a global regex. This is a minor style preference.The regex-from-variable warning is a false positive here since
pdsDomaincomes from trusted configuration (testEnv.pdsUrl), not user input.♻️ Optional: Use replaceAll for cleaner escaping
const pattern = new RegExp( - `^[a-z0-9]{4,20}\\.${pdsDomain.replace(/\./g, '\\.')}$`, + `^[a-z0-9]{4,20}\\.${pdsDomain.replaceAll('.', '\\.')}$`, )🤖 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 - 62, The pattern-building uses pdsDomain.replace(/\./g, '\\.') which static analysis flags; replace it with pdsDomain.replaceAll('.', '\\.') for consistency or, if you prefer to keep the current approach, add a comment explaining pdsDomain is trusted to silence the false positive; update the RegExp construction where pattern is created (referencing pdsDomain and this.userHandle) so the escaped dots are produced via replaceAll('.', '\\.') to build the same regex.e2e/step-definitions/auth.steps.ts (2)
337-342: Synchronize on the submit button's disabled→enabled state instead of error message visibility.The error message persists after the first failed verification attempt. If
toBeVisible()passes on the first failure, it will satisfy immediately on subsequent loop iterations without waiting for the server to process the next verification request.The verify button is disabled before each fetch (line 495 in login-page.ts) and re-enabled only after the response arrives (line 500), providing a reliable per-request synchronization point. Wait for the button to transition from
:disabledback to enabled state to ensure each attempt completes before the next one submits.🤖 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 337 - 342, The loop in auth.steps.ts currently waits for the persistent error message ('#error-msg') which can remain visible across iterations; instead synchronize each attempt on the verify button's disabled→enabled transition. After submitting with the selector '#form-verify-otp .btn-primary', wait for that button to become enabled (e.g. use Playwright's expect(locator).toBeEnabled() or waitFor state) so each request has completed before the next fill/click; this uses the same disable logic implemented in login-page.ts around the verify button.
70-74: Import the session cookie name from the shared constant instead of hardcoding.The demo client explicitly configures the session cookie as
SESSION_COOKIEinpackages/demo/src/lib/session.ts. Import and use this constant instead of hardcoding'session_id'. Additionally, scope thecookies()call totestEnv.demoUrlfor clarity—though the navigation has already occurred, it makes the intent explicit and aligns with best practices for cookie assertions across origins during auth flows.🤖 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 70 - 74, Replace the hardcoded 'session_id' and the unscoped cookies() call: import the SESSION_COOKIE constant (named SESSION_COOKIE) from the demo session module and use it in the cookie lookup (cookies.find(cookie => cookie.name === SESSION_COOKIE)), and call page.context().cookies(testEnv.demoUrl) so the cookies() call is scoped to the demo origin; update the sessionCookie check that currently references sessionCookie?.value to continue to throw the same error if missing.
🤖 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/account-settings.steps.ts`:
- Around line 63-77: The test helper assertGetSessionFails is using the DID
string as a Bearer token (Authorization: Bearer ${did}), which is invalid;
either remove this redundant assertion (the existing assertPdsAccountMissing
flow already verifies deletion via describeRepo) or change assertGetSessionFails
to use a real access token captured during session setup (store the session's
access token when creating the test account and pass that token into
assertGetSessionFails) so the request to
`${testEnv.pdsUrl}/xrpc/com.atproto.server.getSession` uses a JWT access token
instead of the DID.
---
Nitpick comments:
In `@e2e/step-definitions/auth.steps.ts`:
- Around line 337-342: The loop in auth.steps.ts currently waits for the
persistent error message ('#error-msg') which can remain visible across
iterations; instead synchronize each attempt on the verify button's
disabled→enabled transition. After submitting with the selector
'#form-verify-otp .btn-primary', wait for that button to become enabled (e.g.
use Playwright's expect(locator).toBeEnabled() or waitFor state) so each request
has completed before the next fill/click; this uses the same disable logic
implemented in login-page.ts around the verify button.
- Around line 70-74: Replace the hardcoded 'session_id' and the unscoped
cookies() call: import the SESSION_COOKIE constant (named SESSION_COOKIE) from
the demo session module and use it in the cookie lookup (cookies.find(cookie =>
cookie.name === SESSION_COOKIE)), and call
page.context().cookies(testEnv.demoUrl) so the cookies() call is scoped to the
demo origin; update the sessionCookie check that currently references
sessionCookie?.value to continue to throw the same error if missing.
In `@e2e/step-definitions/consent.steps.ts`:
- Around line 1-7: Add a tracked issue for automating Scenario 5 (consent page
shows client branding) by creating a GitHub issue that references the TODO in
e2e/step-definitions/consent.steps.ts, names the missing work as "inject
clientBrandingCss into renderConsent()" and describes that renderConsent() must
accept and apply clientBrandingCss from client metadata (include scenario
id/name and test file path), assign appropriate labels (e2e, enhancement) and
link back to this PR so the TODO is tracked.
In `@e2e/step-definitions/pds.steps.ts`:
- Around line 53-62: The pattern-building uses pdsDomain.replace(/\./g, '\\.')
which static analysis flags; replace it with pdsDomain.replaceAll('.', '\\.')
for consistency or, if you prefer to keep the current approach, add a comment
explaining pdsDomain is trusted to silence the false positive; update the RegExp
construction where pattern is created (referencing pdsDomain and
this.userHandle) so the escaped dots are produced via replaceAll('.', '\\.') to
build the same regex.
In `@features/account-settings.feature`:
- Around line 26-32: The commented-out step "# And the page displays their
current handle" in the feature should be re-enabled (or removed if intentionally
deprecated) to match the implemented UI and existing step definition: update
features/account-settings.feature to uncomment that line so the scenario
includes "And the page displays their current handle", ensuring the step
definition in account-settings.steps.ts is used by the test and that
renderSettingsPage (which renders the "Current Handle" row) is validated; run
the feature tests and remove the step instead only if the handle assertion is no
longer desired.
🪄 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: 43cee5d0-d4b6-4958-a03d-f7ed2737f749
📒 Files selected for processing (20)
.github/workflows/e2e-pr.ymle2e/README.mde2e/cucumber.mjse2e/step-definitions/account-settings.steps.tse2e/step-definitions/auth.steps.tse2e/step-definitions/consent.steps.tse2e/step-definitions/internal-api.steps.tse2e/step-definitions/pds.steps.tse2e/support/flows.tse2e/support/hooks.tse2e/support/mailpit.tse2e/support/utils.tse2e/support/world.tsfeatures/IMPLEMENTATION-PLAN.mdfeatures/account-recovery.featurefeatures/account-settings.featurefeatures/client-branding.featurefeatures/email-delivery.featurefeatures/passwordless-authentication.featurepackages/auth-service/src/routes/account-settings.ts
✅ Files skipped from review due to trivial changes (3)
- features/email-delivery.feature
- features/account-recovery.feature
- e2e/README.md
🚧 Files skipped from review as they are similar to previous changes (8)
- features/passwordless-authentication.feature
- .github/workflows/e2e-pr.yml
- e2e/support/flows.ts
- e2e/support/mailpit.ts
- e2e/step-definitions/internal-api.steps.ts
- e2e/support/utils.ts
- e2e/support/world.ts
- e2e/support/hooks.ts
Kzoeps
changed the title
|
@coderabbitai please review this |
|
✅ Actions performedReview triggered.
|
Kzoeps
force-pushed
the
e2e-test-additional
branch
from
4c8fbbb to
a809c0a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
features/account-settings.feature (1)
26-31: Consider addingThe "User views their account information" scenario relies on
the user is logged into account settings, which internally requires Mailpit for OTP login. While the Mailpit guard gracefully marks steps as pending, adding theThe same consideration applies to the session management (lines 47-53) and account deletion (lines 57-62) scenarios.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/account-settings.feature` around lines 26 - 31, Add the `@email` tag to scenarios that depend on Mailpit/OTP login to make the dependency explicit and filterable: apply `@email` to the "User views their account information" scenario (which uses the step "the user is logged into account settings"), and likewise add `@email` to the session management and account deletion scenarios that depend on OTP-based login; this ensures CI can include/exclude these tests and documents the Mailpit dependency.e2e/step-definitions/account-settings.steps.ts (1)
17-19: Minor: UsereplaceAllfor clarity.SonarCloud suggests using
String.prototype.replaceAll()instead ofreplace()with a global regex. This is a minor readability improvement.♻️ Suggested fix
function escapeForRegex(value: string): string { - return value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') + return value.replaceAll(/[.*+?^${}()|[\]\\]/g, '\\$&') }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/step-definitions/account-settings.steps.ts` around lines 17 - 19, The escapeForRegex function currently uses value.replace with a global regex; update it to use String.prototype.replaceAll for clarity by iterating over the set of regex-special characters and calling value.replaceAll(char, `\\${char}`) (or using an array.reduce over that characters list) so escapeForRegex still returns the same escaped string; reference the function name escapeForRegex and the replacement of the regex-based replace call.
🤖 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/cucumber.mjs`:
- Around line 2-7: The paths array in e2e/cucumber.mjs is missing the new
feature file; update the paths array (the paths variable/entry list) to include
'features/internal-api.feature' alongside the existing entries so Cucumber will
run that feature (i.e., add 'features/internal-api.feature' to the paths array
declaration).
In `@e2e/step-definitions/account-settings.steps.ts`:
- Around line 491-509: Remove the unused step definition whose regex/string is
/^the browser is redirected away from \/account \(signed out\)$/ from
e2e/step-definitions/account-settings.steps.ts; delete the entire Then(...)
block (the anonymous async function that calls getPage(this),
escapeForRegex(testEnv.authUrl), expectations on page URL and heading, and the
final page.goto/expect) so no orphaned step remains—ensure no other references
to that regex string remain in the repository after removal.
---
Nitpick comments:
In `@e2e/step-definitions/account-settings.steps.ts`:
- Around line 17-19: The escapeForRegex function currently uses value.replace
with a global regex; update it to use String.prototype.replaceAll for clarity by
iterating over the set of regex-special characters and calling
value.replaceAll(char, `\\${char}`) (or using an array.reduce over that
characters list) so escapeForRegex still returns the same escaped string;
reference the function name escapeForRegex and the replacement of the
regex-based replace call.
In `@features/account-settings.feature`:
- Around line 26-31: Add the `@email` tag to scenarios that depend on Mailpit/OTP
login to make the dependency explicit and filterable: apply `@email` to the "User
views their account information" scenario (which uses the step "the user is
logged into account settings"), and likewise add `@email` to the session
management and account deletion scenarios that depend on OTP-based login; this
ensures CI can include/exclude these tests and documents the Mailpit dependency.
🪄 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: 8315871e-6ca0-4def-8b9e-771aa46282fb
📒 Files selected for processing (5)
docs/design/testing-gaps.mde2e/cucumber.mjse2e/step-definitions/account-settings.steps.tse2e/support/mailpit.tsfeatures/account-settings.feature
✅ Files skipped from review due to trivial changes (1)
- docs/design/testing-gaps.md
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/support/mailpit.ts
| paths: [ | ||
| 'features/passwordless-authentication.feature', | ||
| 'features/automatic-account-creation.feature', | ||
| 'features/consent-screen.feature', | ||
| 'features/account-settings.feature', | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if internal-api.feature exists in the features directory
fd -t f 'internal-api.feature' features/Repository: hypercerts-org/ePDS
Length of output: 92
Add internal-api.feature to the paths array.
The file features/internal-api.feature exists in the repository but is not listed in the paths array. If this feature file was added in this PR and is ready for testing, include it here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/cucumber.mjs` around lines 2 - 7, The paths array in e2e/cucumber.mjs is
missing the new feature file; update the paths array (the paths variable/entry
list) to include 'features/internal-api.feature' alongside the existing entries
so Cucumber will run that feature (i.e., add 'features/internal-api.feature' to
the paths array declaration).
There was a problem hiding this comment.
as @aspiers mentioned in one of the comments above maybe it shouldnt be tested. also i added it since it was in the initial list of feature files but maybe better to not have it since it isnt really e2e
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
e2e/cucumber.mjs (1)
2-7:⚠️ Potential issue | 🟠 MajorAdd
internal-api.featureto the Cucumberpaths.
features/internal-api.featureis still not included, so the new internal API scenarios won’t execute in this suite.➕ Suggested patch
paths: [ 'features/passwordless-authentication.feature', 'features/automatic-account-creation.feature', 'features/consent-screen.feature', + 'features/internal-api.feature', 'features/account-settings.feature', ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/cucumber.mjs` around lines 2 - 7, The Cucumber `paths` array is missing the new feature file, so add 'features/internal-api.feature' to the existing paths array (the array literal containing 'features/passwordless-authentication.feature', 'features/automatic-account-creation.feature', 'features/consent-screen.feature', 'features/account-settings.feature') so the internal API scenarios run as part of the suite; update the `paths` array in the cucumber configuration to include 'features/internal-api.feature'.
🧹 Nitpick comments (3)
e2e/support/mailpit.ts (1)
89-104: Heuristic OTP extraction assumes uppercase alphanumeric codes.The alphanumeric patterns (
/^([A-Z0-9]{4,12})$/mand/\bis:\s*([A-Z0-9]{4,12})\s*$/m) only match uppercase letters. If the auth service ever generates lowercase alphanumeric OTPs, extraction will fail silently and fall through to the error.This appears intentional based on the OTP alphabet defined elsewhere (
ABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789), but the comment should clarify this assumption.📝 Optional: Add case-insensitive matching or document the assumption
Either add the
iflag for case-insensitivity:- const isolatedAlphanum = /^([A-Z0-9]{4,12})$/m.exec(body) + const isolatedAlphanum = /^([A-Z0-9]{4,12})$/mi.exec(body)Or document the uppercase assumption in the comment block (lines 71-83).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/support/mailpit.ts` around lines 89 - 104, The OTP extraction regexes (isolatedAlphanum and inlineAlphanum) only match uppercase letters and will fail if a lowercase alphanumeric OTP is sent; update the two patterns (the /^([A-Z0-9]{4,12})$/m and /\bis:\s*([A-Z0-9]{4,12})\s*$/m used in the isolatedAlphanum and inlineAlphanum checks) to be case-insensitive by adding the i flag, or alternatively add a clear comment near these heuristics stating the function assumes uppercase-only OTPs to make the intention explicit; adjust tests if needed to reflect the chosen behavior.e2e/step-definitions/auth.steps.ts (1)
54-62: Consider extracting the nested ternary for readability.The charset inference logic is functional but the nested ternary is hard to follow. SonarCloud flagged this as a maintainability concern.
♻️ Suggested 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 54 - 62, The nested ternary that computes otpCharset (using inputModeAttr, patternAttr, otpLength, and testEnv.otpCharset) is hard to read; replace it with a small, well-named helper or an explicit if/else block (e.g., deriveOtpCharset(inputModeAttr, patternAttr, otpLength, testEnv)) that returns 'numeric', 'alphanumeric', or the default testEnv.otpCharset, then call mutateOtpCode('0'.repeat(otpLength), otpCharset) unchanged; reference otpLength, otpCharset, inputModeAttr, patternAttr, testEnv, and mutateOtpCode when locating the code to change.e2e/step-definitions/account-settings.steps.ts (1)
388-441: Session revocation loop is pragmatic but may be flaky.The comment acknowledges this approach's fragility. The loop revokes sessions one at a time until the secondary context is logged out, which works but depends on timing. The 2-second timeout (line 429) helps, but consider increasing it for slower CI environments.
💡 Consider increasing timeout for CI stability
await expect(secondaryPage).toHaveURL( new RegExp(`^${authBase}/account/login(\\?.*)?$`), - { timeout: 2_000 }, + { timeout: 5_000 }, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/step-definitions/account-settings.steps.ts` around lines 388 - 441, The revocation loop is brittle due to the hardcoded 2_000ms timeout when checking secondary session logout; in the When('the user revokes another session', ...) step update the expect call on secondaryPage.toHaveURL (the call using { timeout: 2_000 }) to use a larger timeout (e.g., 10_000) or preferably read a configurable value (e.g., process.env.REVOCATION_TIMEOUT_MS) so slower CI runs have more time; change the numeric timeout at the expect(new RegExp(`^${authBase}/account/login(\\?.*)?$`), { timeout: 2_000 }) location and ensure the env-derived value is parsed to a number and has a sensible default.
🤖 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/pds.steps.ts`:
- Around line 131-141: The step definition for 'the createSession request fails
with an auth error' currently treats any 4xx-or-5xx as a failure; update the
assertion that checks this.lastHttpStatus so it only accepts 4xx statuses
(400–499). In the step function (the anonymous function using
this.lastHttpStatus), replace the loose check (status < 400) with a tighter
check that throws if status is not in the 400–499 range (e.g., if status < 400
|| status >= 500) and keep the existing error messages and guards (the initial
pending check for testEnv.mailpitPass and the "No HTTP status" throw).
---
Duplicate comments:
In `@e2e/cucumber.mjs`:
- Around line 2-7: The Cucumber `paths` array is missing the new feature file,
so add 'features/internal-api.feature' to the existing paths array (the array
literal containing 'features/passwordless-authentication.feature',
'features/automatic-account-creation.feature',
'features/consent-screen.feature', 'features/account-settings.feature') so the
internal API scenarios run as part of the suite; update the `paths` array in the
cucumber configuration to include 'features/internal-api.feature'.
---
Nitpick comments:
In `@e2e/step-definitions/account-settings.steps.ts`:
- Around line 388-441: The revocation loop is brittle due to the hardcoded
2_000ms timeout when checking secondary session logout; in the When('the user
revokes another session', ...) step update the expect call on
secondaryPage.toHaveURL (the call using { timeout: 2_000 }) to use a larger
timeout (e.g., 10_000) or preferably read a configurable value (e.g.,
process.env.REVOCATION_TIMEOUT_MS) so slower CI runs have more time; change the
numeric timeout at the expect(new RegExp(`^${authBase}/account/login(\\?.*)?$`),
{ timeout: 2_000 }) location and ensure the env-derived value is parsed to a
number and has a sensible default.
In `@e2e/step-definitions/auth.steps.ts`:
- Around line 54-62: The nested ternary that computes otpCharset (using
inputModeAttr, patternAttr, otpLength, and testEnv.otpCharset) is hard to read;
replace it with a small, well-named helper or an explicit if/else block (e.g.,
deriveOtpCharset(inputModeAttr, patternAttr, otpLength, testEnv)) that returns
'numeric', 'alphanumeric', or the default testEnv.otpCharset, then call
mutateOtpCode('0'.repeat(otpLength), otpCharset) unchanged; reference otpLength,
otpCharset, inputModeAttr, patternAttr, testEnv, and mutateOtpCode when locating
the code to change.
In `@e2e/support/mailpit.ts`:
- Around line 89-104: The OTP extraction regexes (isolatedAlphanum and
inlineAlphanum) only match uppercase letters and will fail if a lowercase
alphanumeric OTP is sent; update the two patterns (the /^([A-Z0-9]{4,12})$/m and
/\bis:\s*([A-Z0-9]{4,12})\s*$/m used in the isolatedAlphanum and inlineAlphanum
checks) to be case-insensitive by adding the i flag, or alternatively add a
clear comment near these heuristics stating the function assumes uppercase-only
OTPs to make the intention explicit; adjust tests if needed to reflect the
chosen 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: 58435f6d-28ec-42df-ba66-930e305b4f92
📒 Files selected for processing (26)
docs/design/testing-gaps.mde2e/.env.examplee2e/README.mde2e/cucumber.mjse2e/step-definitions/account-settings.steps.tse2e/step-definitions/auth.steps.tse2e/step-definitions/common.steps.tse2e/step-definitions/consent.steps.tse2e/step-definitions/email.steps.tse2e/step-definitions/internal-api.steps.tse2e/step-definitions/pds.steps.tse2e/support/env.tse2e/support/flows.tse2e/support/hooks.tse2e/support/mailpit.tse2e/support/utils.tse2e/support/world.tsfeatures/IMPLEMENTATION-PLAN.mdfeatures/account-recovery.featurefeatures/account-settings.featurefeatures/automatic-account-creation.featurefeatures/client-branding.featurefeatures/consent-screen.featurefeatures/email-delivery.featurefeatures/internal-api.featurefeatures/passwordless-authentication.feature
✅ Files skipped from review due to trivial changes (7)
- features/account-recovery.feature
- docs/design/testing-gaps.md
- features/client-branding.feature
- e2e/.env.example
- features/email-delivery.feature
- e2e/README.md
- e2e/support/utils.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- e2e/support/env.ts
- features/passwordless-authentication.feature
- features/IMPLEMENTATION-PLAN.md
- e2e/step-definitions/email.steps.ts
- e2e/step-definitions/common.steps.ts
- e2e/support/world.ts
- e2e/support/hooks.ts
- features/internal-api.feature
Kzoeps
force-pushed
the
e2e-test-additional
branch
from
a809c0a to
b0d5804
Compare
|
@coderabbitai resume reviews |
|
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
🧹 Nitpick comments (6)
e2e/step-definitions/account-settings.steps.ts (2)
388-441: Session revocation loop is pragmatic but inherently flaky.The comment on lines 401-405 correctly documents the limitation: without product-side identifiers linking UI rows to browser contexts, the test must iterate. The 2-second timeout fallback and explicit error if the secondary session isn't revoked are reasonable safeguards.
Consider adding a brief inline comment noting this is a known limitation for future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/step-definitions/account-settings.steps.ts` around lines 388 - 441, Add a short inline comment clarifying that the loop in the "When('the user revokes another session', ...)" step is a known/flaky workaround due to lack of product-side identifiers (so maintainers understand why we iterate over revokeButtons and use the 2_000ms secondaryPage timeout), placing the note near the existing pragmatic comment block above the for loop (which references revokeButtons, secondaryPage, and revokedSecondarySession).
17-35: Static analysis warnings are false positives here.The
escapeForRegexandassertHandleIsRandomSubdomainfunctions construct regexes fromtestEnvvalues (controlled test configuration), not user input. ReDoS risk is negligible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/step-definitions/account-settings.steps.ts` around lines 17 - 35, The static-analysis ReDoS warnings are false positives because testEnv values are trusted test configuration; update assertHandleIsRandomSubdomain to build the regex safely using the existing escapeForRegex(getPdsDomain()) and replace the manual pdsDomain.replace(...) call, and add a short inline linter suppression comment (e.g., /* eslint-disable-next-line no-redos */) immediately above the RegExp construction in assertHandleIsRandomSubdomain to silence the warning while keeping escapeForRegex and getPdsDomain intact.e2e/step-definitions/auth.steps.ts (1)
54-60: Consider extracting nested ternary for readability.The charset inference logic works correctly but the nested ternary is dense. An extracted helper would improve readability, though this is optional.
♻️ Optional: Extract charset inference
+function inferOtpCharset( + inputModeAttr: string | null, + patternAttr: string | null, + otpLength: number, +): 'numeric' | 'alphanumeric' { + if (inputModeAttr === 'numeric' || patternAttr === `[0-9]{${otpLength}}`) { + return 'numeric' + } + if (patternAttr === `[A-Z0-9]{${otpLength}}`) { + return 'alphanumeric' + } + return testEnv.otpCharset +} - const otpCharset = - inputModeAttr === 'numeric' || patternAttr === `[0-9]{${otpLength}}` - ? 'numeric' - : patternAttr === `[A-Z0-9]{${otpLength}}` - ? 'alphanumeric' - : testEnv.otpCharset + const otpCharset = inferOtpCharset(inputModeAttr, patternAttr, otpLength)🤖 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 54 - 60, Extract the nested ternary that computes otpCharset into a small helper function (e.g., inferOtpCharset) that accepts inputModeAttr, patternAttr, otpLength and testEnv and returns 'numeric' | 'alphanumeric' | testEnv.otpCharset; replace the inline ternary with a call to inferOtpCharset where otpCharset is assigned, keeping the existing logic: prefer 'numeric' when inputModeAttr === 'numeric' or patternAttr === `[0-9]{${otpLength}}`, else 'alphanumeric' when patternAttr === `[A-Z0-9]{${otpLength}}`, otherwise fall back to testEnv.otpCharset.e2e/step-definitions/pds.steps.ts (1)
46-63: Static analysis false positive — ReDoS risk is negligible here.The
pdsDomainvalue comes fromtestEnv.pdsUrl, a controlled test configuration, not user input. The escaped regex pattern is safe. ThereplaceAll/String.rawsuggestions are cosmetic and optional.🤖 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 46 - 63, The regex flagged for ReDoS is a false positive because pdsDomain is derived from the controlled test configuration (testEnv.pdsUrl) rather than user input; update the Then step (the step definition that builds the pattern variable) to add a concise inline comment explaining that testEnv.pdsUrl is trusted/config-controlled and that escaping via pdsDomain.replace(/\./g,'\\.') makes the RegExp safe, so the static analyzer can be ignored here; leave the RegExp construction (pattern = new RegExp(...)) and the validation of this.userHandle unchanged.e2e/step-definitions/internal-api.steps.ts (2)
36-42: Optional DRY refactor for response persistence.The repeated
status/body -> worldassignments can be centralized into a small helper to reduce duplication and future drift.Also applies to: 64-66, 100-102, 131-133, 157-159, 169-171, 181-183
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/step-definitions/internal-api.steps.ts` around lines 36 - 42, Create a small helper (e.g., setLastApiResponse or persistApiResponse) that accepts the result of callInternalApi (an object with status and body) and assigns this.lastHttpStatus = status and this.lastApiResponse = body; then replace the repeated two-line assignments in internal-api.steps.ts (the call sites around the shown block and the other occurrences you noted) with a single call to that helper to centralize response persistence and avoid duplication.
150-156: Extract repeated magic literals into module constants.The incorrect secret and static test identifiers are repeated inline. Hoisting them to
SCREAMING_SNAKE_CASEconstants will improve readability and reduce typo risk.♻️ Proposed refactor
+const INCORRECT_INTERNAL_SECRET = 'definitely-wrong-secret' +const STATIC_TEST_EMAIL = '[email protected]' +const STATIC_TEST_HANDLE = 'somehandle.pds.test' + When( 'the check-handle endpoint is called with an incorrect secret', async function (this: EpdsWorld) { const { status, body } = await callInternalApi( - `/_internal/check-handle?handle=somehandle.pds.test`, - 'definitely-wrong-secret', + `/_internal/check-handle?handle=${STATIC_TEST_HANDLE}`, + INCORRECT_INTERNAL_SECRET, ) @@ When( 'an internal endpoint is called without the secret header', async function (this: EpdsWorld) { const { status, body } = await callInternalApi( - `/_internal/[email protected]`, + `/_internal/account-by-email?email=${STATIC_TEST_EMAIL}`, null, ) @@ When( 'an internal endpoint is called with an incorrect secret', async function (this: EpdsWorld) { const { status, body } = await callInternalApi( - `/_internal/[email protected]`, - 'definitely-wrong-secret', + `/_internal/account-by-email?email=${STATIC_TEST_EMAIL}`, + INCORRECT_INTERNAL_SECRET, )As per coding guidelines, "Constants naming convention: use
SCREAMING_SNAKE_CASEfor module-level magic values,camelCasefor const variables".Also applies to: 165-167, 177-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/step-definitions/internal-api.steps.ts` around lines 150 - 156, Extract repeated magic literals (e.g., 'definitely-wrong-secret' and 'somehandle.pds.test' used in the callInternalApi invocation inside the When step and the similar literals at the other occurrences) into module-level SCREAMING_SNAKE_CASE constants (e.g., INCORRECT_SECRET, TEST_HANDLE) at the top of e2e/step-definitions/internal-api.steps.ts, then replace the inline strings in the When step and the other referenced calls (the callInternalApi usages around the other noted lines) with those constants so the tests use centralized, well-named values.
🤖 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/account-settings.steps.ts`:
- Around line 388-441: Add a short inline comment clarifying that the loop in
the "When('the user revokes another session', ...)" step is a known/flaky
workaround due to lack of product-side identifiers (so maintainers understand
why we iterate over revokeButtons and use the 2_000ms secondaryPage timeout),
placing the note near the existing pragmatic comment block above the for loop
(which references revokeButtons, secondaryPage, and revokedSecondarySession).
- Around line 17-35: The static-analysis ReDoS warnings are false positives
because testEnv values are trusted test configuration; update
assertHandleIsRandomSubdomain to build the regex safely using the existing
escapeForRegex(getPdsDomain()) and replace the manual pdsDomain.replace(...)
call, and add a short inline linter suppression comment (e.g., /*
eslint-disable-next-line no-redos */) immediately above the RegExp construction
in assertHandleIsRandomSubdomain to silence the warning while keeping
escapeForRegex and getPdsDomain intact.
In `@e2e/step-definitions/auth.steps.ts`:
- Around line 54-60: Extract the nested ternary that computes otpCharset into a
small helper function (e.g., inferOtpCharset) that accepts inputModeAttr,
patternAttr, otpLength and testEnv and returns 'numeric' | 'alphanumeric' |
testEnv.otpCharset; replace the inline ternary with a call to inferOtpCharset
where otpCharset is assigned, keeping the existing logic: prefer 'numeric' when
inputModeAttr === 'numeric' or patternAttr === `[0-9]{${otpLength}}`, else
'alphanumeric' when patternAttr === `[A-Z0-9]{${otpLength}}`, otherwise fall
back to testEnv.otpCharset.
In `@e2e/step-definitions/internal-api.steps.ts`:
- Around line 36-42: Create a small helper (e.g., setLastApiResponse or
persistApiResponse) that accepts the result of callInternalApi (an object with
status and body) and assigns this.lastHttpStatus = status and
this.lastApiResponse = body; then replace the repeated two-line assignments in
internal-api.steps.ts (the call sites around the shown block and the other
occurrences you noted) with a single call to that helper to centralize response
persistence and avoid duplication.
- Around line 150-156: Extract repeated magic literals (e.g.,
'definitely-wrong-secret' and 'somehandle.pds.test' used in the callInternalApi
invocation inside the When step and the similar literals at the other
occurrences) into module-level SCREAMING_SNAKE_CASE constants (e.g.,
INCORRECT_SECRET, TEST_HANDLE) at the top of
e2e/step-definitions/internal-api.steps.ts, then replace the inline strings in
the When step and the other referenced calls (the callInternalApi usages around
the other noted lines) with those constants so the tests use centralized,
well-named values.
In `@e2e/step-definitions/pds.steps.ts`:
- Around line 46-63: The regex flagged for ReDoS is a false positive because
pdsDomain is derived from the controlled test configuration (testEnv.pdsUrl)
rather than user input; update the Then step (the step definition that builds
the pattern variable) to add a concise inline comment explaining that
testEnv.pdsUrl is trusted/config-controlled and that escaping via
pdsDomain.replace(/\./g,'\\.') makes the RegExp safe, so the static analyzer can
be ignored here; leave the RegExp construction (pattern = new RegExp(...)) and
the validation of this.userHandle unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72d9899b-dd55-4264-b228-d24805075f90
📒 Files selected for processing (26)
docs/design/testing-gaps.mde2e/.env.examplee2e/README.mde2e/cucumber.mjse2e/step-definitions/account-settings.steps.tse2e/step-definitions/auth.steps.tse2e/step-definitions/common.steps.tse2e/step-definitions/consent.steps.tse2e/step-definitions/email.steps.tse2e/step-definitions/internal-api.steps.tse2e/step-definitions/pds.steps.tse2e/support/env.tse2e/support/flows.tse2e/support/hooks.tse2e/support/mailpit.tse2e/support/utils.tse2e/support/world.tsfeatures/IMPLEMENTATION-PLAN.mdfeatures/account-recovery.featurefeatures/account-settings.featurefeatures/automatic-account-creation.featurefeatures/client-branding.featurefeatures/consent-screen.featurefeatures/email-delivery.featurefeatures/internal-api.featurefeatures/passwordless-authentication.feature
✅ Files skipped from review due to trivial changes (6)
- e2e/support/env.ts
- features/email-delivery.feature
- features/account-recovery.feature
- docs/design/testing-gaps.md
- features/client-branding.feature
- features/IMPLEMENTATION-PLAN.md
🚧 Files skipped from review as they are similar to previous changes (11)
- e2e/.env.example
- e2e/README.md
- e2e/step-definitions/email.steps.ts
- e2e/support/flows.ts
- features/internal-api.feature
- e2e/support/world.ts
- e2e/cucumber.mjs
- features/account-settings.feature
- e2e/support/mailpit.ts
- e2e/support/hooks.ts
- e2e/support/utils.ts
Kzoeps
force-pushed
the
e2e-test-additional
branch
from
8accba8 to
b0d5804
Compare
|
opening this from origin intsead of kzoeps |
|
2 participants
Builds on #42
createAccountViaOAuthandresetBrowserContextextracted to shared helpers, Mailpit inbox clearing switched to per-recipient targeting to prevent cross-scenario OTP bleed"[email protected]"preconditions with dynamic per-run emails; adds@email,@docker-only, and@pendingtags where appropriategetSessiondeletion assertion (superseded bydescribeRepocheck) and removes current handle display from account settings UI (handle update form retained; full handle-change flow tracked as pending)Summary by CodeRabbit
Tests
Chores
Documentation