feat(web): Playwright E2E suite + web support fixes for trips, profile, packs#2363
Merged
Conversation
github-actions
Bot
added
dependencies
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughThis PR adds comprehensive web platform support to the PackRat Expo app, including web-specific layouts, no-op stubs for native-only features, Playwright E2E test infrastructure, a CI/CD workflow for automated web testing, and testID additions across components for test selectability. ChangesExpo Web Platform & E2E Testing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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. |
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a Playwright-based web E2E suite and introduces web-specific shims/fixes across the Expo app and API so trips/packs/profile flows work reliably in browsers.
Changes:
- Added Playwright web E2E infrastructure and ~36 tests (core, trips, packs/items, profile) plus repo tooling exemptions for the new test directory.
- Added/adjusted web-only Expo implementations (providers, stores, mocks, API client, layouts) to replace native-only modules and behaviors.
- API/schema updates to support web flows and sync behavior (trip
deletedupdates, catalog review fields, dev OTP helper endpoint).
Reviewed changes
Copilot reviewed 73 out of 74 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/env/scripts/no-raw-process-env.ts | Allows raw process.env usage in the new Playwright web E2E directory. |
| packages/checks/src/check-type-casts.ts | Excludes e2e-web files from type-cast checks for test infra/shims. |
| packages/api/src/schemas/catalog.ts | Loosens catalog review schema to accept additional/nullable fields. |
| packages/api/src/routes/trips/index.ts | Extends trip update schema/handler to accept deleted for soft-delete sync. |
| packages/api/src/routes/auth/index.ts | Adds a dev-only endpoint to retrieve OTP codes for E2E/support. |
| packages/api-client/src/index.ts | Disables Eden Treaty date reviver to keep datetime fields as strings. |
| package.json | Adds Playwright test runner as a dev dependency. |
| bun.lock | Locks Playwright and related packages. |
| apps/expo/providers/index.web.tsx | Adds a web-specific Providers wrapper removing native-only providers. |
| apps/expo/package.json | Adds Playwright scripts; enables Expo web modal flag for start/web. |
| apps/expo/mocks/react-native-picker.tsx | Adds a web stub for the native picker component. |
| apps/expo/mocks/react-native-maps.ts | Adds a web stub for react-native-maps. |
| apps/expo/mocks/react-native-blob-util.ts | Adds a web stub for react-native-blob-util. |
| apps/expo/mocks/react-native-ai-llama.ts | Adds a web stub for native llama bindings. |
| apps/expo/mocks/react-native-ai-apple.ts | Adds a web stub for Apple AI bindings. |
| apps/expo/mocks/expo-updates.ts | Adds a web stub for expo-updates (reload via window.location). |
| apps/expo/mocks/expo-sqlite-kv-store.ts | Provides a web storage shim backed by localStorage (or memory). |
| apps/expo/mocks/expo-dev-client.ts | Adds a no-op stub for expo-dev-client on web. |
| apps/expo/mocks/async-storage.ts | Adds an SSR-safe async-storage shim for web. |
| apps/expo/metro.config.js | Adds web stubs + resolver tweaks (wasm, condition names, resolveRequest). |
| apps/expo/lib/utils/ImageCacheManager.web.ts | Adds a web no-op image cache manager implementation. |
| apps/expo/lib/testIds.ts | Replaces enum with a centralized testIds registry (plus documentation). |
| apps/expo/lib/hooks/useColorScheme.web.tsx | Removes Android-only nav bar sync, uses NativeWind color scheme on web. |
| apps/expo/lib/constants.web.ts | Adds web constants (e.g., no filesystem image dir). |
| apps/expo/lib/api/client.web.ts | Adds fetch-based API client implementation for web token refresh. |
| apps/expo/global.css | Adds web overrides for NativeWind-emitted themed color classes. |
| apps/expo/features/trips/store/trips.web.ts | Adds web trips store using API + in-memory observable syncing. |
| apps/expo/features/trips/screens/TripListScreen.tsx | Switches to new testIds registry for stable selectors. |
| apps/expo/features/trips/screens/TripDetailScreen.tsx | Adds web-compatible delete confirmation via NativeWindUI Alert ref; adds header testIDs. |
| apps/expo/features/trips/components/TripForm.tsx | Adds missing trip input testIDs and switches to testIds. |
| apps/expo/features/trail-conditions/store/trailConditionReports.web.ts | Adds web trail condition reports store using API + in-memory observable syncing. |
| apps/expo/features/packs/utils/uploadImage.web.ts | Adds web image upload using Fetch + presigned URLs. |
| apps/expo/features/packs/utils/getPackDetailOptions.tsx | Adds pack edit/delete testIDs for web E2E selectors. |
| apps/expo/features/packs/store/packs.web.ts | Adds web packs store using API + in-memory observable syncing. |
| apps/expo/features/packs/store/packingMode.web.ts | Adds web packing mode store (in-memory/sessionStorage). |
| apps/expo/features/packs/store/packWeightHistory.web.ts | Adds web pack-weight-history store and recorder utility. |
| apps/expo/features/packs/store/packItems.web.ts | Adds web pack items store using API syncing and web upload adapter. |
| apps/expo/features/packs/screens/PackListScreen.tsx | Switches pack list create testID usage to testIds. |
| apps/expo/features/packs/screens/PackDetailScreen.tsx | Switches pack detail action testIDs usage to testIds. |
| apps/expo/features/packs/screens/CreatePackItemForm.tsx | Adds item form testIDs for E2E selectors. |
| apps/expo/features/packs/hooks/usePackOwnershipCheck.ts | Makes ownership check reactive via use$ for hydrated store updates. |
| apps/expo/features/packs/components/PackItemCard.tsx | Adds stable item card/more-actions testIDs for web E2E. |
| apps/expo/features/packs/components/PackForm.tsx | Adds pack form name/submit testIDs and switches to testIds. |
| apps/expo/features/packs/components/HorizontalCatalogItemCard.tsx | Adds a stable catalog card testID for web E2E selection. |
| apps/expo/features/packs/components/AddPackItemActions.tsx | Switches add-item action sheet options to testIds. |
| apps/expo/features/pack-templates/store/packTemplates.web.ts | Adds web pack templates store using API + in-memory observable syncing. |
| apps/expo/features/pack-templates/store/packTemplateItems.web.ts | Adds web pack template items store using API + in-memory observable syncing. |
| apps/expo/features/catalog/screens/CatalogItemDetailScreen.tsx | Switches catalog action button testIDs to testIds. |
| apps/expo/features/auth/store/user.web.ts | Adds web user store with localStorage hydration/persistence. |
| apps/expo/features/auth/hooks/useAuthInit.web.ts | Adds web auth init logic using localStorage and router redirect. |
| apps/expo/features/auth/hooks/useAuthActions.web.ts | Adds web auth actions (email/password) and removes native-only flows. |
| apps/expo/features/auth/atoms/authAtoms.web.ts | Adds web auth atoms using atomWithStorage + resilient token storage. |
| apps/expo/features/ai/lib/localModelManager.web.ts | Adds web no-op stub for native-only local AI model management. |
| apps/expo/e2e-web/tests/trips.spec.ts | Adds Playwright E2E coverage for trips CRUD/validation/list behaviors. |
| apps/expo/e2e-web/tests/profile.spec.ts | Adds Playwright E2E coverage for profile name editing and sign-out visibility. |
| apps/expo/e2e-web/tests/packs.spec.ts | Adds Playwright E2E coverage for pack and item CRUD + validation. |
| apps/expo/e2e-web/tests/fixtures.ts | Adds auth-seeded Playwright fixture via storageState. |
| apps/expo/e2e-web/tests/core.spec.ts | Adds core Playwright E2E checks (tabs, packs, trips, catalog, profile, settings, AI, weather). |
| apps/expo/atoms/atomWithSecureStorage.web.ts | Adds web fallback for “secure storage” atoms using localStorage. |
| apps/expo/atoms/atomWithKvStorage.web.ts | Adds web fallback KV storage atoms using localStorage. |
| apps/expo/app/auth/one-time-password.tsx | Guards setNativeProps usage for better web compatibility. |
| apps/expo/app/auth/index.tsx | Switches auth screen testIDs to testIds; adds explicit image sizing for web. |
| apps/expo/app/auth/_layout.tsx | Adds unstable_settings.anchor to stabilize auth routing entry on web. |
| apps/expo/app/auth/(login)/reset-password.tsx | Adds explicit image sizing for web. |
| apps/expo/app/auth/(login)/index.tsx | Switches login testIDs to testIds; adds explicit image sizing for web. |
| apps/expo/app/auth/(login)/forgot-password.tsx | Adds explicit image sizing for web. |
| apps/expo/app/auth/(create-account)/index.tsx | Adds explicit image sizing for web. |
| apps/expo/app/auth/(create-account)/credentials.tsx | Adds explicit image sizing for web. |
| apps/expo/app/_layout.web.tsx | Adds web-specific root layout with Alert plumbing and removed native-only concerns. |
| apps/expo/app/_layout.tsx | Syncs dark mode class to <html> (supports darkMode: 'class' on web). |
| apps/expo/app/(app)/(tabs)/profile/name.tsx | Adds profile name form testIDs for Playwright selectors. |
| apps/expo/app/(app)/(tabs)/profile/index.tsx | Exposes a stable testID for the profile “Name” list row and switches signout testID. |
| apps/expo/app/(app)/(tabs)/_layout.web.tsx | Adds web tabs layout (replaces native tabs implementation). |
| apps/expo/app.config.ts | Switches web output to single-page to support SPA-style navigation/testing. |
Comments suppressed due to low confidence (13)
apps/expo/providers/index.web.tsx:1
React.ReactNodeis referenced but React isn’t imported in this module, which will fail typechecking/compile in TS. Import React as a type (e.g.,import type React from 'react') or useimport type { ReactNode } from 'react'and typechildrenasReactNode.
apps/expo/providers/index.web.tsx:1React.ReactNodeis referenced but React isn’t imported in this module, which will fail typechecking/compile in TS. Import React as a type (e.g.,import type React from 'react') or useimport type { ReactNode } from 'react'and typechildrenasReactNode.
apps/expo/features/trail-conditions/store/trailConditionReports.web.ts:1axiosInstanceinlib/api/client.web.tsexposesget(path)with a single-arg signature, so passing{ params }is a compile-time error and the query params will be ignored at runtime. EncodeupdatedAtinto the URL querystring here, or extend the web client to accept an options object (includingparams) and serialize it consistently for GET requests.
apps/expo/mocks/async-storage.ts:1mergeItem/multiMergewill throw if either existing or incoming values aren’t valid JSON (which is common for token-like keys). Wrap JSON parsing in try/catch and fall back to setting the rawvaluewhen parsing fails (similar to the more defensive logic inmocks/expo-sqlite-kv-store.ts).
apps/expo/mocks/async-storage.ts:1mergeItem/multiMergewill throw if either existing or incoming values aren’t valid JSON (which is common for token-like keys). Wrap JSON parsing in try/catch and fall back to setting the rawvaluewhen parsing fails (similar to the more defensive logic inmocks/expo-sqlite-kv-store.ts).
packages/api/src/routes/auth/index.ts:1- The endpoint is documented as returning the “most recent OTP code”, but
orderBy(oneTimePasswords.expiresAt)(ascending by default) will return the earliest-expiring OTP, which is likely the oldest (and potentially already expired). Order descending (or order by a created/issued timestamp if available) to reliably return the latest OTP.
packages/api/src/routes/auth/index.ts:1 - Even with an
ENVIRONMENT === 'development'guard, this endpoint returns live OTP codes and could be reachable in shared dev/staging environments (or via misconfiguration). Consider adding an additional server-side gate (e.g., require a secret header/token, restrict to localhost, or require an explicitENABLE_DEV_OTP_ENDPOINTenv flag) to reduce accidental exposure.
packages/api/src/routes/trips/index.ts:1 - Using
if ('deleted' in data)will treat{ deleted: undefined }as present and assignupdateData.deleted = undefined, which can lead to inconsistent update behavior depending on how the DB layer serializesundefined. Prefer checkingdata.deleted !== undefinedbefore assigning so only explicit boolean values update the column.
packages/api/src/routes/trips/index.ts:1 - Using
if ('deleted' in data)will treat{ deleted: undefined }as present and assignupdateData.deleted = undefined, which can lead to inconsistent update behavior depending on how the DB layer serializesundefined. Prefer checkingdata.deleted !== undefinedbefore assigning so only explicit boolean values update the column.
apps/expo/features/packs/store/packWeightHistory.web.ts:1 packWeigthHistory*is misspelled (“Weigth”). Since this is a newly introduced web-only module, it’s a good opportunity to rename topackWeightHistoryStore,packWeightHistorySyncState, andPackWeightHistoryStorefor clarity and consistency (and to avoid propagating the typo across imports).
apps/expo/features/packs/store/packWeightHistory.web.ts:1packWeigthHistory*is misspelled (“Weigth”). Since this is a newly introduced web-only module, it’s a good opportunity to rename topackWeightHistoryStore,packWeightHistorySyncState, andPackWeightHistoryStorefor clarity and consistency (and to avoid propagating the typo across imports).
apps/expo/features/packs/store/packItems.web.ts:1uploadImage(fileName, blobOrDataUrl)is being called asuploadImage(data.image, data.image). Ifdata.imageis a data URL, blob URL, or URI, using it asfileNamewill produce an invalid/oversized remote key (and may include characters not suitable for object keys). Pass a real filename (e.g., derived basename + extension) or generate one (e.g.,${id}.jpg) and keep the second argument as the data/blob URL.
packages/api-client/src/index.ts:1- Setting
parseDate: falsechanges the API client’s runtime response shape globally (date-like strings will no longer be revived toDate). If any existing consumers relied onDateobjects, this is a breaking behavioral change. Consider making this configurable viaApiClientConfig(or scoping it to only the clients that validate with Zod string schemas), and documenting the expected response types for datetime fields.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ErrorBoundary, | ||
| } from 'expo-router'; | ||
|
|
||
| export let appAlert: React.RefObject<AlertMethods | null>; |
There was a problem hiding this comment.
React.RefObject is used but React isn’t imported, which will break TS compilation. Prefer importing the type (import type { RefObject } from 'react') and declaring export let appAlert: RefObject<AlertMethods | null>; (or import type React if you want to keep React.RefObject).
| function RootLayout() { | ||
| useInitialAndroidBarSync(); | ||
|
|
||
| appAlert = useRef<AlertMethods>(null); |
There was a problem hiding this comment.
React.RefObject is used but React isn’t imported, which will break TS compilation. Prefer importing the type (import type { RefObject } from 'react') and declaring export let appAlert: RefObject<AlertMethods | null>; (or import type React if you want to keep React.RefObject).
![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: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (6)
apps/expo/features/packs/store/packingMode.web.ts-15-17 (1)
15-17:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate parsed storage payload before
set.
JSON.parsemay return any JSON shape; setting it directly can violate the declaredRecord<string, Record<string, boolean>>contract and break consumers.Proposed fix
+const isPackingModeState = ( + value: unknown, +): value is Record<string, Record<string, boolean>> => { + if (!value || typeof value !== 'object') return false; + + return Object.values(value as Record<string, unknown>).every((inner) => { + if (!inner || typeof inner !== 'object') return false; + return Object.values(inner as Record<string, unknown>).every( + (flag) => typeof flag === 'boolean', + ); + }); +}; + if (typeof window !== 'undefined') { const stored = sessionStorage.getItem('packrat_packing_mode'); if (stored) { try { - packingModeStore.set(JSON.parse(stored)); + const parsed: unknown = JSON.parse(stored); + if (isPackingModeState(parsed)) { + packingModeStore.set(parsed); + } else { + sessionStorage.removeItem('packrat_packing_mode'); + } } catch { sessionStorage.removeItem('packrat_packing_mode'); } } }As per coding guidelines:
**/*.{ts,tsx}: Use proper TypeScript types or unknown instead of any.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/features/packs/store/packingMode.web.ts` around lines 15 - 17, The code currently calls JSON.parse(stored) and passes the result directly to packingModeStore.set which can accept invalid shapes; change this to parse into unknown, validate the parsed value against the expected type Record<string, Record<string, boolean>> (e.g., implement an isPackingModeRecord(value): value is Record<string, Record<string, boolean>> type guard), and only call packingModeStore.set(parsed) when the guard returns true; if validation fails, skip the set (or reset to a safe default) and optionally log an error so consumers of packingModeStore remain type-safe.apps/expo/lib/hooks/useColorScheme.web.tsx-17-26 (1)
17-26:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winToggle against the resolved scheme, not the raw one.
This hook exposes
'light'as the fallback scheme, buttoggleColorScheme()still branches on the rawcolorScheme. If NativeWind returnsundefinedinitially, the first toggle sets'light'again and does nothing.Suggested fix
function useColorScheme() { const { colorScheme, setColorScheme: setNativeWindColorScheme } = useNativewindColorScheme(); + const resolvedColorScheme = colorScheme ?? 'light'; function setColorScheme(scheme: 'light' | 'dark') { setNativeWindColorScheme(scheme); } function toggleColorScheme() { - return setColorScheme(colorScheme === 'light' ? 'dark' : 'light'); + return setColorScheme(resolvedColorScheme === 'light' ? 'dark' : 'light'); } return { - colorScheme: colorScheme ?? 'light', + colorScheme: resolvedColorScheme, isDarkColorScheme: colorScheme === 'dark', setColorScheme, toggleColorScheme, - colors: COLORS[colorScheme ?? 'light'], + colors: COLORS[resolvedColorScheme], }; }As per coding guidelines, "Use NativeWind (Tailwind for React Native) with CSS variable-based color system and manual dark mode toggle for mobile styling."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/lib/hooks/useColorScheme.web.tsx` around lines 17 - 26, toggleColorScheme currently flips the raw colorScheme which can be undefined; compute the resolved scheme (e.g., use colorScheme ?? 'light') and branch against that when toggling so the first toggle actually switches from the fallback; update toggleColorScheme to read the resolved value before calling setColorScheme, and ensure any uses like colors: COLORS[...] and isDarkColorScheme also reference the same resolved value (symbols: toggleColorScheme, colorScheme, setColorScheme, isDarkColorScheme, colors, COLORS).apps/expo/app/_layout.web.tsx-23-26 (1)
23-26:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize
ErrorBoundaryre-export formatting (Biome).This formatter mismatch is currently failing checks.
Suggested fix
-export { - ErrorBoundary, -} from 'expo-router'; +export { ErrorBoundary } from 'expo-router';As per coding guidelines
**/*.{js,jsx,ts,tsx,json}: Enforce 2 spaces indentation, 100 character line width, and single quotes via Biome 2.0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/app/_layout.web.tsx` around lines 23 - 26, The re-export of ErrorBoundary is misformatted for Biome (extra newlines/spacing); update the export of the ErrorBoundary symbol to the normalized single-line form (export { ErrorBoundary } from 'expo-router';) using single quotes, 2-space indentation and 100-char line width so it conforms to the project's Biome formatting rules.apps/expo/features/packs/utils/uploadImage.web.ts-10-13 (1)
10-13:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReformat the
uploadImagesignature to satisfy Biome.This is currently CI-blocking per formatter output.
Suggested fix
-export const uploadImage = async (fileName: string, blobOrDataUrl: string): Promise<string | undefined> => { +export const uploadImage = async ( + fileName: string, + blobOrDataUrl: string, +): Promise<string | undefined> => {As per coding guidelines
**/*.{js,jsx,ts,tsx,json}: Enforce 2 spaces indentation, 100 character line width, and single quotes via Biome 2.0.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/features/packs/utils/uploadImage.web.ts` around lines 10 - 13, The function signature for uploadImage must be reformatted to satisfy Biome rules: update the export const uploadImage declaration so it adheres to 2-space indentation, 100-character max line width and single-quote style (e.g., break parameters/signature across lines if needed and align the return type and async keyword accordingly) — locate the export const uploadImage = async (fileName: string, blobOrDataUrl: string): Promise<string | undefined> => { and reformat spacing/line breaks to match the repository Biome formatter rules.apps/expo/app/_layout.web.tsx-9-9 (1)
9-9:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unused
userStoreimport to unblock lint.This import is flagged by Biome and is not referenced in this module.
Suggested fix
-import { userStore } from 'expo-app/features/auth/store';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/app/_layout.web.tsx` at line 9, Remove the unused import userStore from the module: delete the import specifier "userStore" in the import statement that imports from 'expo-app/features/auth/store' (or remove the entire import if nothing else is imported) so the unused symbol userStore is no longer referenced and the Biome lint error is resolved.apps/expo/features/pack-templates/store/packTemplateItems.web.ts-41-46 (1)
41-46:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFail fast when the update payload has no
id.This handler accepts partial data, but a missing
idcurrently turns into/api/pack-templates/items/undefined. Please guard it before building the URL.Suggested fix
const updatePackTemplateItem = async ({ id, ...data }: Partial<PackTemplateItem>): Promise<PackTemplateItem> => { try { + if (!id) { + throw new Error('Pack template item id is required for updates'); + } const response = await axiosInstance.patch(`/api/pack-templates/items/${id}`, data); return response.data; } catch (error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/features/pack-templates/store/packTemplateItems.web.ts` around lines 41 - 46, The updatePackTemplateItem function accepts a Partial<PackTemplateItem> but uses id directly to build the URL, which can produce /api/pack-templates/items/undefined; guard against a missing id at the start of updatePackTemplateItem by validating that id is present (e.g., if (!id) throw or return a rejected Promise) before calling axiosInstance.patch(`/api/pack-templates/items/${id}`, data) so the handler fails fast and avoids constructing an invalid URL.
🧹 Nitpick comments (4)
apps/expo/features/packs/components/HorizontalCatalogItemCard.tsx (1)
38-40: ⚡ Quick winUse the shared test-ID factory here.
This reintroduces a hand-built selector string right next to the new centralized registry. Using
testIds.items.catalogCard(item.id)keeps the component and the Playwright selectors from drifting.Suggested fix
import type { CatalogItem } from 'expo-app/features/catalog/types'; import { useColorScheme } from 'expo-app/lib/hooks/useColorScheme'; +import { testIds } from 'expo-app/lib/testIds'; import { TouchableWithoutFeedback, View } from 'react-native'; ... <View - testID={`catalog-item-card-${item.id}`} + testID={testIds.items.catalogCard(item.id)} className={`rounded-lg flex-row gap-3 border p-4 bg-red🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/features/packs/components/HorizontalCatalogItemCard.tsx` around lines 38 - 40, The testID prop in HorizontalCatalogItemCard is using a hand-built string (`catalog-item-card-${item.id}`); replace it with the centralized factory call testIds.items.catalogCard(item.id) to keep selectors consistent. Update the View's testID to use testIds.items.catalogCard(item.id) and add or update the import for testIds (from the shared test-id registry) at the top of the file if missing. Ensure the component still receives item.id and run unit/playwright tests to confirm selectors resolve.apps/expo/package.json (1)
38-39: Add the new web suite to CI.
test:webis useful, but it only prevents regressions once a CI job gates merges on it. Right now the new selectors and web shims can still drift silently between local runs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/package.json` around lines 38 - 39, CI is not running the new "test:web" (and optionally "test:web:ui") npm scripts, so add a step to the existing CI workflow that invokes npm run test:web (or yarn test:web) as part of the test job (or create a parallel job if you prefer isolation); locate the workflow that runs the test matrix and add a command step that installs deps and runs the "test:web" script, and if you want UI debugging include "test:web:ui" behind a conditional matrix flag so the web suite is executed on each PR merge to prevent silent regressions.apps/expo/e2e-web/tests/packs.spec.ts (2)
124-130: ⚡ Quick winWait for the delete request to finish before asserting list absence.
Right now the test navigates immediately after click, so list assertions can race backend persistence.
Suggested fix
+ const deleteRequest = page.waitForResponse( + (r) => + r.url().includes(`/api/packs/${packId}`) && + (r.request().method() === 'DELETE' || + r.request().method() === 'PUT' || + r.request().method() === 'PATCH'), + { timeout: 20_000 }, + ); + const deleteButton = page.getByTestId('packs:delete'); await deleteButton.waitFor({ timeout: 15_000 }); await deleteButton.click(); + const deleteResponse = await deleteRequest; + expect(deleteResponse.ok()).toBeTruthy(); // After deletion the app should navigate away; go to list and confirm pack is gone await page.goto(`${BASE_URL}/packs`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/e2e-web/tests/packs.spec.ts` around lines 124 - 130, The test races the backend because it navigates immediately after clicking the delete button; update the flow around deleteButton, click, and navigation so the test waits for the deletion request to finish (e.g., await page.waitForResponse(...) that matches the DELETE /packs or the specific API route for the pack) or await page.waitForNavigation() tied to the action, then call page.goto(`${BASE_URL}/packs`) and assert that page.getByText(packName) is not visible; ensure you reference the same deleteButton, BASE_URL and packName symbols so the wait is tied to the actual delete action before asserting list absence.
239-243: ⚡ Quick winReplace fixed 1s sleeps with URL assertions.
The hardcoded delays are brittle and can still miss slow navigations.
Suggested fix
- // Wait a moment for any navigation to settle - await page.waitForTimeout(1_000); - - // Should still be on the create form (validation prevented navigation) - expect(page.url()).toBe(formUrl); + await expect(page).toHaveURL(formUrl); @@ - await page.waitForTimeout(1_000); - - // Should still be on the create item form - expect(page.url()).toBe(formUrl); + await expect(page).toHaveURL(formUrl);Also applies to: 260-264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/e2e-web/tests/packs.spec.ts` around lines 239 - 243, Replace the brittle fixed sleep (page.waitForTimeout(1_000)) with an explicit URL wait/assertion: remove the waitForTimeout call and instead use Playwright's URL helpers to wait for and assert the navigation, e.g. replace the wait + expect(page.url()).toBe(formUrl) pattern with await expect(page).toHaveURL(formUrl) (or await page.waitForURL(formUrl) followed by the expect) for the block containing page.waitForTimeout(1_000) and the same change for the other occurrence around the lines referencing page.waitForTimeout and expect(page.url()).toBe(formUrl).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/expo/e2e-web/tests/trips.spec.ts`:
- Around line 188-205: After clicking the modal confirm
(deleteConfirmBtn.click()), the test needs to wait for the backend delete
mutation to complete before navigating to /trips; update the flow after
deleteConfirmBtn to wait for the network delete/soft-delete response or the
GraphQL mutation (e.g., match request method/path or operation name like
"DeleteTrip") using Playwright's waitForResponse or waitForRequest, or
alternatively wait for the trip detail DOM to be removed (use the trip detail
locator's waitFor state 'detached'), then proceed with waitForURL/goto and load
idle checks; reference deleteConfirmBtn, deleteButton, and the subsequent
waitForURL/goto steps when applying the change.
In `@apps/expo/features/auth/atoms/authAtoms.web.ts`:
- Around line 20-31: The code reads localStorage during atom setup
(createJSONStorage and resilientTokenStorage.getItem) which crashes outside the
browser; wrap all direct localStorage access and initialization in a browser
guard (e.g. check typeof window !== "undefined" and that window.localStorage
exists) and provide a safe fallback storage when not available. Modify the
createJSONStorage call and resilientTokenStorage methods (getItem and any other
reads/writes) to use the guarded accessor or an in-memory/no-op storage
implementation so atom initialization and reads return initialValue/null safely
in non-browser environments while preserving normal localStorage behavior in the
browser.
In `@apps/expo/features/auth/hooks/useAuthActions.web.ts`:
- Around line 119-137: The signOut function currently clears tokens and local
storage but does not reset the in-memory user state, causing the previous user's
profile to persist; update the signOut finally block (after clearLocalData and
before clearing loading state) to also clear the user store by calling the
appropriate reset function or setter (e.g., userStore.reset() or setUser(null) /
setUserProfile(null)) so the in-memory profile is cleared on sign-out; ensure
you reference the user store API used in this module (userStore, setUser,
setUserProfile, or equivalent) and keep this user-reset step alongside
setToken(null)/setRefreshToken(null).
In `@apps/expo/features/auth/hooks/useAuthInit.web.ts`:
- Around line 13-29: The effect in useAuthInit.web.ts currently calls
localStorage.getItem directly and can throw in blocked storage contexts; wrap
both localStorage.getItem calls in a try/catch inside the useEffect so any
SecurityError is caught and treated as "no token / not skipped" (i.e., set
hasSkippedLogin = null/undefined and accessToken = null), ensure
setIsLoading(false) still runs and the existing redirect logic using
router.replace({ pathname: '/auth', params: { showSkipLoginBtn: 'true',
redirectTo: '/' } }) remains unchanged, and keep references to setIsLoading,
router.replace, hasSkippedLogin, and accessToken when making the change.
In `@apps/expo/features/pack-templates/store/packTemplates.web.ts`:
- Around line 34-37: The updatePackTemplate function currently accepts a
Partial<PackTemplate> allowing id to be omitted, leading to requests to
/api/pack-templates/undefined; change the parameter type so id is required
(e.g., require Pick<PackTemplate,'id'> combined with Partial of the rest or make
id: string) and update the signature of updatePackTemplate accordingly, and add
a short runtime guard inside updatePackTemplate that throws or rejects if id is
missing to prevent building an invalid URL (refer to updatePackTemplate and
PackTemplate to locate the code).
In `@apps/expo/features/packs/store/packs.web.ts`:
- Around line 34-37: The updatePack function currently types its parameter as
Partial<PackInStore>, making id optional and risking requests to
/api/packs/undefined; change the parameter type to require id (for example use
Pick<PackInStore,'id'> & Partial<Omit<PackInStore,'id'>> or an explicit { id:
string; } plus the rest) and add a runtime guard at the start of updatePack
(throw or return an error if id is missing) so the axiosInstance.put call always
targets a valid /api/packs/{id} endpoint; updatePack is the function to modify.
In `@apps/expo/features/packs/store/packWeightHistory.web.ts`:
- Around line 67-73: recordPackWeight currently assumes obs(packsStore,
packId).peek() returns a pack; add a guard to bail out early if pack is
undefined (and optionally if packId is stale) before calling computePackWeights
or generating an id so you don't persist invalid history. Specifically, in
recordPackWeight check the result of packsStore.peek() (the local variable pack)
and return immediately when falsy; only fetch packItems, call
computePackWeights, and write the history record when pack is present.
In `@apps/expo/features/packs/utils/uploadImage.web.ts`:
- Around line 69-70: Check the fetch response before converting to a Blob: after
calling fetch(url) (the const res = await fetch(url) call) verify res.ok and
handle non-2xx responses (e.g., throw an error or return a rejected Promise)
instead of blindly returning res.blob(), so failed fetches or HTML error pages
aren’t uploaded as image data; update the function that performs this fetch to
check res.ok and include the response status/message in the thrown error.
- Around line 17-21: remoteFileName is being built using userStore.id.peek()
which can be undefined if the store isn't hydrated, producing keys like
"undefined-<fileName>"; before constructing remoteFileName or calling
getPresignedUrl, explicitly guard for a missing user id (check
userStore.id.peek() or equivalent) and handle it by either throwing a clear
error (e.g., "missing authenticated user id when uploading image") or
awaiting/triggering store hydration/authentication, then proceed to build
remoteFileName and call getPresignedUrl; update references in this block
(remoteFileName, userStore.id.peek(), getPresignedUrl) accordingly so uploads
never use an undefined user id.
In `@apps/expo/features/trips/screens/TripDetailScreen.tsx`:
- Around line 89-92: After deleting the trip in the onPress handler in
TripDetailScreen (the async block that calls deleteTrip(id)), guard the
navigation so router.back() is only called when router.canGoBack() returns true;
otherwise navigate to a safe fallback (e.g., router.replace('/trips') or another
appropriate route). Update the onPress flow that currently awaits deleteTrip(id
as string) then calls router.back() to instead check router.canGoBack() and call
router.back() only if true, otherwise call router.replace(...) to avoid silent
failures when there's no history.
In `@apps/expo/features/trips/store/trips.web.ts`:
- Around line 34-37: The updateTrip function accepts a parameter typed as
Partial<TripInStore> which makes id optional and can cause invalid URLs; change
the parameter type to require id while keeping other fields optional (for
example use an intersection like { id: TripInStore['id'] } &
Partial<Omit<TripInStore,'id'>> or Required<Pick<TripInStore,'id'>> &
Partial<TripInStore>) so updateTrip({ id, ...data }) always has an id when
calling axiosInstance.put(`/api/trips/${id}`, data); update the function
signature for updateTrip accordingly.
In `@apps/expo/global.css`:
- Around line 7-56: The utility classes (e.g., .text-foreground, .bg-background,
.border-border) only exist inside the `@media` screen block and therefore still
follow system dark-mode variables; duplicate/mirror that block so the same rules
are also applied when the root .dark class is present by adding a .dark-prefixed
rule set (e.g., .dark .text-foreground { color: rgb(var(--foreground)); } and
likewise for .text-muted-foreground, .text-card-foreground, .bg-card,
.bg-primary, .bg-secondary, .bg-muted, .bg-accent, .bg-destructive,
.border-border, etc.) so class-driven theme toggles will use the same
var(...)–backed colors.
In `@apps/expo/lib/api/client.web.ts`:
- Around line 68-95: The request function currently returns {data, status} for
non-2xx responses which prevents consumers from catching failures; update the
request function (async function request<T>) to, after parsing the response
body, check res.ok (or res.status in 200–299) and if false throw an Error-like
object that includes the HTTP status and parsed error payload (e.g., { message,
status, data }) so callers' catch blocks run; apply the same change to the other
similar fetch helper referenced around lines 105-108 so all non-success HTTP
responses throw instead of returning data.
In `@apps/expo/metro.config.js`:
- Around line 10-17: The global resolver currently sets unstable_conditionNames
to include 'browser', which forces browser-specific condition resolution on all
platforms; change the config.resolver to set unstable_conditionNames to
['require','default','react-native'] and move the 'browser' condition into
unstable_conditionsByPlatform.web (e.g.,
config.resolver.unstable_conditionsByPlatform = {
...(config.resolver?.unstable_conditionsByPlatform ?? {}), web:
[...(config.resolver?.unstable_conditionsByPlatform?.web ?? []), 'browser'] })
so that only web builds resolve the browser export condition while iOS/Android
use the default set; update references around config.resolver and
unstable_conditionNames/unstable_conditionsByPlatform.web accordingly.
In `@apps/expo/mocks/async-storage.ts`:
- Around line 19-27: mergeItem and multiMerge perform blind JSON.parse on stored
and incoming values which will throw on malformed/non-JSON payloads; wrap
parsing of existing and incoming values in try/catch (or use a safeParse helper)
so malformed JSON falls back to treating the value as a raw string/object
instead of throwing, then perform the merge using the successfully-parsed
objects when available (or overwrite/concatenate safely when parsing fails),
keep the same return behavior and ensure the guard logic is applied in both
mergeItem and multiMerge.
In `@apps/expo/mocks/react-native-picker.tsx`:
- Around line 30-31: The onChange handler currently reads e.target.value which
is always a string and loses numeric Picker.Item values; change the rendering of
option elements to store the original value as a JSON-encoded data attribute
(e.g., data-value=JSON.stringify(itemValue)) and update the onChange in the
component that uses selectedValue and onValueChange to read the selected
option's data-value (e.target.selectedOptions[0].getAttribute('data-value')),
JSON.parse it (with try/catch fallback to the raw string) and pass that parsed
value into onValueChange so numeric values are preserved.
In `@packages/api/src/routes/auth/index.ts`:
- Around line 519-524: The query that fetches otp currently orders by
oneTimePasswords.expiresAt ascending and doesn't exclude expired codes, so
change the query in the auth route to filter out expired OTPs (e.g., add a where
clause comparing oneTimePasswords.expiresAt to the current time) and order by
oneTimePasswords.expiresAt descending so it returns the newest unexpired OTP
(keep the existing limit(1) and select of oneTimePasswords.code to locate the
change).
---
Minor comments:
In `@apps/expo/app/_layout.web.tsx`:
- Around line 23-26: The re-export of ErrorBoundary is misformatted for Biome
(extra newlines/spacing); update the export of the ErrorBoundary symbol to the
normalized single-line form (export { ErrorBoundary } from 'expo-router';) using
single quotes, 2-space indentation and 100-char line width so it conforms to the
project's Biome formatting rules.
- Line 9: Remove the unused import userStore from the module: delete the import
specifier "userStore" in the import statement that imports from
'expo-app/features/auth/store' (or remove the entire import if nothing else is
imported) so the unused symbol userStore is no longer referenced and the Biome
lint error is resolved.
In `@apps/expo/features/pack-templates/store/packTemplateItems.web.ts`:
- Around line 41-46: The updatePackTemplateItem function accepts a
Partial<PackTemplateItem> but uses id directly to build the URL, which can
produce /api/pack-templates/items/undefined; guard against a missing id at the
start of updatePackTemplateItem by validating that id is present (e.g., if (!id)
throw or return a rejected Promise) before calling
axiosInstance.patch(`/api/pack-templates/items/${id}`, data) so the handler
fails fast and avoids constructing an invalid URL.
In `@apps/expo/features/packs/store/packingMode.web.ts`:
- Around line 15-17: The code currently calls JSON.parse(stored) and passes the
result directly to packingModeStore.set which can accept invalid shapes; change
this to parse into unknown, validate the parsed value against the expected type
Record<string, Record<string, boolean>> (e.g., implement an
isPackingModeRecord(value): value is Record<string, Record<string, boolean>>
type guard), and only call packingModeStore.set(parsed) when the guard returns
true; if validation fails, skip the set (or reset to a safe default) and
optionally log an error so consumers of packingModeStore remain type-safe.
In `@apps/expo/features/packs/utils/uploadImage.web.ts`:
- Around line 10-13: The function signature for uploadImage must be reformatted
to satisfy Biome rules: update the export const uploadImage declaration so it
adheres to 2-space indentation, 100-character max line width and single-quote
style (e.g., break parameters/signature across lines if needed and align the
return type and async keyword accordingly) — locate the export const uploadImage
= async (fileName: string, blobOrDataUrl: string): Promise<string | undefined>
=> { and reformat spacing/line breaks to match the repository Biome formatter
rules.
In `@apps/expo/lib/hooks/useColorScheme.web.tsx`:
- Around line 17-26: toggleColorScheme currently flips the raw colorScheme which
can be undefined; compute the resolved scheme (e.g., use colorScheme ?? 'light')
and branch against that when toggling so the first toggle actually switches from
the fallback; update toggleColorScheme to read the resolved value before calling
setColorScheme, and ensure any uses like colors: COLORS[...] and
isDarkColorScheme also reference the same resolved value (symbols:
toggleColorScheme, colorScheme, setColorScheme, isDarkColorScheme, colors,
COLORS).
---
Nitpick comments:
In `@apps/expo/e2e-web/tests/packs.spec.ts`:
- Around line 124-130: The test races the backend because it navigates
immediately after clicking the delete button; update the flow around
deleteButton, click, and navigation so the test waits for the deletion request
to finish (e.g., await page.waitForResponse(...) that matches the DELETE /packs
or the specific API route for the pack) or await page.waitForNavigation() tied
to the action, then call page.goto(`${BASE_URL}/packs`) and assert that
page.getByText(packName) is not visible; ensure you reference the same
deleteButton, BASE_URL and packName symbols so the wait is tied to the actual
delete action before asserting list absence.
- Around line 239-243: Replace the brittle fixed sleep
(page.waitForTimeout(1_000)) with an explicit URL wait/assertion: remove the
waitForTimeout call and instead use Playwright's URL helpers to wait for and
assert the navigation, e.g. replace the wait + expect(page.url()).toBe(formUrl)
pattern with await expect(page).toHaveURL(formUrl) (or await
page.waitForURL(formUrl) followed by the expect) for the block containing
page.waitForTimeout(1_000) and the same change for the other occurrence around
the lines referencing page.waitForTimeout and expect(page.url()).toBe(formUrl).
In `@apps/expo/features/packs/components/HorizontalCatalogItemCard.tsx`:
- Around line 38-40: The testID prop in HorizontalCatalogItemCard is using a
hand-built string (`catalog-item-card-${item.id}`); replace it with the
centralized factory call testIds.items.catalogCard(item.id) to keep selectors
consistent. Update the View's testID to use testIds.items.catalogCard(item.id)
and add or update the import for testIds (from the shared test-id registry) at
the top of the file if missing. Ensure the component still receives item.id and
run unit/playwright tests to confirm selectors resolve.
In `@apps/expo/package.json`:
- Around line 38-39: CI is not running the new "test:web" (and optionally
"test:web:ui") npm scripts, so add a step to the existing CI workflow that
invokes npm run test:web (or yarn test:web) as part of the test job (or create a
parallel job if you prefer isolation); locate the workflow that runs the test
matrix and add a command step that installs deps and runs the "test:web" script,
and if you want UI debugging include "test:web:ui" behind a conditional matrix
flag so the web suite is executed on each PR merge to prevent silent
regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e95c9ace-ca36-41f1-b5c1-e7486be53a9e
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (73)
apps/expo/app.config.tsapps/expo/app/(app)/(tabs)/_layout.web.tsxapps/expo/app/(app)/(tabs)/profile/index.tsxapps/expo/app/(app)/(tabs)/profile/name.tsxapps/expo/app/_layout.tsxapps/expo/app/_layout.web.tsxapps/expo/app/auth/(create-account)/credentials.tsxapps/expo/app/auth/(create-account)/index.tsxapps/expo/app/auth/(login)/forgot-password.tsxapps/expo/app/auth/(login)/index.tsxapps/expo/app/auth/(login)/reset-password.tsxapps/expo/app/auth/_layout.tsxapps/expo/app/auth/index.tsxapps/expo/app/auth/one-time-password.tsxapps/expo/atoms/atomWithKvStorage.web.tsapps/expo/atoms/atomWithSecureStorage.web.tsapps/expo/e2e-web/tests/core.spec.tsapps/expo/e2e-web/tests/fixtures.tsapps/expo/e2e-web/tests/packs.spec.tsapps/expo/e2e-web/tests/profile.spec.tsapps/expo/e2e-web/tests/trips.spec.tsapps/expo/features/ai/lib/localModelManager.web.tsapps/expo/features/auth/atoms/authAtoms.web.tsapps/expo/features/auth/hooks/useAuthActions.web.tsapps/expo/features/auth/hooks/useAuthInit.web.tsapps/expo/features/auth/store/user.web.tsapps/expo/features/catalog/screens/CatalogItemDetailScreen.tsxapps/expo/features/pack-templates/store/packTemplateItems.web.tsapps/expo/features/pack-templates/store/packTemplates.web.tsapps/expo/features/packs/components/AddPackItemActions.tsxapps/expo/features/packs/components/HorizontalCatalogItemCard.tsxapps/expo/features/packs/components/PackForm.tsxapps/expo/features/packs/components/PackItemCard.tsxapps/expo/features/packs/hooks/usePackOwnershipCheck.tsapps/expo/features/packs/screens/CreatePackItemForm.tsxapps/expo/features/packs/screens/PackDetailScreen.tsxapps/expo/features/packs/screens/PackListScreen.tsxapps/expo/features/packs/store/packItems.web.tsapps/expo/features/packs/store/packWeightHistory.web.tsapps/expo/features/packs/store/packingMode.web.tsapps/expo/features/packs/store/packs.web.tsapps/expo/features/packs/utils/getPackDetailOptions.tsxapps/expo/features/packs/utils/uploadImage.web.tsapps/expo/features/trail-conditions/store/trailConditionReports.web.tsapps/expo/features/trips/components/TripForm.tsxapps/expo/features/trips/screens/TripDetailScreen.tsxapps/expo/features/trips/screens/TripListScreen.tsxapps/expo/features/trips/store/trips.web.tsapps/expo/global.cssapps/expo/lib/api/client.web.tsapps/expo/lib/constants.web.tsapps/expo/lib/hooks/useColorScheme.web.tsxapps/expo/lib/testIds.tsapps/expo/lib/utils/ImageCacheManager.web.tsapps/expo/metro.config.jsapps/expo/mocks/async-storage.tsapps/expo/mocks/expo-dev-client.tsapps/expo/mocks/expo-sqlite-kv-store.tsapps/expo/mocks/expo-updates.tsapps/expo/mocks/react-native-ai-apple.tsapps/expo/mocks/react-native-ai-llama.tsapps/expo/mocks/react-native-blob-util.tsapps/expo/mocks/react-native-maps.tsapps/expo/mocks/react-native-picker.tsxapps/expo/package.jsonapps/expo/providers/index.web.tsxpackage.jsonpackages/api-client/src/index.tspackages/api/src/routes/auth/index.tspackages/api/src/routes/trips/index.tspackages/api/src/schemas/catalog.tspackages/checks/src/check-type-casts.tspackages/env/scripts/no-raw-process-env.ts
Comment on lines
+188
to
+205
| const deleteButton = page.getByTestId('trips:delete'); | ||
| await deleteButton.waitFor({ timeout: 10_000 }); | ||
| await deleteButton.click(); | ||
|
|
||
| // react-native Alert.alert is a no-op on web; the app uses NativeWindUI Alert (DOM modal). | ||
| // Use exact:true so "E2E-DeleteTrip-..." trip names don't match as false positives. | ||
| const deleteConfirmBtn = page.getByText('Delete', { exact: true }); | ||
| await deleteConfirmBtn.waitFor({ state: 'visible', timeout: 10_000 }); | ||
| await deleteConfirmBtn.click(); | ||
|
|
||
| // router.back() SPA-navigates away from the trip detail. | ||
| // Wait for URL to change (either to /trips or /) | ||
| await page.waitForURL((url) => !url.pathname.startsWith('/trip/'), { timeout: 15_000 }); | ||
| await page.waitForLoadState('networkidle'); | ||
|
|
||
| // Navigate to trips list to confirm trip is gone | ||
| await page.goto(`${BASE_URL}/trips`); | ||
| await page.waitForLoadState('networkidle'); |
Contributor
There was a problem hiding this comment.
Wait for the delete mutation before reloading the list.
This flow reloads immediately after confirming the modal, but unlike create/edit it never blocks on the soft-delete request. A fast goto('/trips') can race the backend write and make the spec intermittently re-fetch the just-deleted trip.
Suggested fix
+ const deletePromise = page.waitForResponse(
+ (r) =>
+ r.url().includes('/api/trips') &&
+ ['PUT', 'PATCH', 'DELETE'].includes(r.request().method()),
+ { timeout: 20_000 },
+ );
+
const deleteConfirmBtn = page.getByText('Delete', { exact: true });
await deleteConfirmBtn.waitFor({ state: 'visible', timeout: 10_000 });
await deleteConfirmBtn.click();
+ await deletePromise;
// router.back() SPA-navigates away from the trip detail.📝 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
| const deleteButton = page.getByTestId('trips:delete'); | |
| await deleteButton.waitFor({ timeout: 10_000 }); | |
| await deleteButton.click(); | |
| // react-native Alert.alert is a no-op on web; the app uses NativeWindUI Alert (DOM modal). | |
| // Use exact:true so "E2E-DeleteTrip-..." trip names don't match as false positives. | |
| const deleteConfirmBtn = page.getByText('Delete', { exact: true }); | |
| await deleteConfirmBtn.waitFor({ state: 'visible', timeout: 10_000 }); | |
| await deleteConfirmBtn.click(); | |
| // router.back() SPA-navigates away from the trip detail. | |
| // Wait for URL to change (either to /trips or /) | |
| await page.waitForURL((url) => !url.pathname.startsWith('/trip/'), { timeout: 15_000 }); | |
| await page.waitForLoadState('networkidle'); | |
| // Navigate to trips list to confirm trip is gone | |
| await page.goto(`${BASE_URL}/trips`); | |
| await page.waitForLoadState('networkidle'); | |
| const deleteButton = page.getByTestId('trips:delete'); | |
| await deleteButton.waitFor({ timeout: 10_000 }); | |
| await deleteButton.click(); | |
| // react-native Alert.alert is a no-op on web; the app uses NativeWindUI Alert (DOM modal). | |
| // Use exact:true so "E2E-DeleteTrip-..." trip names don't match as false positives. | |
| const deletePromise = page.waitForResponse( | |
| (r) => | |
| r.url().includes('/api/trips') && | |
| ['PUT', 'PATCH', 'DELETE'].includes(r.request().method()), | |
| { timeout: 20_000 }, | |
| ); | |
| const deleteConfirmBtn = page.getByText('Delete', { exact: true }); | |
| await deleteConfirmBtn.waitFor({ state: 'visible', timeout: 10_000 }); | |
| await deleteConfirmBtn.click(); | |
| await deletePromise; | |
| // router.back() SPA-navigates away from the trip detail. | |
| // Wait for URL to change (either to /trips or /) | |
| await page.waitForURL((url) => !url.pathname.startsWith('/trip/'), { timeout: 15_000 }); | |
| await page.waitForLoadState('networkidle'); | |
| // Navigate to trips list to confirm trip is gone | |
| await page.goto(`${BASE_URL}/trips`); | |
| await page.waitForLoadState('networkidle'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/expo/e2e-web/tests/trips.spec.ts` around lines 188 - 205, After clicking
the modal confirm (deleteConfirmBtn.click()), the test needs to wait for the
backend delete mutation to complete before navigating to /trips; update the flow
after deleteConfirmBtn to wait for the network delete/soft-delete response or
the GraphQL mutation (e.g., match request method/path or operation name like
"DeleteTrip") using Playwright's waitForResponse or waitForRequest, or
alternatively wait for the trip detail DOM to be removed (use the trip detail
locator's waitFor state 'detached'), then proceed with waitForURL/goto and load
idle checks; reference deleteConfirmBtn, deleteButton, and the subsequent
waitForURL/goto steps when applying the change.
Comment on lines
+68
to
+95
| async function request<T>( | ||
| method: string, | ||
| path: string, | ||
| body?: unknown, | ||
| retry = true, | ||
| ): Promise<{ data: T; status: number }> { | ||
| const token = await getToken(); | ||
| const headers: Record<string, string> = { | ||
| 'Content-Type': 'application/json', | ||
| Accept: 'application/json', | ||
| }; | ||
| if (token) headers.Authorization = `Bearer ${token}`; | ||
|
|
||
| const res = await fetch(`${API_URL}${path}`, { | ||
| method, | ||
| headers, | ||
| body: body != null ? JSON.stringify(body) : undefined, | ||
| }); | ||
|
|
||
| if (res.status === 401 && retry) { | ||
| const newToken = await refreshTokens(); | ||
| if (newToken) return request<T>(method, path, body, false); | ||
| } | ||
|
|
||
| const data = await res.json().catch(() => null); | ||
| // safe-cast: caller is responsible for specifying the correct response type T | ||
| return { data: data as T, status: res.status }; | ||
| } |
Contributor
There was a problem hiding this comment.
Throw on non-2xx responses in request to preserve error flow.
Right now failed HTTP responses still return { data, status }, so store-level catch blocks won’t run and error payloads can be consumed as valid data.
Proposed fix
async function request<T>(
@@
): Promise<{ data: T; status: number }> {
@@
if (res.status === 401 && retry) {
const newToken = await refreshTokens();
if (newToken) return request<T>(method, path, body, false);
}
const data = await res.json().catch(() => null);
+ if (!res.ok) {
+ const message =
+ data && typeof data === 'object' && 'message' in data
+ ? String((data as { message?: unknown }).message ?? `HTTP ${res.status}`)
+ : `HTTP ${res.status}`;
+ throw { message, status: res.status };
+ }
// safe-cast: caller is responsible for specifying the correct response type T
return { data: data as T, status: res.status };
}
@@
export const handleApiError = (error: unknown): { message: string; status?: number } => {
+ if (error && typeof error === 'object' && 'message' in error) {
+ const e = error as { message?: unknown; status?: unknown };
+ return {
+ message: typeof e.message === 'string' ? e.message : 'An unknown error occurred',
+ status: typeof e.status === 'number' ? e.status : undefined,
+ };
+ }
if (error instanceof Error) return { message: error.message };
return { message: 'An unknown error occurred' };
};Also applies to: 105-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/expo/lib/api/client.web.ts` around lines 68 - 95, The request function
currently returns {data, status} for non-2xx responses which prevents consumers
from catching failures; update the request function (async function request<T>)
to, after parsing the response body, check res.ok (or res.status in 200–299) and
if false throw an Error-like object that includes the HTTP status and parsed
error payload (e.g., { message, status, data }) so callers' catch blocks run;
apply the same change to the other similar fetch helper referenced around lines
105-108 so all non-success HTTP responses throw instead of returning data.
Comment on lines
+10
to
+17
| config.resolver = { | ||
| ...config.resolver, | ||
| assetExts: [...(config.resolver?.assetExts ?? []), 'wasm'], | ||
| // Exclude the ESM "import" condition so packages like Jotai resolve to their | ||
| // CJS builds instead of .mjs files that contain import.meta (invalid in | ||
| // Metro's __d() CJS module wrapper). | ||
| unstable_conditionNames: ['require', 'default', 'react-native', 'browser'], | ||
| }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual file to confirm the current state
find . -name "metro.config.js" -o -name "metro.config.*" | head -20Repository: PackRat-AI/PackRat
Length of output: 90
🏁 Script executed:
# Check the specific file mentioned
cat apps/expo/metro.config.jsRepository: PackRat-AI/PackRat
Length of output: 2622
🌐 Web query:
Metro bundler unstable_conditionNames platform-specific configuration
💡 Result:
In Metro bundler, unstable_conditionNames and unstable_conditionsByPlatform are experimental resolver options for configuring conditional exports in package.json when unstable_enablePackageExports is enabled (default in recent versions). unstable_conditionNames is an array of condition names asserted globally for all platforms when resolving exports. Defaults to ['require'] generally, or ['require', 'react-native'] in React Native. unstable_conditionsByPlatform is an object mapping platforms (e.g., from platforms like 'ios', 'android', 'web') to additional condition name arrays. These merge with unstable_conditionNames per platform. Defaults to { web: ['browser'] }. This enables platform-specific configuration: for web bundles, conditions become ['require', 'browser']; for React Native platforms, typically ['require', 'react-native']. Example metro.config.js: const config = { resolver: { unstable_conditionNames: ['require', 'react-native'], unstable_conditionsByPlatform: { web: ['browser'], ios: ['react-native'], android: ['react-native'] } } }; Conditions resolve in package-defined order; Metro follows Node.js exports spec where earlier keys take precedence.
Citations:
- 1: https://metrobundler.dev/docs/configuration/
- 2: https://metrobundler.dev/docs/package-exports/
- 3: https://www.mintlify.com/facebook/react-native/devtools/metro
🌐 Web query:
Metro bundler unstable_conditionsByPlatform documentation
💡 Result:
unstable_conditionsByPlatform (full name: unstable_conditionsByPlatformExperimental) is a Metro bundler configuration option under resolver. It specifies additional condition names to dynamically assert by platform when interpreting the "exports" field in package.json, effective only when unstable_enablePackageExports is true (default). Type: {[platform: string]: Array} Default: { web: ['browser'] } With defaults, for web bundles it asserts conditions new Set(['require', 'browser']) (merging with unstable_conditionNames default ['require']), and for other platforms new Set(['require']). These conditions are resolved in the order specified by each package's exports field, following the package exports spec where earlier entries take precedence. Source: Official Metro documentation at https://metrobundler.dev/docs/configuration/ (published 2026-04-24).
Citations:
Scope browser to web-only Metro conditions.
unstable_conditionNames is asserted globally across all platforms. Metro's defaults already provide browser via unstable_conditionsByPlatform.web, so including it in the global list causes iOS and Android bundles to also resolve browser export conditions, leading to incorrect cross-platform module resolution. Keep the global set at ['require', 'default', 'react-native'] and add browser only to unstable_conditionsByPlatform.web. (metrobundler.dev)
Suggested config shape
config.resolver = {
...config.resolver,
assetExts: [...(config.resolver?.assetExts ?? []), 'wasm'],
- unstable_conditionNames: ['require', 'default', 'react-native', 'browser'],
+ unstable_conditionNames: ['require', 'default', 'react-native'],
+ unstable_conditionsByPlatform: {
+ ...(config.resolver?.unstable_conditionsByPlatform ?? {}),
+ web: ['browser'],
+ },
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/expo/metro.config.js` around lines 10 - 17, The global resolver
currently sets unstable_conditionNames to include 'browser', which forces
browser-specific condition resolution on all platforms; change the
config.resolver to set unstable_conditionNames to
['require','default','react-native'] and move the 'browser' condition into
unstable_conditionsByPlatform.web (e.g.,
config.resolver.unstable_conditionsByPlatform = {
...(config.resolver?.unstable_conditionsByPlatform ?? {}), web:
[...(config.resolver?.unstable_conditionsByPlatform?.web ?? []), 'browser'] })
so that only web builds resolve the browser export condition while iOS/Android
use the default set; update references around config.resolver and
unstable_conditionNames/unstable_conditionsByPlatform.web accordingly.
Comment on lines
+30
to
+31
| value={selectedValue ?? ''} | ||
| onChange={(e) => onValueChange?.(e.target.value)} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In React DOM, what is the runtime type of e.target.valueforevents, and does return a number or a string?
💡 Result:
In React DOM, the runtime type of e.target.value for events is always a string. This is because React uses native DOM elements, and the HTMLSelectElement.value property returns a string (DOMString), as specified by MDN Web APIs. Even if you set , the browser's value attribute coerces the number to a string "123", so e.target.value is "123" (typeof string), not the number 123.
Citations:
- 1: https://react.dev/reference/react-dom/components/select
- 2: https://developer.mozilla.org/en-US/docs/Web/API/HTMLSelectElement/value
- 3: https://developer.mozilla.org/en-US/docs/Web/API/HTMLSelectElement
- 4: https://stackoverflow.com/questions/33256274/typesafe-select-onchange-event-using-reactjs-and-typescript
- 5: https://stackoverflow.com/questions/72804348/react-selectbox-number-value-is-passed-as-string
- 6: https://stackoverflow.com/questions/41773778/sending-number-instead-of-string-from-select-in-react-component/41773876#41773876
🏁 Script executed:
# Check if the file exists and view its contents
cat -n apps/expo/mocks/react-native-picker.tsxRepository: PackRat-AI/PackRat
Length of output: 1659
Preserve numeric Picker.Item values when forwarding onValueChange.
The <select> element always coerces option values to strings. Currently, passing numeric values will return strings from onChange, breaking the declared (value: string | number) contract and downstream consumers expecting the original type.
💡 Proposed fix
function Picker({ selectedValue, onValueChange, children }: PickerProps) {
const options: ItemProps[] = [];
React.Children.forEach(children, (child) => {
if (React.isValidElement(child) && child.type === PickerItem) {
// safe-cast: child.type === PickerItem guard above ensures props match ItemProps
options.push(child.props as ItemProps);
}
});
+
+ const valueMap = new Map(options.map((opt) => [String(opt.value), opt.value] as const));
return (
<select
- value={selectedValue ?? ''}
- onChange={(e) => onValueChange?.(e.target.value)}
+ value={selectedValue == null ? '' : String(selectedValue)}
+ onChange={(e) => {
+ const raw = e.target.value;
+ onValueChange?.(valueMap.get(raw) ?? raw);
+ }}
style={{
display: 'block',
width: '100%',
padding: '8px',
borderRadius: '6px',
border: '1px solid `#ccc`',
fontSize: '16px',
}}
>📝 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
| value={selectedValue ?? ''} | |
| onChange={(e) => onValueChange?.(e.target.value)} | |
| function Picker({ selectedValue, onValueChange, children }: PickerProps) { | |
| const options: ItemProps[] = []; | |
| React.Children.forEach(children, (child) => { | |
| if (React.isValidElement(child) && child.type === PickerItem) { | |
| // safe-cast: child.type === PickerItem guard above ensures props match ItemProps | |
| options.push(child.props as ItemProps); | |
| } | |
| }); | |
| const valueMap = new Map(options.map((opt) => [String(opt.value), opt.value] as const)); | |
| return ( | |
| <select | |
| value={selectedValue == null ? '' : String(selectedValue)} | |
| onChange={(e) => { | |
| const raw = e.target.value; | |
| onValueChange?.(valueMap.get(raw) ?? raw); | |
| }} | |
| style={{ | |
| display: 'block', | |
| width: '100%', | |
| padding: '8px', | |
| borderRadius: '6px', | |
| border: '1px solid `#ccc`', | |
| fontSize: '16px', | |
| }} | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/expo/mocks/react-native-picker.tsx` around lines 30 - 31, The onChange
handler currently reads e.target.value which is always a string and loses
numeric Picker.Item values; change the rendering of option elements to store the
original value as a JSON-encoded data attribute (e.g.,
data-value=JSON.stringify(itemValue)) and update the onChange in the component
that uses selectedValue and onValueChange to read the selected option's
data-value (e.target.selectedOptions[0].getAttribute('data-value')), JSON.parse
it (with try/catch fallback to the raw string) and pass that parsed value into
onValueChange so numeric values are preserved.
Comment on lines
+519
to
+524
| const [otp] = await db | ||
| .select({ code: oneTimePasswords.code }) | ||
| .from(oneTimePasswords) | ||
| .where(eq(oneTimePasswords.userId, user.id)) | ||
| .orderBy(oneTimePasswords.expiresAt) | ||
| .limit(1); |
Contributor
There was a problem hiding this comment.
Return the newest unexpired OTP here.
orderBy(oneTimePasswords.expiresAt) is ascending, so this route currently picks the oldest row for the user, not the newest one. It also doesn't filter out expired codes, which means repeated auth flows can start retrieving stale OTPs from this helper.
Suggested fix
-import { and, eq, getTableColumns, gt } from 'drizzle-orm';
+import { and, desc, eq, getTableColumns, gt } from 'drizzle-orm';
...
const [otp] = await db
.select({ code: oneTimePasswords.code })
.from(oneTimePasswords)
- .where(eq(oneTimePasswords.userId, user.id))
- .orderBy(oneTimePasswords.expiresAt)
+ .where(
+ and(
+ eq(oneTimePasswords.userId, user.id),
+ gt(oneTimePasswords.expiresAt, new Date()),
+ ),
+ )
+ .orderBy(desc(oneTimePasswords.expiresAt))
.limit(1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/api/src/routes/auth/index.ts` around lines 519 - 524, The query that
fetches otp currently orders by oneTimePasswords.expiresAt ascending and doesn't
exclude expired codes, so change the query in the auth route to filter out
expired OTPs (e.g., add a where clause comparing oneTimePasswords.expiresAt to
the current time) and order by oneTimePasswords.expiresAt descending so it
returns the newest unexpired OTP (keep the existing limit(1) and select of
oneTimePasswords.code to locate the change).
Contributor
Deploying packrat-guides with
|
| Latest commit: |
9a71fe4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a94c8d47.packrat-guides-6gq.pages.dev |
| Branch Preview URL: | https://feat-web-e2e-testids.packrat-guides-6gq.pages.dev |
Contributor
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
packrat-admin | 9a71fe4 | Commit Preview URL Branch Preview URL |
May 14 2026, 11:09 PM |
Contributor
Deploying packrat-landing with
|
| Latest commit: |
9a71fe4
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://bfacee9b.packrat-landing.pages.dev |
| Branch Preview URL: | https://feat-web-e2e-testids.packrat-landing.pages.dev |
andrew-bierman
commented
May 1, 2026
| if (isString(info.item)) { | ||
| return <ListSectionHeader {...info} />; | ||
| } | ||
| const testID = info.item.id === 'name' ? testIds.profile.nameEditBtn : undefined; |
andrew-bierman
commented
May 1, 2026
| inputRef.current?.setNativeProps({ | ||
| selection: { start: 0, end: value?.toString().length }, | ||
| }); | ||
| if (typeof inputRef.current?.setNativeProps === 'function') { |
andrew-bierman
commented
May 1, 2026
|
|
||
| // Check for existing session or skipped login on app load | ||
| useEffect(() => { | ||
| const navigate = (target: Parameters<typeof router.replace>[0]) => { |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 62 out of 63 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (1)
apps/expo/features/packs/components/HorizontalCatalogItemCard.tsx:45
classNameincludes a hard-codedbg-red, which will force these cards to render with a red background regardless of selection state. If this was left over from debugging, remove it; otherwise consider moving the background class entirely into the conditional branch so it matches the intended styling.
testID={`catalog-item-card-${item.id}`}
className={`rounded-lg flex-row gap-3 border p-4 bg-red
${
isSelectable && restProps.selected
? 'border-primary bg-primary/5'
: 'border-border bg-card'
}`}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+30
to
+38
| const navigate = (target: Parameters<typeof router.replace>[0]) => { | ||
| // On web, expo-router's navigationRef may not be ready yet during | ||
| // Strict Mode double-mount — defer by one event loop tick. | ||
| if (Platform.OS === 'web') { | ||
| setTimeout(() => router.replace(target), 0); | ||
| } else { | ||
| router.replace(target); | ||
| } | ||
| }; |
There was a problem hiding this comment.
setTimeout(() => router.replace(...), 0) is used to work around web navigationRef readiness, but the timer isn’t cleared on unmount. In React Strict Mode (double-mount), the first mount can schedule a replace that still fires after the component unmounts, causing unexpected/duplicate navigation. Store the timeout id and clear it in the effect cleanup (or use a mounted flag) for the web path.
|
|
||
| export { ErrorBoundary } from 'expo-router'; | ||
|
|
||
| export let appAlert: React.RefObject<AlertMethods | null>; |
There was a problem hiding this comment.
appAlert is typed as React.RefObject<...> but React isn’t imported, so this will fail typechecking (Cannot find namespace 'React'). Import type React from 'react' or switch the type to import('react').RefObject<...>.
Suggested change
| export let appAlert: React.RefObject<AlertMethods | null>; | |
| export let appAlert: import('react').RefObject<AlertMethods | null>; |
Comment on lines
+39
to
40
| testID={`catalog-item-card-${item.id}`} | ||
| className={`rounded-lg flex-row gap-3 border p-4 bg-red |
There was a problem hiding this comment.
This testID string duplicates the centralized testIds.items.catalogCard(id) helper added in lib/testIds.ts. Using the helper keeps IDs consistent across components/tests and avoids hand-interpolated strings drifting over time.
Comment on lines
+18
to
+22
| const API_URL = process.env.API_URL ?? 'http://localhost:8787'; | ||
| const DB_URL = | ||
| process.env.NEON_DATABASE_URL ?? | ||
| 'postgresql://packrat-neon-db_owner:npg_6YTmFJhaSV8d@ep-billowing-rice-a4p2xhgf-pooler.us-east-1.aws.neon.tech/packrat-neon-db?sslmode=require'; | ||
|
|
There was a problem hiding this comment.
DB_URL has a hard-coded Neon connection string (including credentials) as a fallback when NEON_DATABASE_URL is unset. This leaks secrets into the repo and also makes local runs accidentally hit a remote DB. Remove the hardcoded URL and require NEON_DATABASE_URL (or fail fast with a clear error) for the DB-backed fallback path.
Comment on lines
+519
to
+524
| const [otp] = await db | ||
| .select({ code: oneTimePasswords.code }) | ||
| .from(oneTimePasswords) | ||
| .where(eq(oneTimePasswords.userId, user.id)) | ||
| .orderBy(desc(oneTimePasswords.expiresAt)) | ||
| .limit(1); |
There was a problem hiding this comment.
This query is documented/labelled as returning the most recent OTP, but orderBy(oneTimePasswords.expiresAt) will return the earliest-expiring row first. Order by expiresAt descending (and ideally filter out already-expired OTPs) so the endpoint actually returns the latest code.
| * - ActionSheetProvider (@expo/react-native-action-sheet uses React.Children.only which breaks on web) | ||
| * Metro automatically picks this file over providers/index.tsx for web builds. | ||
| */ | ||
| export function Providers({ children }: { children: React.ReactNode }) { |
There was a problem hiding this comment.
React.ReactNode is used in the props type, but React isn’t imported in this module. With TS jsx: react-jsx, this will error (Cannot find namespace 'React'). Import type React from 'react' or change the annotation to children: import('react').ReactNode / ReactNode.
| import { StatusBar } from 'expo-status-bar'; | ||
| import '../global.css'; | ||
|
|
||
| import { Alert, type AlertMethods } from '@packrat-ai/nativewindui'; |
There was a problem hiding this comment.
This file imports NativeWindUI directly from @packrat-ai/nativewindui, while the rest of the app uses the wrapper re-export @packrat/ui/nativewindui. For consistency (and to keep the option of swapping/re-wrapping the UI lib in one place), prefer importing from @packrat/ui/nativewindui here too.
Suggested change
| import { Alert, type AlertMethods } from '@packrat-ai/nativewindui'; | |
| import { Alert, type AlertMethods } from '@packrat/ui/nativewindui'; |
Comment on lines
+10
to
+13
| * `TestIds` is a backward-compat alias that maps the old PascalCase enum keys | ||
| * to the same underlying strings. Maestro flows reference these values via | ||
| * `id:` selectors — keep the string values stable. | ||
| * |
There was a problem hiding this comment.
The header comment claims there is a backward-compat TestIds alias, but the module only exports testIds. Either add the promised TestIds export (if callers still need it) or update/remove this documentation to avoid misleading usage.
andrew-bierman
force-pushed
the
feat/web-e2e-testids
branch
from
956f15b to
68c4288
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/expo/lib/utils/getRelativeTime.ts (1)
4-10:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winType annotation conflicts with
as constassertion.The explicit
Array<{ key: string; seconds: number }>annotation on line 4 defeats theas constassertion on line 10. The assertion produces a readonly tuple with literal types ("months",2592000, etc.), but the annotation forces the variable to retain mutable, widened types (string,number). This is either a type error under strict checking or undermines the stated goal of "tightening literal type inference."Remove the explicit annotation and let TypeScript infer the const type.
🔧 Proposed fix
-const UNITS: Array<{ key: string; seconds: number }> = [ +const UNITS = [ { key: 'months', seconds: 2592000 }, { key: 'weeks', seconds: 604800 }, { key: 'days', seconds: 86400 }, { key: 'hours', seconds: 3600 }, { key: 'minutes', seconds: 60 }, ] as const;🤖 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/utils/getRelativeTime.ts` around lines 4 - 10, The UNITS constant's explicit type annotation conflicts with the trailing "as const" and prevents literal readonly inference; remove the explicit annotation on the UNITS declaration (leave the initializer with "as const") so TypeScript can infer the readonly tuple and literal "key" and numeric values, and update any downstream code if it relied on widened types to use the inferred literal/readonly types from UNITS.
♻️ Duplicate comments (5)
apps/expo/features/packs/utils/uploadImage.web.ts (2)
57-71:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate fetch response before converting to Blob.
The non-
data:path accepts failed fetches and can upload error-page HTML as image content.🛡️ Suggested fix
// blob: URL or http URL — fetch it const res = await fetch(url); + if (!res.ok) { + throw new Error(`Failed to fetch image source: ${res.status}`); + } return res.blob();🤖 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/packs/utils/uploadImage.web.ts` around lines 57 - 71, The urlToBlob function currently fetches non-data URLs and directly returns res.blob() even on HTTP errors; update urlToBlob to check the fetch response (res.ok or status) after the await fetch(url) and throw or reject with a clear error (including res.status/res.statusText and the URL) when the response is not ok so callers don't receive error-page HTML as an image; ensure the thrown error is informative and references the urlToBlob function and the local variable res so reviewers can find the change.
20-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard against missing user id before generating the remote object key.
If the store is not hydrated/authenticated yet,
userStore.id.peek()can beundefined, producing keys likeundefined-${fileName}that cause bad uploads and hard-to-debug orphaned objects.🛡️ Suggested fix
+ const userId = userStore.id.peek(); + if (!userId) { + throw new Error('Cannot upload image: missing authenticated user id'); + } - const remoteFileName = `${userStore.id.peek()}-${fileName}`; + const remoteFileName = `${userId}-${fileName}`;🤖 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/packs/utils/uploadImage.web.ts` around lines 20 - 24, The code currently builds remoteFileName using userStore.id.peek(), which can be undefined; update the uploadImage flow in uploadImage.web.ts to first read and validate the user id (e.g., const userId = userStore.id.peek()), and if falsy either await store hydration/authentication or throw a clear error (with context like "missing user id for upload") before building remoteFileName and calling getPresignedUrl; ensure remoteFileName construction and the subsequent call to getPresignedUrl(remoteFileName, type) only run when a valid userId exists to avoid creating keys like "undefined-…".apps/expo/features/trips/screens/TripDetailScreen.tsx (1)
89-92:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard
router.back()withcanGoBack()check.After deletion,
router.back()will fail silently if there's no navigation history (e.g., direct link or page refresh). The user would remain on the detail screen of a deleted trip.🔧 Suggested fix
onPress: async () => { await deleteTrip(id as string); - router.back(); + if (router.canGoBack()) { + router.back(); + } else { + router.replace('/(app)/(tabs)/trips'); + } },🤖 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/screens/TripDetailScreen.tsx` around lines 89 - 92, The onPress handler calls deleteTrip(id) then unconditionally router.back(), which can fail or leave the user on a deleted trip if there is no history; update the handler in TripDetailScreen to await deleteTrip(id as string) and then call router.canGoBack() before router.back(), and if canGoBack() is false use a fallback navigation (e.g., router.replace or router.push to the trips list) to ensure the user leaves the deleted trip view; reference the deleteTrip function and router.canGoBack()/router.back() methods when making the change.apps/expo/metro.config.js (1)
10-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope
browserto web-only Metro conditions.The
browsercondition inunstable_conditionNamesis asserted globally across all platforms, causing iOS and Android to incorrectly resolve browser-specific exports. Move it tounstable_conditionsByPlatform.webinstead.Recommended fix
config.resolver = { ...config.resolver, assetExts: [...(config.resolver?.assetExts ?? []), 'wasm'], - unstable_conditionNames: ['require', 'default', 'react-native', 'browser'], + unstable_conditionNames: ['require', 'default', 'react-native'], + unstable_conditionsByPlatform: { + web: ['browser'], + }, };🤖 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/metro.config.js` around lines 10 - 17, The resolver currently sets unstable_conditionNames including 'browser' globally in config.resolver which makes iOS/Android pick browser-only exports; change the config so unstable_conditionNames excludes 'browser' (keep ['require','default','react-native']) and instead set unstable_conditionsByPlatform.web to include ['browser'] so only web Metro uses the browser condition; update references around config.resolver, unstable_conditionNames and add unstable_conditionsByPlatform.web accordingly.apps/expo/playwright/tests/trips.spec.ts (1)
194-206:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWait for delete API response before navigating.
After clicking the delete confirmation button (line 196), the test doesn't wait for the backend mutation to complete. The URL/networkidle wait might not be sufficient if
syncedCrudflushes asynchronously. A race with the subsequentgoto('/trips')could cause intermittent failures.Recommended fix
+ const deletePromise = page.waitForResponse( + (r) => + r.url().includes('/api/trips') && + ['PUT', 'PATCH', 'DELETE'].includes(r.request().method()), + { timeout: 20_000 }, + ); + const deleteConfirmBtn = page.getByText('Delete', { exact: true }); await deleteConfirmBtn.waitFor({ state: 'visible', timeout: 10_000 }); await deleteConfirmBtn.click(); + await deletePromise; // router.back() SPA-navigates away from the trip detail.🤖 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/trips.spec.ts` around lines 194 - 206, The test clicks the deleteConfirmBtn but doesn't wait for the backend delete mutation to finish, causing a race when later calling page.goto(`${BASE_URL}/trips`); fix by waiting for the delete API response (or the GraphQL mutation) that corresponds to the trip deletion before navigating: after triggering deleteConfirmBtn.click(), use page.waitForResponse (or waitForRequest matching the DELETE endpoint or the GraphQL operation name) that checks the request URL/operation includes the trip id and that the response status indicates success, then proceed to page.waitForURL/page.goto and assert tripName is gone.
🤖 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 121-135: The workflow uses mutable tags for the artifact upload
steps ("Upload Playwright report on failure" and "Upload Playwright traces on
failure") via uses: actions/upload-artifact@v7; replace those mutable tags with
the corresponding pinned commit SHAs (e.g.,
actions/upload-artifact@<commit-sha>) so both uses entries reference an exact
commit SHA instead of `@v7`, keeping the step names and inputs (name, path,
retention-days) unchanged.
- Around line 58-80: Replace the mutable action tags with their pinned commit
SHAs to eliminate supply-chain risk: update the uses entries for
actions/checkout (currently "actions/checkout@v6"), oven-sh/setup-bun (currently
"oven-sh/setup-bun@v2"), and actions/cache (currently "actions/cache@v4") to use
the corresponding full commit SHAs instead of the version tags; locate and edit
the uses lines for those identifiers in the workflow, verify each SHA is the
intended release commit for that action, and keep the rest of the step
configuration (inputs, env, run commands) unchanged.
In `@apps/expo/app/_layout.web.tsx`:
- Around line 24-29: appAlert is declared as a mutable let and then reassigned
inside RootLayout on each render; instead create the ref once at module
initialization and stop reassigning it. Replace the top-level declaration with a
stable ref (e.g., export const appAlert = React.createRef<AlertMethods |
null>()), remove the appAlert = useRef... line from RootLayout, and pass that
module-level appAlert into the Alert component so the ref is created once and
not recreated on every render.
In `@apps/expo/app/auth/one-time-password.tsx`:
- Around line 270-274: The code checks inputRef.current?.setNativeProps but then
dereferences inputRef.current again, risking a strict-null-safety race; fix by
caching the ref in a local variable (e.g., const ref = inputRef.current) and
then checking and calling ref.setNativeProps (or use optional chaining on the
call) so the check and call use the same non-null reference — update the block
around inputRef.current.setNativeProps in one-time-password.tsx to use the
cached ref (or optional chaining) to satisfy TypeScript strict null checks.
In `@apps/expo/features/ai/lib/localModelManager.web.ts`:
- Around line 11-13: Update the web stub getLocalModel to match the native
signature: import the LlamaLanguageModel type from '@react-native-ai/llama' and
change the function return type from null to "LlamaLanguageModel | null" while
still returning null (i.e., keep the implementation but adjust the type
declaration for getLocalModel).
In `@apps/expo/features/auth/hooks/useAuthInit.ts`:
- Around line 65-73: The _navigate helper wraps router.replace to defer web
navigation but is never used; replace all direct router.replace(...) calls in
this module with _navigate(...) so navigation benefits from the web Strict Mode
workaround—locate occurrences of router.replace inside useAuthInit (the calls
mentioned around the auth flow) and change them to _navigate with the same
arguments so behavior is identical but deferred on Platform.OS === 'web'.
In `@apps/expo/mocks/expo-sqlite-kv-store.ts`:
- Around line 8-9: The current rawGet function now reads
localStorage.getItem(key) directly and will miss legacy keys prefixed with
'__kv__' causing data loss; add a one-time migration helper (e.g.,
migrateLegacyKeys or migrateOldPrefixedKeys) that runs on first module/init
load, iterates localStorage keys for entries starting with '__kv__', and copies
each value to the unprefixed key (and optionally removes the old prefixed key),
then call this migration before any use of rawGet/memFallback so existing
'__kv__...' entries are preserved for functions like rawGet.
- Around line 23-26: rawKeys currently returns Object.keys(window.localStorage)
which exposes all origins' keys and can cause namespace collisions; update
rawKeys (and related logic using isClient and memFallback) to only return keys
that belong to this store by filtering window.localStorage keys by the store's
prefix (e.g., KEY_PREFIX or the existing prefix constant used elsewhere in this
module) and strip that prefix when returning, or if deliberate, add a clear
comment/docstring in the file stating that this KV store intentionally owns the
entire localStorage namespace; make sure to reference and use the exact prefix
symbol used in this module (e.g., KEY_PREFIX) when implementing the filter.
In `@apps/expo/package.json`:
- Around line 38-39: The package.json scripts "test:web" and "test:web:ui" call
"playwright test" but `@playwright/test` is missing from devDependencies; add
"@playwright/test" to devDependencies in apps/expo/package.json (or run the
equivalent install command) so the scripts can run, ensuring the dependency
version aligns with your monorepo policy and updating lockfile if present.
In `@apps/expo/playwright/tests/core.spec.ts`:
- Line 252: The test uses a brittle icon-based selector
page.getByText('').click();—replace it with a stable accessible selector:
either update the AI chat send button component to include a testID/data-testid
and select it with page.getByTestId('send-button').click(), or use an ARIA role
selector like page.getByRole('button', { name: /send/i }).click(); modify the
test to target the new selector and ensure the AI chat send button component
(the button element) exposes the matching testId or accessible label.
In `@apps/expo/playwright/tests/fixtures.ts`:
- Around line 1-8: The test fixtures import from '@playwright/test' (see the
imports for Browser, BrowserContext, Page, and test), but the package is missing
from devDependencies; add "@playwright/test" to the devDependencies in the
package.json for the Expo app (pick a compatible version with your project,
e.g., the latest stable), then run your package manager install (npm/yarn/pnpm)
to update node_modules so the fixtures can import test, Browser, BrowserContext,
and Page successfully.
In `@apps/expo/playwright/tests/globalSetup.ts`:
- Around line 19-21: The DB_URL constant currently contains a hardcoded
connection string with embedded credentials; remove that plaintext fallback and
require the environment variable NEON_DATABASE_URL to be present (or replace the
fallback with a clearly invalid placeholder like "NEON_DATABASE_URL_NOT_SET") so
credentials are not committed; update the globalSetup logic that references
DB_URL to throw a clear error or exit early if NEON_DATABASE_URL is missing so
tests fail fast and developers must provide a proper secret.
In `@apps/expo/playwright/tests/packs.spec.ts`:
- Line 182: The test currently swallows API errors by using await
editPromise.catch(() => null) which can mask failed PUT/PATCH mutations; locate
the editPromise usage in the test (the variable named editPromise) and either
remove the .catch so the test fails on rejection or replace it with an explicit
assertion that the mutation succeeded (e.g., await the response and assert
status/ok), ensuring any failure from editPromise surfaces and prevents the test
from continuing to check the UI.
In `@package.json`:
- Line 62: Update the `@playwright/test` dependency entry in package.json from
"^1.59.1" to the newer stable range "^1.60.0"; locate the dependency key
"@playwright/test" and change its version string to "^1.60.0", then run your
package manager (npm install or yarn install) and update any lockfile to ensure
the new version is installed and CI uses it.
---
Outside diff comments:
In `@apps/expo/lib/utils/getRelativeTime.ts`:
- Around line 4-10: The UNITS constant's explicit type annotation conflicts with
the trailing "as const" and prevents literal readonly inference; remove the
explicit annotation on the UNITS declaration (leave the initializer with "as
const") so TypeScript can infer the readonly tuple and literal "key" and numeric
values, and update any downstream code if it relied on widened types to use the
inferred literal/readonly types from UNITS.
---
Duplicate comments:
In `@apps/expo/features/packs/utils/uploadImage.web.ts`:
- Around line 57-71: The urlToBlob function currently fetches non-data URLs and
directly returns res.blob() even on HTTP errors; update urlToBlob to check the
fetch response (res.ok or status) after the await fetch(url) and throw or reject
with a clear error (including res.status/res.statusText and the URL) when the
response is not ok so callers don't receive error-page HTML as an image; ensure
the thrown error is informative and references the urlToBlob function and the
local variable res so reviewers can find the change.
- Around line 20-24: The code currently builds remoteFileName using
userStore.id.peek(), which can be undefined; update the uploadImage flow in
uploadImage.web.ts to first read and validate the user id (e.g., const userId =
userStore.id.peek()), and if falsy either await store hydration/authentication
or throw a clear error (with context like "missing user id for upload") before
building remoteFileName and calling getPresignedUrl; ensure remoteFileName
construction and the subsequent call to getPresignedUrl(remoteFileName, type)
only run when a valid userId exists to avoid creating keys like "undefined-…".
In `@apps/expo/features/trips/screens/TripDetailScreen.tsx`:
- Around line 89-92: The onPress handler calls deleteTrip(id) then
unconditionally router.back(), which can fail or leave the user on a deleted
trip if there is no history; update the handler in TripDetailScreen to await
deleteTrip(id as string) and then call router.canGoBack() before router.back(),
and if canGoBack() is false use a fallback navigation (e.g., router.replace or
router.push to the trips list) to ensure the user leaves the deleted trip view;
reference the deleteTrip function and router.canGoBack()/router.back() methods
when making the change.
In `@apps/expo/metro.config.js`:
- Around line 10-17: The resolver currently sets unstable_conditionNames
including 'browser' globally in config.resolver which makes iOS/Android pick
browser-only exports; change the config so unstable_conditionNames excludes
'browser' (keep ['require','default','react-native']) and instead set
unstable_conditionsByPlatform.web to include ['browser'] so only web Metro uses
the browser condition; update references around config.resolver,
unstable_conditionNames and add unstable_conditionsByPlatform.web accordingly.
In `@apps/expo/playwright/tests/trips.spec.ts`:
- Around line 194-206: The test clicks the deleteConfirmBtn but doesn't wait for
the backend delete mutation to finish, causing a race when later calling
page.goto(`${BASE_URL}/trips`); fix by waiting for the delete API response (or
the GraphQL mutation) that corresponds to the trip deletion before navigating:
after triggering deleteConfirmBtn.click(), use page.waitForResponse (or
waitForRequest matching the DELETE endpoint or the GraphQL operation name) that
checks the request URL/operation includes the trip id and that the response
status indicates success, then proceed to page.waitForURL/page.goto and assert
tripName is gone.
🪄 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: dc03017e-69d6-4612-a494-b32fc67a7a0b
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock,!bun.lock
📒 Files selected for processing (46)
.github/workflows/web-e2e-tests.ymlapps/expo/.gitignoreapps/expo/app/(app)/(tabs)/_layout.web.tsxapps/expo/app/(app)/(tabs)/profile/index.tsxapps/expo/app/(app)/_layout.tsxapps/expo/app/_layout.tsxapps/expo/app/_layout.web.tsxapps/expo/app/auth/(login)/index.tsxapps/expo/app/auth/one-time-password.tsxapps/expo/atoms/atomWithSecureStorage.web.tsapps/expo/features/ai/lib/localModelManager.web.tsapps/expo/features/auth/hooks/useAuthActions.tsapps/expo/features/auth/hooks/useAuthInit.tsapps/expo/features/packs/components/HorizontalCatalogItemCard.tsxapps/expo/features/packs/hooks/usePackOwnershipCheck.tsapps/expo/features/packs/utils/uploadImage.web.tsapps/expo/features/trips/components/TripForm.tsxapps/expo/features/trips/screens/TripDetailScreen.tsxapps/expo/lib/Picker.tsxapps/expo/lib/Picker.web.tsxapps/expo/lib/appleAuthentication.tsapps/expo/lib/appleAuthentication.web.tsapps/expo/lib/constants.web.tsapps/expo/lib/devClient.tsapps/expo/lib/devClient.web.tsapps/expo/lib/hooks/useColorScheme.web.tsxapps/expo/lib/updates.tsapps/expo/lib/updates.web.tsapps/expo/lib/utils/ImageCacheManager.web.tsapps/expo/lib/utils/getRelativeTime.tsapps/expo/metro.config.jsapps/expo/mocks/expo-sqlite-kv-store.tsapps/expo/mocks/react-native-community-datetimepicker.tsxapps/expo/mocks/react-native-google-signin.tsapps/expo/mocks/react-native-keyboard-controller.tsxapps/expo/package.jsonapps/expo/playwright/playwright.config.tsapps/expo/playwright/tests/core.spec.tsapps/expo/playwright/tests/fixtures.tsapps/expo/playwright/tests/globalSetup.tsapps/expo/playwright/tests/packs.spec.tsapps/expo/playwright/tests/profile.spec.tsapps/expo/playwright/tests/trips.spec.tsapps/expo/providers/index.web.tsxapps/expo/vitest.config.tspackage.json
Comment on lines
+58
to
+80
| uses: actions/checkout@v6 | ||
|
|
||
| - name: Setup Bun | ||
| uses: oven-sh/setup-bun@v2 | ||
| with: | ||
| bun-version: latest | ||
| cache: true | ||
|
|
||
| - name: Cache node_modules | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: node_modules | ||
| key: node-modules-${{ runner.os }}-${{ hashFiles('bun.lock') }} | ||
| restore-keys: | | ||
| node-modules-${{ runner.os }}- | ||
|
|
||
| - name: Install dependencies | ||
| env: | ||
| PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN: ${{ secrets.PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN }} | ||
| run: bun install --frozen-lockfile | ||
|
|
||
| - name: Install Playwright browsers | ||
| run: bunx playwright install chromium --with-deps |
Contributor
There was a problem hiding this comment.
Pin third-party GitHub Actions to commit SHAs.
Third-party actions are using mutable tags (@v6, @v2, @v4) instead of pinned commit SHAs. This creates supply-chain attack risk.
As per coding guidelines: "Pin third-party actions to a full commit SHA (not a mutable tag) to prevent supply-chain attacks."
Pin these actions to specific commit SHAs:
- Line 58:
actions/checkout@v6 - Line 61:
oven-sh/setup-bun@v2 - Line 67:
actions/cache@v4
🤖 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 58 - 80, Replace the
mutable action tags with their pinned commit SHAs to eliminate supply-chain
risk: update the uses entries for actions/checkout (currently
"actions/checkout@v6"), oven-sh/setup-bun (currently "oven-sh/setup-bun@v2"),
and actions/cache (currently "actions/cache@v4") to use the corresponding full
commit SHAs instead of the version tags; locate and edit the uses lines for
those identifiers in the workflow, verify each SHA is the intended release
commit for that action, and keep the rest of the step configuration (inputs,
env, run commands) unchanged.
Comment on lines
+121
to
+135
| - name: Upload Playwright report on failure | ||
| if: failure() | ||
| uses: actions/upload-artifact@v7 | ||
| with: | ||
| name: playwright-report | ||
| path: apps/expo/playwright-report/ | ||
| retention-days: 7 | ||
|
|
||
| - name: Upload Playwright traces on failure | ||
| if: failure() | ||
| uses: actions/upload-artifact@v7 | ||
| with: | ||
| name: playwright-traces | ||
| path: apps/expo/test-results/ | ||
| retention-days: 7 |
Contributor
There was a problem hiding this comment.
Pin artifact upload actions to commit SHAs.
The actions/upload-artifact@v7 actions (lines 123, 131) use mutable tags. Pin them to commit SHAs per the security guideline.
🤖 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 121 - 135, The workflow
uses mutable tags for the artifact upload steps ("Upload Playwright report on
failure" and "Upload Playwright traces on failure") via uses:
actions/upload-artifact@v7; replace those mutable tags with the corresponding
pinned commit SHAs (e.g., actions/upload-artifact@<commit-sha>) so both uses
entries reference an exact commit SHA instead of `@v7`, keeping the step names and
inputs (name, path, retention-days) unchanged.
Comment on lines
+24
to
+29
| export let appAlert: React.RefObject<AlertMethods | null>; | ||
|
|
||
| function RootLayout() { | ||
| useInitialAndroidBarSync(); | ||
|
|
||
| appAlert = useRef<AlertMethods>(null); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Module-level mutable export is reassigned on every render.
appAlert is exported as a mutable let, then reassigned inside RootLayout on every render (line 29). This works but is unconventional. Consider initializing the ref once at module level:
♻️ Cleaner pattern
-export let appAlert: React.RefObject<AlertMethods | null>;
+export const appAlert = React.createRef<AlertMethods>();
function RootLayout() {
useInitialAndroidBarSync();
- appAlert = useRef<AlertMethods>(null);Then pass appAlert directly to the Alert component.
🤖 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/_layout.web.tsx` around lines 24 - 29, appAlert is declared as
a mutable let and then reassigned inside RootLayout on each render; instead
create the ref once at module initialization and stop reassigning it. Replace
the top-level declaration with a stable ref (e.g., export const appAlert =
React.createRef<AlertMethods | null>()), remove the appAlert = useRef... line
from RootLayout, and pass that module-level appAlert into the Alert component so
the ref is created once and not recreated on every render.
Comment on lines
+11
to
+13
| export function getLocalModel(): null { | ||
| return null; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find and compare native localModelManager implementation
# Search for the native implementation file
fd -e ts -e tsx 'localModelManager' apps/expo/features/ai/lib/ --exec cat {}Repository: PackRat-AI/PackRat
Length of output: 9743
Fix getLocalModel() return type to match native implementation.
The web stub declares null as the return type, but the native implementation returns LlamaLanguageModel | null. Change the web stub to:
import { type LlamaLanguageModel } from '@react-native-ai/llama';
export function getLocalModel(): LlamaLanguageModel | null {
return null;
}This ensures type safety across platform-specific builds—consuming code will expect the same type on web and native.
🤖 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/ai/lib/localModelManager.web.ts` around lines 11 - 13,
Update the web stub getLocalModel to match the native signature: import the
LlamaLanguageModel type from '@react-native-ai/llama' and change the function
return type from null to "LlamaLanguageModel | null" while still returning null
(i.e., keep the implementation but adjust the type declaration for
getLocalModel).
| // Send a message | ||
| await page.getByRole('textbox', { name: /Ask about this pack/i }).fill('List 3 essential items.'); | ||
| // Send button is icon-only with no accessible name; use the arrow-up icon character | ||
| await page.getByText('').click(); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Icon-based selector is brittle.
Using the Unicode arrow-up icon character '' directly will break if the icon font changes. Consider adding a testID to the send button in the AI chat component, or use a more stable selector like getByRole('button', { name: /send/i }) with an accessible label.
🤖 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` at line 252, The test uses a brittle
icon-based selector page.getByText('').click();—replace it with a stable
accessible selector: either update the AI chat send button component to include
a testID/data-testid and select it with page.getByTestId('send-button').click(),
or use an ARIA role selector like page.getByRole('button', { name: /send/i
}).click(); modify the test to target the new selector and ensure the AI chat
send button component (the button element) exposes the matching testId or
accessible label.
| ); | ||
|
|
||
| await page.getByTestId('items:submit').click(); | ||
| await editPromise.catch(() => null); |
Contributor
There was a problem hiding this comment.
Don't silently swallow edit mutation errors.
await editPromise.catch(() => null) suppresses any API errors from the PUT/PATCH request. If the mutation fails, the test continues and might incorrectly pass when checking for the updated name in the UI. Either let the promise reject (remove the .catch), or explicitly validate the response was successful.
Recommended fix
- await editPromise.catch(() => null);
+ const editResponse = await editPromise;
+ expect(editResponse.ok()).toBeTruthy();📝 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 editPromise.catch(() => null); | |
| const editResponse = await editPromise; | |
| expect(editResponse.ok()).toBeTruthy(); |
🤖 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/packs.spec.ts` at line 182, The test currently
swallows API errors by using await editPromise.catch(() => null) which can mask
failed PUT/PATCH mutations; locate the editPromise usage in the test (the
variable named editPromise) and either remove the .catch so the test fails on
rejection or replace it with an explicit assertion that the mutation succeeded
(e.g., await the response and assert status/ok), ensuring any failure from
editPromise surfaces and prevents the test from continuing to check the UI.
| "devDependencies": { | ||
| "@biomejs/biome": "2.4.6", | ||
| "@manypkg/cli": "^0.24.0", | ||
| "@playwright/test": "^1.59.1", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the latest version of `@playwright/test` on npm
npm view `@playwright/test` version
npm view `@playwright/test` versions --json | jq -r '.[-5:]'Repository: PackRat-AI/PackRat
Length of output: 217
🌐 Web query:
What is the latest stable version of @playwright/test as of May 2026?
💡 Result:
Latest stable version of @playwright/test as of 2026-05-12: 1.60.0 [1][2]. This is reflected as the current package “Viewing @playwright/test version 1.60.0” on npm’s package page [1], and the Playwright release notes list “Version 1.60” (matching the release train) [2].
Citations:
Update to the latest stable version of @playwright/test.
The version ^1.59.1 is valid, but version 1.60.0 is now the latest stable release. Consider updating to ^1.60.0 to ensure access to recent fixes and features.
🤖 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 `@package.json` at line 62, Update the `@playwright/test` dependency entry in
package.json from "^1.59.1" to the newer stable range "^1.60.0"; locate the
dependency key "@playwright/test" and change its version string to "^1.60.0",
then run your package manager (npm install or yarn install) and update any
lockfile to ensure the new version is installed and CI uses it.
- Remove unused userStore import from _layout.web.tsx (biome) - Reformat uploadImage.web.ts signature (biome) - Exclude **/*.web.ts from vitest coverage: browser-API files can't run in Node environment and were pulling statements below 75%
…e-auth stubs Add metro WEB_STUBS for @react-native-google-signin/google-signin and expo-apple-authentication so the native useAuthActions.ts works on web without a separate .web.ts variant. The stubs return false/reject for sign-in flows that aren't available on web. Also commit previously-untracked files that the E2E tests and web build depend on: playwright.config.ts, globalSetup.ts, packrat.web.ts (Treaty client), and the bottom-sheet/datetimepicker/keyboard-controller mocks. Add e2e-web/.auth-tokens.json to .gitignore (contains real credentials).
…ts variants authAtoms.ts: - Extract shared kvStorage adapter (dedup inline objects) - Add getOnInit: true so the stored token is read eagerly on app start (prevents null-auth flash on web where kv-store stub hits localStorage) - With kv-store stub storing/reading raw strings, no JSON-parse layer is needed — authAtoms.web.ts resilientTokenStorage is obsolete Delete authAtoms.web.ts: native file + kv-store localStorage stub is now equivalent. Both store raw JWT strings; no JSON-serialization mismatch. Delete packrat.web.ts: packrat.ts uses Storage.getItem which on web goes through the kv-store stub → localStorage (raw string). No .web.ts needed.
…tPlugin
Create lib/storage/persistPlugin.ts (native: observablePersistSqlite) and
lib/storage/persistPlugin.web.ts (web: ObservablePersistLocalStorage) so
all LegendState stores get the right persistence plugin without per-store
platform variants. The kv-store stub + apiClient (Treaty) already work on
web, so no other differences remain in the store layer.
Delete 12 .web.ts files:
- 8 feature store variants (trips, packs, packItems, packWeightHistory,
packingMode, packTemplates, packTemplateItems, trailConditionReports)
- user.web.ts (renamed persist key to 'packrat_user' in native file to
match E2E fixture seeding and existing localStorage key)
- useAuthInit.web.ts (GoogleSignin now stubbed; add Platform.OS==='web'
guard for the setTimeout(0) navigation defer needed in Strict Mode)
- atomWithKvStorage.web.ts (kv-store stub makes native async path work)
- client.web.ts (no remaining consumers; uploadImage.web.ts migrated to
apiClient.upload.presigned.get() same as the native file)
- tsconfig.json: change ignoreDeprecations from "6.0" to "5.0" (valid TS 5.x value; "6.0" caused TS5103 which aborted type-checking entirely) - getRelativeTime.ts: import TFunction from i18next and mark UNITS as const so template-literal keys are typed narrowly enough to satisfy the typed t() overloads - react-native-maps.ts: type style prop as StyleProp<ViewStyle> instead of unknown - gorhom-bottom-sheet.tsx: delete mock — @gorhom/bottom-sheet v5 has native web support; remove from WEB_STUBS - user.ts: revert persist name back to 'user' (was 'packrat_user') to avoid renaming the native SQLite table for existing users - e2e-web/tests/fixtures.ts: match localStorage key to 'user' to stay in sync with persist name
Builds the Expo web app (expo export -p web), serves it on port 8081 with SPA fallback routing, then runs the existing Playwright suite. Requires these repo secrets already used by the iOS E2E job: PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN, NEON_DEV_DATABASE_URL, EXPO_PUBLIC_API_URL, EXPO_PUBLIC_GOOGLE_WEB_CLIENT_ID, EXPO_PUBLIC_R2_PUBLIC_URL, EXPO_PUBLIC_SENTRY_DSN, EXPO_PUBLIC_GOOGLE_MAPS_API_KEY
globalSetup: add priority-2 login path — when TEST_EMAIL + TEST_PASSWORD are set, log in against the API instead of registering an ephemeral user. This mirrors how the iOS/Android Maestro jobs work: seed the user in the dev DB, then authenticate with known credentials. workflow: add secrets verification step, seed E2E user via db:seed:e2e-user (same as other e2e jobs), use E2E_TEST_EMAIL / E2E_TEST_PASSWORD secrets, add EXPO_PUBLIC_API_URL secret check.
These were superseded by lib/persist-plugin.ts and lib/persist-plugin.web.ts from the development branch merge. No remaining imports reference these paths.
Makes the toolchain explicit. Updates package.json scripts, .gitignore, and the check-type-casts/no-raw-process-env allowlists to match.
Move expo-updates, expo-apple-authentication, expo-dev-client, and @react-native-picker/picker out of metro.config.js WEB_STUBS and into lib/*.ts + lib/*.web.ts pairs. Consumers import from expo-app/lib/* so Metro never needs to intercept these packages on web. Only legitimate Metro stubs remain: packages with no .web.ts variant and no opportunity to create one (async-storage, kv-store, etc).
andrew-bierman
force-pushed
the
feat/web-e2e-testids
branch
from
e4f65b8 to
9a71fe4
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (9)
apps/expo/lib/Picker.web.tsx (1)
10-10:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve original
Picker.Itemvalue types inonValueChange.
<select>always returns a string, so numeric item values are currently coerced and break the
string | numbervalue contract implied byItemProps.Proposed fix
type PickerProps = { selectedValue?: string | number | null; - onValueChange?: (value: string) => void; + onValueChange?: (value: string | number) => void; children?: React.ReactNode; style?: React.CSSProperties; }; @@ function Picker({ selectedValue, onValueChange, children }: PickerProps) { const options: ItemProps[] = []; React.Children.forEach(children, (child) => { if (React.isValidElement<ItemProps>(child) && child.type === PickerItem) { options.push(child.props); } }); + const valueMap = new Map(options.map((opt) => [String(opt.value), opt.value] as const)); return ( <select - value={selectedValue ?? ''} - onChange={(e) => onValueChange?.(e.target.value)} + value={selectedValue == null ? '' : String(selectedValue)} + onChange={(e) => { + const raw = e.target.value; + onValueChange?.(valueMap.get(raw) ?? raw); + }} style={{ display: 'block', width: '100%',In React DOM, does HTMLSelectElement.value always return a string even when <option value={123}> is used?Also applies to: 29-31, 41-41
🤖 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/Picker.web.tsx` at line 10, The onValueChange signature currently receives a string because HTMLSelectElement.value is always a string; update the change handler in Picker.web.tsx (the function bound to onChange for the <select>, and the prop onValueChange and Picker.Item/ItemProps handling) to restore the original ItemProps value type: when rendering options, keep a mapping (e.g., an array or Map keyed by option value or index) from option index to the original item.value (string | number) and in the select onChange use event.target.selectedIndex (or the option key) to look up the original value and pass that original typed value to onValueChange instead of the raw string; ensure the mapping is accessible where options are rendered and cleaned up appropriately.apps/expo/features/ai/lib/localModelManager.web.ts (1)
11-13:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix getLocalModel() return type to match native implementation.
The web stub declares
nullas the return type, but the native implementation returnsLlamaLanguageModel | null. Consuming code expects the same type on both platforms.🔧 Proposed fix
+import { type LlamaLanguageModel } from '@react-native-ai/llama'; + -export function getLocalModel(): null { +export function getLocalModel(): LlamaLanguageModel | null { return null; }As per coding guidelines, use proper TypeScript types to ensure type safety across platform-specific builds.
🤖 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/ai/lib/localModelManager.web.ts` around lines 11 - 13, The return type of the web stub getLocalModel is incorrect; change its signature to return LlamaLanguageModel | null (matching the native implementation) while keeping the body returning null for the web platform; update the function declaration export function getLocalModel(): LlamaLanguageModel | null and ensure LlamaLanguageModel is imported or referenced where needed so the type resolves.apps/expo/app/auth/one-time-password.tsx (1)
268-274:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix strict null check violation in onFocus handler.
Line 269 checks
inputRef.current?.setNativePropsbut line 270 dereferencesinputRef.currentagain without optional chaining. With strict null checks enabled, TypeScript cannot guarantee the ref didn't become null between the check and the call.🔒 Proposed fix for strict null safety
function onFocus(_e: NativeSyntheticEvent<TargetedEvent>) { - if (typeof inputRef.current?.setNativeProps === 'function') { - inputRef.current.setNativeProps({ - selection: { start: 0, end: value?.toString().length }, - }); - } + const current = inputRef.current; + if (current && typeof current.setNativeProps === 'function') { + current.setNativeProps({ + selection: { start: 0, end: value?.toString().length }, + }); + } }As per coding guidelines, ensure strict null checks are enabled and use no unchecked indexed access.
🤖 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/auth/one-time-password.tsx` around lines 268 - 274, The onFocus handler performs a nullable check on inputRef.current?.setNativeProps but then re-reads inputRef.current without guaranteeing it’s unchanged; to satisfy strictNullChecks, capture the ref into a local const (e.g., const ref = inputRef.current) and test ref && typeof ref.setNativeProps === 'function' before calling ref.setNativeProps({ selection: { start: 0, end: value?.toString().length } }); this ensures onFocus, inputRef, and setNativeProps are non-null when invoked and avoids unchecked access.apps/expo/package.json (1)
38-39:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd
@playwright/testto devDependencies.The scripts invoke
playwright test, but@playwright/testis missing fromdevDependencies. Install it so the scripts can run.#!/bin/bash # Verify `@playwright/test` is missing jq '.devDependencies["@playwright/test"]' apps/expo/package.json🤖 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/package.json` around lines 38 - 39, The package.json devDependencies is missing `@playwright/test` while the scripts "test:web" and "test:web:ui" call `playwright test`; add "@playwright/test" to devDependencies and install it (e.g., using npm/yarn/pnpm) so those scripts can run; update the apps/expo package.json devDependencies to include the appropriate version of "@playwright/test" and run the project package manager install command to persist the change.apps/expo/playwright/tests/globalSetup.ts (1)
19-22:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequire
NEON_DATABASE_URLinstead of using a placeholder fallback.If the env var is missing,
neon(DB_URL)fails later with a less clear error path. Fail immediately with a clear message.Suggested fix
const DB_URL = process.env.NEON_DATABASE_URL ?? - '***REDACTED_DB_URL***'; + (() => { + throw new Error('NEON_DATABASE_URL is required for Playwright global setup'); + })();🤖 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 19 - 22, Replace the silent placeholder fallback for DB_URL with an explicit required check: read process.env.NEON_DATABASE_URL into the DB_URL constant and if it's undefined or empty throw an Error with a clear message (e.g., "NEON_DATABASE_URL is required for tests") so the failure happens immediately; update any usage sites such as neon(DB_URL) to rely on this guaranteed non-empty DB_URL.apps/expo/playwright/tests/packs.spec.ts (1)
181-183:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not swallow edit mutation failures.
Line 182 suppresses request failures, so this test can continue after a broken update path.
Suggested fix
- await editPromise.catch(() => null); + const editResponse = await editPromise; + expect(editResponse.ok()).toBeTruthy();🤖 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/packs.spec.ts` around lines 181 - 183, The test is swallowing edit mutation failures by calling editPromise.catch(() => null) after triggering the submit click (page.getByTestId('items:submit').click()); remove the silent catch and let the promise rejection surface so the test fails (i.e., await editPromise without .catch), or if you need custom failure messaging, replace the catch with a handler that rethrows or calls test.fail/assert with the caught error so failures are not suppressed.apps/expo/features/trips/screens/TripDetailScreen.tsx (1)
89-92:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard back navigation after delete.
Line 91 calls
router.back()unconditionally. With direct entry/reload there may be no history, so delete can succeed but navigation silently fails.Suggested fix
onPress: async () => { await deleteTrip(id as string); - router.back(); + if (router.canGoBack()) { + router.back(); + } else { + router.replace('/trips'); + } },🤖 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/screens/TripDetailScreen.tsx` around lines 89 - 92, The onPress handler currently calls router.back() unconditionally which can fail if there is no history; update the handler that calls deleteTrip(id) (the async callback) to await deleteTrip as now, then check navigation availability (e.g. use router.canGoBack() or equivalent) and call router.back() only if true, otherwise navigate to a safe fallback route using router.replace or router.push (for example the trips list) so the UI always transitions after a successful delete.apps/expo/playwright/tests/trips.spec.ts (1)
194-201:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWait for the delete mutation before asserting post-delete navigation/state.
After clicking confirm, the test does not wait for the backend mutation, so list assertions can race the write.
Suggested fix
+ const deletePromise = page.waitForResponse( + (r) => + r.url().includes('/api/trips') && + ['PUT', 'PATCH', 'DELETE'].includes(r.request().method()), + { timeout: 20_000 }, + ); + const deleteConfirmBtn = page.getByText('Delete', { exact: true }); await deleteConfirmBtn.waitFor({ state: 'visible', timeout: 10_000 }); await deleteConfirmBtn.click(); + const deleteResponse = await deletePromise; + expect(deleteResponse.ok()).toBeTruthy(); // router.back() SPA-navigates away from the trip detail.🤖 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/trips.spec.ts` around lines 194 - 201, After clicking the confirm button (deleteConfirmBtn.click()), wait explicitly for the backend delete mutation to complete before asserting navigation or list state: use a network waiter such as page.waitForResponse(...) or page.waitForRequestFinished that matches the delete endpoint or GraphQL mutation name (e.g., "deleteTrip" or the API route your app calls) and only after that call resolves proceed to page.waitForURL(...) and page.waitForLoadState('networkidle'); update the test to await that response between deleteConfirmBtn.click() and the subsequent waitForURL call so assertions no longer race the write..github/workflows/web-e2e-tests.yml (1)
58-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin third-party actions to full commit SHAs.
These
uses:entries are still mutable tags and should be pinned to immutable SHAs:
actions/checkout@v6oven-sh/setup-bun@v2actions/cache@v4actions/upload-artifact@v7(both occurrences)As per coding guidelines, "Pin third-party actions to a full commit SHA (not a mutable tag) to prevent supply-chain attacks."
Also applies to: 123-131
🤖 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 58 - 68, The workflow uses mutable action tags — update every `uses:` reference to pin to the full commit SHA instead of the floating tags; specifically replace `actions/checkout@v6`, `oven-sh/setup-bun@v2`, `actions/cache@v4`, and both occurrences of `actions/upload-artifact@v7` with their corresponding full commit SHAs (retrieve the canonical SHA from the action repo and substitute it), ensuring each `uses:` entry references the full 40-character commit SHA.
🤖 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 104-110: The "Wait for web server" step's retry loop never returns
non-zero on ultimate failure; update that step so it fails fast when the server
never becomes reachable by detecting the loop's unsuccessful completion and
calling exit 1 (or use a curl check that short-circuits with || exit 1). Locate
the shell loop in the "Wait for web server" step and either add a success flag
and exit 1 if not set after the for loop, or replace the final check with a
conditional that exits non-zero when curl never succeeds.
In `@apps/expo/mocks/react-native-community-datetimepicker.tsx`:
- Around line 40-45: The input is currently uncontrolled because it uses
defaultValue; change it to a controlled input by replacing defaultValue with a
value prop so updates to the value prop re-render the picker; use value={value
!= null ? toInputValue(value, mode) : ''} (or equivalent) and keep min/max using
toInputValue(minimumDate/maximumDate, mode) as before, ensuring handleChange
still updates the parent state so the controlled value stays in sync (refer to
the input element, toInputValue, handleChange, value, minimumDate and
maximumDate).
In `@apps/expo/mocks/react-native-google-signin.ts`:
- Line 3: The mock currently has hasPlayServices returning Promise.resolve(true)
which is incorrect for web; update the hasPlayServices implementation in
react-native-google-signin.ts to return false on web (e.g., return
Promise.resolve(false) in this web mock) or, if this file is shared across
platforms, guard it with a Platform check (Platform.OS === 'web' ?
Promise.resolve(false) : Promise.resolve(true/actualCheck)). Ensure you edit the
hasPlayServices function to reflect the correct web behavior.
In `@apps/expo/playwright/tests/core.spec.ts`:
- Around line 94-104: The test parses packResponse.json() without first
asserting the HTTP success, which causes JSON parse errors when the POST fails;
update the code around the Promise.all block that produces packResponse (and the
similar block at lines 231-240) to first check packResponse.ok (or
packResponse.status) and throw or assert with a clear message containing status
and statusText if not OK, then call packResponse.json() to extract { id: packId
}; do the same for the other response variable so failures produce clear API
error messages instead of parse errors.
In `@apps/expo/playwright/tests/globalSetup.ts`:
- Around line 27-41: The current globalSetup block writes pre-minted tokens even
when the /api/auth/me call fails; change the logic in the conditional that
checks process.env.TEST_ACCESS_TOKEN and process.env.TEST_REFRESH_TOKEN so that
you first verify meRes.ok is true and the user payload exists before calling
fs.writeFileSync(TOKENS_FILE, ...) and returning; if meRes.ok is false or user
is null/invalid, do not write the tokens or return—let the normal auth flow
continue (leave API_URL, TOKENS_FILE, and the fetch to /api/auth/me intact, but
gate the write/return on a successful meRes and valid user).
In `@packages/api/src/routes/trips/index.ts`:
- Line 166: The current check uses "'deleted' in data" which is true even when
data.deleted is undefined; change the guard to only assign when the value is
actually provided (e.g., use an explicit undefined check or nullish coalescing):
replace the "'deleted' in data" condition with "data.deleted !== undefined" (or
similar) so that updateData.deleted is assigned only when data.deleted has a
real value; update the assignment site referencing updateData and data in the
same block.
---
Duplicate comments:
In @.github/workflows/web-e2e-tests.yml:
- Around line 58-68: The workflow uses mutable action tags — update every
`uses:` reference to pin to the full commit SHA instead of the floating tags;
specifically replace `actions/checkout@v6`, `oven-sh/setup-bun@v2`,
`actions/cache@v4`, and both occurrences of `actions/upload-artifact@v7` with
their corresponding full commit SHAs (retrieve the canonical SHA from the action
repo and substitute it), ensuring each `uses:` entry references the full
40-character commit SHA.
In `@apps/expo/app/auth/one-time-password.tsx`:
- Around line 268-274: The onFocus handler performs a nullable check on
inputRef.current?.setNativeProps but then re-reads inputRef.current without
guaranteeing it’s unchanged; to satisfy strictNullChecks, capture the ref into a
local const (e.g., const ref = inputRef.current) and test ref && typeof
ref.setNativeProps === 'function' before calling ref.setNativeProps({ selection:
{ start: 0, end: value?.toString().length } }); this ensures onFocus, inputRef,
and setNativeProps are non-null when invoked and avoids unchecked access.
In `@apps/expo/features/ai/lib/localModelManager.web.ts`:
- Around line 11-13: The return type of the web stub getLocalModel is incorrect;
change its signature to return LlamaLanguageModel | null (matching the native
implementation) while keeping the body returning null for the web platform;
update the function declaration export function getLocalModel():
LlamaLanguageModel | null and ensure LlamaLanguageModel is imported or
referenced where needed so the type resolves.
In `@apps/expo/features/trips/screens/TripDetailScreen.tsx`:
- Around line 89-92: The onPress handler currently calls router.back()
unconditionally which can fail if there is no history; update the handler that
calls deleteTrip(id) (the async callback) to await deleteTrip as now, then check
navigation availability (e.g. use router.canGoBack() or equivalent) and call
router.back() only if true, otherwise navigate to a safe fallback route using
router.replace or router.push (for example the trips list) so the UI always
transitions after a successful delete.
In `@apps/expo/lib/Picker.web.tsx`:
- Line 10: The onValueChange signature currently receives a string because
HTMLSelectElement.value is always a string; update the change handler in
Picker.web.tsx (the function bound to onChange for the <select>, and the prop
onValueChange and Picker.Item/ItemProps handling) to restore the original
ItemProps value type: when rendering options, keep a mapping (e.g., an array or
Map keyed by option value or index) from option index to the original item.value
(string | number) and in the select onChange use event.target.selectedIndex (or
the option key) to look up the original value and pass that original typed value
to onValueChange instead of the raw string; ensure the mapping is accessible
where options are rendered and cleaned up appropriately.
In `@apps/expo/package.json`:
- Around line 38-39: The package.json devDependencies is missing
`@playwright/test` while the scripts "test:web" and "test:web:ui" call `playwright
test`; add "@playwright/test" to devDependencies and install it (e.g., using
npm/yarn/pnpm) so those scripts can run; update the apps/expo package.json
devDependencies to include the appropriate version of "@playwright/test" and run
the project package manager install command to persist the change.
In `@apps/expo/playwright/tests/globalSetup.ts`:
- Around line 19-22: Replace the silent placeholder fallback for DB_URL with an
explicit required check: read process.env.NEON_DATABASE_URL into the DB_URL
constant and if it's undefined or empty throw an Error with a clear message
(e.g., "NEON_DATABASE_URL is required for tests") so the failure happens
immediately; update any usage sites such as neon(DB_URL) to rely on this
guaranteed non-empty DB_URL.
In `@apps/expo/playwright/tests/packs.spec.ts`:
- Around line 181-183: The test is swallowing edit mutation failures by calling
editPromise.catch(() => null) after triggering the submit click
(page.getByTestId('items:submit').click()); remove the silent catch and let the
promise rejection surface so the test fails (i.e., await editPromise without
.catch), or if you need custom failure messaging, replace the catch with a
handler that rethrows or calls test.fail/assert with the caught error so
failures are not suppressed.
In `@apps/expo/playwright/tests/trips.spec.ts`:
- Around line 194-201: After clicking the confirm button
(deleteConfirmBtn.click()), wait explicitly for the backend delete mutation to
complete before asserting navigation or list state: use a network waiter such as
page.waitForResponse(...) or page.waitForRequestFinished that matches the delete
endpoint or GraphQL mutation name (e.g., "deleteTrip" or the API route your app
calls) and only after that call resolves proceed to page.waitForURL(...) and
page.waitForLoadState('networkidle'); update the test to await that response
between deleteConfirmBtn.click() and the subsequent waitForURL call so
assertions no longer race the write.
🪄 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: 11d4f61b-f9f0-490e-85b7-6f754b8eb924
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock,!bun.lock
📒 Files selected for processing (43)
.github/workflows/web-e2e-tests.ymlapps/expo/.gitignoreapps/expo/app/(app)/(tabs)/_layout.web.tsxapps/expo/app/(app)/(tabs)/profile/index.tsxapps/expo/app/_layout.tsxapps/expo/app/_layout.web.tsxapps/expo/app/auth/one-time-password.tsxapps/expo/atoms/atomWithSecureStorage.web.tsapps/expo/features/ai/lib/localModelManager.web.tsapps/expo/features/packs/components/HorizontalCatalogItemCard.tsxapps/expo/features/packs/hooks/usePackOwnershipCheck.tsapps/expo/features/packs/utils/uploadImage.web.tsapps/expo/features/trips/components/TripForm.tsxapps/expo/features/trips/screens/TripDetailScreen.tsxapps/expo/lib/Picker.tsxapps/expo/lib/Picker.web.tsxapps/expo/lib/appleAuthentication.tsapps/expo/lib/appleAuthentication.web.tsapps/expo/lib/constants.web.tsapps/expo/lib/devClient.tsapps/expo/lib/devClient.web.tsapps/expo/lib/hooks/useColorScheme.web.tsxapps/expo/lib/updates.tsapps/expo/lib/updates.web.tsapps/expo/lib/utils/ImageCacheManager.web.tsapps/expo/metro.config.jsapps/expo/mocks/expo-sqlite-kv-store.tsapps/expo/mocks/react-native-community-datetimepicker.tsxapps/expo/mocks/react-native-google-signin.tsapps/expo/package.jsonapps/expo/playwright/playwright.config.tsapps/expo/playwright/tests/core.spec.tsapps/expo/playwright/tests/fixtures.tsapps/expo/playwright/tests/globalSetup.tsapps/expo/playwright/tests/packs.spec.tsapps/expo/playwright/tests/profile.spec.tsapps/expo/playwright/tests/trips.spec.tsapps/expo/providers/index.web.tsxapps/expo/vitest.config.tspackage.jsonpackages/api/src/routes/trips/index.tspackages/checks/src/check-type-casts.tspackages/env/scripts/no-raw-process-env.ts
Comment on lines
+104
to
+110
| - name: Wait for web server | ||
| run: | | ||
| for i in $(seq 1 30); do | ||
| curl -sf http://localhost:8081 && echo "Server ready" && break | ||
| echo "Waiting... ($i/30)" | ||
| sleep 2 | ||
| done |
Contributor
There was a problem hiding this comment.
Fail fast if the web server never comes up.
The loop logs retries but does not exit non-zero after the final attempt, so failures surface later with noisier symptoms.
Suggested fix
- name: Wait for web server
run: |
+ ready=0
for i in $(seq 1 30); do
- curl -sf http://localhost:8081 && echo "Server ready" && break
+ if curl -sf http://localhost:8081; then
+ echo "Server ready"
+ ready=1
+ break
+ fi
echo "Waiting... ($i/30)"
sleep 2
done
+ [ "$ready" -eq 1 ] || { echo "Server failed to start"; 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 around lines 104 - 110, The "Wait for
web server" step's retry loop never returns non-zero on ultimate failure; update
that step so it fails fast when the server never becomes reachable by detecting
the loop's unsuccessful completion and calling exit 1 (or use a curl check that
short-circuits with || exit 1). Locate the shell loop in the "Wait for web
server" step and either add a success flag and exit 1 if not set after the for
loop, or replace the final check with a conditional that exits non-zero when
curl never succeeds.
Comment on lines
+40
to
+45
| <input | ||
| type={inputType} | ||
| defaultValue={toInputValue(value, mode)} | ||
| min={minimumDate ? toInputValue(minimumDate, mode) : undefined} | ||
| max={maximumDate ? toInputValue(maximumDate, mode) : undefined} | ||
| onChange={handleChange} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In React, does changing defaultValueon anafter mount update the displayed value, or isvalue required for controlled updates?
💡 Result:
Changing an input’s defaultValue prop after the component has mounted does NOT update the displayed value. React treats defaultValue as an initial value for an uncontrolled input, and it won’t re-drive the DOM value on subsequent re-renders; if you want the displayed value to track updates from props/state, you must use value (a controlled input) and update it via onChange. React’s docs describe defaultValue as the “initial value” for text inputs [1], while value is what “controls” what’s displayed and is driven by React state [2]. React issue discussions explicitly note that React “doesn’t update when defaultValue changes” and that to keep it updated you need to use value and handle onChange [3].
Citations:
- 1: https://react.dev/reference/react-dom/components/input
- 2: https://legacy.reactjs.org/docs/forms.html
- 3: Changing defaultValue doesn't re-render input, causes hidden state in the db react/react#4101
🏁 Script executed:
fd -a "react-native-community-datetimepicker.tsx" apps/expo/Repository: PackRat-AI/PackRat
Length of output: 139
🏁 Script executed:
cat -n /home/jailuser/git/apps/expo/mocks/react-native-community-datetimepicker.tsxRepository: PackRat-AI/PackRat
Length of output: 2118
Use a controlled input so picker stays in sync when value changes.
Using defaultValue on line 42 means the input is uncontrolled and won't update if the value prop changes after initial render. Switch to a controlled input with the value prop instead.
Fix
- defaultValue={toInputValue(value, mode)}
+ value={toInputValue(value, mode)}🤖 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/mocks/react-native-community-datetimepicker.tsx` around lines 40 -
45, The input is currently uncontrolled because it uses defaultValue; change it
to a controlled input by replacing defaultValue with a value prop so updates to
the value prop re-render the picker; use value={value != null ?
toInputValue(value, mode) : ''} (or equivalent) and keep min/max using
toInputValue(minimumDate/maximumDate, mode) as before, ensuring handleChange
still updates the parent state so the controlled value stays in sync (refer to
the input element, toInputValue, handleChange, value, minimumDate and
maximumDate).
| @@ -0,0 +1,20 @@ | |||
| // Web stub: Google Sign-In is a native-only SDK. On web, sign-in throws immediately. | |||
| export const GoogleSignin = { | |||
| hasPlayServices: (): Promise<boolean> => Promise.resolve(true), | |||
Contributor
There was a problem hiding this comment.
hasPlayServices should return false on web.
Play Services don't exist in browsers. Returning true can mislead code that checks availability before attempting Play Services operations.
🔧 Proposed fix
- hasPlayServices: (): Promise<boolean> => Promise.resolve(true),
+ hasPlayServices: (): Promise<boolean> => Promise.resolve(false),🤖 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/mocks/react-native-google-signin.ts` at line 3, The mock currently
has hasPlayServices returning Promise.resolve(true) which is incorrect for web;
update the hasPlayServices implementation in react-native-google-signin.ts to
return false on web (e.g., return Promise.resolve(false) in this web mock) or,
if this file is shared across platforms, guard it with a Platform check
(Platform.OS === 'web' ? Promise.resolve(false) :
Promise.resolve(true/actualCheck)). Ensure you edit the hasPlayServices function
to reflect the correct web behavior.
Comment on lines
+94
to
+104
| const [packResponse] = await Promise.all([ | ||
| page.waitForResponse((r) => r.url().includes('/api/packs') && r.request().method() === 'POST'), | ||
| (async () => { | ||
| await page.goto(`${BASE_URL}/pack/new`); | ||
| await page.getByRole('textbox', { name: /Pack Name/i }).fill(packName); | ||
| await page.getByTestId('submit-pack-button').click(); | ||
| })(), | ||
| ]); | ||
|
|
||
| const { id: packId } = (await packResponse.json()) as { id: number }; | ||
|
|
Contributor
There was a problem hiding this comment.
Assert pack creation success before parsing JSON.
Line 103 and Line 240 parse JSON before asserting response success. If the POST fails, the test fails with a parse error instead of a clear API failure signal.
Proposed patch
const [packResponse] = await Promise.all([
page.waitForResponse((r) => r.url().includes('/api/packs') && r.request().method() === 'POST'),
@@
]);
+ expect(packResponse.ok()).toBeTruthy();
const { id: packId } = (await packResponse.json()) as { id: number };
@@
const [packResponse] = await Promise.all([
page.waitForResponse((r) => r.url().includes('/api/packs') && r.request().method() === 'POST'),
@@
]);
+ expect(packResponse.ok()).toBeTruthy();
const { id: packId } = (await packResponse.json()) as { id: number };Also applies to: 231-240
🤖 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 94 - 104, The test
parses packResponse.json() without first asserting the HTTP success, which
causes JSON parse errors when the POST fails; update the code around the
Promise.all block that produces packResponse (and the similar block at lines
231-240) to first check packResponse.ok (or packResponse.status) and throw or
assert with a clear message containing status and statusText if not OK, then
call packResponse.json() to extract { id: packId }; do the same for the other
response variable so failures produce clear API error messages instead of parse
errors.
Comment on lines
+27
to
+41
| if (process.env.TEST_ACCESS_TOKEN && process.env.TEST_REFRESH_TOKEN) { | ||
| const meRes = await fetch(`${API_URL}/api/auth/me`, { | ||
| headers: { Authorization: `Bearer ${process.env.TEST_ACCESS_TOKEN}` }, | ||
| }); | ||
| const user = meRes.ok ? ((await meRes.json()) as { user: Record<string, unknown> }).user : null; | ||
| fs.writeFileSync( | ||
| TOKENS_FILE, | ||
| JSON.stringify({ | ||
| accessToken: process.env.TEST_ACCESS_TOKEN, | ||
| refreshToken: process.env.TEST_REFRESH_TOKEN, | ||
| user, | ||
| }), | ||
| ); | ||
| console.log('[globalSetup] Using tokens from TEST_ACCESS_TOKEN env var'); | ||
| return; |
Contributor
There was a problem hiding this comment.
Do not accept invalid pre-minted tokens in Priority 1 path.
This block returns even when /api/auth/me is not OK, which can persist unusable auth tokens and cause downstream false starts.
Suggested fix
if (process.env.TEST_ACCESS_TOKEN && process.env.TEST_REFRESH_TOKEN) {
const meRes = await fetch(`${API_URL}/api/auth/me`, {
headers: { Authorization: `Bearer ${process.env.TEST_ACCESS_TOKEN}` },
});
- const user = meRes.ok ? ((await meRes.json()) as { user: Record<string, unknown> }).user : null;
+ if (!meRes.ok) {
+ throw new Error(`TEST_ACCESS_TOKEN is invalid (${meRes.status})`);
+ }
+ const user = ((await meRes.json()) as { user: Record<string, unknown> }).user;
fs.writeFileSync(
TOKENS_FILE,
JSON.stringify({📝 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 (process.env.TEST_ACCESS_TOKEN && process.env.TEST_REFRESH_TOKEN) { | |
| const meRes = await fetch(`${API_URL}/api/auth/me`, { | |
| headers: { Authorization: `Bearer ${process.env.TEST_ACCESS_TOKEN}` }, | |
| }); | |
| const user = meRes.ok ? ((await meRes.json()) as { user: Record<string, unknown> }).user : null; | |
| fs.writeFileSync( | |
| TOKENS_FILE, | |
| JSON.stringify({ | |
| accessToken: process.env.TEST_ACCESS_TOKEN, | |
| refreshToken: process.env.TEST_REFRESH_TOKEN, | |
| user, | |
| }), | |
| ); | |
| console.log('[globalSetup] Using tokens from TEST_ACCESS_TOKEN env var'); | |
| return; | |
| if (process.env.TEST_ACCESS_TOKEN && process.env.TEST_REFRESH_TOKEN) { | |
| const meRes = await fetch(`${API_URL}/api/auth/me`, { | |
| headers: { Authorization: `Bearer ${process.env.TEST_ACCESS_TOKEN}` }, | |
| }); | |
| if (!meRes.ok) { | |
| throw new Error(`TEST_ACCESS_TOKEN is invalid (${meRes.status})`); | |
| } | |
| const user = ((await meRes.json()) as { user: Record<string, unknown> }).user; | |
| fs.writeFileSync( | |
| TOKENS_FILE, | |
| JSON.stringify({ | |
| accessToken: process.env.TEST_ACCESS_TOKEN, | |
| refreshToken: process.env.TEST_REFRESH_TOKEN, | |
| user, | |
| }), | |
| ); | |
| console.log('[globalSetup] Using tokens from TEST_ACCESS_TOKEN env var'); | |
| return; |
🤖 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 27 - 41, The current
globalSetup block writes pre-minted tokens even when the /api/auth/me call
fails; change the logic in the conditional that checks
process.env.TEST_ACCESS_TOKEN and process.env.TEST_REFRESH_TOKEN so that you
first verify meRes.ok is true and the user payload exists before calling
fs.writeFileSync(TOKENS_FILE, ...) and returning; if meRes.ok is false or user
is null/invalid, do not write the tokens or return—let the normal auth flow
continue (leave API_URL, TOKENS_FILE, and the fetch to /api/auth/me intact, but
gate the write/return on a successful meRes and valid user).
| if ('packId' in data) updateData.packId = data.packId ?? null; | ||
| if ('localUpdatedAt' in data) | ||
| updateData.localUpdatedAt = data.localUpdatedAt ? new Date(data.localUpdatedAt) : null; | ||
| if ('deleted' in data) updateData.deleted = data.deleted; |
Contributor
There was a problem hiding this comment.
Guard against assigning undefined to updateData.deleted.
The check 'deleted' in data is true even when data.deleted is undefined (since the field is optional in the schema). Assigning undefined to updateData.deleted may not match the intended behavior. All other optional fields use nullish coalescing or explicit null checks.
🛡️ Proposed fix
- if ('deleted' in data) updateData.deleted = data.deleted;
+ if (data.deleted !== undefined) updateData.deleted = data.deleted;📝 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 ('deleted' in data) updateData.deleted = data.deleted; | |
| if (data.deleted !== undefined) updateData.deleted = data.deleted; |
🤖 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/routes/trips/index.ts` at line 166, The current check uses
"'deleted' in data" which is true even when data.deleted is undefined; change
the guard to only assign when the value is actually provided (e.g., use an
explicit undefined check or nullish coalescing): replace the "'deleted' in data"
condition with "data.deleted !== undefined" (or similar) so that
updateData.deleted is assigned only when data.deleted has a real value; update
the assignment site referencing updateData and data in the same block.
Collaborator
Author
|
Confirmed MERGEABLE locally — failing checks are the usual iOS/Android E2E flake (per the recent CI hygiene pattern; main is green on those workflows). The Web E2E failure is also worth verifying: from the PR's own scaffolding, that's likely just the new Playwright run reporting against the existing baseline — once green, that becomes the merge signal. Recommendation: merge as-is. The audit's earlier suggestion to split into (1) Playwright setup-only + (2) web fixes is optional polish — at 1934/20/43 the PR is reviewable in one pass, and the web fixes are genuinely required to make the Playwright suite pass (TripDetailScreen Companion context: #2374 closed today as POC superseded by #2366. The chosen web-support direction is #2366's platform-shims approach — this PR's tests + targeted fixes are compatible with that direction. Three small asks before merging if not already addressed:
|
andrew-bierman
added a commit
that referenced
this pull request
May 17, 2026
…(after #2363/#2428 landed) Conflicts: - packages/api/src/routes/trips/index.ts: dev added inline LocationSchema/CreateTripRequestSchema/UpdateTripRequestSchema; this PR already imports the equivalents from @packrat/schemas/trips. Kept HEAD (the extracted-schemas import + body refs to CreateTripBodySchema/UpdateTripBodySchema). - bun.lock: regenerated via bun install. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
8 tasks
andrew-bierman
added a commit
that referenced
this pull request
May 17, 2026
#2428 landed) Conflicts: - apps/expo/features/packs/hooks/usePackOwnershipCheck.ts: HEAD used new single-object obs({store,id}) signature with .peek() (non-reactive); dev rewrote to use$(() => !!obs(packsStore, id).get()) for reactivity. Blended: kept dev's reactive use$ pattern, applied HEAD's single-object obs call form. Post-merge fixes (dev added new files that violated max-params=1): - apps/expo/atoms/atomWithSecureStorage.web.ts: refactor to {key, initialValue} to match native sibling. - apps/expo/features/packs/utils/uploadImage.web.ts: refactor uploadImage/getPresignedUrl/urlToBlob to single-object params; align with native sibling signature. - apps/expo/lib/utils/ImageCacheManager.web.ts: refactor cacheRemoteImage and cacheLocalTempImage to single-object params, matching native ImageCacheManager.ts. - scripts/lint/no-owned-max-params.ts: add /playwright/ to EXCLUDED_PATH_PARTS (parity with /test/ and /__tests__/; playwright fixtures are test code with signatures dictated by Playwright). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Jun 15, 2026
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
TripDetailScreen: replaced no-opreact-native Alert.alertwith NativeWindUIalertRef.current?.alert()so the delete confirmation modal actually renders in browsersTripForm: added missingtestIDto description inputprofile/index: exposedtestIds.profile.nameEditBtnon Name list row so E2E can navigate to/profile/nameusePackOwnershipCheck: switched from non-reactivepeek()to reactiveuse$()so header edit/delete buttons appear after store hydrationdeletedfield inUpdateTripRequestSchemafor syncedCrud soft-delete syncno-raw-process-envexemptsapps/expo/e2e-web/check-type-castsexemptse2e-web/and annotates necessary casts in web shimsKey patterns established
waitForResponse(POST /api/trips)before navigating away ensures the trip is persisted before list re-rendersaria-disabledattribute check (nottoBeDisabled()) for NativeWindUI Pressable on webrouter.back()history before deletinggetByText('Delete', { exact: true })avoids substring-matching trip names like "E2E-DeleteTrip-..."npx playwright test --config=e2e-web/playwright.config.ts(usesworkers: 1from config)Test plan
npx playwright test --config=e2e-web/playwright.config.ts🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores