fix(ci): fix failing unit tests and Playwright global setup#2457
Open
mikib0 wants to merge 6 commits into
Open
fix(ci): fix failing unit tests and Playwright global setup#2457mikib0 wants to merge 6 commits into
mikib0 wants to merge 6 commits into
Conversation
- passwordResetService.test.ts: mock timingSafeEqual with object arg
({ a, b }) to match actual function signature — positional mock caused
OTP comparison to always fail, masking the 'User not found' path
- embeddingHelper.test.ts: fix 7 fallback tests that called
getEmbeddingText(item, existingItem) as positional args instead of
the { item, existingItem } object form, causing TypeError on item.name
- getRelativeTime.test.ts: fix 3 i18n tests calling
getRelativeTime(date, t) positionally instead of { dateValue, t }
- globalSetup.ts: replace removed /api/auth/login with Better Auth's
/api/auth/sign-in/email and read session.token as the access token
Contributor
WalkthroughRefactors tests to object-style args, adds a web PackRat API client, updates Playwright E2E auth seeding (DB session insertion or HTTP sign-in) and token parsing, widens fixture auth shapes, increases Playwright/CI timeouts, updates Expo web providers and small UI behaviors, and adds a lint exclusion. ChangesTest and Playwright E2E changes
sequenceDiagram
participant GlobalSetup
participant Database
participant API
participant FS as FileSystem
GlobalSetup->>Database: lookup user by TEST_EMAIL (if NEON_DATABASE_URL)
Database-->>GlobalSetup: returns user?
alt user found
GlobalSetup->>Database: insert session row (generate token)
Database-->>GlobalSetup: session.token
GlobalSetup->>FS: write .auth-tokens.json { accessToken: session.token, refreshToken: null, user }
else no DB session
GlobalSetup->>API: POST /api/auth/sign-in/email (seed creds)
API-->>GlobalSetup: { session: { token }, user, maybe refreshToken }
GlobalSetup->>FS: write .auth-tokens.json { accessToken: session.token, refreshToken: maybeNull, user }
end
Estimated code review effort 🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Coverage Report for API Unit Tests Coverage (./packages/api)
File CoverageNo changed files found. |
Contributor
Coverage Report for Expo Unit Tests Coverage (./apps/expo)
File CoverageNo changed files found. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/expo/playwright/tests/globalSetup.ts (1)
44-61:⚠️ Potential issue | 🔴 CriticalFix incorrect response destructuring at lines 53–60.
The
/api/auth/sign-in/emailendpoint returns{ token: string; user: {...} }(verified in auth.test.ts), not{ session: { token }, refreshToken?, user }. The current code will fail at runtime when trying to accesssession.token.Update line 53 to:
const { token, user } = (await loginRes.json()) as { token: string; user: Record<string, unknown>; };Then update line 60 to use
tokendirectly instead ofsession.token:JSON.stringify({ accessToken: token, refreshToken: null, user }),Also verify that
/api/auth/verify-email(Priority 3, line 98) returns the expected{ accessToken, refreshToken, user }structure — if it doesn't match, the same fix applies there.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/expo/playwright/tests/globalSetup.ts` around lines 44 - 61, The response destructuring for the sign-in call is wrong: replace extracting { session, refreshToken, user } from the /api/auth/sign-in/email JSON with { token, user } and then write accessToken using token (and set refreshToken to null) when writing TOKENS_FILE; update the variables used in the fs.writeFileSync call accordingly. Also check the verify-email response handling (the code that expects { accessToken, refreshToken, user }) and adjust its destructuring to match the actual API shape if it differs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/expo/playwright/tests/globalSetup.ts`:
- Around line 44-61: The response destructuring for the sign-in call is wrong:
replace extracting { session, refreshToken, user } from the
/api/auth/sign-in/email JSON with { token, user } and then write accessToken
using token (and set refreshToken to null) when writing TOKENS_FILE; update the
variables used in the fs.writeFileSync call accordingly. Also check the
verify-email response handling (the code that expects { accessToken,
refreshToken, user }) and adjust its destructuring to match the actual API shape
if it differs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 52de98cb-3f77-4c27-b894-8bb80dcfbb2d
📒 Files selected for processing (4)
apps/expo/lib/utils/__tests__/getRelativeTime.test.tsapps/expo/playwright/tests/globalSetup.tspackages/api/src/services/__tests__/passwordResetService.test.tspackages/api/src/utils/__tests__/embeddingHelper.test.ts
- no-owned-max-params: add apps/trails/scripts/generate-og-images.ts to EXCLUDED_FILES (same globalThis.fetch override pattern as landing/guides) - overpass/client.test.ts: convert makeResponse(body, status) to object param form to satisfy the one-parameter rule - globalSetup.ts: add Priority 2 DB-direct session creation — when NEON_DATABASE_URL is available, insert a session row and write the token directly, bypassing the HTTP login that 404s when EXPO_PUBLIC_API_URL is a same-origin URL without a local API server
The DB-session path in globalSetup writes refreshToken: null when creating a session directly in the DB (no HTTP auth). Playwright's storageState requires all localStorage values to be strings, so passing null caused every test to fail with "expected string, got object". Guard refresh_token the same way user already is, and type refreshToken as string | null to match what globalSetup actually writes.
Per-test: 30s → 60s, assertion: 10s → 15s, CI job: 30min → 60min.
Two root causes prevented every test from passing: 1. authClient.getSession() on web uses browser cookies, not SecureStore. The test browser has no valid Better Auth session cookie, so getSession returns null and useAuthInit redirects every page to /auth. Fix: add a context-level Playwright route mock for **/api/auth/get-session that returns the seeded user + a synthetic session holding the real DB token. 2. packrat.ts reads the session token from SecureStore via the packrat_cookie key. On web, ExpoSecureStore.web.js is an empty object so getItemAsync throws at runtime, leaving every API call without a Bearer token. Fix: add packrat.web.ts that Metro auto-selects for web builds; it calls getActiveToken() which reads from the (now-intercepted) getSession. Also seeds auth_version=v2 in localStorage so the startup migration in useAuthInit does not delete access_token / refresh_token on first boot.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/expo/lib/api/packrat.web.ts`:
- Around line 18-22: onNeedsReauth currently calls authClient.getSession() and
sets needsReauthAtom without checking for an error; change it to examine the
returned { data, error } from authClient.getSession() and return early if error
is present (do not set store.set(needsReauthAtom, true)) so transient failures
don't force reauth. Locate the onNeedsReauth handler and update the logic to:
await authClient.getSession(), check error first, then check data?.session and
only set needsReauthAtom when there is no error and no session (mirroring the
pattern used in useAuthInit and the similar logic in packrat.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: ASSERTIVE
Plan: Pro
Run ID: f39037a8-0140-44d0-9404-5f2389ab44d8
📒 Files selected for processing (2)
apps/expo/lib/api/packrat.web.tsapps/expo/playwright/tests/fixtures.ts
Comment on lines
+18
to
+22
| onNeedsReauth: async () => { | ||
| const { data } = await authClient.getSession(); | ||
| if (data?.session) return; | ||
| store.set(needsReauthAtom, true); | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify authClient.getSession() return shape and existing usage patterns before applying.
set -euo pipefail
echo "== Locate authClient definition =="
fd -i "auth-client" apps | sed -n '1,20p'
echo "== Inspect getSession call sites with context =="
rg -n -C3 --type=ts --type=tsx '\bauthClient\.getSession\s*\('
echo "== Inspect createApiClient auth hooks typing/contracts =="
rg -n -C3 --type=ts --type=tsx '\bonNeedsReauth\b|getAccessToken|getRefreshToken|onAccessTokenRefreshed' apps packagesRepository: PackRat-AI/PackRat
Length of output: 280
🏁 Script executed:
# Read the auth-client.ts to understand getSession return type
cat -n apps/expo/lib/auth-client.ts | head -50Repository: PackRat-AI/PackRat
Length of output: 1036
🏁 Script executed:
# Read the packrat.web.ts file to see the complete context
cat -n apps/expo/lib/api/packrat.web.tsRepository: PackRat-AI/PackRat
Length of output: 1340
🏁 Script executed:
# Search for all getSession usages in the codebase (fixing the file type issue)
rg -n -C3 'getSession\s*\(' apps packages --type ts -A 5 -B 2Repository: PackRat-AI/PackRat
Length of output: 13294
🏁 Script executed:
# Check if there's a better-auth package and its version
fd package.json apps packages | head -5 | xargs grep -l "better-auth" 2>/dev/null || echo "Checking root package.json"
cat package.json | grep -A5 -B5 "better-auth" 2>/dev/null || echo "No better-auth found in root"Repository: PackRat-AI/PackRat
Length of output: 405
Handle errors from authClient.getSession() before setting reauth state.
onNeedsReauth ignores the error field from authClient.getSession(). If the call fails transiently (network issue, service error), the code sets needsReauthAtom = true and bounces the user to login unnecessarily. The codebase already acknowledges this risk in a similar call at apps/expo/lib/api/packrat.ts line 42–46. The auth initialization code in useAuthInit.ts shows the correct pattern: check error first and return early if present.
Suggested fix
onNeedsReauth: async () => {
- const { data } = await authClient.getSession();
- if (data?.session) return;
+ const { data, error } = await authClient.getSession();
+ if (error) return;
+ if (data?.session) return;
store.set(needsReauthAtom, true);
},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/expo/lib/api/packrat.web.ts` around lines 18 - 22, onNeedsReauth
currently calls authClient.getSession() and sets needsReauthAtom without
checking for an error; change it to examine the returned { data, error } from
authClient.getSession() and return early if error is present (do not set
store.set(needsReauthAtom, true)) so transient failures don't force reauth.
Locate the onNeedsReauth handler and update the logic to: await
authClient.getSession(), check error first, then check data?.session and only
set needsReauthAtom when there is no error and no session (mirroring the pattern
used in useAuthInit and the similar logic in packrat.ts).
…lSetup - Remove mandatory location check before trip sync (trips.ts) - Use destructive style on delete confirmation button (getTripDetailOptions) - Switch globalSetup to Better Auth sign-in/sign-up endpoints; drop OTP verification step now that requireEmailVerification=false + autoSignIn=true returns a token directly - Wrap API fetch calls in try/catch with actionable error messages - Re-add BottomSheetModalProvider to web providers (providers/index.web.tsx) - Use pressSequentially for search input to trigger React onChangeText; broaden version string regex in settings test
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/expo/playwright/tests/globalSetup.ts (1)
54-59:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winTable name mismatch:
usersshould beuser.The query references
FROM usersbut the auth schema defines the table asuser(singular):// packages/api/auth-schema.ts:4 export const user = pgTable('user', { ... });This will fail with "relation 'users' does not exist" when the Priority 2 DB-seeding path runs in CI.
Proposed fix
const rows = await sql` SELECT id, email, name, role, first_name AS "firstName", last_name AS "lastName" - FROM users + FROM "user" WHERE email = ${process.env.TEST_EMAIL.toLowerCase()} LIMIT 1 `;Note: Quote
"user"since it's a reserved word in PostgreSQL.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/expo/playwright/tests/globalSetup.ts` around lines 54 - 59, The SQL query uses the wrong table name "users"; change the FROM clause in the sql call that selects id,email,name,role,... to reference the auth table name "user" (quoted as "user" because user is a reserved PostgreSQL word) so the SELECT executes against the defined pgTable; keep the rest of the query intact (including the WHERE using process.env.TEST_EMAIL.toLowerCase()) and only update the table identifier.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/expo/playwright/tests/globalSetup.ts`:
- Around line 54-59: The SQL query uses the wrong table name "users"; change the
FROM clause in the sql call that selects id,email,name,role,... to reference the
auth table name "user" (quoted as "user" because user is a reserved PostgreSQL
word) so the SELECT executes against the defined pgTable; keep the rest of the
query intact (including the WHERE using process.env.TEST_EMAIL.toLowerCase())
and only update the table identifier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d1e26a84-6cef-474e-9d25-44c746817b7a
📒 Files selected for processing (5)
apps/expo/features/trips/store/trips.tsapps/expo/features/trips/utils/getTripDetailOptions.tsxapps/expo/playwright/tests/core.spec.tsapps/expo/playwright/tests/globalSetup.tsapps/expo/providers/index.web.tsx
💤 Files with no reviewable changes (1)
- apps/expo/features/trips/store/trips.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
passwordResetService.test.ts: Mock fortimingSafeEqualused positional args(a, b)but the real function takes{ a, b }— the mock always returnedfalse, making all OTP-comparison-dependent tests unreachableembeddingHelper.test.ts: 7 fallback tests calledgetEmbeddingText(item, existingItem)as positional args instead of the{ item, existingItem }object form, causingTypeError: Cannot read properties of undefined (reading 'name')getRelativeTime.test.ts: 3 i18n tests calledgetRelativeTime(date, t)positionally instead of{ dateValue, t }, so the translate spy was never invokedglobalSetup.ts(Playwright): Login endpoint/api/auth/loginwas removed in the Better Auth migration — replaced with/api/auth/sign-in/emailand updated response destructuring to readsession.tokenas the access tokenTest plan
bun test:api:unit— 299 tests pass (was: 12 failures in passwordResetService + embeddingHelper)bun test(Expo) — 357 tests pass (was: 1 failure in getRelativeTime)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes / UX
Tests
Chores