fix(e2e): fix iOS login testID and Android ANR failures in Maestro#2274
Open
andrew-bierman wants to merge 119 commits intodevelopmentfrom
Open
fix(e2e): fix iOS login testID and Android ANR failures in Maestro#2274andrew-bierman wants to merge 119 commits intodevelopmentfrom
andrew-bierman wants to merge 119 commits intodevelopmentfrom
Conversation
Contributor
WalkthroughRemoves Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves Maestro E2E stability across iOS and Android by adjusting selectors for iOS login fields and mitigating Android emulator System UI ANR dialogs that can block test interactions.
Changes:
- Update login/invalid-login flows to use iOS placeholder-text selectors for email/password and add pre-tap guards (
extendedWaitUntil) for the auth entry screen. - Add Android ANR-dialog dismissal steps to Maestro setup/auth flows and add extra stabilization/dismissal steps to the Android CI job.
- Improve flow robustness by waiting for catalog data to load before scrolling and correcting the Dashboard tab label used in navigation.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| .maestro/flows/setup/clear-state.yaml | Adds Android-specific ANR dialog dismissal after clear-state launch. |
| .maestro/flows/negative/invalid-login-flow.yaml | Adds ANR handling, waits for auth entry screen by testID, uses platform-specific field selectors, and submits via continue-button. |
| .maestro/flows/auth/login-flow.yaml | Adds ANR handling, waits for sign-in button before tapping, and uses iOS placeholder selectors for login inputs. |
| .maestro/flows/dashboard/dashboard-tiles-flow.yaml | Updates tab navigation selector from “Home” to “Dashboard”. |
| .maestro/flows/catalog/catalog-browse-flow.yaml | Adds an extendedWaitUntil guard for item-count text before scrolling/assertions. |
| .maestro/flows/catalog/catalog-item-detail-flow.yaml | Adds an extendedWaitUntil guard for item-count text before scrolling to priced items. |
| .github/workflows/e2e-tests.yml | Adds a short post-install delay and KEYCODE_BACK cleanup to reduce/clear blocking Android dialogs before Maestro starts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Coverage Report for Expo Unit Tests Coverage (./apps/expo)
File Coverage
|
||||||||||||||||||||||||||||||||||||||
Contributor
Coverage Report for API Unit Tests Coverage (./packages/api)
File CoverageNo changed files found. |
andrew-bierman
force-pushed
the
fix/maestro-tests
branch
from
dcfbf7c to
15e833a
Compare
Contributor
Deploying packrat-guides with
|
| Latest commit: |
bebfbcb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a3aec453.packrat-guides-6gq.pages.dev |
| Branch Preview URL: | https://fix-maestro-tests.packrat-guides-6gq.pages.dev |
Contributor
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
packrat-admin | bebfbcb | Commit Preview URL Branch Preview URL |
Apr 30 2026, 05:58 AM |
github-actions
Bot
added
the
dependencies
Contributor
Deploying packrat-landing with
|
| Latest commit: |
bebfbcb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f99e2f80.packrat-landing.pages.dev |
| Branch Preview URL: | https://fix-maestro-tests.packrat-landing.pages.dev |
…ignout navigation on iOS
router.replace('/auth') queues a REPLACE action through expo-router's routingQueue → useEffect
dispatch chain, which silently drops the action on iOS when NativeTabs state is still present
in the navigation tree during the sign-out transition. resetRoot dispatches RESET directly to
the root Stack via NavigationContainerRefContext, bypassing findDivergentState and the async
queue entirely.
NavigationContainerRefContext from @react-navigation/core defaults to undefined — the optional-chained resetRoot was a no-op on both platforms, breaking Android (which was passing with router.replace) and leaving iOS broken. useNavigationContainerRef() from expo-router returns store.navigationRef, a proxy that is always defined. resetRoot() dispatches RESET directly to the root Stack, bypassing the routingQueue → useImperativeApiEmitter chain that router.replace() uses and that NativeTabs swallows on iOS.
… post-signout navigation resetRoot depends on keyedListeners.getState.root?.().key which can be null and silently drops the action. dispatch(CommonActions.reset) without a target routes directly to the root Stack handler, bypassing both that dependency and expo-router's routingQueue/findDivergentState chain.
On iOS with NativeTabs (UITabBarController), returning null from AppLayout unmounts the React tree but the native view hierarchy persists. navRef.dispatch then fires into an inconsistent navigation state and silently no-ops. Fix: remove the !isAuthenticated null-return guard so the Stack/NativeTabs stays alive when navRef.dispatch(CommonActions.reset) fires. The root Stack transitions to 'auth' while the native views are still responsive, then NativeTabs unmounts naturally as part of the navigation. useAuthInit handles the initial-unauthenticated case via router.replace.
…bs compatibility
navRef.dispatch(CommonActions.reset) from AppLayout's useEffect doesn't reliably
transition the root navigator on iOS — the native UITabBarController intercepts
navigation actions dispatched from effect callbacks after token state clears.
Move router.replace('/auth') to signOut() directly (after the 50ms yield), mirroring
deleteAccount(). Calling from the signOut execution context is immune to NativeTabs
swallowing. Remove the wasAuthenticated ref, both navigation effects, and now-unused
imports from AppLayout.
router.replace('/auth') called from signOut's async context (or an AppLayout
useEffect) is silently dropped on iOS: the action reaches NativeTabs' native
UITabBarController which doesn't have an 'auth' route and discards it.
SignOutGuard is rendered inside <Providers> (JotaiProvider context) but BEFORE
<NavThemeProvider><Stack>, so it sits outside any navigator. When it fires
router.replace from a React effect, the action dispatches directly to the root
Stack with no intermediate navigator to intercept it. Works on both iOS and Android.
Remove the now-redundant router.replace call from signOut().
…avigation
Replaces tokenAtom watch in SignOutGuard with a signOutRequestedAtom that
is only set by signOut(). The previous approach caused a login regression
because tokenAtom transitions null→value during sign-in were
indistinguishable from the sign-out case, causing SignOutGuard to fire
router.replace('/auth') immediately after successful login.
The dedicated atom is set as the final step in signOut() (after clearLocalData,
userStore reset, and 50ms yield), so SignOutGuard only navigates when signOut
has fully completed.
…veTabs on iOS
SignOutGuard in root _layout.tsx used router.replace('/auth') which routes through
the focused navigator chain. On iOS with NativeTabs, this is silently dropped because
'auth' is not a tab route.
Replace with SignOutNavigator rendered inside AppLayout ((app) screen). useNavigation()
there returns the ROOT Stack's navigation prop — dispatching CommonActions.reset directly
to the root, bypassing NativeTabs entirely. Works on both iOS and Android.
useNavigation() inside a layout file returns the current layout's inner Stack navigation, not the root Stack — so CommonActions.reset with 'auth' was silently dropped (auth is not in the inner Stack's routes). useNavigationContainerRef() returns the absolute root NavigationContainerRef, guaranteeing reset dispatches to the root Stack and reaches 'auth' on iOS.
… focus chain Prior approaches (router.replace, useNavigation().dispatch, useNavigationContainerRef().dispatch) all routed through BaseNavigationContainer's listeners.focus[0] → NativeTabs → dropped on iOS. Subscribing to isAuthed via use$() and returning <Redirect href="/auth" /> causes React to unmount the inner Stack (and NativeTabs) before the redirect fires, so router.replace runs outside the NativeTabs focus chain and reaches the root Stack correctly.
…replace router.replace, dispatch(CommonActions.reset), and <Redirect> all route through BaseNavigationContainer.listeners.focus[0]. When the inner Stack and NativeTabs unmount (as part of the redirect render), focus[0] may be cleared, silently dropping the action on iOS. navigationRef.resetRoot() directly sets the root navigation state, bypassing the focused-navigator chain entirely — guaranteed to reach the root Stack. Guard with wasAuthedRef so resetRoot only fires on sign-out (not on cold-start unauthenticated boot, which useAuthInit handles via router.replace).
All prior approaches (router.replace, dispatch(reset), resetRoot) fail on iOS because they route through listeners.focus[0], which terminates at NativeTabs and silently drops the action. Root cause: NativeTabs is the deepest focused navigator while the user is authenticated. Any navigation action dispatched during sign-out reaches NativeTabs first and is dropped before the root Stack can handle it. Fix: AppLayout now shows a spinner when isLoadingAtom=true AND !isAuthed. signOut() already sets isLoadingAtom=true at the start and clears it via several real async operations (clearLocalData). By the time router.replace is called, React has rendered the spinner, NativeTabs is unmounted, and listeners.focus[0] points to the root Stack — so the replace succeeds. The !isAuthed guard ensures the spinner is NOT shown during re-auth sign-in, where isLoadingAtom is also true but the user remains authenticated. isLoadingAtom is intentionally left true after navigation; signIn() resets it via its own finally block when the user signs in again.
…signOut
The previous approach called router.replace('/auth') from inside signOut()'s
async finally block. Even though isLoadingAtom=true causes AppLayout to render
a spinner (unmounting NativeTabs), the router.replace call raced with React's
commit: NativeTabs might still be registered in listeners.focus[0] because
useLayoutEffect cleanups (which remove NativeTabs from the focus chain) run
during the commit phase, not before the async continuation.
Fix: move router.replace('/auth') into a useEffect in AppLayout. useEffect
fires after the commit phase completes, so by the time the effect runs:
1. The spinner is committed (NativeTabs removed via useLayoutEffect cleanup)
2. listeners.focus[0] is the root Stack
3. router.replace('/auth') reaches the root Stack directly
signOut() still sets isLoadingAtom=true (triggering the spinner) and clears
auth state, but no longer fires the navigation itself.
Replace router.replace('/auth') with a direct CommonActions.reset
dispatched to the root Stack navigator via navigation.getParent().
router.replace goes through expo-router's listeners.focus[0], which
can be intercepted by NativeTabs on both platforms. Direct dispatch
to a specific navigator bypasses this chain entirely.
The nested state in the reset ensures auth's sub-navigator starts at
[index] only, preventing the (login) modal from being restored from
retained navigation state after sign-out.
![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: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/api/src/routes/catalog/index.ts (2)
449-452:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHard DELETE violates soft-delete requirement.
As per coding guidelines: "All user-generated content must use soft deletes (never hard DELETE)." This route permanently removes catalog items.
Add a
deletedAtcolumn and update instead of delete:Proposed fix
- await db.delete(catalogItems).where(eq(catalogItems.id, itemId)); + await db + .update(catalogItems) + .set({ deletedAt: new Date() }) + .where(eq(catalogItems.id, itemId)); return { success: true };Ensure the schema has a
deletedAtcolumn and that read queries filter out soft-deleted rows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routes/catalog/index.ts` around lines 449 - 452, This route currently performs a hard delete via db.delete(catalogItems).where(eq(catalogItems.id, itemId)) which violates the soft-delete policy; instead add a nullable deletedAt timestamp column to the catalogItems schema, change the deletion logic in this handler to perform an update that sets deletedAt to the current timestamp (use catalogItems and eq(catalogItems.id, itemId) to locate the row), and update all reads that query catalogItems to exclude soft-deleted rows by checking deletedAt is null (or equivalent isNull/deferred check in the query builder); also add a migration to add the deletedAt column.
284-286:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPipeline failures: invalid
params.idcauses 500 instead of 404.
Number(params.id)returnsNaNfor non-numeric strings and overflows for values exceeding Postgres int4 max (2147483647). Both cases propagate to Drizzle and throwDrizzleQueryError, returning HTTP 500 instead of the expected 404.Validate and coerce the ID before querying:
Proposed fix
.get( '/:id', async ({ params }) => { const db = createDb(); - const itemId = Number(params.id); + const itemId = Number(params.id); + if (!Number.isInteger(itemId) || itemId < 1 || itemId > 2147483647) { + return status(404, { error: 'Catalog item not found' }); + } const item = await db.query.catalogItems.findFirst({The same issue exists in the
/:id/similar,PUT /:id, andDELETE /:idroutes—apply consistent validation to all.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routes/catalog/index.ts` around lines 284 - 286, The route handlers currently convert params.id with Number(params.id) (seen in the async ({ params }) => { const db = createDb(); const itemId = Number(params.id); } block) which yields NaN or overflowed ints and causes DrizzleQueryError/500; instead validate and coerce the id before querying by parsing it as an integer (e.g., parseInt), ensuring it's a finite integer within Postgres int4 bounds (1..2147483647) and/or Number.isSafeInteger, and if invalid return a 404 early; apply the same validation pattern to the /:id/similar, PUT /:id and DELETE /:id handlers so no invalid id reaches Drizzle queries.apps/expo/features/trips/components/TripForm.tsx (1)
129-136:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAwait
createTrip()in the create path.
createTrip()is async, but Line 130 fires it and immediately shows success plusrouter.back(). That can navigate away before the trip exists, and rejected writes will skip thiscatch.Suggested fix
} else { - createTrip(submitData); + await createTrip(submitData); Burnt.toast({ title: t('trips.tripCreatedSuccess'), preset: 'done', }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/features/trips/components/TripForm.tsx` around lines 129 - 136, The create path currently calls createTrip(submitData) without awaiting it, then immediately shows Burnt.toast and calls router.back, which can navigate away before the async write completes; update the create branch to await createTrip(submitData) (or wrap it in a try/catch) and only call Burnt.toast({ title: t('trips.tripCreatedSuccess'), preset: 'done' }) and router.back() after the awaited call succeeds, handling and logging any rejection from createTrip so failures do not get swallowed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/api-tests.yml:
- Line 38: Update the CI step that runs the Bun install command to use a frozen
lockfile to ensure reproducible installs: replace the bare "bun install"
invocation in the workflow step with a frozen-lockfile variant by adding the
--frozen-lockfile flag so the API test workflow uses "bun install
--frozen-lockfile".
In @.github/workflows/checks.yml:
- Line 34: Replace the bare "run: bun install" invocation in the workflow step
with a frozen install to prevent lockfile drift: update the step that currently
contains "run: bun install" to run "bun install --frozen-lockfile" so CI will
fail if the lockfile is out of sync and avoid unintended dependency changes.
In @.github/workflows/copilot-setup-steps.yml:
- Line 90: Replace the non-deterministic CI step that runs "run: bun install"
with a frozen-lockfile invocation to prevent dependency drift; update the
workflow step that currently contains "run: bun install" to use "run: bun
install --frozen-lockfile" so CI uses the exact lockfile for installs.
In @.github/workflows/e2e-tests.yml:
- Line 87: Update the CI step that runs bun install to use the frozen lockfile
flag: replace the existing run command that executes "bun install" with a run
command that includes "--frozen-lockfile" so the workflow uses "bun install
--frozen-lockfile" to prevent lockfile drift and ensure reproducible builds.
- Line 346: In the Android job restore the frozen lockfile flag by changing the
install step that currently reads "run: bun install" to include the flag, i.e.,
"run: bun install --frozen-lockfile"; update the Android job's install step so
the Bun install command enforces the lockfile and prevents automatic changes to
lock files during CI.
In @.github/workflows/migrations.yml:
- Line 47: Update the CI workflow step that runs the package install so it uses
a frozen lockfile: change the run command that currently invokes "bun install"
to include the --frozen-lockfile flag so installs are deterministic during
migrations; locate the run step that contains "bun install" in the
migrations.yml workflow and replace it with the frozen-lockfile variant.
In @.github/workflows/sync-guides-r2.yml:
- Line 42: Replace the mutable install step "run: bun install" in the CI
workflow with an immutable install by adding the frozen-lockfile flag; update
the workflow step that currently contains the literal "run: bun install" so it
runs "bun install --frozen-lockfile" to enforce reproducible dependency
resolution in CI.
In @.github/workflows/unit-tests.yml:
- Line 58: Replace the bare CI install commands that run "bun install" with
frozen lockfile installs by changing the run steps that call bun install (the
workflow steps containing the "bun install" command) to use "bun install
--frozen-lockfile" so dependency resolution is deterministic; update both
occurrences of the "bun install" run step in the workflow file accordingly.
In @.maestro/flows/dashboard/dashboard-tiles-flow.yaml:
- Around line 14-15: The current assertVisible using text: "Dashboard" can match
a tab bar label and not prove the Dashboard screen loaded; change the assertion
in the dashboard flow to check a Dashboard-only selector (e.g., a header node or
dedicated testID) instead of the generic text — locate the assertVisible step
that references text: "Dashboard" and replace it with an assertion targeting the
unique Dashboard element (for example assertVisible: testID: "dashboard-header"
or assertVisible: text: "DashboardHeader") so the test verifies the actual
screen content rather than the tab label.
In @.maestro/flows/negative/invalid-login-flow.yaml:
- Around line 79-90: The iOS branch of the negative login flow wrongly asserts
on the "Email" placeholder which disappears after typing; update the iOS runFlow
block that uses assertVisible to check for a stable post-submit element (e.g.,
continue-button) instead of text: "Email" so it matches the Android branch
behavior which checks id: "email-input"; modify the assertVisible inside the
runFlow where when: platform: iOS to reference the persistent element
(continue-button) so the negative flow passes when the form remains visible.
In @.maestro/flows/trips/create-trip-flow.yaml:
- Around line 157-169: The iOS flow's success check is using the generic testID
"trip-list-item" which matches any row and yields false positives; update the
app or flow so the check targets the created trip specifically: either modify
TripListScreen (e.g., the row rendering logic that currently sets testID
"trip-list-item" in TripListScreen.tsx) to include the trip name in the per-row
accessibility label/testID (for example `${TRIP_NAME}` or
`trip-list-item-${trip.name}`), or change the iOS flow to wait for that specific
selector (replace the extendedWaitUntil id: "trip-list-item" with the new id
that includes `${TRIP_NAME}`) so the test reliably waits for the exact created
trip.
In `@apps/expo/app/`(app)/_layout.tsx:
- Around line 321-337: The headerLeft callbacks in getTripNewOptions and
getPackNewOptions call the useRouter() hook directly which violates the Rules of
Hooks; extract the Pressable into a dedicated React component (e.g.,
CancelTripButton) that calls useRouter() internally and accepts an onCancel
prop, then replace headerLeft: () => { ... } with headerLeft: () =>
<CancelTripButton onCancel={...} /> so hooks are only used inside a component.
In `@apps/expo/features/auth/hooks/useAuthActions.ts`:
- Around line 306-313: In deleteAccount(), move the isAuthed.set(false) call to
before clearing tokens/storage so the auth-refresh guards behave like signOut();
specifically, call isAuthed.set(false) at the start of the block that currently
calls setToken(null), setRefreshToken(null), await clearLocalData(),
userStore.set(null) and router.replace('/auth'), ensuring no in-flight refresh
can repopulate auth state after deletion; update the block around the
deleteAccount() function (and ensure consistency with signOut()) so isAuthed is
flipped first, then null out tokens via setToken/setRefreshToken, call
clearLocalData(), update userStore, wait, and finally router.replace('/auth').
In `@apps/expo/features/catalog/hooks/useCatalogItems.ts`:
- Around line 40-52: Before returning the fallback raw catalog object in
useCatalogItems.ts when CatalogItemsResponseSchema.safeParse(data) fails, emit a
warning or telemetry that includes parseResult.error so API/schema drift is
visible; locate the parseResult variable (result of
CatalogItemsResponseSchema.safeParse) and add a single logging/telemetry call
(e.g., logger.warn/console.warn or telemetry.captureException) that records a
concise message plus parseResult.error before returning the raw
items/totalCount/page/limit/totalPages fallback.
In `@apps/expo/features/packs/components/PackForm.tsx`:
- Around line 124-128: In the create branch of PackForm.tsx, await the async
createPack(...) call before showing the success toast and calling router.back so
failures are caught and the UI only advances on success; specifically, change
the flow around createPack, Burnt.toast and router.back so createPack is awaited
(await createPack({ ...value, category: value.category })) and only after that
resolves call Burnt.toast(t('packs.packCreatedSuccess')) and router.back();
ensure you do not append a .catch that circumvents the surrounding try/catch so
errors propagate to the existing error handler.
In `@apps/expo/features/trips/store/trips.ts`:
- Around line 15-18: The listTrips function takes getParams typed as any and
directly mutates getParams.mode = 'merge'; replace the any with a minimal
interface (e.g., an object type that optionally includes mode: string and other
expected fields) and add a runtime type guard before mutating (check that
getParams is a non-null object and typeof getParams === 'object' and that it's
safe to assign mode) so you only set getParams.mode when the parameter conforms
to the expected shape; update the function signature for listTrips(getParams:
YourParamsType) and perform the guard before the assignment to avoid unsafe
mutations.
---
Outside diff comments:
In `@apps/expo/features/trips/components/TripForm.tsx`:
- Around line 129-136: The create path currently calls createTrip(submitData)
without awaiting it, then immediately shows Burnt.toast and calls router.back,
which can navigate away before the async write completes; update the create
branch to await createTrip(submitData) (or wrap it in a try/catch) and only call
Burnt.toast({ title: t('trips.tripCreatedSuccess'), preset: 'done' }) and
router.back() after the awaited call succeeds, handling and logging any
rejection from createTrip so failures do not get swallowed.
In `@packages/api/src/routes/catalog/index.ts`:
- Around line 449-452: This route currently performs a hard delete via
db.delete(catalogItems).where(eq(catalogItems.id, itemId)) which violates the
soft-delete policy; instead add a nullable deletedAt timestamp column to the
catalogItems schema, change the deletion logic in this handler to perform an
update that sets deletedAt to the current timestamp (use catalogItems and
eq(catalogItems.id, itemId) to locate the row), and update all reads that query
catalogItems to exclude soft-deleted rows by checking deletedAt is null (or
equivalent isNull/deferred check in the query builder); also add a migration to
add the deletedAt column.
- Around line 284-286: The route handlers currently convert params.id with
Number(params.id) (seen in the async ({ params }) => { const db = createDb();
const itemId = Number(params.id); } block) which yields NaN or overflowed ints
and causes DrizzleQueryError/500; instead validate and coerce the id before
querying by parsing it as an integer (e.g., parseInt), ensuring it's a finite
integer within Postgres int4 bounds (1..2147483647) and/or Number.isSafeInteger,
and if invalid return a 404 early; apply the same validation pattern to the
/:id/similar, PUT /:id and DELETE /:id handlers so no invalid id reaches Drizzle
queries.
🪄 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: 12736365-71fa-4bac-8c56-db5f04c320f8
📒 Files selected for processing (46)
.github/workflows/api-tests.yml.github/workflows/checks.yml.github/workflows/copilot-setup-steps.yml.github/workflows/e2e-tests.yml.github/workflows/migrations.yml.github/workflows/sync-guides-r2.yml.github/workflows/unit-tests.yml.maestro/flows/auth/login-flow.yaml.maestro/flows/auth/logout-flow.yaml.maestro/flows/catalog/catalog-browse-flow.yaml.maestro/flows/catalog/catalog-item-detail-flow.yaml.maestro/flows/dashboard/dashboard-tiles-flow.yaml.maestro/flows/negative/empty-pack-submit-flow.yaml.maestro/flows/negative/empty-trip-submit-flow.yaml.maestro/flows/negative/invalid-login-flow.yaml.maestro/flows/packs/add-item-actions-flow.yaml.maestro/flows/packs/create-pack-flow.yaml.maestro/flows/packs/pack-detail-flow.yaml.maestro/flows/setup/clear-state.yaml.maestro/flows/trips/create-trip-flow.yaml.maestro/flows/trips/trip-detail-flow.yamlapps/expo/app/(app)/(tabs)/profile/index.tsxapps/expo/app/(app)/_layout.tsxapps/expo/features/auth/atoms/authAtoms.tsapps/expo/features/auth/hocs/withAuthWall.tsxapps/expo/features/auth/hooks/useAuthActions.tsapps/expo/features/auth/store/index.tsapps/expo/features/catalog/components/CatalogItemCard.tsxapps/expo/features/catalog/hooks/useCatalogItems.tsapps/expo/features/catalog/screens/CatalogItemDetailScreen.tsxapps/expo/features/catalog/screens/CatalogItemsScreen.tsxapps/expo/features/packs/components/PackForm.tsxapps/expo/features/packs/components/SearchResults.tsxapps/expo/features/packs/screens/PackListScreen.tsxapps/expo/features/packs/store/packs.tsapps/expo/features/trips/components/TripCard.tsxapps/expo/features/trips/components/TripForm.tsxapps/expo/features/trips/screens/TripDetailScreen.tsxapps/expo/features/trips/screens/TripListScreen.tsxapps/expo/features/trips/store/trips.tsapps/expo/lib/api/packrat.tsapps/expo/lib/i18n/locales/en.jsonapps/expo/lib/testIds.tsapps/expo/lib/utils/getRelativeTime.tspackages/api/src/routes/catalog/index.tspackages/api/src/schemas/catalog.ts
| env: | ||
| PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN: ${{ secrets.PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN }} | ||
| run: bun install --frozen-lockfile | ||
| run: bun install |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/api-tests.yml | head -50Repository: PackRat-AI/PackRat
Length of output: 1519
Use frozen lockfile for API test workflow installs.
Line 38 uses bare bun install which allows mutable dependency versions in CI. Add --frozen-lockfile to ensure reproducible builds.
Suggested fix
- run: bun install
+ run: bun install --frozen-lockfile📝 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
| run: bun install | |
| run: bun install --frozen-lockfile |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/api-tests.yml at line 38, Update the CI step that runs the
Bun install command to use a frozen lockfile to ensure reproducible installs:
replace the bare "bun install" invocation in the workflow step with a
frozen-lockfile variant by adding the --frozen-lockfile flag so the API test
workflow uses "bun install --frozen-lockfile".
| env: | ||
| PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN: ${{ secrets.PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN }} | ||
| run: bun install --frozen-lockfile | ||
| run: bun install |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and read it
if [ -f .github/workflows/checks.yml ]; then
echo "=== File .github/workflows/checks.yml exists ==="
echo "=== Content with line numbers ==="
cat -n .github/workflows/checks.yml | head -50
else
echo "File not found"
git ls-files | grep -E '\.github/workflows'
fiRepository: PackRat-AI/PackRat
Length of output: 2085
🏁 Script executed:
# Let me check if there are other bun install commands in the file
rg "bun install" .github/workflows/checks.ymlRepository: PackRat-AI/PackRat
Length of output: 87
Use --frozen-lockfile to prevent lockfile drift in CI.
Line 34 uses bare bun install, which allows the lockfile to drift during CI runs. This reduces reproducibility and increases the risk of unexpected dependency changes. Per coding guidelines: "Prefer bun install --frozen-lockfile over bare bun install in CI."
Suggested fix
- run: bun install
+ run: bun install --frozen-lockfile📝 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
| run: bun install | |
| run: bun install --frozen-lockfile |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/checks.yml at line 34, Replace the bare "run: bun install"
invocation in the workflow step with a frozen install to prevent lockfile drift:
update the step that currently contains "run: bun install" to run "bun install
--frozen-lockfile" so CI will fail if the lockfile is out of sync and avoid
unintended dependency changes.
| env: | ||
| PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN: ${{ secrets.PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN }} | ||
| run: bun install --frozen-lockfile | ||
| run: bun install |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and examine line 90 with context
if [ -f ".github/workflows/copilot-setup-steps.yml" ]; then
echo "=== File exists ==="
wc -l ".github/workflows/copilot-setup-steps.yml"
echo ""
echo "=== Context around line 90 ==="
sed -n '85,95p' ".github/workflows/copilot-setup-steps.yml"
else
echo "File does not exist"
fiRepository: PackRat-AI/PackRat
Length of output: 708
Use bun install --frozen-lockfile in CI.
Line 90 uses bare bun install, which allows dependency resolution drift between runs. Use bun install --frozen-lockfile for deterministic and safer CI installs.
Suggested fix
- run: bun install
+ run: bun install --frozen-lockfile📝 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
| run: bun install | |
| run: bun install --frozen-lockfile |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/copilot-setup-steps.yml at line 90, Replace the
non-deterministic CI step that runs "run: bun install" with a frozen-lockfile
invocation to prevent dependency drift; update the workflow step that currently
contains "run: bun install" to use "run: bun install --frozen-lockfile" so CI
uses the exact lockfile for installs.
| env: | ||
| PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN: ${{ secrets.PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN }} | ||
| run: bun install --frozen-lockfile | ||
| run: bun install |
Contributor
There was a problem hiding this comment.
Restoring --frozen-lockfile per CI guidelines.
As per coding guidelines: "Prefer bun install --frozen-lockfile over bare bun install in CI." This prevents lockfile drift and ensures reproducible builds.
Proposed fix
- run: bun install
+ run: bun install --frozen-lockfile📝 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
| run: bun install | |
| run: bun install --frozen-lockfile |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/e2e-tests.yml at line 87, Update the CI step that runs bun
install to use the frozen lockfile flag: replace the existing run command that
executes "bun install" with a run command that includes "--frozen-lockfile" so
the workflow uses "bun install --frozen-lockfile" to prevent lockfile drift and
ensure reproducible builds.
| env: | ||
| PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN: ${{ secrets.PACKRAT_NATIVEWIND_UI_GITHUB_TOKEN }} | ||
| run: bun install --frozen-lockfile | ||
| run: bun install |
Contributor
There was a problem hiding this comment.
Same issue: restore --frozen-lockfile for Android job.
Proposed fix
- run: bun install
+ run: bun install --frozen-lockfile📝 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
| run: bun install | |
| run: bun install --frozen-lockfile |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/e2e-tests.yml at line 346, In the Android job restore the
frozen lockfile flag by changing the install step that currently reads "run: bun
install" to include the flag, i.e., "run: bun install --frozen-lockfile"; update
the Android job's install step so the Bun install command enforces the lockfile
and prevents automatic changes to lock files during CI.
Comment on lines
+321
to
+337
| const getTripNewOptions = (t: TranslationFunction) => ({ | ||
| title: t('trips.createTrip'), | ||
| presentation: 'modal' as const, | ||
| animation: 'slide_from_bottom' as const, | ||
| headerLeft: () => { | ||
| const router = useRouter(); | ||
| return ( | ||
| <Pressable | ||
| testID={TestIds.CancelTripFormButton} | ||
| onPress={() => router.back()} | ||
| className="px-2" | ||
| > | ||
| <Text className="text-primary">{t('common.cancel')}</Text> | ||
| </Pressable> | ||
| ); | ||
| }, | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Can useRouter()from Expo Router be called inside a React Navigation screen option callback likeheaderLeft: () => { ... }, or do the Rules of Hooks require moving that call into a React component or custom hook?
💡 Result:
No, useRouter from Expo Router cannot be called directly inside a React Navigation screen option callback like headerLeft: => { const router = useRouter; ... }. The Rules of Hooks require hooks to be called from React function components or custom hooks, and these callbacks are plain functions invoked by React Navigation, not React components. Calling hooks there violates the Rules of Hooks, leading to errors like "React has detected a change in the order of Hooks" or invalid hook calls. To use useRouter for navigation in header buttons, create a separate React component or custom hook that calls useRouter, then pass it as the headerLeft value wrapped in an arrow function: headerLeft: => where CustomHeaderButton is: function CustomHeaderButton { const router = useRouter; return <Pressable onPress={ => router.back}>Back; } This works because the callback returns a React component that properly manages its own hook calls. Official Expo Router docs confirm headerLeft expects a function returning a React element, and React Navigation docs emphasize render functions for headers to enable proper hook usage inside components. Issues in Expo Router repo (#167) explicitly show hook errors in direct callback usage and recommend component wrappers.
Citations:
- 1: https://docs.expo.dev/router/advanced/stack/
- 2: React has detected a change in the order of Hooks called by HeaderConfig. expo/router#167
- 3: Passing a React Component (without wrapping it in a function) to
headerandheaderLeftmakes anyuseNavigationcall in the custom headerLeft component return the wrong navigator react-navigation/react-navigation#12076 - 4: https://reactnavigation.org/docs/header-buttons/
- 5: https://stackoverflow.com/questions/79515467/expo-react-native-stack-headerright-with-custom-component-does-not-redirect-usin
Extract useRouter() into a separate component—calling hooks inside headerLeft callbacks violates the Rules of Hooks.
The headerLeft callback is a plain function invoked by React Navigation, not a React component, so hook calls there will fail or produce unpredictable behavior. Create a dedicated component that calls useRouter() and return it from headerLeft:
Example fix
const CancelTripButton = ({ onCancel }: { onCancel: () => void }) => {
const router = useRouter();
return (
<Pressable
testID={TestIds.CancelTripFormButton}
onPress={() => router.back()}
className="px-2"
>
<Text className="text-primary">Cancel</Text>
</Pressable>
);
};
const getTripNewOptions = (t: TranslationFunction) => ({
title: t('trips.createTrip'),
presentation: 'modal' as const,
animation: 'slide_from_bottom' as const,
headerLeft: () => <CancelTripButton onCancel={() => {}} />,
});Also applies to: lines 358–374 in getPackNewOptions()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/expo/app/`(app)/_layout.tsx around lines 321 - 337, The headerLeft
callbacks in getTripNewOptions and getPackNewOptions call the useRouter() hook
directly which violates the Rules of Hooks; extract the Pressable into a
dedicated React component (e.g., CancelTripButton) that calls useRouter()
internally and accepts an onCancel prop, then replace headerLeft: () => { ... }
with headerLeft: () => <CancelTripButton onCancel={...} /> so hooks are only
used inside a component.
Comment on lines
306
to
+313
| setToken(null); | ||
| setRefreshToken(null); | ||
| await clearLocalData(); | ||
| await Updates.reloadAsync(); | ||
| userStore.set(null); | ||
| isAuthed.set(false); | ||
| await new Promise<void>((resolve) => setTimeout(resolve, 50)); | ||
| // safe-cast: '/auth' is a compile-time string literal; Expo Router's Href accepts string paths directly. | ||
| router.replace('/auth' as Href); |
Contributor
There was a problem hiding this comment.
Flip isAuthed before clearing tokens in deleteAccount().
signOut() now sets isAuthed first so the refresh guards in apps/expo/lib/api/packrat.ts:28-43 stop writing refreshed tokens back into auth state. This path still clears tokens/storage while isAuthed is true, so an in-flight refresh can repopulate auth state after account deletion. Move isAuthed.set(false) to the top of this block as well.
Possible fix
- setToken(null);
- setRefreshToken(null);
- await clearLocalData();
- userStore.set(null);
- isAuthed.set(false);
+ isAuthed.set(false);
+ setToken(null);
+ setRefreshToken(null);
+ await clearLocalData();
+ userStore.set(null);
await new Promise<void>((resolve) => setTimeout(resolve, 50));
router.replace('/auth' as Href);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/expo/features/auth/hooks/useAuthActions.ts` around lines 306 - 313, In
deleteAccount(), move the isAuthed.set(false) call to before clearing
tokens/storage so the auth-refresh guards behave like signOut(); specifically,
call isAuthed.set(false) at the start of the block that currently calls
setToken(null), setRefreshToken(null), await clearLocalData(),
userStore.set(null) and router.replace('/auth'), ensuring no in-flight refresh
can repopulate auth state after deletion; update the block around the
deleteAccount() function (and ensure consistency with signOut()) so isAuthed is
flipped first, then null out tokens via setToken/setRefreshToken, call
clearLocalData(), update userStore, wait, and finally router.replace('/auth').
Comment on lines
+40
to
+52
| const parseResult = CatalogItemsResponseSchema.safeParse(data); | ||
| if (parseResult.success) return parseResult.data; | ||
| // Fallback: strict per-field validation failed (e.g., live DB has weightUnit values outside the | ||
| // enum like 'lbs'). Return raw items so the UI still renders rather than entering error state. | ||
| return { | ||
| items: (data?.items ?? []) as unknown as ReturnType< | ||
| typeof CatalogItemsResponseSchema.parse | ||
| >['items'], | ||
| totalCount: Number(data?.totalCount ?? 0), | ||
| page: Number(data?.page ?? 1), | ||
| limit: Number(data?.limit ?? 20), | ||
| totalPages: Number(data?.totalPages ?? 1), | ||
| }; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Log the schema-fallback path before returning raw catalog data.
Keeping the UI alive here makes sense, but this now silently masks API/schema drift. At minimum, emit a warning or telemetry with parseResult.error before falling back so invalid payloads can be diagnosed instead of persisting unnoticed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/expo/features/catalog/hooks/useCatalogItems.ts` around lines 40 - 52,
Before returning the fallback raw catalog object in useCatalogItems.ts when
CatalogItemsResponseSchema.safeParse(data) fails, emit a warning or telemetry
that includes parseResult.error so API/schema drift is visible; locate the
parseResult variable (result of CatalogItemsResponseSchema.safeParse) and add a
single logging/telemetry call (e.g., logger.warn/console.warn or
telemetry.captureException) that records a concise message plus
parseResult.error before returning the raw
items/totalCount/page/limit/totalPages fallback.
Comment on lines
+124
to
+128
| } else { | ||
| createPack({ ...value, category: value.category }); | ||
| Burnt.toast({ title: t('packs.packCreatedSuccess'), preset: 'done' }); | ||
| } | ||
| router.back(); |
Contributor
There was a problem hiding this comment.
Await createPack() before showing success and closing the form.
The create branch still fires createPack() without await, so the success toast and router.back() run before the pack is persisted. If the request fails, that rejection also bypasses this try/catch.
Suggested fix
} else {
- createPack({ ...value, category: value.category });
+ await createPack({ ...value, category: value.category });
Burnt.toast({ title: t('packs.packCreatedSuccess'), preset: 'done' });
}
router.back();📝 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
| } else { | |
| createPack({ ...value, category: value.category }); | |
| Burnt.toast({ title: t('packs.packCreatedSuccess'), preset: 'done' }); | |
| } | |
| router.back(); | |
| } else { | |
| await createPack({ ...value, category: value.category }); | |
| Burnt.toast({ title: t('packs.packCreatedSuccess'), preset: 'done' }); | |
| } | |
| router.back(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/expo/features/packs/components/PackForm.tsx` around lines 124 - 128, In
the create branch of PackForm.tsx, await the async createPack(...) call before
showing the success toast and calling router.back so failures are caught and the
UI only advances on success; specifically, change the flow around createPack,
Burnt.toast and router.back so createPack is awaited (await createPack({
...value, category: value.category })) and only after that resolves call
Burnt.toast(t('packs.packCreatedSuccess')) and router.back(); ensure you do not
append a .catch that circumvents the surrounding try/catch so errors propagate
to the existing error handler.
Comment on lines
+15
to
+18
| const listTrips = async (getParams: any) => { | ||
| // Force merge mode on every list sync (including initial when lastSync is null), | ||
| // so obs$.set() is never called and local-only items are never wiped. | ||
| getParams.mode = 'merge'; |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
cat -n apps/expo/features/trips/store/trips.ts | head -30Repository: PackRat-AI/PackRat
Length of output: 1665
🏁 Script executed:
rg "listTrips" apps/expo/features/trips/store/ -A 3 -B 3Repository: PackRat-AI/PackRat
Length of output: 1154
🏁 Script executed:
cat -n apps/expo/features/trips/store/trips.ts | sed -n '30,60p'Repository: PackRat-AI/PackRat
Length of output: 1852
🏁 Script executed:
cat -n apps/expo/features/trips/store/trips.ts | sed -n '60,100p'Repository: PackRat-AI/PackRat
Length of output: 1271
Replace any with a minimal type definition and add a type guard before mutating getParams.
The parameter typed as any violates the TypeScript guideline. Since getParams is mutated directly on line 18 without a type guard, add defensive narrowing:
Suggested fix
-// biome-ignore lint/suspicious/noExplicitAny: crud.js getParams is untyped
-const listTrips = async (getParams: any) => {
+type ListTripsParams = { mode?: string };
+const listTrips = async (getParams: ListTripsParams | undefined) => {
// Force merge mode on every list sync (including initial when lastSync is null),
// so obs$.set() is never called and local-only items are never wiped.
- getParams.mode = 'merge';
+ if (getParams && typeof getParams === 'object') {
+ getParams.mode = 'merge';
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/expo/features/trips/store/trips.ts` around lines 15 - 18, The listTrips
function takes getParams typed as any and directly mutates getParams.mode =
'merge'; replace the any with a minimal interface (e.g., an object type that
optionally includes mode: string and other expected fields) and add a runtime
type guard before mutating (check that getParams is a non-null object and typeof
getParams === 'object' and that it's safe to assign mode) so you only set
getParams.mode when the parameter conforms to the expected shape; update the
function signature for listTrips(getParams: YourParamsType) and perform the
guard before the assignment to avoid unsafe mutations.
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.
Root Causes
iOS:
email-inputnot found (all runs)TextFieldfrom NativeWindUI inside anaccessible={false}FormSection doesn't exposetestIDto Maestro's XCTest accessibility snapshot. The screenshot from CI (run 24836570288) showed the login form loading correctly with "Email" and "Password" placeholder text visible — butid: "email-input"was never found.Fix: Use placeholder text selectors (
text: "Email"/text: "Password") on iOS. Android keepsid:selectors which work via resource-id.Android:
sign-in-email-buttonnot found (all runs)The Android emulator's System UI raises an ANR dialog ("System UI isn't responding") during boot. This dialog overlays the PackRat auth screen, blocking all taps. The CI screenshot confirmed the auth screen was loaded in the background but the ANR was on top.
Fix: Add conditional ANR dismissal (
runFlow when: visible: "isn't responding") inclear-state.yamlandlogin-flow.yaml. Also add a 10s stability wait + KEYCODE_BACK cleanup in the CI script to reduce ANR frequency.Changes
login-flow.yaml: platform-specific email/password selectors + ANR dismissal +extendedWaitUntilguard before first tapclear-state.yaml: ANR dismissal afterlaunchApp: clearState: truee2e-tests.yml: Android script step addssleep 10+adb input keyevent KEYCODE_BACKafter installdashboard-tiles-flow.yaml: fix tab label"Home"→"Dashboard"(translation keynavigation.dashboard= "Dashboard")catalog-browse-flow.yaml/catalog-item-detail-flow.yaml:extendedWaitUntilfor items to load before scrollinginvalid-login-flow.yaml: useid: "continue-button"(avoids iOS/Android button label difference), add ANR handlingTest Plan
email-inputfailure)sign-in-email-buttonfailure)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Improvements