🐛 fix(web-e2e): close the last 9 gaps — playwright suite 36/36 on web#2537
Open
andrew-bierman wants to merge 101 commits into
Open
🐛 fix(web-e2e): close the last 9 gaps — playwright suite 36/36 on web#2537andrew-bierman wants to merge 101 commits into
andrew-bierman wants to merge 101 commits into
Conversation
Platform shims (lib/*.ts + lib/*.web.ts): - lib/Picker: web <select> replacing @react-native-picker/picker Metro stub - lib/appleAuthentication: no-op shim (unavailable on web) - lib/updates: window.location.reload() shim replacing expo-updates Metro stub - lib/devClient: empty shim (expo-dev-client not needed on web) - lib/constants.web.ts, lib/utils/ImageCacheManager.web.ts, lib/hooks/useColorScheme.web.tsx Web layout adapters: - app/_layout.web.tsx: web root layout - app/(app)/(tabs)/_layout.web.tsx: tab layout without NativeTabs Store/atom web shims: atomWithSecureStorage.web.ts, uploadImage.web.ts, localModelManager.web.ts Metro stubs cleaned up (removed expo-updates, expo-dev-client, apple-auth, picker — now use lib/ abstractions) Playwright e2e suite: playwright/ with auth, packs, trips, profile flows Bug fixes: - OTP: orderBy desc(expiresAt) to fetch newest code first - trips/index.ts: suppress deleted-trip 404s - one-time-password.tsx: guard setNativeProps on web - profile/index.tsx: expo-updates → lib/updates; DataItem testID field
NativeTabs has built-in web support (NativeTabsView.web.js), so _layout.web.tsx and (tabs)/_layout.web.tsx were not needed. providers/index.web.tsx was also unnecessary — bottom-sheet v5 and action-sheet work on web without a separate provider file. atomWithSecureStorage.web.ts was unused (no callers).
Replace globalSetup direct API fetch with a browser-based login flow matching the Maestro pattern — credentials entered through the UI, storageState saved for test reuse. Removes API_URL dependency from CI entirely. Also suppress empty-string text node false positives in RNW dev mode.
…entry screen button The entry screen's sign-in-email-button is a Link+Button (Slot) combo that can be slow to settle in the CI static export. Navigating directly to the login modal route opens the form immediately with all testIDs visible.
Switch goto to waitUntil:'load' to avoid networkidle hanging on API polling. Log page URL, all data-testid elements, and take a screenshot so the CI artifact reveals what actually renders in the static export.
Two Zod validation errors prevented the app from rendering at all: 1. EXPO_PUBLIC_GOOGLE_IOS_CLIENT_ID was required but never provided to the web build — it's iOS-only (guarded by Platform.OS === 'ios'). Made optional in the schema. 2. EXPO_PUBLIC_R2_PUBLIC_URL secret was set without an https:// prefix. Added a normalization step in CI that prepends https:// if missing. Also threads EXPO_PUBLIC_GOOGLE_IOS_CLIENT_ID through the build step with a placeholder fallback so native schema parity is maintained.
…h diagnosis Log API responses matching /auth|/login|/session paths, take a screenshot 5s after the continue-button click, and print the post-login URL so CI artifacts reveal whether the login API call is succeeding.
…split The old secret was last set in Sep 2024 and returns 404 for auth routes. Replace with EXPO_PUBLIC_API_URL_DEV (development/PRs) and EXPO_PUBLIC_API_URL_PROD (main/PRs to main). Set via: gh secret set EXPO_PUBLIC_API_URL_DEV --repo PackRat-AI/PackRat gh secret set EXPO_PUBLIC_API_URL_PROD --repo PackRat-AI/PackRat
…ion) Simpler single secret matching the existing E2E_TEST_EMAIL/PASSWORD pattern. Set via: gh secret set E2E_EXPO_PUBLIC_API_URL --repo PackRat-AI/PackRat
NativeWind h-8/w-8 className wasn't applying to the Image component on web, causing packrat-app-icon-gradient.png to render at its natural 1080x1080px and cover the entire auth screen. Add explicit style prop as a reliable fallback.
Network listeners, screenshot, waitForTimeout, and REPORT_DIR were temporary diagnostics used during CI auth debugging. Login flow is stable now — clean them up.
NativeWind className props (h-X, w-X, h-full, w-full) don't reliably apply to RN Image on web because <img> elements size to their intrinsic dimensions and ignore flex/CSS sizing. Apply explicit style only on web via Platform.select so native continues to use NativeWind classes. Affects: auth screens, pack cards, detail screens, template cards, user avatars, item cards and their wrapper components.
- Patch BackHandler in polyfills.js to a silent no-op on web; RNW's stub logs an error on every addEventListener call (expo-router uses it internally on every screen mount). Fixing the root cause also eliminates the downstream text-node console errors, which came from the error toast that rendered in response to BackHandler calls. - Update globalSetup to flow through the auth UI naturally: navigate to /auth, click the Sign In button, fill email/password, submit. Removes the previous direct-URL shortcut to /auth/(login). - Fix Image sizing on login modal (auth/(login)/index.tsx) — same Platform.select web style fix applied to all other Image components.
FlashList and nativewindui produce transient empty-string children during reconciliation in RNW dev mode. Suppress just those console.error calls in polyfills.ts (web + dev only), next to the BackHandler no-op patch. Remove the now-orphaned comment from _layout.tsx.
- Add testIDs for trip date buttons/inputs to testIds.ts and TripForm.tsx - Add testID prop to DateTimePicker web mock (data-testid on the input) - Use native HTMLInputElement value setter in tests — Playwright's fill() doesn't reliably trigger React's onChange on input[type="date"] - Patch nativewindui LargeTitleHeader to forward searchBar.testID to the search Button so catalog search is testable by testID - Add testID to CatalogItemsScreen searchBar prop - Fix strict mode violations: scope pack name assertions to visible elements via :text():visible (Expo Router keeps inactive tabs in DOM) - Fix delete item selector: use exact string 'Delete' so item names containing the word don't match the menu option - Fix validation test: skip submit click when button is aria-disabled (onChange validation already shows the error)
- PackItemCard: replace Alert.alert (no-op on web) with NativeWindUI Alert so item delete confirmation renders as a DOM modal on web - packs.spec.ts: after clicking "Delete" in action sheet, click "OK" in the NativeWindUI Alert modal to confirm deletion - core.spec.ts: open the AddPackItemActions sheet via add-item-button before clicking add-from-catalog-option (BottomSheetModal content is not in DOM until presented); add networkidle waits to avoid router.back() race condition from form submission
- DateTimePicker mock: switch from uncontrolled (defaultValue) to
controlled (value + useState) so Playwright's native-setter +
event dispatch reliably fires React's onChange
- fillDateInput: dispatch both 'input' and 'change' events to cover
all React version behaviors (core.spec.ts + trips.spec.ts)
- HorizontalCatalogItemCard: add testID so getByTestId(/^catalog-item-card-/)
finds items in CatalogBrowserModal
- CatalogItemsScreen: add testID to total-items-count Text element,
avoiding hidden-tab-panel false matches on the regex locator
- testIds.ts: add catalog.totalItemsCount constant
- core.spec.ts catalog tests: use testID for items count; replace
waitForLoadState('networkidle') with targeted element wait to prevent
"page closed" on live catalog feeds
- packs.spec.ts delete item: retry Delete click up to 5× to defeat
action sheet entrance animation (225ms) that silently drops clicks
while isAnimating=true
testIds.packs.nameInput was renamed from 'packs:name-input' to 'pack-name-input'
in development branch (Maestro compatibility). Update the 3 test references.
Also remove redundant waitForLoadState('networkidle') before catalog browser modal
(continuous sync prevents networkidle; direct goto is sufficient), add 60s timeout
to catalog test, and extend catalog item card wait to 25s for CI network latency.
…i type testID is in local src but absent from the installed package version CI uses. Replace catalog:search-btn wait with catalog:total-items-count in E2E test.
…ativewindui 2.0.6 testID exists in the 2.0.5 runtime but is absent from the published type definition. Cast avoids the excess-property-check until nativewindui PR #14 is merged and bumped.
…h nativewindui testID - Move waitForResponse registration to just before submit in createTrip helper and core.spec create-trip test; the 20s timer was starting before navigation+form-fill which routinely exceeded the budget on slow CI runners - Add test.setTimeout(60_000) to create-pack tests that had none or only 30s; the default 30s was too tight for navigation+fill+POST on CI - Add patches/@PackRat-AI+nativewindui@2.0.5.patch to patchedDependencies so CI installs get testID={props.searchBar.testID} forwarded to the Button; the published 2.0.5 lacks this line (it lands in 2.0.6 via nativewindui PR #14)
… retry loop
trips.ts: remove `if (!tripData.location) throw` guard that prevented syncedCrud
from ever POSTing trips created without a location (all E2E test trips). The API
accepts `location: null` via LocationSchema.nullable().optional().
core.spec.ts: replace `getByRole('textbox', { name: /Pack Name/i })` with
`getByTestId('pack-name-input')` in three tests — the role selector fails on CI.
packs.spec.ts: replace 5-attempt retry loop in delete-item test with a single
`waitForTimeout(350)` (outlasts 225ms entrance animation) + one click. The old
loop would hang 30s per iteration when the action sheet closed after the first
successful click and subsequent iterations waited for the gone 'Delete' element.
… tests
- Patch Alert.tsx to bypass broken useAugmentedRef (returns {} on mount);
replace with direct useImperativeHandle so alertRef.current.alert() works
on web — fixes delete-item and delete-trip E2E tests
- Add testID to Dates section in TripDetailScreen; update trips.spec.ts to
use getByTestId instead of getByText('Dates') which matched hidden tab panels
…ey fix) The previous workflow pointed playwright at the deployed API ($EXPO_PUBLIC_API_URL) but this branch's e2e changes assume a fully local stack — Docker Postgres + Neon HTTP proxy + wrangler dev — with localhost-trusted origins, ENVIRONMENT=development gating, etc. Adopt the workflow from PR #2541 (fix/web-e2e-local-api), which: - boots a local Neon proxy + Postgres via docker compose - writes a CI-only .dev.vars with valid placeholder schema entries - generates a no-AI-binding wrangler.e2e.json so wrangler dev doesn't need a CF login for the binding - runs wrangler dev locally, serves the exported web SPA on :8081 - drives playwright against that fully-local stack Re-apply the cache-key fix from c760b79 on top: hash workspace package.json files too and drop the wildcard restore-keys so the @babel/helper-compilation-targets nested lru-cache@5 doesn't disappear between bun.lock-equivalent updates. This file overlaps with PR #2541; the divergence is just the cache key hardening. Whichever lands second can keep that delta in a follow-up.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/expo/lib/api/packrat.ts (2)
39-46:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle failed
getSession()probes before changing auth state.Both new
authClient.getSession()calls assume success. If the auth lookup rejects or returns{ error, data: null },getAccessTokensilently drops the token andonNeedsReauthstill flipsneedsReauthAtom, logging the user out on a transient auth/network failure. Add breadcrumbs, wrap each call intry/catch, and only force reauth after a successful probe confirms the session is gone.As per coding guidelines, "All new code performing async operations or calling external services must include Sentry instrumentation with breadcrumbs before significant operations and captureException in every catch block" and "API client responses must be checked with
if (error || !data)before using the data object".Also applies to: 54-60
🤖 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.ts` around lines 39 - 46, The getAccessToken path currently assumes authClient.getSession() succeeds and drops the token on any failure; wrap each authClient.getSession() call in try/catch, add Sentry.addBreadcrumb before calling it and Sentry.captureException in the catch, and only clear cachedToken / set needsReauthAtom (or call onNeedsReauth) after a successful probe that returns no session (i.e. check if (error || !data || !data.session) before using data.session.token); update references to cachedToken and cachedTokenExpiresAt accordingly and ensure transient network errors do not trigger onNeedsReauth.
20-22:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInvalidate the web token cache on normal auth transitions.
Line 42 can keep returning a previously cached bearer token for up to 30 seconds, but Lines 55-56 only clear that cache from
onNeedsReauth. A normal sign-out / sign-in or account switch will not hit that path, so the next web requests can still carry the previous session's token. Clear this cache from the explicit auth success/sign-out flow too.Also applies to: 40-46, 55-56
🤖 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.ts` around lines 20 - 22, The token cache (WEB_TOKEN_CACHE_MS, cachedToken, cachedTokenExpiresAt) must be cleared on normal auth transitions as well as onNeedsReauth; update the explicit auth success/sign-out/account-switch handlers to set cachedToken = null and cachedTokenExpiresAt = 0 so getWebToken() cannot return a stale bearer token for up to 30s. Locate the auth transition handlers (e.g., the signIn/signOut/account switch or onAuthChange functions and the existing onNeedsReauth path) and add cache invalidation there using those exact symbols.apps/expo/app/(app)/ai-chat.tsx (1)
199-211:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix incorrect cookie-auth fallback + add required auth/Sentry handling in
headers()
headers()may return{}butDefaultChatTransport’s HTTP requests default toRequest.credentials: 'same-origin', so cross-origin${clientEnvs.EXPO_PUBLIC_API_URL}won’t receive session cookies; the comment claiming the browser still carries cookies viacredentials:'include'is incorrect—setcredentials: 'include'onDefaultChatTransport(or adjust fetch) or remove reliance on cookie auth.- The async
headers()callsauthClient.getSession()without Sentry breadcrumbs/captureException and without checking{ error, data }(should handleif (error || !data)), so failures could silently produce{}—wrap with try/catch, breadcrumb,Sentry.captureException, and only read token when{ data }is valid.🤖 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/app/`(app)/ai-chat.tsx around lines 199 - 211, Update the async headers() implementation used by DefaultChatTransport so it doesn't rely on cookies cross-origin and adds Sentry/error handling: change transport/fetch to include credentials:'include' (or otherwise ensure DefaultChatTransport uses credentials:'include' when calling expoFetch) rather than assuming cookies are sent, and wrap the authClient.getSession() call in try/catch so you check for returned { error, data } before reading data.session.token; on error or exception add a Sentry breadcrumb and call Sentry.captureException, then return either { Authorization: `Bearer ${token}` } when a valid token exists or {} otherwise. Ensure you update the transport/fetch configuration (DefaultChatTransport or where fetch is passed) and the headers() function accordingly..github/workflows/web-e2e-tests.yml (1)
43-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun the new catalog seed in CI.
This workflow still seeds only the test user.
packages/api/src/db/seed-e2e-catalog.tsnever runs here, so the catalog/search Playwright cases still depend on preexisting rows in the dev DB and can fail on a fresh or cleaned database. Add the catalog seed before Playwright, and gate the job onOPENAI_API_KEYtoo.💡 Proposed fix
- id: check name: Verify E2E secrets are available env: E2E_TEST_EMAIL: ${{ secrets.E2E_TEST_EMAIL }} NEON_DATABASE_URL: ${{ secrets.NEON_DEV_DATABASE_URL }} + OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} run: | - if [ -n "$E2E_TEST_EMAIL" ] && [ -n "$NEON_DATABASE_URL" ]; then + if [ -n "$E2E_TEST_EMAIL" ] && [ -n "$NEON_DATABASE_URL" ] && [ -n "$OPENAI_API_KEY" ]; then echo "ready=true" >> "$GITHUB_OUTPUT" else echo "ready=false" >> "$GITHUB_OUTPUT" echo "::notice::E2E secrets not configured — skipping Web E2E tests" fi @@ - name: Seed E2E test user in dev DB run: bun run --filter `@packrat/api` db:seed:e2e-user env: NEON_DATABASE_URL: ${{ secrets.NEON_DEV_DATABASE_URL }} E2E_TEST_EMAIL: ${{ env.TEST_EMAIL }} E2E_TEST_PASSWORD: ${{ secrets.E2E_TEST_PASSWORD }} + + - name: Seed E2E catalog in dev DB + run: bun run packages/api/src/db/seed-e2e-catalog.ts + env: + NEON_DATABASE_URL: ${{ secrets.NEON_DEV_DATABASE_URL }} + OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}Also applies to: 110-116
🤖 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 @.github/workflows/web-e2e-tests.yml around lines 43 - 47, Add execution of the catalog seed script and require OPENAI_API_KEY in the secrets-check step: in the "Verify E2E secrets are available" step (and the similar block at lines ~110-116), add OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }} to the env mapping and insert a run step before Playwright that executes packages/api/src/db/seed-e2e-catalog.ts (run it with your repo's Node/ts-node/pnpm command so it seeds the catalog/search rows). Ensure the seed step runs after secrets are set and before the Playwright test step so the catalog data exists for the E2E tests.
🤖 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/app/`(app)/ai-chat.tsx:
- Around line 208-211: The headers async block that calls
authClient.getSession() should be wrapped in try/catch inside the headers
function (the async headers: () => { ... }) to detect failures and avoid
silently falling back to unauthenticated requests; before calling
authClient.getSession() add a Sentry.addBreadcrumb describing
"authClient.getSession start", and in the catch block call
Sentry.captureException(err) and return an empty header object or rethrow as
appropriate; also verify the response by checking for error || !data (or
data?.session) after the call and treat that as an error path that gets captured
with Sentry before returning the fallback headers. Ensure you reference the
headers async function and authClient.getSession when making these changes.
In `@apps/expo/features/trips/hooks/useDeleteTrip.ts`:
- Around line 26-28: The current code in the useDeleteTrip hook throws a new
Error(`Trip delete failed (${response.status})`) which discards the original
response.error and its stack; instead, when response.error exists rethrow the
original error object (throw response.error) so the catch block and
Sentry.captureException receive the original error and stack; if response.error
is falsy but you still need to signal failure, throw a new Error that includes
status and any relevant response info—ensure the catch block calls
Sentry.captureException on the original error object (or the new Error only when
no original error exists).
---
Outside diff comments:
In @.github/workflows/web-e2e-tests.yml:
- Around line 43-47: Add execution of the catalog seed script and require
OPENAI_API_KEY in the secrets-check step: in the "Verify E2E secrets are
available" step (and the similar block at lines ~110-116), add OPENAI_API_KEY:
${{ secrets.OPENAI_API_KEY }} to the env mapping and insert a run step before
Playwright that executes packages/api/src/db/seed-e2e-catalog.ts (run it with
your repo's Node/ts-node/pnpm command so it seeds the catalog/search rows).
Ensure the seed step runs after secrets are set and before the Playwright test
step so the catalog data exists for the E2E tests.
In `@apps/expo/app/`(app)/ai-chat.tsx:
- Around line 199-211: Update the async headers() implementation used by
DefaultChatTransport so it doesn't rely on cookies cross-origin and adds
Sentry/error handling: change transport/fetch to include credentials:'include'
(or otherwise ensure DefaultChatTransport uses credentials:'include' when
calling expoFetch) rather than assuming cookies are sent, and wrap the
authClient.getSession() call in try/catch so you check for returned { error,
data } before reading data.session.token; on error or exception add a Sentry
breadcrumb and call Sentry.captureException, then return either { Authorization:
`Bearer ${token}` } when a valid token exists or {} otherwise. Ensure you update
the transport/fetch configuration (DefaultChatTransport or where fetch is
passed) and the headers() function accordingly.
In `@apps/expo/lib/api/packrat.ts`:
- Around line 39-46: The getAccessToken path currently assumes
authClient.getSession() succeeds and drops the token on any failure; wrap each
authClient.getSession() call in try/catch, add Sentry.addBreadcrumb before
calling it and Sentry.captureException in the catch, and only clear cachedToken
/ set needsReauthAtom (or call onNeedsReauth) after a successful probe that
returns no session (i.e. check if (error || !data || !data.session) before using
data.session.token); update references to cachedToken and cachedTokenExpiresAt
accordingly and ensure transient network errors do not trigger onNeedsReauth.
- Around line 20-22: The token cache (WEB_TOKEN_CACHE_MS, cachedToken,
cachedTokenExpiresAt) must be cleared on normal auth transitions as well as
onNeedsReauth; update the explicit auth success/sign-out/account-switch handlers
to set cachedToken = null and cachedTokenExpiresAt = 0 so getWebToken() cannot
return a stale bearer token for up to 30s. Locate the auth transition handlers
(e.g., the signIn/signOut/account switch or onAuthChange functions and the
existing onNeedsReauth path) and add cache invalidation there using those exact
symbols.
🪄 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: 1f4e1ca9-dedc-4ae3-945b-a8189e09d250
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock,!bun.lock
📒 Files selected for processing (10)
.github/workflows/web-e2e-tests.ymlapps/expo/app/(app)/ai-chat.tsxapps/expo/features/trips/hooks/useDeleteTrip.tsapps/expo/features/trips/screens/TripDetailScreen.tsxapps/expo/lib/api/packrat.tsapps/expo/lib/auth-client.tsapps/expo/lib/i18n/locales/en.jsonpackage.jsonpackages/api/src/db/seed-e2e-catalog.tspackages/api/src/index.ts
Comment on lines
+208
to
+211
| headers: async (): Promise<Record<string, string>> => { | ||
| const { data } = await authClient.getSession(); | ||
| const token = data?.session?.token; | ||
| return token ? { Authorization: `Bearer ${token}` } : {}; |
Contributor
There was a problem hiding this comment.
Capture auth lookup failures before sending chat requests.
This new async session lookup is on the request path, but there is no breadcrumb, captureException, or explicit error || !data handling if authClient.getSession() fails. A transient auth failure silently becomes an unauthenticated send. Wrap this in try/catch and capture before falling back.
As per coding guidelines, "All new code performing async operations or calling external services must include Sentry instrumentation with breadcrumbs before significant operations and captureException in every catch block" and "API client responses must be checked with if (error || !data) before using the data object".
🤖 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/app/`(app)/ai-chat.tsx around lines 208 - 211, The headers async
block that calls authClient.getSession() should be wrapped in try/catch inside
the headers function (the async headers: () => { ... }) to detect failures and
avoid silently falling back to unauthenticated requests; before calling
authClient.getSession() add a Sentry.addBreadcrumb describing
"authClient.getSession start", and in the catch block call
Sentry.captureException(err) and return an empty header object or rethrow as
appropriate; also verify the response by checking for error || !data (or
data?.session) after the call and treat that as an error path that gets captured
with Sentry before returning the fallback headers. Ensure you reference the
headers async function and authClient.getSession when making these changes.
Comment on lines
+26
to
+28
| if (response.error && response.status !== 404) { | ||
| throw new Error(`Trip delete failed (${response.status})`); | ||
| } |
Contributor
There was a problem hiding this comment.
Don't wrap the API error in a new Error before capturing to Sentry.
Line 27 throws new Error(...) with only the status code, discarding the original response.error object. The catch block then sends this wrapper to Sentry, losing the original stack trace and error details.
As per coding guidelines: "Never wrap the original error in new Error(...) before passing to Sentry's captureException — this loses the original stack and context."
🛡️ Proposed fix
try {
const response = await apiClient.trips({ tripId: id }).delete();
if (response.error && response.status !== 404) {
- throw new Error(`Trip delete failed (${response.status})`);
+ throw response.error;
}
} catch (error) {📝 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
| if (response.error && response.status !== 404) { | |
| throw new Error(`Trip delete failed (${response.status})`); | |
| } | |
| if (response.error && response.status !== 404) { | |
| throw response.error; | |
| } |
🤖 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/features/trips/hooks/useDeleteTrip.ts` around lines 26 - 28, The
current code in the useDeleteTrip hook throws a new Error(`Trip delete failed
(${response.status})`) which discards the original response.error and its stack;
instead, when response.error exists rethrow the original error object (throw
response.error) so the catch block and Sentry.captureException receive the
original error and stack; if response.error is falsy but you still need to
signal failure, throw a new Error that includes status and any relevant response
info—ensure the catch block calls Sentry.captureException on the original error
object (or the new Error only when no original error exists).
# Conflicts: # apps/expo/features/trips/hooks/useDeleteTrip.ts # apps/expo/features/trips/screens/TripDetailScreen.tsx
Contributor
Coverage Report for packages/overpass (./packages/overpass)
File CoverageNo changed files found. |
Contributor
Coverage Report for packages/units (./packages/units)
File CoverageNo changed files found. |
Contributor
Coverage Report for packages/mcp (./packages/mcp)
File CoverageNo changed files found. |
Contributor
Coverage Report for packages/analytics (./packages/analytics)
File CoverageNo changed files found. |
Contributor
Coverage Report for apps/expo (./apps/expo)
File CoverageNo changed files found. |
Contributor
Coverage Report for packages/api (./packages/api)
File Coverage
|
||||||||||||||||||||||||||||||||||||||
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/web-e2e-tests.yml (1)
80-80:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPin all third-party actions to commit SHAs.
Per coding guidelines, third-party actions must be pinned to full commit SHAs to prevent supply-chain attacks. Mutable tags like
@v6can be moved by attackers.🔒 Proposed fix: pin to commit SHAs
First, look up the current commit SHA for each action version, then apply:
- - uses: actions/checkout@v6 + - uses: actions/checkout@<SHA> # v6.x.x- - uses: oven-sh/setup-bun@v2 + - uses: oven-sh/setup-bun@<SHA> # v2.x.x- - uses: actions/cache@v4 + - uses: actions/cache@<SHA> # v4.x.x- - uses: actions/upload-artifact@v7 + - uses: actions/upload-artifact@<SHA> # v7.x.xAlso applies to: 83-83, 89-89, 247-247, 255-255
🤖 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 @.github/workflows/web-e2e-tests.yml at line 80, Replace mutable action tags with immutable commit SHAs for all third-party actions referenced by "uses:" (e.g., the actions/checkout@v6 entry and the other entries flagged at lines 83, 89, 247, 255); find each "uses: <owner>/<repo>@<tag>" occurrence in the workflow, look up the corresponding full commit SHA for the specified tag in the upstream repository, and update the YAML to use the pinned form "uses: <owner>/<repo>@<full-commit-sha>" for each action.Source: Coding guidelines
🤖 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 @.github/workflows/web-e2e-tests.yml:
- Line 209: The failing branch echoes an error and cats /tmp/wrangler.log but
will itself fail if the log file doesn't exist; update the error handling for
the command string `echo "::error::API did not become ready — wrangler log:";
cat /tmp/wrangler.log; exit 1` to defensively handle a missing file (for
example, use `cat /tmp/wrangler.log || true` or a test like `[ -f
/tmp/wrangler.log ] && cat /tmp/wrangler.log`) so the error message prints
without causing a secondary failure.
- Line 214: Replace the hardcoded port in the EXPO_PUBLIC_API_URL environment
entry so it uses the workflow environment variable instead of "8787"; update the
EXPO_PUBLIC_API_URL value to reference ${{ env.API_PORT }} (the existing
API_PORT env var) wherever "8787" is currently hardcoded (notably the
EXPO_PUBLIC_API_URL entries).
In `@apps/expo/app/`(app)/ai-chat.tsx:
- Around line 157-160: The current useEffect launches
releaseLocalModel().then(() => initLocalModel(isAuthenticated)) without
sequencing, cancellation or error handling; introduce a sequence guard token
(e.g., localSequence or abortId) scoped inside the effect and only run
initLocalModel(isAuthenticated) if the token matches the latest value to prevent
stale init from older promises, await releaseLocalModel() in a try/catch, add
Sentry.addBreadcrumb before calling releaseLocalModel and before initLocalModel,
and in each catch call Sentry.captureException(err) and handle failures
gracefully (no unhandled promise). Ensure references to releaseLocalModel,
initLocalModel, featureFlags.enableLocalAI, isAppleIntelligenceAvailable, and
isAuthenticated are used so the guard and instrumentation wrap this specific
chain.
In `@packages/api/src/index.ts`:
- Around line 216-237: Wrap the call to sweepInvalidItemLogs inside a
captureApiException block within the scheduled(controller: ScheduledController,
env: Env) handler: add a Sentry breadcrumb before invoking sweepInvalidItemLogs,
call captureApiException around the awaited sweepInvalidItemLogs({ env })
invocation, and on error capture include structured tags (e.g., cron pattern
from controller.cron) plus contextual fields (partial result if any, iterations,
capped, retentionDays) before rethrowing or returning; update logging to record
the same contextual fields on success so observability is consistent.
---
Outside diff comments:
In @.github/workflows/web-e2e-tests.yml:
- Line 80: Replace mutable action tags with immutable commit SHAs for all
third-party actions referenced by "uses:" (e.g., the actions/checkout@v6 entry
and the other entries flagged at lines 83, 89, 247, 255); find each "uses:
<owner>/<repo>@<tag>" occurrence in the workflow, look up the corresponding full
commit SHA for the specified tag in the upstream repository, and update the YAML
to use the pinned form "uses: <owner>/<repo>@<full-commit-sha>" for each action.
🪄 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: 07585b49-8eb3-4aa3-a627-c34d2fe10d86
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock,!bun.lock
📒 Files selected for processing (7)
.github/workflows/web-e2e-tests.ymlapps/expo/app/(app)/ai-chat.tsxapps/expo/app/(app)/demo/index.tsxapps/expo/features/trips/screens/TripDetailScreen.tsxpackage.jsonpackages/api/src/index.tspackages/api/src/utils/env-validation.ts
💤 Files with no reviewable changes (1)
- apps/expo/app/(app)/demo/index.tsx
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/web-e2e-tests.yml (1)
80-80:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPin all third-party actions to commit SHAs.
Per coding guidelines, third-party actions must be pinned to full commit SHAs to prevent supply-chain attacks. Mutable tags like
@v6can be moved by attackers.🔒 Proposed fix: pin to commit SHAs
First, look up the current commit SHA for each action version, then apply:
- - uses: actions/checkout@v6 + - uses: actions/checkout@<SHA> # v6.x.x- - uses: oven-sh/setup-bun@v2 + - uses: oven-sh/setup-bun@<SHA> # v2.x.x- - uses: actions/cache@v4 + - uses: actions/cache@<SHA> # v4.x.x- - uses: actions/upload-artifact@v7 + - uses: actions/upload-artifact@<SHA> # v7.x.xAlso applies to: 83-83, 89-89, 247-247, 255-255
🤖 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 @.github/workflows/web-e2e-tests.yml at line 80, Replace mutable action tags with immutable commit SHAs for all third-party actions referenced by "uses:" (e.g., the actions/checkout@v6 entry and the other entries flagged at lines 83, 89, 247, 255); find each "uses: <owner>/<repo>@<tag>" occurrence in the workflow, look up the corresponding full commit SHA for the specified tag in the upstream repository, and update the YAML to use the pinned form "uses: <owner>/<repo>@<full-commit-sha>" for each action.Source: Coding guidelines
🤖 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 @.github/workflows/web-e2e-tests.yml:
- Line 209: The failing branch echoes an error and cats /tmp/wrangler.log but
will itself fail if the log file doesn't exist; update the error handling for
the command string `echo "::error::API did not become ready — wrangler log:";
cat /tmp/wrangler.log; exit 1` to defensively handle a missing file (for
example, use `cat /tmp/wrangler.log || true` or a test like `[ -f
/tmp/wrangler.log ] && cat /tmp/wrangler.log`) so the error message prints
without causing a secondary failure.
- Line 214: Replace the hardcoded port in the EXPO_PUBLIC_API_URL environment
entry so it uses the workflow environment variable instead of "8787"; update the
EXPO_PUBLIC_API_URL value to reference ${{ env.API_PORT }} (the existing
API_PORT env var) wherever "8787" is currently hardcoded (notably the
EXPO_PUBLIC_API_URL entries).
In `@apps/expo/app/`(app)/ai-chat.tsx:
- Around line 157-160: The current useEffect launches
releaseLocalModel().then(() => initLocalModel(isAuthenticated)) without
sequencing, cancellation or error handling; introduce a sequence guard token
(e.g., localSequence or abortId) scoped inside the effect and only run
initLocalModel(isAuthenticated) if the token matches the latest value to prevent
stale init from older promises, await releaseLocalModel() in a try/catch, add
Sentry.addBreadcrumb before calling releaseLocalModel and before initLocalModel,
and in each catch call Sentry.captureException(err) and handle failures
gracefully (no unhandled promise). Ensure references to releaseLocalModel,
initLocalModel, featureFlags.enableLocalAI, isAppleIntelligenceAvailable, and
isAuthenticated are used so the guard and instrumentation wrap this specific
chain.
In `@packages/api/src/index.ts`:
- Around line 216-237: Wrap the call to sweepInvalidItemLogs inside a
captureApiException block within the scheduled(controller: ScheduledController,
env: Env) handler: add a Sentry breadcrumb before invoking sweepInvalidItemLogs,
call captureApiException around the awaited sweepInvalidItemLogs({ env })
invocation, and on error capture include structured tags (e.g., cron pattern
from controller.cron) plus contextual fields (partial result if any, iterations,
capped, retentionDays) before rethrowing or returning; update logging to record
the same contextual fields on success so observability is consistent.
---
Outside diff comments:
In @.github/workflows/web-e2e-tests.yml:
- Line 80: Replace mutable action tags with immutable commit SHAs for all
third-party actions referenced by "uses:" (e.g., the actions/checkout@v6 entry
and the other entries flagged at lines 83, 89, 247, 255); find each "uses:
<owner>/<repo>@<tag>" occurrence in the workflow, look up the corresponding full
commit SHA for the specified tag in the upstream repository, and update the YAML
to use the pinned form "uses: <owner>/<repo>@<full-commit-sha>" for each action.
🪄 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: 07585b49-8eb3-4aa3-a627-c34d2fe10d86
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock,!bun.lock
📒 Files selected for processing (7)
.github/workflows/web-e2e-tests.ymlapps/expo/app/(app)/ai-chat.tsxapps/expo/app/(app)/demo/index.tsxapps/expo/features/trips/screens/TripDetailScreen.tsxpackage.jsonpackages/api/src/index.tspackages/api/src/utils/env-validation.ts
💤 Files with no reviewable changes (1)
- apps/expo/app/(app)/demo/index.tsx
🛑 Comments failed to post (4)
.github/workflows/web-e2e-tests.yml (2)
209-209: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add defensive handling for missing wrangler log.
If
wrangler devfails before creating/tmp/wrangler.log, line 209'scatwill fail with a confusing error. Line 243 already handles this with|| true.🛡️ Proposed fix
- echo "::error::API did not become ready — wrangler log:"; cat /tmp/wrangler.log; exit 1 + echo "::error::API did not become ready — wrangler log:"; cat /tmp/wrangler.log || echo "(log file not found)"; exit 1📝 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.echo "::error::API did not become ready — wrangler log:"; cat /tmp/wrangler.log || echo "(log file not found)"; exit 1🤖 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 @.github/workflows/web-e2e-tests.yml at line 209, The failing branch echoes an error and cats /tmp/wrangler.log but will itself fail if the log file doesn't exist; update the error handling for the command string `echo "::error::API did not become ready — wrangler log:"; cat /tmp/wrangler.log; exit 1` to defensively handle a missing file (for example, use `cat /tmp/wrangler.log || true` or a test like `[ -f /tmp/wrangler.log ] && cat /tmp/wrangler.log`) so the error message prints without causing a secondary failure.
214-214: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Use
${{ env.API_PORT }}instead of hardcoding the port.Lines 214 and 237 hardcode
8787, but the workflow definesAPI_PORT: "8787"at line 76 and uses${API_PORT}elsewhere (lines 145, 175, 200). Use the env var for consistency.♻️ Proposed fix
env: - EXPO_PUBLIC_API_URL: http://localhost:8787 + EXPO_PUBLIC_API_URL: http://localhost:${{ env.API_PORT }}env: BASE_URL: http://localhost:8081 - API_URL: http://localhost:8787 + API_URL: http://localhost:${{ env.API_PORT }}Also applies to: 237-237
🤖 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 @.github/workflows/web-e2e-tests.yml at line 214, Replace the hardcoded port in the EXPO_PUBLIC_API_URL environment entry so it uses the workflow environment variable instead of "8787"; update the EXPO_PUBLIC_API_URL value to reference ${{ env.API_PORT }} (the existing API_PORT env var) wherever "8787" is currently hardcoded (notably the EXPO_PUBLIC_API_URL entries).apps/expo/app/(app)/ai-chat.tsx (1)
157-160:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winSerialize auth-driven local model reinitialization.
If
isAuthenticatedflips again before the firstreleaseLocalModel()settles, the older promise can finish last and callinitLocalModel()with stale auth state. That can leave the local provider/tooling out of sync with the current session, and any rejection here is currently unhandled. Use a cancellation/sequence guard around this chain and capture failures instead of firing it as an unchecked promise.As per coding guidelines, "All new code performing async operations or calling external services must include Sentry instrumentation with breadcrumbs before significant operations and captureException in every catch block".
🤖 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/app/`(app)/ai-chat.tsx around lines 157 - 160, The current useEffect launches releaseLocalModel().then(() => initLocalModel(isAuthenticated)) without sequencing, cancellation or error handling; introduce a sequence guard token (e.g., localSequence or abortId) scoped inside the effect and only run initLocalModel(isAuthenticated) if the token matches the latest value to prevent stale init from older promises, await releaseLocalModel() in a try/catch, add Sentry.addBreadcrumb before calling releaseLocalModel and before initLocalModel, and in each catch call Sentry.captureException(err) and handle failures gracefully (no unhandled promise). Ensure references to releaseLocalModel, initLocalModel, featureFlags.enableLocalAI, isAppleIntelligenceAvailable, and isAuthenticated are used so the guard and instrumentation wrap this specific chain.Source: Coding guidelines
packages/api/src/index.ts (1)
216-237:
⚠️ Potential issue | 🟠 Major | ⚖️ Poor tradeoffAdd explicit try/catch with captureApiException around sweepInvalidItemLogs.
The scheduled handler calls
sweepInvalidItemLogs(a database service method) without a try/catch block. Per coding guidelines, every service method interacting with the database must have acaptureApiExceptioncall. WhilewithSentrywill catch uncaught errors, explicit error handling would:
- Add a breadcrumb before the operation
- Capture operation-specific context (cron pattern, partial results)
- Include structured tags for observability
🛡️ Proposed fix to add Sentry instrumentation
async scheduled(controller: ScheduledController, env: Env): Promise<void> { setWorkerEnv(enrichEnv(env) as unknown as Record<string, unknown>); if (controller.cron === '0 9 * * *') { + try { const result = await sweepInvalidItemLogs({ env }); console.log( `[retention] invalid_item_logs sweep: deleted=${result.deleted} ` + `iterations=${result.iterations} capped=${result.capped} ` + `retentionDays=${result.retentionDays}`, ); if (result.capped) { console.warn( `[retention] invalid_item_logs sweep hit max-iterations cap; ` + `remaining expired rows will be swept on the next run`, ); } - return; + } catch (error) { + captureApiException({ + error, + operation: 'scheduled.sweepInvalidItemLogs', + tags: { cron: controller.cron }, + extra: { scheduledTime: controller.scheduledTime }, + }); + throw error; + } + return; } throw new Error(`Unknown cron: ${controller.cron}`); },📝 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.async scheduled(controller: ScheduledController, env: Env): Promise<void> { setWorkerEnv(enrichEnv(env) as unknown as Record<string, unknown>); if (controller.cron === '0 9 * * *') { try { const result = await sweepInvalidItemLogs({ env }); console.log( `[retention] invalid_item_logs sweep: deleted=${result.deleted} ` + `iterations=${result.iterations} capped=${result.capped} ` + `retentionDays=${result.retentionDays}`, ); if (result.capped) { console.warn( `[retention] invalid_item_logs sweep hit max-iterations cap; ` + `remaining expired rows will be swept on the next run`, ); } } catch (error) { captureApiException({ error, operation: 'scheduled.sweepInvalidItemLogs', tags: { cron: controller.cron }, extra: { scheduledTime: controller.scheduledTime }, }); throw error; } return; } throw new Error(`Unknown cron: ${controller.cron}`); },🤖 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 `@packages/api/src/index.ts` around lines 216 - 237, Wrap the call to sweepInvalidItemLogs inside a captureApiException block within the scheduled(controller: ScheduledController, env: Env) handler: add a Sentry breadcrumb before invoking sweepInvalidItemLogs, call captureApiException around the awaited sweepInvalidItemLogs({ env }) invocation, and on error capture include structured tags (e.g., cron pattern from controller.cron) plus contextual fields (partial result if any, iterations, capped, retentionDays) before rethrowing or returning; update logging to record the same contextual fields on success so observability is consistent.Source: Coding guidelines
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/web-e2e-tests.yml:
- Around line 124-132: The e2e catalog seed step fails because
db:seed:e2e-catalog calls the OpenAI embeddings API and seed-e2e-catalog.ts
(around lines 170-171) requires OPENAI_API_KEY, but the workflow step env only
sets NEON_DATABASE_URL, E2E_TEST_EMAIL, and E2E_TEST_PASSWORD; fix by either
adding OPENAI_API_KEY to that step's env from a repository secret (e.g., set
OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}) so db:seed:e2e-catalog can call
the API, or change the seeding flow to use pre-computed embeddings (or guard in
seed-e2e-catalog.ts to skip API calls when embeddings are present) and remove
the runtime dependency on .dev.vars placeholder.
In `@apps/expo/playwright/tests/core.spec.ts`:
- Around line 190-192: The test is coupling to internal data-testid composition;
replace the brittle page.getByTestId(/^catalog:item-.*Headlamp/i).first() check
with an assertion that targets observable behavior (visible text or role) — for
example use page.getByText(/Headlamp/i).first() or locate items by the catalog
item test-id prefix (page.getByTestId(/^catalog:item-/)) and then filter by
visible text (locator.filter({ hasText: 'Headlamp' })) so the assertion no
longer depends on how testIds.catalog.item(id) composes IDs.
🪄 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: 1c38609e-352e-4270-9f8c-ea20f3c3d50a
📒 Files selected for processing (9)
.github/workflows/web-e2e-tests.ymlapps/expo/app/(app)/ai-chat.tsxapps/expo/features/ai/components/ChatBubble.tsxapps/expo/features/trips/utils/getTripDetailOptions.tsxapps/expo/lib/testIds.tsapps/expo/playwright/tests/core.spec.tsapps/expo/playwright/tests/packs.spec.tsapps/expo/playwright/tests/trips.spec.tspackages/api/package.json
Comment on lines
+190
to
+192
| await expect(page.getByTestId(/^catalog:item-.*Headlamp/i).first()).toBeVisible({ | ||
| timeout: 10_000, | ||
| }); |
Contributor
There was a problem hiding this comment.
Search assertion is coupled to internal data-testid composition.
Line 190 asserts Headlamp inside catalog:item-* test IDs, but testIds.catalog.item(id) only
guarantees an ID suffix. This is brittle and can fail even when filtering works.
Proposed fix
- await expect(page.getByTestId(/^catalog:item-.*Headlamp/i).first()).toBeVisible({
- timeout: 10_000,
- });
+ await expect(page.getByTestId(/^catalog:item-/).first()).toBeVisible({ timeout: 10_000 });
+ await expect(page.getByText(/headlamp/i).first()).toBeVisible({ timeout: 10_000 });As per coding guidelines, "Avoid testing implementation details; test observable behaviour."
📝 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
| await expect(page.getByTestId(/^catalog:item-.*Headlamp/i).first()).toBeVisible({ | |
| timeout: 10_000, | |
| }); | |
| await expect(page.getByTestId(/^catalog:item-/).first()).toBeVisible({ timeout: 10_000 }); | |
| await expect(page.getByText(/headlamp/i).first()).toBeVisible({ timeout: 10_000 }); |
🤖 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/core.spec.ts` around lines 190 - 192, The test is
coupling to internal data-testid composition; replace the brittle
page.getByTestId(/^catalog:item-.*Headlamp/i).first() check with an assertion
that targets observable behavior (visible text or role) — for example use
page.getByText(/Headlamp/i).first() or locate items by the catalog item test-id
prefix (page.getByTestId(/^catalog:item-/)) and then filter by visible text
(locator.filter({ hasText: 'Headlamp' })) so the assertion no longer depends on
how testIds.catalog.item(id) composes IDs.
Source: Coding guidelines
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/api/src/db/seed-e2e-catalog.ts (2)
167-174:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a timeout to the OpenAI embeddings request.
The current
fetchcan hang indefinitely on network stalls, which can block CI until the job-level timeout. Set an explicit per-request timeout and fail fast with context.🤖 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 `@packages/api/src/db/seed-e2e-catalog.ts` around lines 167 - 174, The fetch call that builds `res` for OpenAI embeddings can hang; add a per-request timeout using an AbortController: create an AbortController, pass controller.signal into the fetch options (the call that posts to 'https://api.openai.com/v1/embeddings' with model 'text-embedding-3-small' and input `values`), set a setTimeout to call controller.abort() after a chosen timeout (e.g. 10s), and clear the timer after the fetch completes; ensure the abort error is handled and surfaced with context so the function that seeds E2E catalog fails fast instead of hanging.
211-220:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast on embedding/result mismatches instead of silently skipping rows.
if (!item || !embedding) continue;can hide partial failures and still logSeeded ${newItems.length}. Validateembeddings.length === newItems.length(and each embedding dimension) before inserts, then throw on mismatch.Suggested fix
const embeddings = await embedAll({ values: newItems.map((i) => `${i.name}. ${i.description}`), openAiKey, }); + if (embeddings.length !== newItems.length) { + throw new Error( + `Embedding count mismatch: expected ${newItems.length}, got ${embeddings.length}`, + ); + } for (let i = 0; i < newItems.length; i++) { const item = newItems[i]; const embedding = embeddings[i]; - if (!item || !embedding) continue; + if (!item || !embedding || embedding.length !== 1536) { + throw new Error(`Invalid embedding at index ${i}`); + } await db.insert(schema.catalogItems).values({Also applies to: 232-233
🤖 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 `@packages/api/src/db/seed-e2e-catalog.ts` around lines 211 - 220, The seed silently skips rows when embeddings are missing; instead, after calling embedAll (function embedAll) validate that embeddings.length === newItems.length and that each embedding has the expected dimension (e.g., non-empty array or expected length) before performing inserts into schema.catalogItems via db.insert; if any mismatch is found, throw an error describing the index and mismatch so the process fails fast and does not report a misleading "Seeded" count (also apply same validation to the other embedding usage around the code referenced at lines ~232-233).
🤖 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/features/packs/utils/getPackDetailOptions.tsx`:
- Around line 25-30: The deleteAndNavigate function currently calls
deletePack(id) and only navigates if router.canGoBack() is true, leaving users
on a deleted detail page when they landed directly; update deleteAndNavigate to
call deletePack(id) and then, if router.canGoBack() call router.back(),
otherwise navigate to a safe fallback route (e.g., the packs list) using the
router navigation method (e.g., router.push or router.replace) so the user is
redirected after confirmed delete.
In `@packages/api/src/services/__tests__/embeddingService.test.ts`:
- Around line 142-143: The test is asserting internal call paths
(mockEmbed/embedMany and createAIProvider) which couples tests to
implementation; remove the two expects checking not.toHaveBeenCalled() (and the
similar ones at the other location) and instead assert observable outputs
(response shape, values, and side-effects) from the function under test (e.g.,
returned embeddings/metadata or DB calls). If you need to guarantee those
internals were not invoked, change the stub for embed/embedMany or
createAIProvider to throw when called so the test will fail if they are invoked;
update assertions to validate the final result content/shape rather than
internal mock call counts.
---
Outside diff comments:
In `@packages/api/src/db/seed-e2e-catalog.ts`:
- Around line 167-174: The fetch call that builds `res` for OpenAI embeddings
can hang; add a per-request timeout using an AbortController: create an
AbortController, pass controller.signal into the fetch options (the call that
posts to 'https://api.openai.com/v1/embeddings' with model
'text-embedding-3-small' and input `values`), set a setTimeout to call
controller.abort() after a chosen timeout (e.g. 10s), and clear the timer after
the fetch completes; ensure the abort error is handled and surfaced with context
so the function that seeds E2E catalog fails fast instead of hanging.
- Around line 211-220: The seed silently skips rows when embeddings are missing;
instead, after calling embedAll (function embedAll) validate that
embeddings.length === newItems.length and that each embedding has the expected
dimension (e.g., non-empty array or expected length) before performing inserts
into schema.catalogItems via db.insert; if any mismatch is found, throw an error
describing the index and mismatch so the process fails fast and does not report
a misleading "Seeded" count (also apply same validation to the other embedding
usage around the code referenced at lines ~232-233).
🪄 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: ba56d75f-6fc3-4854-902f-149cb980787a
📒 Files selected for processing (9)
.github/workflows/web-e2e-tests.yml.maestro/flows/packs/create-pack-flow.yamlapps/expo/features/packs/components/PackForm.tsxapps/expo/features/packs/utils/getPackDetailOptions.tsxapps/expo/playwright/tests/core.spec.tsapps/expo/playwright/tests/packs.spec.tspackages/api/src/db/seed-e2e-catalog.tspackages/api/src/services/__tests__/embeddingService.test.tspackages/api/src/services/embeddingService.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
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
Closes out the remaining web playwright failures left after
feat/web-support-mvp(#2366) merged. Targetsdevelopment; 36/36 green locally onchromechannel with 2 workers, no PWHEADED.Cumulative fixes (relative to development tip):
Providers + shims
apps/expo/providers/index.web.tsx: restoreBottomSheetModalProviderandActionSheetProvider(withuseCustomActionSheet) on web. Gorhom 5.x runs on web via reanimated + gesture-handler, andPackDetailScreenrendersBottomSheetViewinline; without the modal provider it threwBottomSheetModalInternalContext cannot be used outside…. ActionSheetProvider is needed so the more-actions menu actually shows a "Delete" row.apps/expo/mocks/react-native-community-datetimepicker.tsx: forwardtestIDfrom the RN prop to adata-testidon the rendered<input>so playwright can target trip date inputs.Auth & data paths
apps/expo/lib/api/packrat.ts: on web,expoClientshort-circuits andexpo-secure-storeis an empty stub. Fall through toauthClient.getSession()with a 30s in-memory token cache so apiClient's per-request lookup doesn't hammer/api/auth/get-sessionand trip Better Auth's rate limit.apps/expo/lib/auth-client.ts: setfetchOptions.credentials='include'so the cross-origin session cookie is sent on web (expoClient.initis short-circuited there).packages/api/src/auth/index.ts: disable the 100/min Better Auth rate limit whenBETTER_AUTH_URLis a localhost URL. Local web dev legitimately spamsget-sessionviauseSession+ apiClient and trips it within seconds.Trip delete
apps/expo/features/trips/hooks/useDeleteTrip.ts: keep the explicitapiClient.trips({tripId}).delete()call.UpdateTripBodySchemadoesn't exposedeleted, so the syncedCrud PUT path silently strips it and the trip never deletes server-side — the trips route already supports soft-delete via DELETE, use it.apps/expo/features/trips/screens/TripDetailScreen.tsx: replace the imperative NativeWindUI Alert ref with a Platform.OS-gated path —window.confirmon web (RN-web'sAlert.alertignores buttononPresscallbacks, so the destructive handler never fires),Alert.alerton native.AI chat
apps/expo/app/(app)/ai-chat.tsx:authClient.useSession()resolves asynchronously, butDefaultChatTransportcapturedBearer ${token}in headers at construction time. When the user hit send before the hook resolved, the request shippedBearer null→ 401. Switch headers to an async function that reads from a tokenRef driven byuseSession(), with a directauthClient.getSession()fallback when the ref is still empty.Test plan
bun check:allgreen (no-duplicate-deps, no-owned-max-params, react-doctor, etc.)bunx tsc --noEmitcleanPW_CHANNEL=chrome PW_WORKERS=2Summary by CodeRabbit
Bug Fixes
New Features
Improvements
Tests