Repository navigation
e2e test refactor with playwright - #63
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Playwright E2E infrastructure and tests (signup, social auth, tenant, users), Mailpit and Postgres fixtures and helpers, a test environment startup script, Docker Mailpit service, Playwright config and dependency updates, a small identity adapter change to enable signup emails for tests, and a documentation debt entry. No production runtime logic altered. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant Page as Playwright Page
participant API as API Server
participant Mailpit as Mailpit
participant DB as PostgreSQL
rect rgba(100,150,200,0.5)
Note over Test,DB: Sign Up + Verification Flow
Test->>Test: generate unique email
Test->>DB: cleanupUser(email)
Test->>Page: fill signup form & submit
Page->>API: POST /auth/signup
API->>DB: create unverified user
API->>Mailpit: send verification email
Test->>Mailpit: waitForEmail(recipient)
Mailpit-->>Test: return email with verification link
Test->>Page: navigate to verification link
Page->>API: complete verification
API->>DB: mark user verified
Test->>DB: isEmailVerified(email)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 25
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/e2e/auth.spec.ts`:
- Around line 50-57: The current try/catch around the db.userExists(userEmail)
call swallows real DB errors; remove the silent catch so failures surface (or in
the catch call Jest's fail/throw a new Error) so the test stops with a clear
message; locate the try/catch surrounding db.userExists(userEmail) in
auth.spec.ts and either delete the try/catch block entirely or replace the catch
body with a failing assertion/throw that includes the original error and context
(e.g. mention userEmail and that the DB check failed).
- Around line 5-9: The top-level variable userEmail is initialized at module
load but immediately overwritten in the first test, so remove the dead
initialization: change the declaration "let userEmail =
`auth-test-${Date.now()}@example.com`;" to an uninitialized declaration "let
userEmail;" (or "let userEmail: string;") and ensure the existing assignment
inside the first test remains (or alternatively move the timestamped
initialization into that test). Update any references to userEmail to rely on
the test-local assignment so the module-level initial value is not used.
- Line 63: Replace the unreliable call to page.waitForLoadState('networkidle')
with a deterministic assertion that the post-redirect state is reached: use
page.waitForURL or expect(page).toHaveURL with the expected URL (or
page.waitForSelector for a specific element that appears after redirect) instead
of page.waitForLoadState('networkidle') so the test explicitly waits for the
known navigation or visible element.
In `@apps/web/e2e/fixtures/db.ts`:
- Around line 51-87: The makeSystemAdmin function performs deletion and
insertion across multiple queries without a transaction; change it to get a
client from this.pool (e.g., const client = await this.pool.connect()), call
client.query('BEGIN'), run the DELETE from "member" and the INSERT into "member"
(and any preceding INSERTs that must be atomic) using the client instead of
this.pool, then client.query('COMMIT') on success and client.query('ROLLBACK')
in the catch block, ensuring client.release() in a finally block so the user is
never left without a membership on partial failure.
In `@apps/web/e2e/fixtures/mailpit.ts`:
- Around line 18-22: The constructor in apps/web/e2e/fixtures/mailpit.ts
currently hardcodes baseUrl to 'http://localhost:8025/api/v1'; change it to read
a configurable environment variable (e.g., process.env.MAILPIT_URL or
process.env.MAILPIT_BASE_URL) and fall back to 'http://localhost:8025' if unset,
then append '/api/v1' to form this.baseUrl; ensure the constructor (and the
baseUrl property) handles optional trailing slashes so the resulting URL is
valid in CI/Docker environments.
- Around line 87-101: The extractLink function currently grabs body = email.HTML
|| email.Text which prevents trying the other part if the chosen part exists but
the regex doesn't match; update extractLink to first attempt the regex against
email.HTML (if present) and if that yields no match then attempt matching
against email.Text, throwing the same error only if neither part produces a
capturing group; keep the HTML entity decode step (replaceAll('&','&')) and
the console log, and reference the email.HTML, email.Text, extractLink and match
variables when locating the change.
- Around line 64-69: When fetching the full message with
this.apiContext.get(`${this.baseUrl}/message/${msgSummary.ID}`) you must check
msgResponse.ok before calling msgResponse.json(); if !msgResponse.ok read the
body/text for diagnostics (await msgResponse.text()) and either throw an
informative error including status and body or log and return/continue so you
don't attempt to parse an error response into EmailMessage; update the code
around msgResponse and fullMessage to handle this failure path cleanly.
In `@apps/web/e2e/social-auth.spec.ts`:
- Around line 14-34: Tests "Sign In with Google (Mocked)" and "Sign Up with
Google (Mocked)" only assert button visibility; after clicking googleBtn they
should verify an observable outcome. Update the two tests to assert navigation
or network activity: for navigation use page.url() or
expect(page).toHaveURL(...) after await googleBtn.click(), and/or await
page.waitForResponse(resp => resp.url().includes('mocked-google-endpoint') &&
resp.status() === 200) to confirm the auth request fired. Ensure you reference
the existing googleBtn variable and keep waits/timeouts appropriate so the
assertions are deterministic.
- Around line 6-11: The mock in the page.route handler uses a hardcoded
'http://localhost:5174' which will break if base URL/port changes; update the
handler in the async (route) => { ... } block to compute the callback origin
dynamically (for example derive origin via new URL(page.url()).origin or use
page.context().baseURL) and build the json.url using that origin plus
'/auth/callback?code=mock_google_code', then call route.fulfill with the
constructed json so the test will follow the configured base URL instead of the
hardcoded localhost:5174.
In `@apps/web/e2e/start-test-env.sh`:
- Around line 21-36: Replace the fixed sleep calls with active health-check
polling: implement a loop that repeatedly curls the API health endpoint (e.g.,
VITE_API_URL's /health or http://localhost:3002/health) before starting the web,
and after launching the web process (pnpm --filter web dev) poll the web health
endpoint (e.g., http://localhost:5174/ or a known /health or /favicon.ico) until
it responds 200; use WEB_PID to detect background process exit, fail fast if the
process dies, and enforce a configurable timeout (e.g., 60s) that prints
apps/web/e2e/web.log and exits non-zero on failure so CI gets logs.
- Around line 1-5: Add "set -e" at the top of start-test-env.sh and add an EXIT
trap that kills any child/backgrounded processes (capture PIDs when you start
the API and Web services and store them in variables) to ensure cleanup on
script exit or when Playwright terminates the script; update the existing
lsof/kill logic to run in the trap as well or use kill -- -$PID to terminate
process groups so no orphaned API/Web processes remain (refer to
start-test-env.sh and the existing lsof lines to locate where to add set -e, PID
captures, and the trap).
In `@apps/web/e2e/tenant.spec.ts`:
- Around line 63-127: The "Create Tenant" test is brittle due to speculative
navigation and multiple fallback selectors (page.goto('/onboarding' |
'/admin/tenants'), createBtn/createTenantBtn checks, try/catch on
getByLabel/getByPlaceholder); replace this with a single authoritative flow:
navigate to the known tenant-creation route with page.goto('/admin/tenants') (or
the project’s canonical URL), remove all alternative commented routes and
console logs, use deterministic selectors for the create action and form inputs
(replace createBtn/createTenantBtn and the try/catch around
getByLabel/getByPlaceholder with the exact role/label/placeholder used in the
app), and if the UI is not yet stable convert the test declaration test('Create
Tenant'...) to test.fixme(...) with a TODO comment referencing stabilization
work instead of keeping multiple fallbacks.
- Line 17: The beforeEach currently calls mailpit.deleteAllMessages(), which
wipes global mailbox and causes flakes when specs run in parallel; remove that
call and instead perform targeted cleanup or filtering: replace
deleteAllMessages() with a scoped deletion (e.g., mailpit.deleteMessages or
equivalent that accepts a recipient/filter) for the test's specific recipient,
or avoid deletion and update waitForEmail to filter by recipient/email subject
and assert only on messages matching that recipient; locate usages in the
beforeEach and the waitForEmail call to apply the change.
- Line 96: The test uses page.waitForLoadState('networkidle') which is flaky;
replace it in tenant.spec.ts by waiting for a deterministic signal of readiness
instead — e.g., use page.waitForSelector with a stable element selector that
indicates the tenant page is ready (a header, form, or data grid) or use
page.waitForURL with the expected route; update the line with
page.waitForLoadState('networkidle') to one of these deterministic waits
(reference: the page object used in the test and the tenant.spec.ts test case)
so the test no longer depends on networkidle.
In `@apps/web/e2e/users.spec.ts`:
- Line 17: The call to mailpit.deleteAllMessages() clears Mailpit globally and
breaks parallel tests; update the cleanup to only remove messages for this test
by querying Mailpit for messages that match the test-specific recipient or a
test-only header/pattern and then delete those messages (e.g., use
mailpit.getMessages()/search and call mailpit.deleteMessage(id) or a filtered
delete API) instead of mailpit.deleteAllMessages(); ensure the selector uses the
same test email or unique pattern used when sending messages so other tests'
emails remain intact.
- Around line 7-50: Extract the repeated signup→verify→promote→relogin flow into
a shared helper named createVerifiedAdmin(page, mailpit, db, opts) (e.g.,
e2e/helpers/auth-setup.ts); move the logic that generates adminEmail, signs up
via page.fill/page.getByRole, waits for mailpit.waitForEmail and uses
mailpit.extractLink, calls db.makeSystemAdmin, clears cookies via
page.context().clearCookies(), performs the login steps (page.goto, page.fill,
page.getByRole) and waits for the dashboard
(expect(page.getByText('Dashboard').first()).toBeVisible); have the helper
return { adminEmail, adminPassword } and accept options (like role) so tests
call await createVerifiedAdmin(page, mailpit, db, { role: 'system' }) in
beforeEach to replace the duplicated block in users.spec.ts, tenant.spec.ts and
auth.spec.ts.
- Line 59: Move the test.describe.configure({ mode: 'serial' }) call to the top
of the describe block so configuration appears before any hooks; locate the
describe(...) that wraps these tests in apps/web/e2e/users.spec.ts and place the
test.describe.configure call immediately inside that describe, above beforeEach
and afterEach hooks and any tests, ensuring the configuration runs first.
In `@docker-compose.yml`:
- Around line 44-45: YAML lint fails because inline comments need at least two
spaces before them; update the port mapping lines containing the strings
"8025:8025" and "1025:1025" in docker-compose.yml so each has two spaces before
the trailing comment (e.g., ensure there are two spaces between the value and
the "# UI" / "# SMTP" comments) to satisfy YAMLlint formatting rules.
- Around line 38-47: Add mailpit to the api service's dependency list so the api
won't start before Mailpit is started: update the docker-compose service named
"api" to include "mailpit" under its depends_on (using the simple condition form
since mailpit lacks a healthcheck), e.g., add depends_on: mailpit: condition:
service_started (or the equivalent short form supported by your Compose
version), referencing the "api" and "mailpit" service names so startup ordering
ensures Mailpit is listening on port 1025 before api attempts SMTP connections.
In `@packages/identity/src/adapters/better-auth.adapter.ts`:
- Line 138: Revert the temporary test hack by restoring sendOnSignUp to its
original behavior (remove or set sendOnSignUp back to false) in the adapter
configuration where sendOnSignUp is currently set; do not keep the hardcoded
true. If tests require email-on-signup, implement a test-only override instead:
read an env var (e.g., TEST_SEND_ON_SIGNUP) or a config flag in the adapter
initialization (only applied when process.env.NODE_ENV === 'test') or set the
adapter option from test setup code, and update tests to set that env/config
value rather than changing sendOnSignUp in better-auth.adapter.ts. Ensure the
unique symbol sendOnSignUp in this file is the only place changed.
- Around line 143-153: Remove the debug file-write block that uses
appendFileSync and writes token, url, and user.email to /tmp (exposes
credentials and PII and blocks the event loop); instead delete the entire
try/import fs block referencing appendFileSync and token/url/user.email in the
BetterAuth adapter, or replace it with a non-PII, non-blocking log (use
fs.promises.appendFile only for ephemeral, non-sensitive traces) and a strict
environment guard (e.g., a dedicated DEBUG_EMAIL_CAPTURE flag) if you must
capture emails for tests — otherwise integrate test-mode email capture via
Mailpit as described in the PR rather than writing tokens to disk.
In `@scripts/cleanup-test-users.ts`:
- Around line 35-47: The cleanupTestUsers function currently catches errors in
its inner catch and only logs them, causing the top-level cleanupTestUsers()
Promise to resolve and .then(() => process.exit(0)) to run; change the inner
catch inside cleanupTestUsers to re-throw the caught error (after logging) so
the Promise rejects and the outer .catch runs and exits with non-zero status;
keep the finally block (await pool.end()) to ensure cleanup always runs before
the error is propagated.
- Around line 14-26: The cleanup only deletes emails matching
'auth-test-%'—update both SQL calls (the DELETE FROM "verification" and DELETE
FROM "user" executed via pool.query) to include all test prefixes used by e2e
tests (e.g., email/identifier LIKE 'auth-test-%' OR LIKE 'tenant-admin-%' OR
LIKE 'user-admin-%' OR LIKE 'auth-signin-%' OR LIKE 'invitee-%') so verification
rows and users for those patterns are removed, or alternatively add a clear
comment/documentation above these pool.query calls stating the script
intentionally only handles auth-test- if you prefer not to broaden the patterns.
- Around line 19-26: Before deleting users in scripts/cleanup-test-users.ts,
explicitly delete verification records that match the test email pattern: run a
DELETE on the verification table using identifier LIKE 'auth-test-%' (or the
same identifier pattern used in e2e tests) and optionally RETURNING
identifier/email for logging; do this deletion prior to the DELETE FROM "user"
query so orphaned verification rows are removed (mirrors DbFixture.cleanupUser
behavior).
In `@TECHNICAL_DEBT.md`:
- Around line 76-82: The headings "Low Priority" and "Completed Items" were
incorrectly demoted to third-level headers (###) and should be restored to
second-level (##) to match "High Priority" and "Medium Priority" and make them
peer sections; update the "Low Priority" and "Completed Items" headings in
TECHNICAL_DEBT.md to use the same header level (## Low Priority and ## Completed
Items) so they appear at the same document hierarchy as "High Priority" and
"Medium Priority".
Comment on lines
+14
to
+26
| await pool.query(` | ||
| DELETE FROM "verification" | ||
| WHERE identifier LIKE 'auth-test-%' | ||
| `); | ||
|
|
||
| // 2. Delete users matching the test pattern | ||
| // The pattern used in e2e tests is `auth-test-${Date.now()}@example.com` | ||
| // So we look for emails starting with 'auth-test-' | ||
| const result = await pool.query(` | ||
| DELETE FROM "user" | ||
| WHERE email LIKE 'auth-test-%' | ||
| RETURNING email; | ||
| `); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all test email patterns used in e2e specs
find . -type f -name '*.spec.ts' -o -name '*.e2e.ts' | head -20Repository: pramodnarayana/nexiom
Length of output: 1244
🏁 Script executed:
#!/bin/bash
# Find all email patterns in e2e test files
rg -n '@(test|example)\.com' --type ts | grep -i 'email\|auth\|user' | head -30Repository: pramodnarayana/nexiom
Length of output: 3406
🏁 Script executed:
#!/bin/bash
# Search for tenant-admin pattern mentioned in review
rg -n 'tenant-admin' --type tsRepository: pramodnarayana/nexiom
Length of output: 154
🏁 Script executed:
#!/bin/bash
# Search for user-mgmt pattern mentioned in review
rg -n 'user-mgmt' --type tsRepository: pramodnarayana/nexiom
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Look at the cleanup script completely
fd cleanup-test-users.ts -x cat -n {}Repository: pramodnarayana/nexiom
Length of output: 1820
Cleanup script only targets auth-test-% — misses other test user patterns.
The e2e tests use multiple email patterns that this cleanup script won't handle:
tenant-admin-*in tenant testsuser-admin-*in user testsauth-signin-*in auth testsinvitee-*in user tests
Either broaden the LIKE pattern to catch these prefixes (e.g., '%-admin-%' OR email LIKE '%-signin-%' OR ...), or document that this script is auth-test-only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/cleanup-test-users.ts` around lines 14 - 26, The cleanup only deletes
emails matching 'auth-test-%'—update both SQL calls (the DELETE FROM
"verification" and DELETE FROM "user" executed via pool.query) to include all
test prefixes used by e2e tests (e.g., email/identifier LIKE 'auth-test-%' OR
LIKE 'tenant-admin-%' OR LIKE 'user-admin-%' OR LIKE 'auth-signin-%' OR LIKE
'invitee-%') so verification rows and users for those patterns are removed, or
alternatively add a clear comment/documentation above these pool.query calls
stating the script intentionally only handles auth-test- if you prefer not to
broaden the patterns.
Comment on lines
+35
to
+47
| } catch (error) { | ||
| console.error('❌ Error cleaning up test users:', error); | ||
| } finally { | ||
| await pool.end(); | ||
| } | ||
| } | ||
|
|
||
| cleanupTestUsers() | ||
| .then(() => process.exit(0)) | ||
| .catch((error) => { | ||
| console.error(error); | ||
| process.exit(1); | ||
| }); |
Contributor
There was a problem hiding this comment.
Error is swallowed — script exits 0 on failure.
The inner try/catch (line 35) catches and logs the error but does not re-throw. The outer .then(() => process.exit(0)) on line 43 always runs, so the script reports success even when the cleanup fails. The outer .catch on line 44 is unreachable.
Proposed fix — re-throw from inner catch
} catch (error) {
console.error('❌ Error cleaning up test users:', error);
+ throw error;
} finally {
await pool.end();
}📝 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
| } catch (error) { | |
| console.error('❌ Error cleaning up test users:', error); | |
| } finally { | |
| await pool.end(); | |
| } | |
| } | |
| cleanupTestUsers() | |
| .then(() => process.exit(0)) | |
| .catch((error) => { | |
| console.error(error); | |
| process.exit(1); | |
| }); | |
| } catch (error) { | |
| console.error('❌ Error cleaning up test users:', error); | |
| throw error; | |
| } finally { | |
| await pool.end(); | |
| } | |
| } | |
| cleanupTestUsers() | |
| .then(() => process.exit(0)) | |
| .catch((error) => { | |
| console.error(error); | |
| process.exit(1); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/cleanup-test-users.ts` around lines 35 - 47, The cleanupTestUsers
function currently catches errors in its inner catch and only logs them, causing
the top-level cleanupTestUsers() Promise to resolve and .then(() =>
process.exit(0)) to run; change the inner catch inside cleanupTestUsers to
re-throw the caught error (after logging) so the Promise rejects and the outer
.catch runs and exits with non-zero status; keep the finally block (await
pool.end()) to ensure cleanup always runs before the error is propagated.
Comment on lines
+76
to
+82
| ### Low Priority | ||
|
|
||
| *No items currently tracked* | ||
|
|
||
| --- | ||
|
|
||
| ## Completed Items | ||
| ### Completed Items |
Contributor
There was a problem hiding this comment.
Heading hierarchy broken — Low Priority and Completed Items demoted to ###.
High Priority (line 7) and Medium Priority (line 70) use ##, but Low Priority and Completed Items were changed to ###. This makes them appear as subsections of "Medium Priority" rather than peer sections in the document outline.
Proposed fix
-### Low Priority
+## Low Priority-### Completed Items
+## Completed Items🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TECHNICAL_DEBT.md` around lines 76 - 82, The headings "Low Priority" and
"Completed Items" were incorrectly demoted to third-level headers (###) and
should be restored to second-level (##) to match "High Priority" and "Medium
Priority" and make them peer sections; update the "Low Priority" and "Completed
Items" headings in TECHNICAL_DEBT.md to use the same header level (## Low
Priority and ## Completed Items) so they appear at the same document hierarchy
as "High Priority" and "Medium Priority".
pramodnarayana
marked this pull request as draft
February 19, 2026 15:52
pramodnarayana
marked this pull request as ready for review
February 19, 2026 15:53
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/e2e/auth.spec.ts`:
- Around line 4-9: Move the shared variables userEmail and userPassword out of
the top-level test.describe scope and into the Sign Up test body where they are
actually used: remove the declarations "let userEmail: string;" and "const
userPassword = 'password123';" from the describe block and instead declare and
initialize them inside the Sign Up test (the test that performs signup and
cleanup in try/finally). This keeps scope local to the Sign Up test, avoids
false cross-test coupling, and leaves other tests (e.g., the Sign In test) to
use their own local variables if needed.
- Around line 84-112: Replace the manual signup/verify/logout sequence with the
existing helper createVerifiedUser to avoid duplication: import
createVerifiedUser from helpers/auth-setup at the top of the test file, then
remove the block that visits /signup, fills inputs, waits for mailpit, verifies
via verificationUrl, checks db.isEmailVerified, clears cookies and navigates to
/login, and instead call createVerifiedUser(page, mailpit, db, { email:
signInEmail, password: signInPassword }) (adjust options shape to match the
helper); ensure the helper invocation returns/sets the expected logged-out or
logged-in state for the rest of the test.
In `@apps/web/e2e/fixtures/db.ts`:
- Around line 96-108: Wrap the two DELETE statements in cleanupUser inside a DB
transaction to ensure atomicity: start a transaction (BEGIN) before deleting
from "verification" and "user", COMMIT if both succeed, and ROLLBACK on any
error; use the existing this.pool.query call pattern and ensure errors are
propagated after rollback so test teardown failures are visible. Reference the
cleanupUser method and the two SQL statements ('DELETE FROM "verification" WHERE
identifier = $1' and 'DELETE FROM "user" WHERE email = $1') when adding the
BEGIN/COMMIT/ROLLBACK and try/catch flow.
In `@apps/web/e2e/fixtures/mailpit.ts`:
- Line 109: The public method deleteAllMessages lacks an explicit return type;
update its signature from "async deleteAllMessages()" to include an explicit
Promise<void> return type (i.e., "async deleteAllMessages(): Promise<void>") so
it matches the other public methods (preserve existing implementation inside the
function).
- Line 55: The recipient matching uses t.Name === recipient which is incorrect
because recipient is an email address; update the predicate used in the
recipientMatch computation (the arrow function iterating msgSummary.To) to only
compare t.Address to recipient (remove the t.Name === recipient check) so the
match is based solely on email addresses.
In `@apps/web/e2e/helpers/auth-setup.ts`:
- Around line 1-4: The file currently imports from '@playwright/test' twice
(importing Page and expect separately); consolidate into a single import
statement that imports both symbols together (e.g., import { type Page, expect }
from '@playwright/test' or import { type Page, type Expect } as appropriate),
remove the duplicate import line, and ensure the type-only import stays prefixed
with "type" for Page while keeping expect as a regular import so consumers like
Page and expect are available where used.
- Line 56: The regex in mailpit.extractLink hardcodes "localhost" which breaks
when BETTER_AUTH_URL points elsewhere; update the extraction to derive the
expected host from process.env.BETTER_AUTH_URL (or use a host-agnostic pattern)
before calling mailpit.extractLink so verificationUrl is matched correctly.
Specifically, build or escape the host component from
process.env.BETTER_AUTH_URL and use that in the regex passed to
mailpit.extractLink (or replace the host with a generic host pattern like
https?:\/\/[^\/\s]+) so the match works in containerized/CI environments; update
the code referencing verificationUrl accordingly.
In `@apps/web/e2e/social-auth.spec.ts`:
- Around line 52-53: The Sign Up test uses expect(googleBtn).toBeVisible()
without an explicit timeout, causing flaky CI runs; update the assertion to
include a deterministic timeout (e.g., pass { timeout: 5000 }) so the call to
expect(googleBtn).toBeVisible() mirrors the Sign In test's timeout behavior
(keep references to page.getByRole('button', { name: /google/i }) and the
googleBtn variable).
In `@apps/web/e2e/start-test-env.sh`:
- Around line 4-9: The cleanup() trap handler uses $API_PID and $WEB_PID even
when they may be unset; update cleanup() to defensively check and only call
kill/wait for PIDs that are set (e.g., if [ -n "$API_PID" ]; then kill
"$API_PID" 2>/dev/null; wait "$API_PID" 2>/dev/null; fi and similarly for
$WEB_PID) so the trap (registered by trap cleanup EXIT INT TERM) won't attempt
to kill empty variables or emit errors when the script exits early.
In `@apps/web/e2e/tenant.spec.ts`:
- Around line 4-27: Move the test.describe.configure({ mode: 'serial' }) call to
the very top of the "Tenant Management" describe block, before any hooks or
tests; specifically, place the test.describe.configure invocation before the
test.beforeEach and test.afterEach blocks so the configuration is applied
according to Playwright's contract (refer to test.describe.configure,
test.beforeEach, test.afterEach, and the "Tenant Management" describe).
In `@apps/web/e2e/users.spec.ts`:
- Around line 28-62: Add a navigation step to validate the extracted invite link
so the end-to-end acceptance path is exercised: after extracting inviteLink
(from mailpit.extractLink) call page.goto(inviteLink) and assert the invite
acceptance UI loads (e.g., check for a heading or form element like
getByRole('heading', { name: /accept invitation/i }) or the signup form fields)
and, if applicable, complete the acceptance flow to a success screen; keep this
inside the existing test ('Invite User Flow') and ensure cleanup
(db.cleanupUser) still runs in the finally block.
In `@docker-compose.yml`:
- Around line 62-63: The docker-compose.yml currently sets SMTP_SECURE and
MAIL_MOCK as YAML booleans which Docker Compose rejects; update the environment
variable values for SMTP_SECURE and MAIL_MOCK to be strings (e.g., wrap them in
quotes) so they are valid env values for Docker Compose, locating the entries
named SMTP_SECURE and MAIL_MOCK in the docker-compose.yml and converting their
values from unquoted false to a quoted "false" (or another string
representation) accordingly.
---
Duplicate comments:
In `@apps/web/e2e/auth.spec.ts`:
- Line 82: The call to mailpit.deleteAllMessages() clears Mailpit globally and
breaks parallel tests; replace it with a scoped delete that only removes
messages for the test's recipient(s). Update the teardown in this spec to call
the Mailpit client method that deletes by recipient (e.g., use deleteMessages or
deleteMessagesForRecipient on the mailpit instance) and pass the test's unique
email address (or other identifying filter) instead of deleteAllMessages(),
ensuring only this test's mailbox is cleaned up.
In `@docker-compose.yml`:
- Around line 44-45: The YAML lint warning is due to missing a single space
before the inline comments on the port mappings ("8025:8025" and "1025:1025");
update those lines to ensure there is exactly one space before the comment
(e.g., "8025:8025 # UI" and "1025:1025 # SMTP") so the inline comments follow
YAMLlint spacing rules.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/e2e/auth.spec.ts`:
- Around line 50-52: The test comment claims the user should be "unverified" but
the code only calls db.userExists(userEmail); update the test to assert the
user's verification status explicitly: call the DB method that fetches the user
record (e.g., db.getUser(userEmail) or db.findUserByEmail) and then
expect(user.isVerified).toBe(false) (in addition to or instead of
expect(exists).toBe(true)); ensure you reference the existing userEmail variable
and replace or augment the db.userExists check with the explicit
verification-status assertion.
- Around line 5-7: Update the misleading comment inside the test.describe block:
remove or replace the line "// Shared user credentials for the suite, generated
once per run" so it accurately reflects that each test generates its own
credentials; locate the comment near the test.describe('Authentication Flows',
() => { ... }) block in auth.spec.ts and either delete the comment or change it
to state that credentials are generated per test (e.g., "Each test generates its
own email/password").
- Around line 44-46: The test uses a hardcoded regex for mailpit.extractLink
(verificationUrl) which breaks in CI/Docker; change it to derive the URL regex
from process.env.BETTER_AUTH_URL like the createVerifiedUser helper in
auth-setup.ts does—escape the env value into a safe regex and use that to build
the pattern passed to mailpit.extractLink so extraction works across
environments.
In `@apps/web/e2e/fixtures/db.ts`:
- Line 17: The three async methods close(), makeSystemAdmin(), and cleanupUser()
are missing explicit Promise<void> return type annotations; update their
signatures to include : Promise<void> (e.g., async close(): Promise<void> { ...
}) to match the style used by isEmailVerified() and userExists() and keep the
API consistent across the fixture.
In `@apps/web/e2e/fixtures/mailpit.ts`:
- Around line 44-47: The code currently does console.warn on non-ok Mailpit
responses and immediately continues, which skips the existing poll delay and
busy-spins; modify the non-ok branch (the block that checks response.ok()) to
await the same delay used later (await new Promise(resolve =>
setTimeout(resolve, 500))) before executing continue so each error response
incurs the 500ms backoff; keep the console.warn and continue behavior but ensure
the delay is awaited in that branch to prevent rapid re-polling.
In `@apps/web/e2e/start-test-env.sh`:
- Around line 32-43: The API health-check loop should also verify the process is
still alive each iteration; add a liveness check using the background PID (e.g.,
API_PID) inside the for loop that runs curl: if kill -0 "$API_PID" >/dev/null
2>&1 fails, immediately print a clear crash message, dump apps/web/e2e/api.log,
and exit non‑zero instead of waiting the full timeout. Apply the same pattern to
the Web poll loop using the Web PID (e.g., WEB_PID) so crashes are detected and
reported instantly.
In `@apps/web/e2e/tenant.spec.ts`:
- Around line 31-58: The test "Create Tenant" currently leaves a persistent
organization row because afterEach only calls cleanupUser and cleanupUser
cascades to member but does not delete the parent organization; add a DB-level
cleanup for the created tenant by calling a new or existing DbFixture method
(e.g., DbFixture.cleanupTenant(slug) or cleanupOrganization(slug)) using the
tenantName/slug produced in the test, either in a finally block inside the test
or from afterEach, ensuring the created organization record is deleted from the
database after each run instead of being orphaned; update db.ts to expose the
cleanupOrganization/cleanupTenant helper if it doesn't exist and call that
helper from the test teardown.
In `@apps/web/e2e/users.spec.ts`:
- Line 57: Replace the hardcoded localhost regex used when calling
mailpit.extractLink in users.spec.ts (the inviteLink extraction) with a
host-agnostic pattern or by deriving the host from the same env/utility used in
auth-setup.ts (e.g., FRONTEND_URL or the BETTER_AUTH_URL-derived pattern) so the
invite URL is matched in CI/Docker; update the mailpit.extractLink invocation to
use that dynamic/host-agnostic pattern instead of
/(http:\/\/localhost:\d+\/invite\/[^"\s]+)/.
---
Duplicate comments:
In `@apps/web/e2e/users.spec.ts`:
- Around line 50-54: Remove the redundant options and unreachable assertion:
call page.getByText('Invitation sent').first() (omit the no-op { exact: false })
and drop the assert expect(inviteEmailObj).toBeDefined() because
mailpit.waitForEmail(inviteeEmail, 'You have been invited to join an
organization') throws on timeout and will never return undefined; after awaiting
mailpit.waitForEmail assign to inviteEmailObj and assert its contents (e.g.,
subject/body) instead if further checks are needed.
In `@docker-compose.yml`:
- Around line 44-45: In docker-compose.yml, the inline comments after the port
mappings "- "8025:8025" # UI" and "- "1025:1025" # SMTP" need an extra space
before the '#' to satisfy YAML linting; update those two lines (the literal
strings "8025:8025" and "1025:1025") so there are two spaces between the port
mapping and the '#' (e.g., change '... "8025:8025" # UI' to have one more space
before '#') to resolve the YAML comment spacing warning.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/e2e/auth.spec.ts`:
- Line 37: Replace the legacy Playwright selector usage page.locator('text=Check
your email') with the recommended API page.getByText('Check your email') so the
assertion remains await expect(page.getByText('Check your email')).toBeVisible({
timeout: 15000 });; update the locator call in the test where page.locator is
used to use page.getByText to match the rest of the file and other specs.
In `@apps/web/e2e/fixtures/db.ts`:
- Around line 124-141: The cleanupOrganization function currently catches errors
and only logs a warning, hiding failures; make it consistent with cleanupUser by
re-throwing after rollback so tests fail on cleanup problems: inside
cleanupOrganization's catch block (where it calls client.query('ROLLBACK') and
console.warn), replace or augment the console.warn with a throw of the caught
error (e.g., throw error) so the caller/test sees the failure; alternatively, if
intended as best-effort, add a brief doc comment above cleanupOrganization
explaining that failures are non-fatal and why, but prefer the re-throw to match
cleanupUser behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/e2e/auth.spec.ts`:
- Around line 60-61: The negative URL check in apps/web/e2e/auth.spec.ts using
await expect(page).not.toHaveURL(/\/verify-email\?/) can pass on error pages;
update the test to include a complementary positive assertion after that line —
for example assert the expected redirect target URL or a stable page
element/text (use expect(page).toHaveURL('<expected-path>') or
expect(page.getByText('<expected-heading>')).toBeVisible()) to ensure you
actually landed on the intended page; modify the test around the existing
not.toHaveURL(...) assertion to add this positive check referencing that exact
assertion location.
In `@apps/web/e2e/fixtures/db.ts`:
- Line 133: The cleanup currently deletes organizations by name using await
client.query('DELETE FROM "organization" WHERE name = $1', [name]); which can
remove multiple rows; change the cleanup to accept and use a unique identifier
(preferably the PRIMARY KEY `id` or the UNIQUE `slug`) instead of `name`, update
the function signature and all callers to pass the `id` or `slug` returned from
the creation step, and replace the SQL with DELETE FROM "organization" WHERE id
= $1 (or WHERE slug = $1) so only the intended organization row is removed.
Comment on lines
+60
to
+61
| // Assert we were redirected away from the verification URL | ||
| await expect(page).not.toHaveURL(/\/verify-email\?/, { timeout: 10000 }); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Negative URL assertion passes on error pages — consider a positive destination check.
not.toHaveURL(/\/verify-email\?/) is satisfied by any URL, including /500 or /error. A complementary positive assertion on the expected redirect target (or a visible element on that page) would make the verification step more meaningful.
♻️ Proposed refinement
// Assert we were redirected away from the verification URL
await expect(page).not.toHaveURL(/\/verify-email\?/, { timeout: 10000 });
+ // Verify landing on a known post-verification destination
+ await expect(page.getByRole('main').getByText('Dashboard')).toBeVisible({ timeout: 10000 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/e2e/auth.spec.ts` around lines 60 - 61, The negative URL check in
apps/web/e2e/auth.spec.ts using await
expect(page).not.toHaveURL(/\/verify-email\?/) can pass on error pages; update
the test to include a complementary positive assertion after that line — for
example assert the expected redirect target URL or a stable page element/text
(use expect(page).toHaveURL('<expected-path>') or
expect(page.getByText('<expected-heading>')).toBeVisible()) to ensure you
actually landed on the intended page; modify the test around the existing
not.toHaveURL(...) assertion to add this positive check referencing that exact
assertion location.
| // but if strict FKs exist without cascade, we might need to delete members first. | ||
| // Assuming ON DELETE CASCADE is set up or we'll wrap in try/catch. | ||
| // For now, let's try direct delete. | ||
| await client.query('DELETE FROM "organization" WHERE name = $1', [name]); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the cleanupOrganization function and surrounding context
cd apps/web/e2e/fixtures || cd .
cat -n db.ts | sed -n '125,145p'Repository: pramodnarayana/nexiom
Length of output: 966
🏁 Script executed:
# Search for the organization table schema/migrations
find . -type f \( -name "*.sql" -o -name "*migration*" -o -name "*schema*" \) | grep -i org | head -20Repository: pramodnarayana/nexiom
Length of output: 47
🏁 Script executed:
# Look for database schema definitions in TypeScript/JS files
rg -l "organization" --type ts --type js | grep -v node_modules | grep -v dist | head -10Repository: pramodnarayana/nexiom
Length of output: 536
🏁 Script executed:
# Search for any UNIQUE constraints on organization.name
rg "organization.*unique|unique.*name" -i --type ts --type js --type sql | head -20Repository: pramodnarayana/nexiom
Length of output: 1027
🏁 Script executed:
# Read the organization table definition from the initial schema
cat apps/api/drizzle/0000_initial_schema.sql | grep -A 30 'CREATE TABLE "organization"'Repository: pramodnarayana/nexiom
Length of output: 1181
🏁 Script executed:
# Also check if there's an ID column and how it's structured
rg "organization" apps/api/drizzle/0000_initial_schema.sql -A 20 -B 5 | head -60Repository: pramodnarayana/nexiom
Length of output: 2118
🏁 Script executed:
# Check if the test fixture tracks organization ID
rg "createOrganization|cleanupOrganization" apps/web/e2e/fixtures/db.ts -B 3 -A 10Repository: pramodnarayana/nexiom
Length of output: 804
🏁 Script executed:
# Check the createOrganization function to see what it returns
rg "async createOrganization" apps/web/e2e/fixtures/db.ts -A 20Repository: pramodnarayana/nexiom
Length of output: 47
🏁 Script executed:
# Also check if there are examples of how these functions are used in tests
rg "createOrganization\|cleanupOrganization" apps/web/e2e --type ts -B 2 -A 2 | head -50Repository: pramodnarayana/nexiom
Length of output: 47
🏁 Script executed:
# Let's examine the entire db.ts file to understand all methods
wc -l apps/web/e2e/fixtures/db.tsRepository: pramodnarayana/nexiom
Length of output: 96
🏁 Script executed:
# Read the file to see all methods and the createOrganization function
cat -n apps/web/e2e/fixtures/db.ts | head -150Repository: pramodnarayana/nexiom
Length of output: 6439
Use the organization id (PRIMARY KEY) or slug (UNIQUE) instead of name for cleanup to ensure test isolation.
The organization table has no UNIQUE constraint on name, allowing multiple organizations with identical names. DELETE FROM "organization" WHERE name = $1 will silently delete all matching rows, which creates test isolation issues when concurrent tests create organizations with the same name.
Since id is the PRIMARY KEY and slug has a UNIQUE constraint, either would be a safer choice for precise cleanup. Consider modifying the method signature to accept id or slug, or update callers to pass the organization ID from the creation step.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/e2e/fixtures/db.ts` at line 133, The cleanup currently deletes
organizations by name using await client.query('DELETE FROM "organization" WHERE
name = $1', [name]); which can remove multiple rows; change the cleanup to
accept and use a unique identifier (preferably the PRIMARY KEY `id` or the
UNIQUE `slug`) instead of `name`, update the function signature and all callers
to pass the `id` or `slug` returned from the creation step, and replace the SQL
with DELETE FROM "organization" WHERE id = $1 (or WHERE slug = $1) so only the
intended organization row is removed.
pramodnarayana
marked this pull request as draft
February 19, 2026 19:08
pramodnarayana
marked this pull request as ready for review
February 19, 2026 19:08
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Tests
Chores
Documentation