[Misc] Adding more tests#42
Merged
Merged
Conversation
Added more compatibility questions
Added compatibility question tests and verifications Updated tests to cover Keywords and Headline changes recently made Updated tests to cover all of the big5 personality traits
Updated signUp.spec.ts to use new fixture Updated Account information variable names Deleted "deleteUserFixture.ts" as it was incorporated into the "base.ts" file
Updated seedDatabase.ts to throw an error if the user already exists, to also add display names and usernames so they seedUser func acts like a normal basic user Some organising of the google auth code
Added account deletion checks
Updated settings POM
|
@O-Bots is attempting to deploy a commit to the Compass Team on Vercel. A member of the Team first needs to authorize it. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Member
|
All the test pass so it's great. Would be great to clean it up. Either fix it if it's an error somewhere, or remove the log if it's expected. |
Collaborator
Author
|
Yeah this is the issue I was mentioning with the google auth |
Member
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (12)
web/components/measurement-system-toggle.tsx (1)
20-21:⚠️ Potential issue | 🟡 MinorFix the
data-testidtypo before selectors depend on it.Line 21 spells
measurementasmeasuerment, so any selector using the correctly spelled id will not match this switch.✏️ Proposed fix
- data-testid="measuerment-system-toggle" + data-testid="measurement-system-toggle"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/components/measurement-system-toggle.tsx` around lines 20 - 21, The data-testid on the Switch is misspelled ("measuerment-system-toggle") causing selectors to fail; update the data-testid attribute on the Switch in measurement-system-toggle.tsx to the correctly spelled "measurement-system-toggle" so tests and selectors can locate the component (look for the Switch element and its data-testid attribute in the MeasurementSystemToggle component).tests/e2e/web/pages/AuthPage.ts (1)
43-59:⚠️ Potential issue | 🟠 MajorTighten the popup event handling.
Line 45 listens for any new page in the browser context, and Line 59 starts waiting for
closeonly after the submit click. Both patterns can make this Google auth flow flaky.♻️ Proposed fix
async getGooglePopupPage(): Promise<Page> { const [popup] = await Promise.all([ - this.page.context().waitForEvent('page'), + this.page.waitForEvent('popup'), this.clickGoogleButton(), ]) await popup.waitForLoadState() return popup } @@ - await popup.getByText('Sign in with Google.com', {exact: true}).click() - await popup.waitForEvent('close') + await Promise.all([ + popup.waitForEvent('close'), + popup.getByText('Sign in with Google.com', {exact: true}).click(), + ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/pages/AuthPage.ts` around lines 43 - 59, getGooglePopupPage currently listens for any new context page and can race with unrelated pages; change its waitForEvent('page') call to use a predicate that ensures the new page was opened by the current page (e.g., check p.opener() === this.page) and consider waiting for a deterministic load state like 'domcontentloaded' in getGooglePopupPage; in signInToGoogleAccount start waiting for the popup close before performing the submit click by creating a closePromise from popup.waitForEvent('close') and then clicking the 'Sign in with Google.com' button and awaiting closePromise, so the flow is robust and not flaky (references: getGooglePopupPage, signInToGoogleAccount, popup.waitForEvent('close'), this.page.context().waitForEvent('page')).tests/e2e/utils/seedDatabase.ts (1)
19-20:⚠️ Potential issue | 🟠 MajorKeep
seedDbUseridempotent.Line 88 turns a shared seeding helper back into exception-driven flow control. That makes reusable E2E accounts brittle: any caller that does not special-case
"User already Exists"will fail setup instead of reusing the seeded record.♻️ Proposed fix
-export async function seedDbUser(userInfo: UserAccountInformationForSeeding, profileType?: string) { +export async function seedDbUser( + userInfo: UserAccountInformationForSeeding, + profileType?: string, +): Promise<boolean> { @@ - await pg.tx(async (tx: any) => { + return pg.tx(async (tx: any) => { const preexistingUser = await getUser(userId, tx) - if (preexistingUser) throw new Error('User already Exists') + if (preexistingUser) return false @@ await insert(tx, 'profiles', profileData) + return true }) } @@ userInfo.user_id = await firebaseSignUp(userInfo.email, userInfo.password) if (userInfo.user_id) { - await seedDbUser(userInfo, profileType ?? 'full') + await seedDbUser(userInfo, profileType ?? 'full') }Also applies to: 86-88, 119-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/utils/seedDatabase.ts` around lines 19 - 20, The seedDbUser helper is not idempotent: instead of letting a "User already Exists" exception propagate, make seedDbUser safe to call multiple times by first checking for an existing user or using an upsert/insert-with-conflict strategy via the Supabase client returned by createSupabaseDirectClient; if an insert fails with a unique-violation/duplicate-user error (the current catch around the insert at seedDbUser), swallow that error and fetch & return the existing user record so callers can reuse seeded accounts without special-casing the exception. Ensure the change covers the current insert/throw paths referenced in seedDbUser (including the error handling around the insert and any subsequent fetch logic)..env.test (1)
21-22:⚠️ Potential issue | 🟠 MajorThis regresses standalone Playwright execution paths.
DIRECT_SUPABASE_SETUP_IN_PLAYWRIGHT=falsedisables dynamic Supabase env injection, so directnpx playwright testruns can fail when required vars are not pre-exported.🔧 Suggested fix
-# Playwright -DIRECT_SUPABASE_SETUP_IN_PLAYWRIGHT=false +# Playwright +DIRECT_SUPABASE_SETUP_IN_PLAYWRIGHT=true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.test around lines 21 - 22, The change sets DIRECT_SUPABASE_SETUP_IN_PLAYWRIGHT=false which prevents Playwright from dynamically injecting Supabase env vars and breaks standalone runs; revert or change this to DIRECT_SUPABASE_SETUP_IN_PLAYWRIGHT=true (or remove the override) so the dynamic Supabase env injection remains enabled for direct `npx playwright test` execution, ensuring required variables are available when tests run.tests/e2e/web/utils/networkUtils.ts (1)
18-23:⚠️ Potential issue | 🟠 MajorThrowing from teardown getter obscures real test failures.
The pipeline failure confirms this: when a test fails before the
accounts:signInWithIdproute is hit, this closure throws during fixture teardown, masking the original error. Returnundefinedinstead and let the cleanup caller no-op when nothing was captured.🛠️ Proposed fix
- return () => { - if (!accountIdTokenAndLocalId) { - throw new Error('Sign-in was never intercepted — did the test actually sign in?') - } - return accountIdTokenAndLocalId - } + return () => accountIdTokenAndLocalId }Then update the
AuthObjectreturn type and callers inbase.ts:export async function getAuthAccountInfo(page: Page): Promise<() => AuthObject | undefined>And in
base.ts, guard the cleanup:const authObject = getAuthObject() if (authObject) { await deleteUser('Google', undefined, authObject) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/utils/networkUtils.ts` around lines 18 - 23, The teardown getter currently throws if no sign-in was intercepted, which masks original test failures; modify the closure in tests/e2e/web/utils/networkUtils.ts to return undefined instead of throwing when accountIdTokenAndLocalId is falsy, update the exported getAuthAccountInfo signature to Promise<() => AuthObject | undefined> (or equivalent AuthObject | undefined) and then update callers (e.g., in base.ts) to guard cleanup logic by checking the result (const authObject = getAuthObject(); if (authObject) await deleteUser('Google', undefined, authObject)) so cleanup becomes a no-op when nothing was captured.tests/e2e/web/specs/onboardingFlow.spec.ts (4)
6-8:⚠️ Potential issue | 🟠 MajorThe manual
page.close()hook is still racing fixture teardown.Playwright already manages the page lifecycle in this suite. Closing it in
afterEachmakes cleanup order harder to reason about and can interfere with teardown/debug artifacts when a test fails.Proposed fix
test.describe('onboardingFlow', () => { - test.afterEach(async ({page}) => { - await page.close() - }) test.describe('when given valid input', () => {#!/bin/bash # Show the manual page teardown and the shared fixture cleanup. sed -n '1,12p' tests/e2e/web/specs/onboardingFlow.spec.ts rg -n -C3 'await use|deleteUser\(' tests/e2e/web/fixtures/base.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/specs/onboardingFlow.spec.ts` around lines 6 - 8, The test teardown races with Playwright's built-in fixture lifecycle because the suite manually closes the page in the test.afterEach hook; remove the manual teardown by deleting the test.afterEach(async ({ page }) => await page.close()) block so the Playwright page fixture can manage lifecycle and teardown/debug artifacts consistently (look for the test.afterEach usage and page.close call in onboardingFlow.spec.ts and rely on the page fixture from the shared fixtures instead).
246-252:⚠️ Potential issue | 🟠 MajorThe Google flows still depend on an empty-password side effect.
Both specs call
fillPasswordField('')just to makesignInToGoogleAccount()reliable. That workaround is leaking a readiness concern out ofAuthPage, and it lines up with the current timeout atclickSkipOnboardingButton().Spec-side cleanup once the page object owns the wait
- await authPage.fillPasswordField('') //The test only passes when this is added...something is weird here await authPage.signInToGoogleAccount( googleAccountOne.email, googleAccountOne.display_name, googleAccountOne.username, ) @@ - await authPage.fillPasswordField('') //The test only passes when this is added...something is weird here await authPage.signInToGoogleAccount( googleAccountTwo.email, googleAccountTwo.display_name, googleAccountTwo.username, )#!/bin/bash # Compare the workaround in the spec with the current Google auth helper implementation. sed -n '234,260p' tests/e2e/web/specs/onboardingFlow.spec.ts sed -n '338,356p' tests/e2e/web/specs/onboardingFlow.spec.ts rg -n -C3 '\bsignInToGoogleAccount\b|\bfillPasswordField\b' tests/e2e/web/pages/AuthPage.tsAlso applies to: 350-356
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/specs/onboardingFlow.spec.ts` around lines 246 - 252, The spec is relying on a side-effect from fillPasswordField('') to make signInToGoogleAccount() reliable; move that readiness logic into the page object: update AuthPage.signInToGoogleAccount (and/or AuthPage.fillPasswordField) to explicitly wait for the Google auth form/password input (or the post-redirect state) to be visible and enabled before performing clicks or typing, and remove the spec-level fillPasswordField('') calls; also ensure any related timing in clickSkipOnboardingButton is handled by the page method (increase its internal wait/timeout or waitForSelector) so tests no longer need the workaround.
301-336:⚠️ Potential issue | 🟠 MajorThese account-deletion scenarios still clean up the same user twice.
Each test deletes the account through Settings and then the fixture teardown runs
deleteUser()again. That is theNo user found with id ...noise already called out on this PR, and it will keep masking real cleanup regressions until teardown can skip already-deleted accounts.Also applies to: 338-379
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/specs/onboardingFlow.spec.ts` around lines 301 - 336, The test "should successfully delete an account created via email and password" deletes the user via settings (settingsPage.clickDeleteAccountButton) but the suite teardown still calls deleteUser(), causing duplicate-deletion noise; update the test/teardown flow so teardown skips already-deleted accounts: after the in-test deletion (e.g., after settingsPage.clickDeleteAccountButton or settingsPage.fillDeleteAccountSurvey), mark the created account as deleted (set a flag on the fakerAccount object or store a deletedUsers set) or record the deleted userId, and modify the global fixture teardown function deleteUser() to check that marker (or attempt a safe existence check via userInformationFromDb or an API and skip if not found) before trying to delete; ensure the marker uses the same identifier produced by registerWithEmail so the teardown can reliably detect and skip already-deleted users.
163-165:⚠️ Potential issue | 🟡 MinorPersisted height assertions still drop the inches component.
Both checks only compare
feet * 12, so5'11"and5'0"assert as the same value. That leaves truncation bugs invisible to the suite.Proposed fix
await expect(dbInfo.profile.height_in_inches).toEqual( - Number(onboardingAccount.height?.feet) * 12, + Number(onboardingAccount.height?.feet) * 12 + + Number(onboardingAccount.height?.inches ?? 0), ) @@ -await expect(dbInfo.profile.height_in_inches).toEqual(Number(fakerAccount.height?.feet) * 12) +await expect(dbInfo.profile.height_in_inches).toEqual( + Number(fakerAccount.height?.feet) * 12 + Number(fakerAccount.height?.inches ?? 0), +)Also applies to: 424-426
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/specs/onboardingFlow.spec.ts` around lines 163 - 165, The assertion only compares dbInfo.profile.height_in_inches to Number(onboardingAccount.height?.feet) * 12, dropping the inches component; update the expectation to include the inches field (e.g., Number(onboardingAccount.height?.feet) * 12 + Number(onboardingAccount.height?.inches ?? 0)) and ensure you coerce undefined to 0 so the test handles missing inches. Apply the same change to the duplicate assertion later in the file that uses the same dbInfo.profile.height_in_inches and onboardingAccount.height?.feet check.tests/e2e/web/pages/homePage.ts (1)
15-17:⚠️ Potential issue | 🟠 Major
homePageLinkis still targeting the wrong route.
verifyHomePageLinks()andverifySignedInHomePage()now assert againsta[href="/home"], but the Home nav entry is/. That makes the shared home-page checks bind to the wrong anchor.Proposed fix
- this.homePageLink = page.locator('a[href="/home"]') + this.homePageLink = page.locator('a[href="/"]')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/pages/homePage.ts` around lines 15 - 17, The homePageLink locator is pointing to the wrong route; update the selector used by homePageLink from 'a[href="/home"]' to the root route 'a[href="/"]' so that verifyHomePageLinks() and verifySignedInHomePage() assert against the correct Home nav entry; locate and modify the homePageLink assignment in the page object (symbol: homePageLink) to use the '/' href.tests/e2e/web/pages/settingsPage.ts (1)
100-115:⚠️ Potential issue | 🟠 Major
unhideProfiles()still isn't scoped to the matched row.
hiddenProfilesSection.count()counts the container, and the locator inside the loop is recreated against the whole section each time. The finalnth(matchIndex)click therefore is not tied to the row that actually containsprofile.Proposed fix
async unhideProfiles(profile: string) { await expect(this.hiddenProfilesSection).toBeVisible() - const hiddenProfiles = await this.hiddenProfilesSection.count() - let matchIndex = -1 - for (let i = 0; i < hiddenProfiles; i++) { - const target = await this.hiddenProfilesSection.getByRole('link', {name: `${profile}`}) - if (target) { - matchIndex = i - } - } - await this.hiddenProfilesSection - .locator('div') - .nth(matchIndex) - .getByRole('button', {name: 'Unhide'}) - .click() + const rows = this.hiddenProfilesSection.locator('div') + const rowCount = await rows.count() + + for (let i = 0; i < rowCount; i++) { + const row = rows.nth(i) + if (await row.getByRole('link', {name: profile}).count()) { + await row.getByRole('button', {name: 'Unhide'}).click() + return + } + } + + throw new Error(`Hidden profile ${profile} was not found`) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/pages/settingsPage.ts` around lines 100 - 115, The unhideProfiles function currently queries the whole hiddenProfilesSection each loop and then clicks nth(matchIndex) on the section, so the clicked "Unhide" button may not be in the row that matched; change the loop to iterate the row locators (e.g., use this.hiddenProfilesSection.locator('div').nth(i)), within each row call getByRole('link', {name: profile}) to check for existence and set matchIndex, and finally perform the .getByRole('button', {name: 'Unhide'}).click() on that same row locator (the one at matchIndex) so the click is scoped to the matched row; ensure you guard for matchIndex === -1 and throw or assert if not found.tests/e2e/web/utils/deleteUser.ts (1)
14-31:⚠️ Potential issue | 🟠 MajorCleanup still ties auth deletion to the presence of a profile row.
The email/password branch only calls
deleteAccount()whenuserInformationFromDb()succeeds, and the Google branch still lets the expectedNo user found...DB case fall into noisy teardown logging. That is the same cleanup noise already showing up in CI, and it can also leave Firebase auth users behind. Delete auth independently, suppress only the specific DB-not-found case arounddeleteFromDb(), and rethrow everything else.Also applies to: 33-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/utils/deleteUser.ts` around lines 14 - 31, The cleanup currently only deletes Firebase auth for Email/Password when userInformationFromDb(...) returns true and lets DB "No user found" errors produce noisy logs; change the logic so deleteAccount(...) is always invoked after successful login (call deleteAccount(loginInfo?.data.idToken) regardless of userInformationFromDb result), then call deleteFromDb(...) inside a try/catch that only swallows the specific "No user found" / not-found error and rethrows any other errors; apply the same pattern to the Google branch (use findUser(authInfo.idToken) only to decide whether to attempt DB deletion but still always call deleteAccount(authInfo.idToken), and wrap deleteFromDb(authInfo.localId) in the same selective catch).
🧹 Nitpick comments (5)
tests/e2e/utils/seedDatabase.ts (1)
106-117: Replace the positionalseedUserparams with an options object.This helper now takes five optional strings, so
profileType,displayName, anduserNameare easy to swap at call sites. An object parameter would make the contract self-describing.♻️ Proposed refactor
-export async function seedUser( - email?: string | undefined, - password?: string | undefined, - profileType?: string | undefined, - displayName?: string | undefined, - userName?: string | undefined, -) { +export async function seedUser({ + email, + password, + profileType, + displayName, + userName, +}: { + email?: string + password?: string + profileType?: string + displayName?: string + userName?: string +} = {}) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/utils/seedDatabase.ts` around lines 106 - 117, Change seedUser to accept a single options object instead of positional strings: replace the five optional parameters (email, password, profileType, displayName, userName) with an object parameter (e.g., opts) and destructure { email, password, profileType, displayName, userName } inside the function; populate the UserAccountInformationForSeeding instance the same way but using the destructured properties, update all call sites to pass an object literal with named fields to seedUser, and keep function name seedUser and class UserAccountInformationForSeeding unchanged so existing imports remain valid.tests/e2e/web/pages/socialPage.ts (1)
12-113: Add docstrings to the sharedSocialPagehelpers.This page object is exposed through the shared fixtures, but its public navigation, verification, and click methods are undocumented.
As per coding guidelines, "Ensure all public functions have docstrings."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/pages/socialPage.ts` around lines 12 - 113, The SocialPage class and its public methods (SocialPage, goToSocialPage, verifySocialPage, verifySocialLinks, clickDiscordButton, clickRedditButton, clickStoatButton, clickXButton, clickInstagramButton, clickGitHubButton, clickEmailButton) lack docstrings; add concise JSDoc/TSDoc comments for the class, constructor, and each public method describing purpose, parameters (e.g., page in constructor) and return behavior (promises/side effects) following the project's docstring style so these helpers are documented for shared fixtures and auto-generated docs.tests/e2e/web/pages/organizationPage.ts (1)
3-134: Document the sharedOrganizationPageAPI.This class is now part of the common E2E fixture surface, but its public navigation, verification, and click helpers are undocumented.
As per coding guidelines, "Ensure all public functions have docstrings."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/pages/organizationPage.ts` around lines 3 - 134, The OrganizationPage class (OrganizationPage, constructor, goToOrganizationPage, verifyOrganizationPage, verifyOrganizationLinks, and all click... methods like clickAboutCompassLink, clickConstitutionLink, etc.) lacks docstrings; add concise JSDoc comments for each public member describing what it does, input params (e.g., the Page passed to the constructor), return/async behavior (Promise<void>), and any side effects (navigation, assertions, clicks) so consumers of the shared E2E fixture can understand usage; place the JSDoc immediately above each method and the class declaration following the project's JSDoc style.tests/e2e/web/utils/accountInformation.ts (1)
139-156: Remove unusedidvariables indev_one_accountanddev_two_account.Both functions generate
const id = crypto.randomUUID().slice(0, 6)but never use it. The identifiers are now constant.♻️ Proposed fix
dev_one_account: () => { - const id = crypto.randomUUID().slice(0, 6) return { email: `dev_1@compass.com`, password: 'dev_1Password', display_name: 'Dev1.Compass', username: `Dev1.Connections`, } }, dev_two_account: () => { - const id = crypto.randomUUID().slice(0, 6) return { email: 'dev_2@compass.com', password: 'dev_2Password', display_name: 'Dev2.Compass', username: `Dev2.Connections_`, } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/utils/accountInformation.ts` around lines 139 - 156, Remove the unused id variables generated in dev_one_account and dev_two_account: delete the lines that declare "const id = crypto.randomUUID().slice(0, 6)" in both functions so there are no unused identifiers; keep the returned objects intact and ensure no other code references id in those functions.tests/e2e/web/utils/deleteUser.ts (1)
7-11: Add upfront guards for the required cleanup inputs.Right now the
'Email/Password'path can still call Firebase withundefinedcredentials, while the'Google'path silently no-ops whenauthInfois missing. A couple of early guards would make teardown failures deterministic and flatten the branching.As per coding guidelines, "We use early returns to avoid deep nesting." and "Check for edge cases like null values or empty arrays."Proposed guard clauses
export async function deleteUser( authType: AuthType, account?: UserAccountInformation, authInfo?: AuthObject, ) { + if (authType === 'Email/Password' && (!account?.email || !account?.password)) { + throw new Error('Email/Password cleanup requires an account with email and password.') + } + + if (authType === 'Google' && !authInfo) { + throw new Error('Google cleanup requires authInfo.') + } + try {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/utils/deleteUser.ts` around lines 7 - 11, The deleteUser function lacks upfront validation leading to hidden failures; add early guard clauses at the top of deleteUser to validate inputs per authType: if authType === 'Email/Password' assert that account is provided and contains the required email/password/credential fields before calling Firebase, and if authType === 'Google' assert that authInfo is provided (and contains the expected token/uid) before proceeding; on missing required data return early (or throw a clear error) so the Email/Password branch never calls Firebase with undefined credentials and the Google branch does not silently no-op, flattening subsequent branching in deleteUser.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/utils/firebaseUtils.ts`:
- Around line 28-35: findUser currently returns the raw AxiosResponse which
makes deleteUser's existence check ineffective; change findUser (in
tests/e2e/utils/firebaseUtils.ts) to await the POST, extract and return the user
record or null (e.g., response.data.users?.[0] || null) so callers like
deleteUser can test for a missing user with if (!googleAuthUser) rather than
relying on exceptions; ensure any HTTP errors still propagate as exceptions.
In `@tests/e2e/web/fixtures/base.ts`:
- Around line 44-56: The teardown for googleAccountOne and googleAccountTwo
calls getAuthAccountInfo() and immediately invokes getAuthObject(), which will
throw if sign-in was never intercepted; change the teardown to call
getAuthObject() and guard the cleanup by checking the returned value (e.g.,
const authObject = getAuthObject(); if (authObject) await deleteUser('Google',
undefined, authObject)), so deleteUser is only invoked when an auth object
exists; update both fixtures (googleAccountOne and googleAccountTwo) to use this
guarded pattern after networkUtils is changed to return undefined instead of
throwing.
In `@tests/e2e/web/specs/onboardingFlow.spec.ts`:
- Around line 533-535: The test block under test.describe('when an error
occurs') contains a no-op test called test('placeholder', async ({}) => {})
which falsely marks the error-path as covered; remove this placeholder or
replace it with an actual negative-path assertion that exercises the error
condition and asserts expected behavior (for example, trigger the failing flow
in the onboarding flow helper and assert an error message or redirect using
expect). Update the block referencing test.describe('when an error occurs') and
test('placeholder'...) to either delete the test or implement a failing scenario
that asserts the app's error-handling behavior.
In `@web/components/nav/sidebar.tsx`:
- Line 55: The test id is currently placed on the inner scrollable div and
doesn't encompass the auth links, so move the data-testid="sidebar" from the
inner div to the root container element of the Sidebar component (the outermost
wrapper that contains both the scrollable group and the auth links) so the
entire navigation (including "Sign in"/"Sign out") is within the same
page-object scope; update the JSX in the Sidebar component (the top-level
wrapper in web/components/nav/sidebar.tsx) accordingly and verify tests select
the root element.
In `@web/components/theme-icon.tsx`:
- Line 34: The button in the ThemeIcon component (the element with
data-testid="settings-dark-light-toggle" and onClick={toggleTheme}) lacks an
explicit type and will default to type="submit" inside forms; update that button
to include type="button" to prevent accidental form submissions when rendered
inside a form.
---
Duplicate comments:
In @.env.test:
- Around line 21-22: The change sets DIRECT_SUPABASE_SETUP_IN_PLAYWRIGHT=false
which prevents Playwright from dynamically injecting Supabase env vars and
breaks standalone runs; revert or change this to
DIRECT_SUPABASE_SETUP_IN_PLAYWRIGHT=true (or remove the override) so the dynamic
Supabase env injection remains enabled for direct `npx playwright test`
execution, ensuring required variables are available when tests run.
In `@tests/e2e/utils/seedDatabase.ts`:
- Around line 19-20: The seedDbUser helper is not idempotent: instead of letting
a "User already Exists" exception propagate, make seedDbUser safe to call
multiple times by first checking for an existing user or using an
upsert/insert-with-conflict strategy via the Supabase client returned by
createSupabaseDirectClient; if an insert fails with a
unique-violation/duplicate-user error (the current catch around the insert at
seedDbUser), swallow that error and fetch & return the existing user record so
callers can reuse seeded accounts without special-casing the exception. Ensure
the change covers the current insert/throw paths referenced in seedDbUser
(including the error handling around the insert and any subsequent fetch logic).
In `@tests/e2e/web/pages/AuthPage.ts`:
- Around line 43-59: getGooglePopupPage currently listens for any new context
page and can race with unrelated pages; change its waitForEvent('page') call to
use a predicate that ensures the new page was opened by the current page (e.g.,
check p.opener() === this.page) and consider waiting for a deterministic load
state like 'domcontentloaded' in getGooglePopupPage; in signInToGoogleAccount
start waiting for the popup close before performing the submit click by creating
a closePromise from popup.waitForEvent('close') and then clicking the 'Sign in
with Google.com' button and awaiting closePromise, so the flow is robust and not
flaky (references: getGooglePopupPage, signInToGoogleAccount,
popup.waitForEvent('close'), this.page.context().waitForEvent('page')).
In `@tests/e2e/web/pages/homePage.ts`:
- Around line 15-17: The homePageLink locator is pointing to the wrong route;
update the selector used by homePageLink from 'a[href="/home"]' to the root
route 'a[href="/"]' so that verifyHomePageLinks() and verifySignedInHomePage()
assert against the correct Home nav entry; locate and modify the homePageLink
assignment in the page object (symbol: homePageLink) to use the '/' href.
In `@tests/e2e/web/pages/settingsPage.ts`:
- Around line 100-115: The unhideProfiles function currently queries the whole
hiddenProfilesSection each loop and then clicks nth(matchIndex) on the section,
so the clicked "Unhide" button may not be in the row that matched; change the
loop to iterate the row locators (e.g., use
this.hiddenProfilesSection.locator('div').nth(i)), within each row call
getByRole('link', {name: profile}) to check for existence and set matchIndex,
and finally perform the .getByRole('button', {name: 'Unhide'}).click() on that
same row locator (the one at matchIndex) so the click is scoped to the matched
row; ensure you guard for matchIndex === -1 and throw or assert if not found.
In `@tests/e2e/web/specs/onboardingFlow.spec.ts`:
- Around line 6-8: The test teardown races with Playwright's built-in fixture
lifecycle because the suite manually closes the page in the test.afterEach hook;
remove the manual teardown by deleting the test.afterEach(async ({ page }) =>
await page.close()) block so the Playwright page fixture can manage lifecycle
and teardown/debug artifacts consistently (look for the test.afterEach usage and
page.close call in onboardingFlow.spec.ts and rely on the page fixture from the
shared fixtures instead).
- Around line 246-252: The spec is relying on a side-effect from
fillPasswordField('') to make signInToGoogleAccount() reliable; move that
readiness logic into the page object: update AuthPage.signInToGoogleAccount
(and/or AuthPage.fillPasswordField) to explicitly wait for the Google auth
form/password input (or the post-redirect state) to be visible and enabled
before performing clicks or typing, and remove the spec-level
fillPasswordField('') calls; also ensure any related timing in
clickSkipOnboardingButton is handled by the page method (increase its internal
wait/timeout or waitForSelector) so tests no longer need the workaround.
- Around line 301-336: The test "should successfully delete an account created
via email and password" deletes the user via settings
(settingsPage.clickDeleteAccountButton) but the suite teardown still calls
deleteUser(), causing duplicate-deletion noise; update the test/teardown flow so
teardown skips already-deleted accounts: after the in-test deletion (e.g., after
settingsPage.clickDeleteAccountButton or settingsPage.fillDeleteAccountSurvey),
mark the created account as deleted (set a flag on the fakerAccount object or
store a deletedUsers set) or record the deleted userId, and modify the global
fixture teardown function deleteUser() to check that marker (or attempt a safe
existence check via userInformationFromDb or an API and skip if not found)
before trying to delete; ensure the marker uses the same identifier produced by
registerWithEmail so the teardown can reliably detect and skip already-deleted
users.
- Around line 163-165: The assertion only compares
dbInfo.profile.height_in_inches to Number(onboardingAccount.height?.feet) * 12,
dropping the inches component; update the expectation to include the inches
field (e.g., Number(onboardingAccount.height?.feet) * 12 +
Number(onboardingAccount.height?.inches ?? 0)) and ensure you coerce undefined
to 0 so the test handles missing inches. Apply the same change to the duplicate
assertion later in the file that uses the same dbInfo.profile.height_in_inches
and onboardingAccount.height?.feet check.
In `@tests/e2e/web/utils/deleteUser.ts`:
- Around line 14-31: The cleanup currently only deletes Firebase auth for
Email/Password when userInformationFromDb(...) returns true and lets DB "No user
found" errors produce noisy logs; change the logic so deleteAccount(...) is
always invoked after successful login (call
deleteAccount(loginInfo?.data.idToken) regardless of userInformationFromDb
result), then call deleteFromDb(...) inside a try/catch that only swallows the
specific "No user found" / not-found error and rethrows any other errors; apply
the same pattern to the Google branch (use findUser(authInfo.idToken) only to
decide whether to attempt DB deletion but still always call
deleteAccount(authInfo.idToken), and wrap deleteFromDb(authInfo.localId) in the
same selective catch).
In `@tests/e2e/web/utils/networkUtils.ts`:
- Around line 18-23: The teardown getter currently throws if no sign-in was
intercepted, which masks original test failures; modify the closure in
tests/e2e/web/utils/networkUtils.ts to return undefined instead of throwing when
accountIdTokenAndLocalId is falsy, update the exported getAuthAccountInfo
signature to Promise<() => AuthObject | undefined> (or equivalent AuthObject |
undefined) and then update callers (e.g., in base.ts) to guard cleanup logic by
checking the result (const authObject = getAuthObject(); if (authObject) await
deleteUser('Google', undefined, authObject)) so cleanup becomes a no-op when
nothing was captured.
In `@web/components/measurement-system-toggle.tsx`:
- Around line 20-21: The data-testid on the Switch is misspelled
("measuerment-system-toggle") causing selectors to fail; update the data-testid
attribute on the Switch in measurement-system-toggle.tsx to the correctly
spelled "measurement-system-toggle" so tests and selectors can locate the
component (look for the Switch element and its data-testid attribute in the
MeasurementSystemToggle component).
---
Nitpick comments:
In `@tests/e2e/utils/seedDatabase.ts`:
- Around line 106-117: Change seedUser to accept a single options object instead
of positional strings: replace the five optional parameters (email, password,
profileType, displayName, userName) with an object parameter (e.g., opts) and
destructure { email, password, profileType, displayName, userName } inside the
function; populate the UserAccountInformationForSeeding instance the same way
but using the destructured properties, update all call sites to pass an object
literal with named fields to seedUser, and keep function name seedUser and class
UserAccountInformationForSeeding unchanged so existing imports remain valid.
In `@tests/e2e/web/pages/organizationPage.ts`:
- Around line 3-134: The OrganizationPage class (OrganizationPage, constructor,
goToOrganizationPage, verifyOrganizationPage, verifyOrganizationLinks, and all
click... methods like clickAboutCompassLink, clickConstitutionLink, etc.) lacks
docstrings; add concise JSDoc comments for each public member describing what it
does, input params (e.g., the Page passed to the constructor), return/async
behavior (Promise<void>), and any side effects (navigation, assertions, clicks)
so consumers of the shared E2E fixture can understand usage; place the JSDoc
immediately above each method and the class declaration following the project's
JSDoc style.
In `@tests/e2e/web/pages/socialPage.ts`:
- Around line 12-113: The SocialPage class and its public methods (SocialPage,
goToSocialPage, verifySocialPage, verifySocialLinks, clickDiscordButton,
clickRedditButton, clickStoatButton, clickXButton, clickInstagramButton,
clickGitHubButton, clickEmailButton) lack docstrings; add concise JSDoc/TSDoc
comments for the class, constructor, and each public method describing purpose,
parameters (e.g., page in constructor) and return behavior (promises/side
effects) following the project's docstring style so these helpers are documented
for shared fixtures and auto-generated docs.
In `@tests/e2e/web/utils/accountInformation.ts`:
- Around line 139-156: Remove the unused id variables generated in
dev_one_account and dev_two_account: delete the lines that declare "const id =
crypto.randomUUID().slice(0, 6)" in both functions so there are no unused
identifiers; keep the returned objects intact and ensure no other code
references id in those functions.
In `@tests/e2e/web/utils/deleteUser.ts`:
- Around line 7-11: The deleteUser function lacks upfront validation leading to
hidden failures; add early guard clauses at the top of deleteUser to validate
inputs per authType: if authType === 'Email/Password' assert that account is
provided and contains the required email/password/credential fields before
calling Firebase, and if authType === 'Google' assert that authInfo is provided
(and contains the expected token/uid) before proceeding; on missing required
data return early (or throw a clear error) so the Email/Password branch never
calls Firebase with undefined credentials and the Google branch does not
silently no-op, flattening subsequent branching in deleteUser.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 05772d70-5e08-4d2b-b9f9-c7567625e431
📒 Files selected for processing (33)
.env.test.vscode/settings.jsonplaywright.config.tstests/e2e/backend/utils/userInformation.tstests/e2e/utils/firebaseUtils.tstests/e2e/utils/seedDatabase.tstests/e2e/web/SPEC_CONFIG.tstests/e2e/web/fixtures/base.tstests/e2e/web/fixtures/signInFixture.tstests/e2e/web/pages/AuthPage.tstests/e2e/web/pages/homePage.tstests/e2e/web/pages/organizationPage.tstests/e2e/web/pages/settingsPage.tstests/e2e/web/pages/socialPage.tstests/e2e/web/specs/onboardingFlow.spec.tstests/e2e/web/specs/postSignIn.spec.tstests/e2e/web/specs/signIn.spec.tstests/e2e/web/specs/signUp.spec.tstests/e2e/web/utils/accountInformation.tstests/e2e/web/utils/deleteUser.tstests/e2e/web/utils/networkUtils.tstests/e2e/web/utils/testCleanupHelpers.tsweb/components/connection-preferences-settings.tsxweb/components/font-picker.tsxweb/components/measurement-system-toggle.tsxweb/components/nav/profile-summary.tsxweb/components/nav/sidebar-item.tsxweb/components/nav/sidebar.tsxweb/components/profile/delete-account-survey-modal.tsxweb/components/settings/hidden-profiles-modal.tsxweb/components/switch-setting.tsxweb/components/theme-icon.tsxweb/components/widgets/short-toggle.tsx
💤 Files with no reviewable changes (2)
- tests/e2e/web/specs/postSignIn.spec.ts
- web/components/nav/sidebar-item.tsx
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Collaborator
Author
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/web/specs/onboardingFlow.spec.ts (1)
157-159:⚠️ Potential issue | 🟡 MinorHeight assertion ignores inches component.
The assertion on line 157-158 calculates
Number(onboardingAccount.height?.feet) * 12but does not account for the inches value. The correct formula should beNumber(onboardingAccount.height?.feet) * 12 + Number(onboardingAccount.height?.inches). Although all current fixtures have inches set to'0', this is a latent bug—if a fixture is added with non-zero inches (e.g.,feet: '5', inches: '11'), the assertion would pass incorrectly with60instead of the expected71.This same issue appears on line 420 with the
fakerAccountassertion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/specs/onboardingFlow.spec.ts` around lines 157 - 159, The height assertion is only converting feet to inches and ignores the inches component; update the expectation for dbInfo.profile.height_in_inches to compute Number(onboardingAccount.height?.feet) * 12 + Number(onboardingAccount.height?.inches) (ensuring safe Number() conversion for nullable fields) and make the same change for the similar assertion that uses fakerAccount at the other occurrence (the assertion referencing dbInfo.profile.height_in_inches and fakerAccount.height).
♻️ Duplicate comments (1)
tests/e2e/web/utils/deleteUser.ts (1)
24-29:⚠️ Potential issue | 🟡 MinorGoogle branch lacks error handling for
deleteFromDb.Unlike the Email/Password branch, the Google branch doesn't handle errors from
deleteFromDb. If the DB record doesn't exist (e.g., user was already deleted in-app), this will throw and potentially mask the successful auth deletion.Proposed fix
if (authType === 'Google' && authInfo) { const googleAuthUser = await findUser(authInfo.idToken) if (!googleAuthUser) return await deleteAccount(authInfo.idToken) - await deleteFromDb(authInfo.localId) + try { + await deleteFromDb(authInfo.localId) + } catch (dbError: any) { + if (!dbError?.message?.includes('No user found')) { + console.warn('Unexpected DB error during Google cleanup:', dbError) + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/utils/deleteUser.ts` around lines 24 - 29, The Google branch (inside the authType === 'Google' path) calls deleteFromDb without error handling, which can throw if the DB record is missing; wrap the DB deletion in a try/catch (or check existence before deleting) so a failing deleteFromDb does not bubble up and mask the successful deleteAccount call—specifically modify the block that uses findUser(authInfo.idToken), deleteAccount(authInfo.idToken) and deleteFromDb(authInfo.localId) to catch and log/ignore DB-not-found errors while still allowing other unexpected errors to surface.
🧹 Nitpick comments (8)
tests/e2e/web/pages/homePage.ts (1)
161-196: Consider extracting common sidebar assertions into a helper.Both
verifyHomePageLinks()andverifySignedInHomePage()share many of the same sidebar assertions (About, FAQ, Vote, Events, What's new, Socials, Organization, Contact). Extracting these into a private helper would reduce duplication and maintenance burden.Suggested refactor
private async verifyCommonSidebarLinks() { await expect(this.sidebarAbout).toBeVisible() await expect(this.sidebarFaq).toBeVisible() await expect(this.sidebarVote).toBeVisible() await expect(this.sidebarEvents).toBeVisible() await expect(this.sidebarWhatsNew).toBeVisible() await expect(this.sidebarSocials).toBeVisible() await expect(this.sidebarOrganization).toBeVisible() await expect(this.sidebarContact).toBeVisible() } async verifyHomePageLinks() { await expect(this.homePageLink).toBeVisible() await this.verifyCommonSidebarLinks() await expect(this.signUpButton).toBeVisible() await expect(this.signInLink).toBeVisible() await expect(this.localePicker).toBeVisible() } async verifySignedInHomePage(displayName: string) { await expect(this.homePageLink).toBeVisible() await expect(this.profileLink).toBeVisible() await expect(this.profileLink).toContainText(displayName) await expect(this.sidebarPeople).toBeVisible() await expect(this.sidebarNotifs).toBeVisible() await expect(this.sidebarMessages).toBeVisible() await expect(this.sidebarSettings).toBeVisible() await this.verifyCommonSidebarLinks() await expect(this.signOutLink).toBeVisible() await expect(this.signUpButton).not.toBeVisible() await expect(this.signInLink).not.toBeVisible() await expect(this.localePicker).not.toBeVisible() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/pages/homePage.ts` around lines 161 - 196, Extract the repeated sidebar assertions into a private helper: create a private async verifyCommonSidebarLinks() that contains the shared expects for sidebarAbout, sidebarFaq, sidebarVote, sidebarEvents, sidebarWhatsNew, sidebarSocials, sidebarOrganization and sidebarContact, then call this helper from both verifyHomePageLinks() and verifySignedInHomePage(displayName) instead of duplicating those expects; ensure the helper is marked private and awaited where used.tests/e2e/web/utils/accountInformation.ts (2)
149-156: Remove unusedidvariable.Same issue as
dev_one_account- theidis generated but not used.Proposed fix
dev_two_account: () => { - const id = crypto.randomUUID().slice(0, 6) return { email: 'dev_2@compass.com', password: 'dev_2Password', display_name: 'Dev2.Compass', username: `Dev2.Connections_`, } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/utils/accountInformation.ts` around lines 149 - 156, The dev_two_account factory creates an unused local variable id (const id = crypto.randomUUID().slice(0, 6)) which should be removed; edit the dev_two_account function to delete the unused id declaration (matching the fix applied to dev_one_account) so no unused variables remain and linting will pass.
139-146: Remove unusedidvariable.The
idvariable is generated but never used sinceusernameare now fixed values.Proposed fix
dev_one_account: () => { - const id = crypto.randomUUID().slice(0, 6) return { email: `dev_1@compass.com`, password: 'dev_1Password', display_name: 'Dev1.Compass', username: `Dev1.Connections`, } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/utils/accountInformation.ts` around lines 139 - 146, In the dev_one_account factory function remove the unused id variable generation (const id = crypto.randomUUID().slice(0, 6)) since email and username are now fixed; update the dev_one_account function to simply return the object without declaring or using id to eliminate the unused-variable warning (refer to dev_one_account in tests/e2e/web/utils/accountInformation.ts).tests/e2e/web/pages/AuthPage.ts (1)
52-62: Consider adding explicit wait for popup readiness before interactions.The method relies on
getGooglePopupPage()which callswaitForLoadState(), but the subsequent interactions with 'Add new account' and form fields may still race if those elements render asynchronously. Consider adding a visibility check before the first interaction.Proposed improvement
async signInToGoogleAccount(email: string, display_name?: string, username?: string) { const popup = await this.getGooglePopupPage() + await popup.getByText('Add new account', {exact: true}).waitFor({state: 'visible'}) await popup.getByText('Add new account', {exact: true}).click() await popup.getByLabel('Email').fill(email)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/pages/AuthPage.ts` around lines 52 - 62, The signInToGoogleAccount method can race with async rendering after getGooglePopupPage(); to fix, after obtaining popup from getGooglePopupPage() explicitly wait for the popup UI to be ready before interacting (e.g., use popup.waitForSelector or call .waitFor({state: 'visible'}) on the 'Add new account' locator and/or the Email input), then proceed to click and fill fields; update signInToGoogleAccount to wait for visibility of popup.getByText('Add new account', {exact: true}) and popup.getByLabel('Email') (and optionally Display name / Screen name) before calling click()/fill().tests/e2e/utils/seedDatabase.ts (4)
110-116: Add a docstring for exportedseedUser.This public helper now has additional parameters and behavior but no function-level docstring.
As per coding guidelines, "Ensure all public functions have docstrings".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/utils/seedDatabase.ts` around lines 110 - 116, The exported function seedUser lacks a docstring; add a concise JSDoc/TSDoc comment above the exported async function seedUser describing its purpose, parameters (email, password, profileType, displayName, userName) including types and optionality, return value (what it resolves to or side-effects like creating a DB user), and any important behavior (defaults, side effects, thrown errors). Ensure the comment is placed immediately above the seedUser declaration and follows the project's docstring style (/** ... */) so linters and docs pick it up.
20-22: Use primitivebooleaninstead of boxedBoolean.Line 22 uses
Promise<Boolean>. In TypeScript,Promise<boolean>is the correct primitive type.♻️ Proposed fix
export async function seedDbUser( userInfo: UserAccountInformationForSeeding, profileType?: string, -): Promise<Boolean> { +): Promise<boolean> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/utils/seedDatabase.ts` around lines 20 - 22, The function signature in tests/e2e/utils/seedDatabase.ts uses the boxed type Promise<Boolean>; change that to the primitive type Promise<boolean> in the function declaration (replace Promise<Boolean> with Promise<boolean>) and update any matching overloads or exported type annotations in the same file that reference Promise<Boolean> so the return type consistently uses the primitive boolean.
124-125: Move new string literals to constants.Line 124 (
'full') and Line 125 (log message) introduce hardcoded strings in changed code.As per coding guidelines, "Flag any hardcoded strings; they should be in the constants file".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/utils/seedDatabase.ts` around lines 124 - 125, The new hardcoded strings ('full' and the debug message "User created in Firebase and Supabase:") should be moved to constants; update the call in seedDbUser to use a constant (e.g., PROFILE_TYPE_FULL) instead of the literal 'full', and replace the debug log message with a constant (e.g., LOG_USER_CREATED) concatenated or formatted with userInfo.email; add these new constants to the shared constants file and import them into tests/e2e/utils/seedDatabase.ts so seedDbUser(...) and debug(...) use the constants instead of inline literals.
89-89: Dropanyfor transaction typing.Line 89 uses
tx: any, which removes type safety in this shared helper.As per coding guidelines, "TypeScript: Avoid 'any', use explicit types".♻️ Proposed fix
- return pg.tx(async (tx: any) => { + return pg.tx(async tx => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/utils/seedDatabase.ts` at line 89, Replace the unsafe tx: any in the async callback passed to pg.tx with an explicit transaction type from the DB library used in this repo (e.g., PoolClient from 'pg' or ITask/ITaskContext from 'pg-promise'); import that type at the top of tests/e2e/utils/seedDatabase.ts and annotate the callback parameter (the tx in the pg.tx(...) lambda) with it so the transaction helper in seedDatabase.ts has proper typing instead of any.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/utils/seedDatabase.ts`:
- Around line 120-121: The seeding now reads from userInfo.userName so callers
that pass only displayName must not trigger a random username; ensure
userInfo.userName is only changed when an explicit userName is provided (do not
overwrite or generate a new username when only displayName is present). Update
the logic around userInfo, displayName and userName (references: userInfo,
displayName, userName) so that if userName is undefined the existing
userInfo.userName remains unchanged.
- Around line 96-97: The username field is set to
cleanUsername(userInfo.userName) which can return an empty string for
symbol/whitespace-heavy input; before inserting the user (in
tests/e2e/utils/seedDatabase.ts where the user object is built with username:
cleanUsername(userInfo.userName), data: {}), validate the normalized username
and guard against empty values—either skip creating/inserting that user or throw
a clear error; add a conditional that checks the result of
cleanUsername(userInfo.userName) and handles the empty-case appropriately (e.g.,
log/throw or generate a fallback) so no invalid empty username is inserted.
In `@tests/e2e/web/pages/settingsPage.ts`:
- Around line 152-158: The test uses a brittle positional selector in
fillDeleteAccountSurvey; replace the .locator('div').nth(1) click with a stable
selection based on the option text or value and parameterize it. Update the
fillDeleteAccountSurvey signature to accept an optional selectedReason (default
to the common label like "I found a connection elsewhere"), then assert modal
and reasons visibility, click this.deleteSurveyReasons.getByText(selectedReason)
(or equivalent text/value-based selector) and then continue to assert
this.deleteSurveyDetails visibility and fill(reason); keep existing expectations
but remove the positional nth selector.
---
Outside diff comments:
In `@tests/e2e/web/specs/onboardingFlow.spec.ts`:
- Around line 157-159: The height assertion is only converting feet to inches
and ignores the inches component; update the expectation for
dbInfo.profile.height_in_inches to compute
Number(onboardingAccount.height?.feet) * 12 +
Number(onboardingAccount.height?.inches) (ensuring safe Number() conversion for
nullable fields) and make the same change for the similar assertion that uses
fakerAccount at the other occurrence (the assertion referencing
dbInfo.profile.height_in_inches and fakerAccount.height).
---
Duplicate comments:
In `@tests/e2e/web/utils/deleteUser.ts`:
- Around line 24-29: The Google branch (inside the authType === 'Google' path)
calls deleteFromDb without error handling, which can throw if the DB record is
missing; wrap the DB deletion in a try/catch (or check existence before
deleting) so a failing deleteFromDb does not bubble up and mask the successful
deleteAccount call—specifically modify the block that uses
findUser(authInfo.idToken), deleteAccount(authInfo.idToken) and
deleteFromDb(authInfo.localId) to catch and log/ignore DB-not-found errors while
still allowing other unexpected errors to surface.
---
Nitpick comments:
In `@tests/e2e/utils/seedDatabase.ts`:
- Around line 110-116: The exported function seedUser lacks a docstring; add a
concise JSDoc/TSDoc comment above the exported async function seedUser
describing its purpose, parameters (email, password, profileType, displayName,
userName) including types and optionality, return value (what it resolves to or
side-effects like creating a DB user), and any important behavior (defaults,
side effects, thrown errors). Ensure the comment is placed immediately above the
seedUser declaration and follows the project's docstring style (/** ... */) so
linters and docs pick it up.
- Around line 20-22: The function signature in tests/e2e/utils/seedDatabase.ts
uses the boxed type Promise<Boolean>; change that to the primitive type
Promise<boolean> in the function declaration (replace Promise<Boolean> with
Promise<boolean>) and update any matching overloads or exported type annotations
in the same file that reference Promise<Boolean> so the return type consistently
uses the primitive boolean.
- Around line 124-125: The new hardcoded strings ('full' and the debug message
"User created in Firebase and Supabase:") should be moved to constants; update
the call in seedDbUser to use a constant (e.g., PROFILE_TYPE_FULL) instead of
the literal 'full', and replace the debug log message with a constant (e.g.,
LOG_USER_CREATED) concatenated or formatted with userInfo.email; add these new
constants to the shared constants file and import them into
tests/e2e/utils/seedDatabase.ts so seedDbUser(...) and debug(...) use the
constants instead of inline literals.
- Line 89: Replace the unsafe tx: any in the async callback passed to pg.tx with
an explicit transaction type from the DB library used in this repo (e.g.,
PoolClient from 'pg' or ITask/ITaskContext from 'pg-promise'); import that type
at the top of tests/e2e/utils/seedDatabase.ts and annotate the callback
parameter (the tx in the pg.tx(...) lambda) with it so the transaction helper in
seedDatabase.ts has proper typing instead of any.
In `@tests/e2e/web/pages/AuthPage.ts`:
- Around line 52-62: The signInToGoogleAccount method can race with async
rendering after getGooglePopupPage(); to fix, after obtaining popup from
getGooglePopupPage() explicitly wait for the popup UI to be ready before
interacting (e.g., use popup.waitForSelector or call .waitFor({state:
'visible'}) on the 'Add new account' locator and/or the Email input), then
proceed to click and fill fields; update signInToGoogleAccount to wait for
visibility of popup.getByText('Add new account', {exact: true}) and
popup.getByLabel('Email') (and optionally Display name / Screen name) before
calling click()/fill().
In `@tests/e2e/web/pages/homePage.ts`:
- Around line 161-196: Extract the repeated sidebar assertions into a private
helper: create a private async verifyCommonSidebarLinks() that contains the
shared expects for sidebarAbout, sidebarFaq, sidebarVote, sidebarEvents,
sidebarWhatsNew, sidebarSocials, sidebarOrganization and sidebarContact, then
call this helper from both verifyHomePageLinks() and
verifySignedInHomePage(displayName) instead of duplicating those expects; ensure
the helper is marked private and awaited where used.
In `@tests/e2e/web/utils/accountInformation.ts`:
- Around line 149-156: The dev_two_account factory creates an unused local
variable id (const id = crypto.randomUUID().slice(0, 6)) which should be
removed; edit the dev_two_account function to delete the unused id declaration
(matching the fix applied to dev_one_account) so no unused variables remain and
linting will pass.
- Around line 139-146: In the dev_one_account factory function remove the unused
id variable generation (const id = crypto.randomUUID().slice(0, 6)) since email
and username are now fixed; update the dev_one_account function to simply return
the object without declaring or using id to eliminate the unused-variable
warning (refer to dev_one_account in tests/e2e/web/utils/accountInformation.ts).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 555c985e-3221-46ba-9245-9a61522b6b92
📒 Files selected for processing (13)
playwright.config.tstests/e2e/utils/seedDatabase.tstests/e2e/web/pages/AuthPage.tstests/e2e/web/pages/homePage.tstests/e2e/web/pages/settingsPage.tstests/e2e/web/specs/onboardingFlow.spec.tstests/e2e/web/specs/signIn.spec.tstests/e2e/web/specs/signUp.spec.tstests/e2e/web/utils/accountInformation.tstests/e2e/web/utils/deleteUser.tstests/e2e/web/utils/testCleanupHelpers.tsweb/components/measurement-system-toggle.tsxweb/components/nav/sidebar.tsx
✅ Files skipped from review due to trivial changes (1)
- web/components/measurement-system-toggle.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- web/components/nav/sidebar.tsx
- playwright.config.ts
- tests/e2e/web/specs/signUp.spec.ts
- tests/e2e/web/specs/signIn.spec.ts
- tests/e2e/web/utils/testCleanupHelpers.ts
Comment on lines
+96
to
97
| username: cleanUsername(userInfo.userName), | ||
| data: {}, |
There was a problem hiding this comment.
Guard against empty usernames after normalization.
Line 96 can normalize symbol/whitespace-heavy input to an empty username, then insert invalid data.
🛡️ Proposed fix
- await insert(tx, 'users', {
+ const normalizedUsername = cleanUsername(userInfo.userName).trim()
+ if (!normalizedUsername) throw new Error('seedDbUser: username is empty after normalization')
+
+ await insert(tx, 'users', {
id: userId,
name: userInfo.name,
- username: cleanUsername(userInfo.userName),
+ username: normalizedUsername,
data: {},
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/utils/seedDatabase.ts` around lines 96 - 97, The username field is
set to cleanUsername(userInfo.userName) which can return an empty string for
symbol/whitespace-heavy input; before inserting the user (in
tests/e2e/utils/seedDatabase.ts where the user object is built with username:
cleanUsername(userInfo.userName), data: {}), validate the normalized username
and guard against empty values—either skip creating/inserting that user or throw
a clear error; add a conditional that checks the result of
cleanUsername(userInfo.userName) and handles the empty-case appropriately (e.g.,
log/throw or generate a fallback) so no invalid empty username is inserted.
Comment on lines
+152
to
+158
| async fillDeleteAccountSurvey(reason: string) { | ||
| await expect(this.deleteSurveyModal).toBeVisible() | ||
| await expect(this.deleteSurveyReasons).toBeVisible() | ||
| await this.deleteSurveyReasons.locator('div').nth(1).click() | ||
| await expect(this.deleteSurveyDetails).toBeVisible() | ||
| await this.deleteSurveyDetails.fill(reason) | ||
| } |
There was a problem hiding this comment.
Brittle positional selector for survey reason selection.
Using .locator('div').nth(1) to select a survey reason is fragile and depends on the exact DOM structure. If the component's markup changes, this will silently select the wrong option or fail.
Per the context snippet, the RadioGroup.Option elements are what need to be clicked. Consider using a more stable selector based on the reason's text or value.
Proposed improvement
async fillDeleteAccountSurvey(reason: string) {
await expect(this.deleteSurveyModal).toBeVisible()
await expect(this.deleteSurveyReasons).toBeVisible()
- await this.deleteSurveyReasons.locator('div').nth(1).click()
+ // Click on a specific reason by its visible text for stability
+ await this.deleteSurveyReasons.getByText('I found a connection elsewhere').click()
await expect(this.deleteSurveyDetails).toBeVisible()
await this.deleteSurveyDetails.fill(reason)
}Alternatively, consider parameterizing the reason selection:
async fillDeleteAccountSurvey(reason: string, selectedReason: string = 'I found a connection elsewhere') {
await expect(this.deleteSurveyModal).toBeVisible()
await expect(this.deleteSurveyReasons).toBeVisible()
await this.deleteSurveyReasons.getByText(selectedReason).click()
await expect(this.deleteSurveyDetails).toBeVisible()
await this.deleteSurveyDetails.fill(reason)
}📝 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
| async fillDeleteAccountSurvey(reason: string) { | |
| await expect(this.deleteSurveyModal).toBeVisible() | |
| await expect(this.deleteSurveyReasons).toBeVisible() | |
| await this.deleteSurveyReasons.locator('div').nth(1).click() | |
| await expect(this.deleteSurveyDetails).toBeVisible() | |
| await this.deleteSurveyDetails.fill(reason) | |
| } | |
| async fillDeleteAccountSurvey(reason: string) { | |
| await expect(this.deleteSurveyModal).toBeVisible() | |
| await expect(this.deleteSurveyReasons).toBeVisible() | |
| await this.deleteSurveyReasons.getByText(reason).click() | |
| await expect(this.deleteSurveyDetails).toBeVisible() | |
| await this.deleteSurveyDetails.fill(reason) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/web/pages/settingsPage.ts` around lines 152 - 158, The test uses a
brittle positional selector in fillDeleteAccountSurvey; replace the
.locator('div').nth(1) click with a stable selection based on the option text or
value and parameterize it. Update the fillDeleteAccountSurvey signature to
accept an optional selectedReason (default to the common label like "I found a
connection elsewhere"), then assert modal and reasons visibility, click
this.deleteSurveyReasons.getByText(selectedReason) (or equivalent
text/value-based selector) and then continue to assert this.deleteSurveyDetails
visibility and fill(reason); keep existing expectations but remove the
positional nth selector.
Comment on lines
-239
to
+284
| await progressToRequiredForm(homePage, authPage, fakerAccount, onboardingPage) | ||
| await registerWithEmail(homePage, authPage, fakerAccount) | ||
| await onboardingPage.clickSkipOnboardingButton() | ||
| await signUpPage.fillDisplayName(fakerAccount.display_name) | ||
| await signUpPage.fillUsername(fakerAccount.username) | ||
| await signUpPage.clickNextButton() | ||
| await signUpPage.clickNextButton() //Skip optional information | ||
| await profilePage.clickCloseButton() | ||
| await onboardingPage.clickRefineProfileButton() | ||
|
|
Member
There was a problem hiding this comment.
Could you DRY this? It's duplicated 3+ times across the files. In other words, move it to the a reusable helper function.
Member
There was a problem hiding this comment.
Actuallty, can we also remove should successfully skip the onboarding flow since all its logic is already in should successfully delete an account created via email and password?
We could just keep the later and rename it should successfully skip account creation and then delete it created (email / password).
Or else, if we prefer one test per tested feature, we should not do account creation through the onboarding in the second test since it should just check account deletion. For tests like that which do not verify the onboarding, we should create and sign in the account programmatically using seedUser() and then handleEmailPasswordSignIn().
Member
There was a problem hiding this comment.
Let's do method 2 by separating tested features? Method one would be since we can't create google accounts in the UI.
So for account deletion testing, we create accounts programmatically (both email/pwd and google) and we check that the get deleted correctly through E2E.
Collaborator
Author
There was a problem hiding this comment.
I think it would cover more bases if we do method 1, removing the should successfully skip the onboarding flow and have the logic covered by should successfully delete an account created via email and password, having both would leave a better paper trail of what was tested (<- my original thinking) but its not really necessary.
I would say account deletion is part of the user flow, so having it tested naturally seems more end to end, we can leave the seedUser() accounts for testing things within other user flows.
Let me know what you think about this
Member
There was a problem hiding this comment.
@coderabbitai Give suggestions on the industry level practices there
There was a problem hiding this comment.
@MartinBraquet Here's a breakdown of industry-level practices relevant to this debate:
Core Principle: One Test, One Concern
The dominant industry position (as reflected in the Testing Trophy, Playwright docs, and Cypress best practice guides) is:
Each E2E test should verify exactly one user-facing behaviour. Setup that isn't under test should be as fast and reliable as possible — ideally programmatic.
This favours Method 2.
Why Method 2 is the industry-standard approach
| Concern | Method 1 (natural flow) | Method 2 (programmatic setup) |
|---|---|---|
| Failure diagnosis | Fail could be onboarding or deletion | Fail is always about deletion |
| Speed | Slow — full UI onboarding on every deletion run | Fast — seedUser() is a direct DB/API call |
| Flakiness surface | Large — any onboarding regression breaks deletion tests too | Minimal — only the deletion UI path can flake |
| Test intent clarity | Blurred — "what is this test actually asserting?" | Clear — "this test asserts account deletion works" |
| CI cost | Higher | Lower |
A rule of thumb from the Playwright team: "Use API calls or database helpers for state setup; reserve the browser for what you're actually testing."
Where Method 1 is appropriate
Method 1 (full UI journey) is the right choice for dedicated end-to-end journey tests, i.e. tests whose explicit purpose is to verify an entire user flow. For example:
should successfully complete the onboarding flow with email— this is correctly a full journey test.should successfully complete the onboarding flow with google account— same, and here Method 1 is unavoidable since Google accounts cannot be created programmatically.
The deletion tests are not journey tests for onboarding, so they shouldn't perform onboarding via UI.
Recommended structure
should successfully complete the onboarding flow with email ← full journey, Method 1 ✓
should successfully complete the onboarding flow with google ← full journey, Method 1 (forced) ✓
should successfully skip the onboarding flow ← REMOVE (covered above or redundant)
should successfully delete an account (email/password) ← programmatic seedUser() + UI delete, Method 2
should successfully delete an account (google) ← UI google sign-in + UI delete, Method 1 (forced)
DRY — extract a helper regardless
Regardless of which method is chosen, the repeated 7-line block should be extracted. Since registerWithEmail already exists, a natural extension would be:
/**
* Completes the minimal onboarding flow (skip path) after email registration.
* Fills only display name and username, skips all optional steps.
*/
async function completeMinimalOnboarding(
onboardingPage: OnboardingPage,
signUpPage: SignUpPage,
profilePage: ProfilePage,
account: UserAccountInformation,
) {
await onboardingPage.clickSkipOnboardingButton()
await signUpPage.fillDisplayName(account.display_name)
await signUpPage.fillUsername(account.username)
await signUpPage.clickNextButton()
await signUpPage.clickNextButton() // Skip optional information
await profilePage.clickCloseButton()
await onboardingPage.clickRefineProfileButton()
}This appears in at least 6 tests, so consolidating it reduces maintenance significantly.
Summary recommendation
- Keep the full journey onboarding tests (they're the right place for Method 1).
- Remove
should successfully skip the onboarding flow— it duplicates the onboarding journey tests. - Rewrite email-password deletion to use
seedUser()+handleEmailPasswordSignIn()(Method 2). - Keep Google deletion as UI-driven (Method 1, no alternative).
- Extract the repeated onboarding block into
completeMinimalOnboarding(...).
Member
There was a problem hiding this comment.
Method 2 is likely better, but I get it that you would need to change a few things in the current PR in order to implement it. So I'm happy to merge this PR without the recommended change above :)
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!
MartinBraquet
requested changes
Apr 3, 2026
Comment on lines
-239
to
+284
| await progressToRequiredForm(homePage, authPage, fakerAccount, onboardingPage) | ||
| await registerWithEmail(homePage, authPage, fakerAccount) | ||
| await onboardingPage.clickSkipOnboardingButton() | ||
| await signUpPage.fillDisplayName(fakerAccount.display_name) | ||
| await signUpPage.fillUsername(fakerAccount.username) | ||
| await signUpPage.clickNextButton() | ||
| await signUpPage.clickNextButton() //Skip optional information | ||
| await profilePage.clickCloseButton() | ||
| await onboardingPage.clickRefineProfileButton() | ||
|
|
Member
There was a problem hiding this comment.
Let's do method 2 by separating tested features? Method one would be since we can't create google accounts in the UI.
So for account deletion testing, we create accounts programmatically (both email/pwd and google) and we check that the get deleted correctly through E2E.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
tests/e2e/web/specs/onboardingFlow.spec.ts (3)
241-248:⚠️ Potential issue | 🟠 MajorMove the Google auth readiness check into
AuthPageand gate the suite earlier.Both specs still need
fillPasswordField('')immediately beforesignInToGoogleAccount(), so the readiness workaround is duplicated in the tests instead of owned byAuthPage. In the first Google test, Line 248 also skips headless only after the popup flow has already started. Centralize the wait insidesignInToGoogleAccount()and skip the whole Google path before any auth interaction.Also applies to: 347-352
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/specs/onboardingFlow.spec.ts` around lines 241 - 248, The tests are relying on a duplicated readiness workaround (calling authPage.fillPasswordField('') before signInToGoogleAccount) and skipping headless too late; move that readiness check into AuthPage.signInToGoogleAccount so the method itself awaits whatever DOM/permission/pop-up readiness is required and remove fillPasswordField('') calls from the specs (e.g., references in onboardingFlow.spec.ts around the Google flow). Also gate the entire Google suite earlier by calling test.skip(headless, ...) before any auth interactions (instead of after starting the popup) so the suite exits prior to navigation; apply the same changes to the other occurrence (lines ~347-352).
529-530:⚠️ Potential issue | 🟡 MinorReplace the placeholder with a real error-path assertion or remove it.
This currently makes the error suite pass without covering any failure behavior. If helpful, I can sketch a concrete negative-path case for this block.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/specs/onboardingFlow.spec.ts` around lines 529 - 530, The "when an error occurs" suite contains a no-op placeholder test that makes the suite vacuous; either replace the placeholder test('placeholder', ...) with a concrete negative-path assertion (e.g., simulate the error condition and assert the expected error UI/message or redirect) or remove the entire describe block; locate the test.describe('when an error occurs') block in onboardingFlow.spec.ts and update the inner test to perform the failing scenario using the same helpers/fixtures used by other tests (or delete the suite) so the error path is actually covered.
297-305:⚠️ Potential issue | 🟠 MajorThese deletion specs still appear to clean up the same user twice.
Both tests delete the account in-app through Settings, but they still use fixtures that are typically torn down after
use(). That matches theNo user found...teardown noise called out on the PR. Switch these to a no-cleanup fixture, or mark the account as already deleted so fixture teardown skipsdeleteUser().Verify whether
tests/e2e/web/fixtures/base.tsstill tears these fixtures down withdeleteUser()afteruse():#!/bin/bash rg -n -C3 '\b(deleteUser|fakerAccount|googleAccountTwo)\b' tests/e2e/web/fixtures/base.ts sed -n '297,375p' tests/e2e/web/specs/onboardingFlow.spec.tsAlso applies to: 327-332, 334-342, 370-375
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/specs/onboardingFlow.spec.ts` around lines 297 - 305, The tests "should successfully delete an account created via email and password" (and the other listed blocks) are causing double-delete noise because the fakerAccount fixture teardown still calls deleteUser(); update the tests to either use a no-cleanup fixture variant (e.g., fakerAccountNoCleanup) or mark the account as already deleted so the base fixture's teardown skips deleteUser(). Concretely: locate the fakerAccount usage in these tests and swap to the no-cleanup fixture or set a flag/property (e.g., fakerAccount.deleted = true or fakerAccount.skipTeardown = true) after you perform the in-app deletion so the teardown logic in deleteUser() in the base fixture will detect it and skip attempting deletion. Ensure references to deleteUser(), fakerAccount, and the test titles are updated consistently.tests/e2e/web/utils/deleteUser.ts (1)
22-31:⚠️ Potential issue | 🟠 MajorOnly suppress the explicit “user missing” errors here.
Line 25 currently treats every 400 as benign, and Line 31 logs then swallows everything else. Invalid credentials, bad tokens, and emulator issues will all look like successful cleanup. Only ignore the not-found codes and rethrow the rest.
Suggested tightening
} catch (err: any) { - // Skip deletion if user doesn't exist or other auth errors occur - if ( - err.response?.status === 400 || - err.response?.data?.error?.message?.includes('EMAIL_NOT_FOUND') - ) { + const code = err.response?.data?.error?.message + if (code?.includes('EMAIL_NOT_FOUND')) { console.log(`Email not found, skipping user deletion for ${account?.email}`) return } - console.log(err) + throw err }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/utils/deleteUser.ts` around lines 22 - 31, In the deleteUser.ts catch block for deleteUser (inside tests/e2e/web/utils/deleteUser.ts) stop treating every 400 as benign: only suppress when the error explicitly indicates the user is missing (e.g. err.response?.data?.error?.message includes 'EMAIL_NOT_FOUND' or the exact provider code your API returns), and rethrow any other errors (including other 400s) so upstream failures aren't silently swallowed; update the if condition to check only the specific "EMAIL_NOT_FOUND" case (remove the generic err.response?.status === 400 branch), keep the console.log message for the skipped deletion using account?.email, and throw err for all other cases.
🧹 Nitpick comments (1)
tests/e2e/utils/firebaseUtils.ts (1)
4-16: Add docstrings to the exported Firebase helpers.These exports now define login, lookup, and destructive cleanup behavior, but their required inputs, return shapes, and swallowed error cases are not documented in-file. A short docstring on each helper would make the utilities much easier to call correctly. As per coding guidelines, "Ensure all public functions have docstrings."
Also applies to: 28-35, 62-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/utils/firebaseUtils.ts` around lines 4 - 16, Add missing docstrings to the exported Firebase helper functions (e.g., firebaseLoginEmailPassword, the lookup helper, and the destructive cleanup helper) describing each function's purpose, parameter types (e.g., email?: string, password?: string), the exact return shape (axios Response with auth token / user info or specific object fields), and error behavior (which errors are propagated vs. swallowed). Update the top of each exported function to include a short JSDoc-style comment that lists required/optional params, expected return value structure, and whether the function throws or handles errors internally so callers know how to use firebaseLoginEmailPassword, the lookup function, and the cleanup function correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/web/utils/deleteUser.ts`:
- Around line 7-10: The deleteUser function presently accepts multiple optional
params and can be invoked with missing auth-specific payloads; add a clear
docstring describing the two valid call shapes (e.g., password-based requires
account, google-based requires authInfo), then validate inputs at the top of
deleteUser (use AuthType enum/value to assert required parameter presence) and
throw a descriptive error if the matching credential is missing so teardown
fails fast; alternatively implement two explicit overloads/signatures or
separate helpers (e.g., deleteUserWithPassword and deleteUserWithGoogle) and
ensure callers are updated to pass the correct shape (refer to symbols AuthType,
UserAccountInformation, AuthObject, and function deleteUser).
---
Duplicate comments:
In `@tests/e2e/web/specs/onboardingFlow.spec.ts`:
- Around line 241-248: The tests are relying on a duplicated readiness
workaround (calling authPage.fillPasswordField('') before signInToGoogleAccount)
and skipping headless too late; move that readiness check into
AuthPage.signInToGoogleAccount so the method itself awaits whatever
DOM/permission/pop-up readiness is required and remove fillPasswordField('')
calls from the specs (e.g., references in onboardingFlow.spec.ts around the
Google flow). Also gate the entire Google suite earlier by calling
test.skip(headless, ...) before any auth interactions (instead of after starting
the popup) so the suite exits prior to navigation; apply the same changes to the
other occurrence (lines ~347-352).
- Around line 529-530: The "when an error occurs" suite contains a no-op
placeholder test that makes the suite vacuous; either replace the placeholder
test('placeholder', ...) with a concrete negative-path assertion (e.g., simulate
the error condition and assert the expected error UI/message or redirect) or
remove the entire describe block; locate the test.describe('when an error
occurs') block in onboardingFlow.spec.ts and update the inner test to perform
the failing scenario using the same helpers/fixtures used by other tests (or
delete the suite) so the error path is actually covered.
- Around line 297-305: The tests "should successfully delete an account created
via email and password" (and the other listed blocks) are causing double-delete
noise because the fakerAccount fixture teardown still calls deleteUser(); update
the tests to either use a no-cleanup fixture variant (e.g.,
fakerAccountNoCleanup) or mark the account as already deleted so the base
fixture's teardown skips deleteUser(). Concretely: locate the fakerAccount usage
in these tests and swap to the no-cleanup fixture or set a flag/property (e.g.,
fakerAccount.deleted = true or fakerAccount.skipTeardown = true) after you
perform the in-app deletion so the teardown logic in deleteUser() in the base
fixture will detect it and skip attempting deletion. Ensure references to
deleteUser(), fakerAccount, and the test titles are updated consistently.
In `@tests/e2e/web/utils/deleteUser.ts`:
- Around line 22-31: In the deleteUser.ts catch block for deleteUser (inside
tests/e2e/web/utils/deleteUser.ts) stop treating every 400 as benign: only
suppress when the error explicitly indicates the user is missing (e.g.
err.response?.data?.error?.message includes 'EMAIL_NOT_FOUND' or the exact
provider code your API returns), and rethrow any other errors (including other
400s) so upstream failures aren't silently swallowed; update the if condition to
check only the specific "EMAIL_NOT_FOUND" case (remove the generic
err.response?.status === 400 branch), keep the console.log message for the
skipped deletion using account?.email, and throw err for all other cases.
---
Nitpick comments:
In `@tests/e2e/utils/firebaseUtils.ts`:
- Around line 4-16: Add missing docstrings to the exported Firebase helper
functions (e.g., firebaseLoginEmailPassword, the lookup helper, and the
destructive cleanup helper) describing each function's purpose, parameter types
(e.g., email?: string, password?: string), the exact return shape (axios
Response with auth token / user info or specific object fields), and error
behavior (which errors are propagated vs. swallowed). Update the top of each
exported function to include a short JSDoc-style comment that lists
required/optional params, expected return value structure, and whether the
function throws or handles errors internally so callers know how to use
firebaseLoginEmailPassword, the lookup function, and the cleanup function
correctly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4b3554e9-783e-4fdc-b1c3-bc41edb19d0e
📒 Files selected for processing (8)
tests/e2e/utils/databaseUtils.tstests/e2e/utils/firebaseUtils.tstests/e2e/web/fixtures/base.tstests/e2e/web/pages/AuthPage.tstests/e2e/web/specs/onboardingFlow.spec.tstests/e2e/web/specs/signIn.spec.tstests/e2e/web/utils/deleteUser.tstests/e2e/web/utils/networkUtils.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/e2e/web/utils/networkUtils.ts
- tests/e2e/web/specs/signIn.spec.ts
- tests/e2e/web/pages/AuthPage.ts
- tests/e2e/web/fixtures/base.ts
Comment on lines
+7
to
+10
| export async function deleteUser( | ||
| authType: AuthType, | ||
| account?: UserAccountInformation, | ||
| authInfo?: AuthObject, |
There was a problem hiding this comment.
Fail fast when the auth-specific payload is missing.
Line 15 will send undefined credentials when account is absent, while Line 18 turns Google cleanup into a silent success if authInfo is missing. For teardown helpers, that hides bad fixture state and leaks accounts. Guard the inputs up front or make the two valid call shapes explicit in the function signature; a short docstring here would also make the contract clearer. As per coding guidelines, "Ensure all public functions have docstrings." and "Check for edge cases like null values or empty arrays."
Also applies to: 14-20
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/e2e/web/utils/deleteUser.ts` around lines 7 - 10, The deleteUser
function presently accepts multiple optional params and can be invoked with
missing auth-specific payloads; add a clear docstring describing the two valid
call shapes (e.g., password-based requires account, google-based requires
authInfo), then validate inputs at the top of deleteUser (use AuthType
enum/value to assert required parameter presence) and throw a descriptive error
if the matching credential is missing so teardown fails fast; alternatively
implement two explicit overloads/signatures or separate helpers (e.g.,
deleteUserWithPassword and deleteUserWithGoogle) and ensure callers are updated
to pass the correct shape (refer to symbols AuthType, UserAccountInformation,
AuthObject, and function deleteUser).
There was a problem hiding this comment.
♻️ Duplicate comments (7)
tests/e2e/web/pages/AuthPage.ts (2)
58-59:⚠️ Potential issue | 🟠 MajorArm the popup close listener before clicking submit.
Lines 58-59 still race: a fast close can fire before
waitForEvent('close')is registered, which can leave the test hanging intermittently.Suggested fix
- await popup.getByText('Sign in with Google.com', {exact: true}).click() - await popup.waitForEvent('close') + await Promise.all([ + popup.waitForEvent('close'), + popup.getByText('Sign in with Google.com', {exact: true}).click(), + ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/pages/AuthPage.ts` around lines 58 - 59, The popup close listener is raced with the click: replace the direct await popup.waitForEvent('close') after click by creating the close listener promise before clicking (e.g., call popup.waitForEvent('close') and store the promise), then trigger the click via popup.getByText('Sign in with Google.com', {exact: true}).click(), and finally await the stored close promise; update the code around popup.getByText(...) and popup.waitForEvent('close') accordingly to ensure the listener is armed before the click.
43-49:⚠️ Potential issue | 🟠 MajorMake the Google helper own the page-readiness wait.
The specs still need
fillPasswordField('')before every Google login, so this helper is not fully synchronizing the auth screen yet. Wait for a stable auth-form locator here before clicking Google so callers can just usesignInToGoogleAccount()directly.Suggested adjustment
async getGooglePopupPage(): Promise<Page> { + await expect(this.passwordField).toBeVisible() const [popup] = await Promise.all([ - this.page.context().waitForEvent('page'), + this.page.waitForEvent('popup'), this.clickGoogleButton(), ])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/pages/AuthPage.ts` around lines 43 - 49, Update getGooglePopupPage so it fully synchronizes the Google auth popup: after acquiring the popup (the Promise.all with this.page.context().waitForEvent('page') and this.clickGoogleButton()), call await popup.waitForLoadState() and then await popup.waitForSelector(...) for the stable auth-form locator used by signInToGoogleAccount (e.g., the email/password form or input[type="password"] selector) before returning the popup. This makes getGooglePopupPage own the readiness wait so callers like signInToGoogleAccount no longer need to call fillPasswordField('') beforehand; keep references to getGooglePopupPage, clickGoogleButton, signInToGoogleAccount, and fillPasswordField when making the change.tests/e2e/web/specs/onboardingFlow.spec.ts (3)
284-309:⚠️ Potential issue | 🟠 MajorThese deletion scenarios are still cleaned up twice.
fakerAccountandgoogleAccountTwoboth unconditionally calldeleteUser()intests/e2e/web/fixtures/base.ts:45-58afteruse(). Because these tests also delete the account through Settings, teardown can emit the sameNo user found...noise from CI and also mask a broken in-app delete by cleaning the user afterwards. Use a no-cleanup fixture here, or have fixture teardown skip cleanup once deletion succeeds in-app.Also applies to: 311-344
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/specs/onboardingFlow.spec.ts` around lines 284 - 309, The tests call deleteUser() unconditionally from the fixture teardown (see fixtures/base.ts use() for fakerAccount and googleAccountTwo), causing duplicate deletions when the test already deletes the account through the app; modify the fixture so it either provides a "no-cleanup" variant or tracks deletion status and skips teardown cleanup when in-app deletion succeeded. Specifically, update the fixture logic around deleteUser() to check a flag on the account object (e.g., account.deletedInApp) or expose a separate fixture (e.g., fakerAccountNoCleanup) used by the tests in onboardingFlow.spec.ts, and ensure registerWithEmail / deleteProfileFromSettings set that flag after successful in-app deletion so the teardown skips calling deleteUser().
157-159:⚠️ Potential issue | 🟡 MinorInclude inches in the persisted-height assertions.
Both checks still compare
feet * 12, so a5'11"profile is treated as60. That will miss regressions that drop or truncate the inch component.Suggested fix
- await expect(dbInfo.profile.height_in_inches).toEqual( - Number(onboardingAccount.height?.feet) * 12, - ) + await expect(dbInfo.profile.height_in_inches).toEqual( + Number(onboardingAccount.height?.feet) * 12 + + Number(onboardingAccount.height?.inches ?? 0), + )- await expect(dbInfo.profile.height_in_inches).toEqual(Number(fakerAccount.height?.feet) * 12) + await expect(dbInfo.profile.height_in_inches).toEqual( + Number(fakerAccount.height?.feet) * 12 + Number(fakerAccount.height?.inches ?? 0), + )Also applies to: 383-385
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/specs/onboardingFlow.spec.ts` around lines 157 - 159, The assertion for dbInfo.profile.height_in_inches only multiplies onboardingAccount.height?.feet by 12 and omits the inches component; update the assertion to include the inches field by adding Number(onboardingAccount.height?.inches || 0) to the feet*12 calculation so the expected value equals (feet * 12) + inches for the checks referencing dbInfo.profile.height_in_inches and onboardingAccount.height (e.g., the assertions around dbInfo.profile.height_in_inches and onboardingAccount.height?.feet/inches).
492-493:⚠️ Potential issue | 🟡 MinorReplace the placeholder with a real error-path assertion.
This test always passes without exercising any failure mode, so the suite reports coverage that does not exist.
If you want, I can sketch a concrete negative-path case for this block.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/specs/onboardingFlow.spec.ts` around lines 492 - 493, The test under test.describe('when an error occurs') currently contains only test('placeholder', async ({}) => {}) and must be replaced with a concrete negative-path assertion: simulate the failure condition (e.g., mock the network/api call or force the error state used by the onboarding flow), exercise the same UI actions that would hit that code path, and assert the expected error outcome (for example, that an error banner/toast is visible, an error message text is rendered, the submit button is disabled, or navigation does not proceed). Locate the test block named test.describe('when an error occurs') and replace the placeholder test('placeholder', ...) with a test that sets up the failure (via your app's mocking utilities or route interception), triggers the onboarding step, and asserts the specific error UI/behavior expected.tests/e2e/web/utils/deleteUser.ts (2)
7-11:⚠️ Potential issue | 🟠 MajorValidate the auth-specific payload before cleanup starts.
Line 15 can send
undefinedcredentials, and the Google branch silently no-ops whenauthInfois missing. That is reachable becausegetAuthAccountInfo()can returnundefined as unknown as AuthObjectif the intercept never fires. For teardown helpers, that hides broken fixture state and can leak test accounts. Guard the two supported call shapes up front, and document them on the exported helper.Suggested fix
+/** + * Deletes a test user using either email/password credentials or a Google auth token pair. + */ export async function deleteUser( authType: AuthType, account?: UserAccountInformation, authInfo?: AuthObject, ) { + if (authType === 'Email/Password' && !account) { + throw new Error('deleteUser("Email/Password") requires an account') + } + if (authType === 'Google' && !authInfo) { + throw new Error('deleteUser("Google") requires authInfo') + }As per coding guidelines, "Ensure all public functions have docstrings." and "Check for edge cases like null values or empty arrays."
Also applies to: 14-15, 23-28
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/utils/deleteUser.ts` around lines 7 - 11, The exported helper deleteUser currently allows undefined auth payloads (AuthObject) and can silently no-op for google flows when getAuthAccountInfo returns undefined; update deleteUser to validate the auth-specific call shapes up front: for the account/password flow require a non-empty UserAccountInformation, and for the oauth/google flow require a present AuthObject (AuthType and AuthObject must be consistent), throw a clear error if missing, and add a docstring on deleteUser describing the two supported call shapes and the validation behavior; reference deleteUser, AuthType, UserAccountInformation, AuthObject, and getAuthAccountInfo when making these checks.
16-39:⚠️ Potential issue | 🟠 MajorOnly suppress the specific missing-user cleanup case.
The empty
catchon Lines 16-22 and the broadstatus === 400fallback on Lines 31-38 still swallow unrelated Firebase/DB failures. That keeps teardown noisy and can turn a real cleanup regression into a logged success. Also,findUser()intests/e2e/utils/firebaseUtils.ts:28-36returns a response or throws, so the null check on Line 25 never protects Line 27. LetdeleteAccount()absorbUSER_NOT_FOUND, then catchdeleteFromDb()separately and ignore only the knownNo user foundDB miss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/utils/deleteUser.ts` around lines 16 - 39, The empty inner catch should be removed and error handling tightened: call deleteAccount(loginInfo?.data.idToken) directly (remove the redundant findUser null check because findUser throws), let deleteAccount itself swallow Firebase's USER_NOT_FOUND/EMAIL_NOT_FOUND; then call deleteFromDb(loginInfo?.data.localId) inside its own try/catch and only ignore the known DB "No user found" error from userInformationFromDb/deleteFromDb (match on that specific message), rethrow or log other errors; also tighten the outer catch so it only suppresses the specific missing-user error (check err.response?.data?.error?.message for EMAIL_NOT_FOUND/USER_NOT_FOUND) instead of broadly treating all 400s as success.
🧹 Nitpick comments (3)
tests/e2e/web/utils/testCleanupHelpers.ts (2)
9-64: Add docstrings to these exported helpers.These functions now wrap multi-step registration, sign-in, onboarding, and destructive cleanup flows. Short docstrings would make the required page state and side effects obvious at call sites.
As per coding guidelines, "Ensure all public functions have docstrings."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/utils/testCleanupHelpers.ts` around lines 9 - 64, Add concise docstrings to each exported helper function (registerWithEmail, signinWithEmail, skipOnboardingHeadToProfile, deleteProfileFromSettings) describing its purpose, required input/state (which pages or account fields must be provided), main steps/side effects (e.g., navigation, form fills, account creation or deletion), and any return/throw behavior (e.g., signinWithEmail throws on missing credentials). Place the docstring immediately above each function export using the project's docstring style so callers can understand preconditions and outcomes.
55-62: Extract the deletion reason instead of hardcoding it here.
'Delete me'is shared test data now. Moving it into the constants/test-data module will keep this destructive-flow text consistent across helpers and specs.As per coding guidelines, "Flag any hardcoded strings; they should be in the constants file."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/utils/testCleanupHelpers.ts` around lines 55 - 62, The deleteProfileFromSettings helper currently hardcodes the deletion reason ('Delete me'); update it to import and use the shared constant from the constants/test-data module instead. In the deleteProfileFromSettings function, replace the literal passed to settingsPage.fillDeleteAccountSurvey with the exported constant (e.g., DELETE_ACCOUNT_REASON or the appropriate name from constants/test-data) so all callers (and other helpers) share the same test-data value; keep the existing calls to homePage.clickSettingsLink and settingsPage.clickDeleteAccountButton unchanged.tests/e2e/web/pages/AuthPage.ts (1)
38-59: Document the new Google auth helpers.These public methods now hide popup timing, navigation, and optional profile-field population. A short docstring on each helper would make the expected call order and side effects clearer for the specs that reuse them.
As per coding guidelines, "Ensure all public functions have docstrings."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/web/pages/AuthPage.ts` around lines 38 - 59, Add docstrings to the public helper methods clickGoogleButton, getGooglePopupPage, and signInToGoogleAccount describing their purpose, expected call order, return values, and side effects (e.g., that clickGoogleButton clicks the visible Google button, getGooglePopupPage waits for and returns the popup Page, and signInToGoogleAccount fills optional profile fields and waits for the popup to close). Place a short JS/TS doc comment above each method mentioning input parameters (email, display_name, username) and any awaited events (navigation, popup close) so callers understand timing and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/e2e/web/pages/AuthPage.ts`:
- Around line 58-59: The popup close listener is raced with the click: replace
the direct await popup.waitForEvent('close') after click by creating the close
listener promise before clicking (e.g., call popup.waitForEvent('close') and
store the promise), then trigger the click via popup.getByText('Sign in with
Google.com', {exact: true}).click(), and finally await the stored close promise;
update the code around popup.getByText(...) and popup.waitForEvent('close')
accordingly to ensure the listener is armed before the click.
- Around line 43-49: Update getGooglePopupPage so it fully synchronizes the
Google auth popup: after acquiring the popup (the Promise.all with
this.page.context().waitForEvent('page') and this.clickGoogleButton()), call
await popup.waitForLoadState() and then await popup.waitForSelector(...) for the
stable auth-form locator used by signInToGoogleAccount (e.g., the email/password
form or input[type="password"] selector) before returning the popup. This makes
getGooglePopupPage own the readiness wait so callers like signInToGoogleAccount
no longer need to call fillPasswordField('') beforehand; keep references to
getGooglePopupPage, clickGoogleButton, signInToGoogleAccount, and
fillPasswordField when making the change.
In `@tests/e2e/web/specs/onboardingFlow.spec.ts`:
- Around line 284-309: The tests call deleteUser() unconditionally from the
fixture teardown (see fixtures/base.ts use() for fakerAccount and
googleAccountTwo), causing duplicate deletions when the test already deletes the
account through the app; modify the fixture so it either provides a "no-cleanup"
variant or tracks deletion status and skips teardown cleanup when in-app
deletion succeeded. Specifically, update the fixture logic around deleteUser()
to check a flag on the account object (e.g., account.deletedInApp) or expose a
separate fixture (e.g., fakerAccountNoCleanup) used by the tests in
onboardingFlow.spec.ts, and ensure registerWithEmail / deleteProfileFromSettings
set that flag after successful in-app deletion so the teardown skips calling
deleteUser().
- Around line 157-159: The assertion for dbInfo.profile.height_in_inches only
multiplies onboardingAccount.height?.feet by 12 and omits the inches component;
update the assertion to include the inches field by adding
Number(onboardingAccount.height?.inches || 0) to the feet*12 calculation so the
expected value equals (feet * 12) + inches for the checks referencing
dbInfo.profile.height_in_inches and onboardingAccount.height (e.g., the
assertions around dbInfo.profile.height_in_inches and
onboardingAccount.height?.feet/inches).
- Around line 492-493: The test under test.describe('when an error occurs')
currently contains only test('placeholder', async ({}) => {}) and must be
replaced with a concrete negative-path assertion: simulate the failure condition
(e.g., mock the network/api call or force the error state used by the onboarding
flow), exercise the same UI actions that would hit that code path, and assert
the expected error outcome (for example, that an error banner/toast is visible,
an error message text is rendered, the submit button is disabled, or navigation
does not proceed). Locate the test block named test.describe('when an error
occurs') and replace the placeholder test('placeholder', ...) with a test that
sets up the failure (via your app's mocking utilities or route interception),
triggers the onboarding step, and asserts the specific error UI/behavior
expected.
In `@tests/e2e/web/utils/deleteUser.ts`:
- Around line 7-11: The exported helper deleteUser currently allows undefined
auth payloads (AuthObject) and can silently no-op for google flows when
getAuthAccountInfo returns undefined; update deleteUser to validate the
auth-specific call shapes up front: for the account/password flow require a
non-empty UserAccountInformation, and for the oauth/google flow require a
present AuthObject (AuthType and AuthObject must be consistent), throw a clear
error if missing, and add a docstring on deleteUser describing the two supported
call shapes and the validation behavior; reference deleteUser, AuthType,
UserAccountInformation, AuthObject, and getAuthAccountInfo when making these
checks.
- Around line 16-39: The empty inner catch should be removed and error handling
tightened: call deleteAccount(loginInfo?.data.idToken) directly (remove the
redundant findUser null check because findUser throws), let deleteAccount itself
swallow Firebase's USER_NOT_FOUND/EMAIL_NOT_FOUND; then call
deleteFromDb(loginInfo?.data.localId) inside its own try/catch and only ignore
the known DB "No user found" error from userInformationFromDb/deleteFromDb
(match on that specific message), rethrow or log other errors; also tighten the
outer catch so it only suppresses the specific missing-user error (check
err.response?.data?.error?.message for EMAIL_NOT_FOUND/USER_NOT_FOUND) instead
of broadly treating all 400s as success.
---
Nitpick comments:
In `@tests/e2e/web/pages/AuthPage.ts`:
- Around line 38-59: Add docstrings to the public helper methods
clickGoogleButton, getGooglePopupPage, and signInToGoogleAccount describing
their purpose, expected call order, return values, and side effects (e.g., that
clickGoogleButton clicks the visible Google button, getGooglePopupPage waits for
and returns the popup Page, and signInToGoogleAccount fills optional profile
fields and waits for the popup to close). Place a short JS/TS doc comment above
each method mentioning input parameters (email, display_name, username) and any
awaited events (navigation, popup close) so callers understand timing and
behavior.
In `@tests/e2e/web/utils/testCleanupHelpers.ts`:
- Around line 9-64: Add concise docstrings to each exported helper function
(registerWithEmail, signinWithEmail, skipOnboardingHeadToProfile,
deleteProfileFromSettings) describing its purpose, required input/state (which
pages or account fields must be provided), main steps/side effects (e.g.,
navigation, form fills, account creation or deletion), and any return/throw
behavior (e.g., signinWithEmail throws on missing credentials). Place the
docstring immediately above each function export using the project's docstring
style so callers can understand preconditions and outcomes.
- Around line 55-62: The deleteProfileFromSettings helper currently hardcodes
the deletion reason ('Delete me'); update it to import and use the shared
constant from the constants/test-data module instead. In the
deleteProfileFromSettings function, replace the literal passed to
settingsPage.fillDeleteAccountSurvey with the exported constant (e.g.,
DELETE_ACCOUNT_REASON or the appropriate name from constants/test-data) so all
callers (and other helpers) share the same test-data value; keep the existing
calls to homePage.clickSettingsLink and settingsPage.clickDeleteAccountButton
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 964b4a09-2631-467a-bdf0-f3af9a6a76fa
📒 Files selected for processing (6)
playwright.config.tstests/e2e/web/pages/AuthPage.tstests/e2e/web/specs/onboardingFlow.spec.tstests/e2e/web/utils/accountInformation.tstests/e2e/web/utils/deleteUser.tstests/e2e/web/utils/testCleanupHelpers.ts
✅ Files skipped from review due to trivial changes (1)
- playwright.config.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/web/utils/accountInformation.ts
… and formatting adjustments
…lity and consistency.
…ing and remove redundant tests.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
MartinBraquet
approved these changes
Apr 4, 2026
Member
|
Thanks! |
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Summary by CodeRabbit
New Features
Improvements
Bug Fixes