fix(e2e): fix iOS login testID and Android ANR failures in Maestro#2274
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces frozen Bun installs in CI, adds E2E build-artifact caching and emulator robustness, broad Maestro E2E flow platform stabilizations and testID instrumentation, converts auth to an explicit observable with sign-out signaling, exposes packs/trips refresh helpers, and adds multiple UI/testID and API catalog sort handling updates. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as "GitHub Actions"
participant Maestro as "Maestro (E2E)"
participant Emulator as "Device/Emulator"
participant App as "Expo App"
participant API as "Backend API"
GH->>Maestro: Start E2E job (uses updated workflow)
Maestro->>Emulator: Launch device, apply platform-specific flow
Emulator->>App: Dismiss ANR/keychain dialogs (BACK/sleep), start app
App->>API: Fetch catalog/packs/trips (optional sort)
API-->>App: Return data (safeParse fallback if needed)
Maestro->>App: Drive UI via testIDs (tap/wait/assert)
Maestro-->>GH: Upload artifacts & test results (artifact-cache)
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 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 |
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.
Coverage Report for Expo Unit Tests Coverage (./apps/expo)
File Coverage
|
||||||||||||||||||||||||||||||||||||||
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
Deploying packrat-guides with
|
| Latest commit: |
757c53e
|
| Status: | ⚡️ Build in progress... |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
packrat-admin | 757c53e | Commit Preview URL Branch Preview URL |
May 01 2026, 06:34 PM |
github-actions
Bot
added
the
dependencies
Deploying packrat-landing with
|
| Latest commit: |
757c53e
|
| Status: | 🚫 Build failed. |
…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}
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 |
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.
| 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 |
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.
| 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 |
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.
| 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 |
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.
| 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 |
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.
| 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.
| 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> | ||
| ); | ||
| }, | ||
| }); |
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.
| 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); |
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').
| 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), | ||
| }; |
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.
| } else { | ||
| createPack({ ...value, category: value.category }); | ||
| Burnt.toast({ title: t('packs.packCreatedSuccess'), preset: 'done' }); | ||
| } | ||
| router.back(); |
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.
| } 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.
| 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'; |
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.
navigation.getParent() in AppLayout returns expo-router's internal ContextNavigator navigation (INTERNAL_SLOT_NAME only — no 'auth' route), not the user's root Stack, so the reset silently failed. navigation from useNavigation() in AppLayout IS the user's root Stack nav. Direct dispatch bypasses listeners.focus entirely and resets auth's sub-state to [index].
… sign-in content signOut() sets isLoadingAtom=true and never resets it, relying on AppLayout's spinner to drive navigation. But auth/index.tsx also reads isLoadingAtom via useAuth() and gates behind its own full-screen spinner when it's true. After the CommonActions.reset dispatches the root Stack to 'auth', auth/index mounts but immediately shows a spinner — hiding the "Sign In" button that Maestro asserts on. Reset isLoadingAtom after the dispatch so auth/index renders its content on mount.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/expo/app/(app)/_layout.tsx (1)
327-343:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove
useRouter()out of theheaderLeftcallbacks.This is still the same Rules of Hooks violation noted earlier:
headerLeftis a plain render callback, so callinguseRouter()there can throw or behave unpredictably when these screens mount. Return a component fromheaderLeftand let that component own the hook.Proposed fix
+type ModalCancelButtonProps = { + label: string; + testID: string; +}; + +function ModalCancelButton({ label, testID }: ModalCancelButtonProps) { + const router = useRouter(); + + return ( + <Pressable testID={testID} onPress={() => router.back()} className="px-2"> + <Text className="text-primary">{label}</Text> + </Pressable> + ); +} + 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> - ); - }, + headerLeft: () => ( + <ModalCancelButton + testID={TestIds.CancelTripFormButton} + label={t('common.cancel')} + /> + ), }); const getPackNewOptions = (t: TranslationFunction) => ({ title: t('packs.createPack'), presentation: 'modal' as const, animation: 'fade_from_bottom' as const, - headerLeft: () => { - const router = useRouter(); - return ( - <Pressable - testID={TestIds.CancelPackFormButton} - onPress={() => router.back()} - className="px-2" - > - <Text className="text-primary">{t('common.cancel')}</Text> - </Pressable> - ); - }, + headerLeft: () => ( + <ModalCancelButton + testID={TestIds.CancelPackFormButton} + label={t('common.cancel')} + /> + ), });Also applies to: 364-380
🤖 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 327 - 343, getTripNewOptions currently calls useRouter() inside the headerLeft render callback (Rules of Hooks violation); extract that hook into a small component (e.g., HeaderCancel or TripFormCancelButton) that accepts the translation function/props and calls useRouter() inside it, then have getTripNewOptions.headerLeft return that component (e.g., headerLeft: () => <HeaderCancel t={t} />). Update the analogous headerLeft instance mentioned for lines 364-380 the same way so no render callbacks call hooks directly; ensure the new component uses TestIds.CancelTripFormButton, onPress={() => router.back()}, and renders the translated cancel text.
🤖 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/app/`(app)/_layout.tsx:
- Around line 57-60: Replace the fixed setTimeout-based spinner hide with
navigation-state tracking: after calling
navigation.dispatch(CommonActions.reset(...)) subscribe to the navigation state
change with navigation.addListener('state', handler) (or the appropriate
'state'/'transitionEnd' event for your navigator), move the setIsLoading(false)
call into that handler, and remove the listener once fired (or use the returned
unsubscribe). Update any use of isLoadingAtom/state so the atom is cleared
inside the listener instead of via setTimeout(…,50). Ensure you still handle a
fallback/timeout only if absolutely needed for robustness.
---
Duplicate comments:
In `@apps/expo/app/`(app)/_layout.tsx:
- Around line 327-343: getTripNewOptions currently calls useRouter() inside the
headerLeft render callback (Rules of Hooks violation); extract that hook into a
small component (e.g., HeaderCancel or TripFormCancelButton) that accepts the
translation function/props and calls useRouter() inside it, then have
getTripNewOptions.headerLeft return that component (e.g., headerLeft: () =>
<HeaderCancel t={t} />). Update the analogous headerLeft instance mentioned for
lines 364-380 the same way so no render callbacks call hooks directly; ensure
the new component uses TestIds.CancelTripFormButton, onPress={() =>
router.back()}, and renders the translated cancel text.
🪄 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: 56b5523b-2c00-4843-a0f6-b073527971c2
📒 Files selected for processing (1)
apps/expo/app/(app)/_layout.tsx
| // Small delay lets the navigation commit before the state change propagates | ||
| // to auth/index, avoiding a race between the spinner and the route render. | ||
| setTimeout(() => setIsLoading(false), 50); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd apps/expo && find . -type f -name "_layout.tsx" | head -20Repository: PackRat-AI/PackRat
Length of output: 488
🏁 Script executed:
cat -n apps/expo/app/\(app\)/_layout.tsx | sed -n '45,75p'Repository: PackRat-AI/PackRat
Length of output: 1736
🏁 Script executed:
rg "isLoadingAtom" apps/expo --type ts --type tsx -B 2 -A 2Repository: PackRat-AI/PackRat
Length of output: 90
🏁 Script executed:
rg "useNavigation\|useNavigationState\|useRouter\|navigationState" apps/expo/app/\(app\)/_layout.tsx -B 2 -A 2Repository: PackRat-AI/PackRat
Length of output: 44
🏁 Script executed:
rg "isLoadingAtom" apps/expo --type ts -B 2 -A 2 | head -50Repository: PackRat-AI/PackRat
Length of output: 4148
🏁 Script executed:
rg "useNavigation|useNavigationState|useRouter|navigationState" apps/expo/app/\(app\)/_layout.tsx -B 2 -A 2Repository: PackRat-AI/PackRat
Length of output: 1397
🏁 Script executed:
head -50 apps/expo/app/\(app\)/_layout.tsxRepository: PackRat-AI/PackRat
Length of output: 2920
🏁 Script executed:
rg "addListener|beforeRemove|onStateChange" apps/expo/app/ -A 3Repository: PackRat-AI/PackRat
Length of output: 44
🏁 Script executed:
cat -n apps/expo/app/\(app\)/_layout.tsx | sed -n '40,100p'Repository: PackRat-AI/PackRat
Length of output: 3570
🏁 Script executed:
find apps/expo -name "index.tsx" -path "*/auth/*" -exec grep -l "isLoadingAtom" {} \;Repository: PackRat-AI/PackRat
Length of output: 44
🏁 Script executed:
rg "isLoadingAtom" apps/expo/app/auth/ -B 3 -A 3 | head -60Repository: PackRat-AI/PackRat
Length of output: 44
🏁 Script executed:
find apps/expo -name "index.tsx" -path "*auth*" | head -10Repository: PackRat-AI/PackRat
Length of output: 174
🏁 Script executed:
cat apps/expo/app/auth/index.tsx | head -100Repository: PackRat-AI/PackRat
Length of output: 3682
🏁 Script executed:
rg "isLoadingAtom" apps/expo --type ts --type js -B 2 -A 2 | grep -A 5 -B 5 "auth.*index"Repository: PackRat-AI/PackRat
Length of output: 1036
🏁 Script executed:
rg "navigation\.(addListener|onStateChange|getState)" apps/expo -A 3Repository: PackRat-AI/PackRat
Length of output: 376
🏁 Script executed:
rg "beforeRemove|blur|focus" apps/expo --type ts --type js -B 1 -A 1 | head -40Repository: PackRat-AI/PackRat
Length of output: 2968
🏁 Script executed:
rg "getState|getParent" apps/expo/app/\(app\)/_layout.tsx -B 2 -A 2Repository: PackRat-AI/PackRat
Length of output: 457
🏁 Script executed:
cat apps/expo/features/auth/hooks/useAuthInit.tsRepository: PackRat-AI/PackRat
Length of output: 2065
🏁 Script executed:
rg "CommonActions" apps/expo -B 2 -A 5 | head -50Repository: PackRat-AI/PackRat
Length of output: 2289
🏁 Script executed:
cat -n apps/expo/features/auth/hooks/useAuthActions.ts | sed -n '1,80p'Repository: PackRat-AI/PackRat
Length of output: 3420
🏁 Script executed:
rg "navigation\.dispatch\|navigation\.reset" apps/expo -B 3 -A 3Repository: PackRat-AI/PackRat
Length of output: 44
🏁 Script executed:
rg "addListener.*beforeRemove\|addListener.*state" apps/expo --type ts -B 2 -A 4Repository: PackRat-AI/PackRat
Length of output: 44
🏁 Script executed:
cat apps/expo/features/auth/hooks/useAuthActions.ts | sed -n '70,150p'Repository: PackRat-AI/PackRat
Length of output: 2734
🏁 Script executed:
rg "setTimeout" apps/expo -B 2 -A 2 | grep -A 2 -B 2 "50\|isLoading"Repository: PackRat-AI/PackRat
Length of output: 3601
🏁 Script executed:
rg "requestAnimationFrame\|requestIdleCallback" apps/expo --type tsRepository: PackRat-AI/PackRat
Length of output: 44
🏁 Script executed:
cat apps/expo/package.json | grep -A 2 -B 2 "react-navigation"Repository: PackRat-AI/PackRat
Length of output: 433
🏁 Script executed:
cat apps/expo/app/\(app\)/_layout.tsx | sed -n '49,62p'Repository: PackRat-AI/PackRat
Length of output: 600
🏁 Script executed:
rg "navigation.getState\|onTransitionEnd\|transitionEnd" apps/expo --type tsRepository: PackRat-AI/PackRat
Length of output: 44
🏁 Script executed:
rg "addListener.*state\|addEventListener.*state" apps/expo --type ts -B 1 -A 3Repository: PackRat-AI/PackRat
Length of output: 44
🏁 Script executed:
grep -r "addListener" apps/expo --include="*.ts" --include="*.tsx" | head -20Repository: PackRat-AI/PackRat
Length of output: 287
Replace the 50ms timer with navigation-state tracking.
The fixed 50ms delay is unreliable on slower devices or under load. After navigation.dispatch(CommonActions.reset(...)), use navigation.addListener('state', ...) or similar to detect when the navigation state change has actually completed, then clear isLoadingAtom. This ensures the spinner waits for the stack reset rather than an arbitrary duration.
🤖 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 57 - 60, Replace the fixed
setTimeout-based spinner hide with navigation-state tracking: after calling
navigation.dispatch(CommonActions.reset(...)) subscribe to the navigation state
change with navigation.addListener('state', handler) (or the appropriate
'state'/'transitionEnd' event for your navigator), move the setIsLoading(false)
call into that handler, and remove the listener once fired (or use the returned
unsubscribe). Update any use of isLoadingAtom/state so the atom is cleared
inside the listener instead of via setTimeout(…,50). Ensure you still handle a
fallback/timeout only if absolutely needed for robustness.
iOS CI has slow post-login navigation settling; 20s was not always enough
for expo-router to push (app)/(tabs)/(home) back into the stack after
router.dismissTo('/') resolves. Increase to 35s to match the login-modal
appearance timeout already used earlier in the same flow.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.maestro/flows/auth/login-flow.yaml:
- Around line 92-104: The current retry block uses runFlow with when: visible
which only checks once and can miss async alerts; modify the flow to first call
assertVisible for the expected post-submit state (poll up to 7s) to wait for the
normal outcome, then add a runFlow that checks/dismisses the "Network request
failed" alert (using the existing runFlow + when: visible / tapOn "OK" /
waitForAnimationToEnd / tapOn id: "continue-button" / waitForAnimationToEnd) so
the alert is handled within the polling window; reference the existing runFlow
and replace its immediate check with an assertVisible followed by the
conditional runFlow to ensure retries execute reliably.
🪄 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: 6a7b5482-9d88-4dea-b566-1725c041577f
📒 Files selected for processing (1)
.maestro/flows/auth/login-flow.yaml
| # Handle transient network error: if the API call failed (e.g. "Network request | ||
| # failed" dialog), dismiss the alert and retry once with the same credentials. | ||
| - runFlow: | ||
| when: | ||
| visible: | ||
| text: "Network request failed" | ||
| commands: | ||
| - tapOn: | ||
| text: "OK" | ||
| - waitForAnimationToEnd | ||
| - tapOn: | ||
| id: "continue-button" | ||
| - waitForAnimationToEnd |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Maestro YAML, does runFlowwithwhen: visible wait for an element to appear, or is the condition evaluated only once when the command is reached? What is the recommended pattern for handling an alert that may appear asynchronously after a submit action?
💡 Result:
In Maestro YAML, runFlow with when: visible evaluates the condition once when the command is reached in the flow execution. If false, it skips the runFlow and continues to the next command without waiting or polling. This differs from assertions like assertVisible, which poll the UI for up to 7 seconds (retrying if initially not visible). The recommended pattern for handling an alert that may appear asynchronously after a submit action is to use runFlow with when: visible on the alert element right after the submit. This checks if the alert is present at that moment (after any automatic waits from prior commands) and dismisses it if visible, skipping silently if not. Example: - tapOn: "Submit" - runFlow: when: visible: "Alert Title" # or reliable selector for alert commands: - tapOn: "Dismiss" # or handle alert For alerts taking longer, precede with assertVisible on expected post-submit content or use extendedWaitUntil first, but for optional alerts, the conditional runFlow is idiomatic and prevents flakiness.
Citations:
- 1: https://docs.maestro.dev/maestro-flows/flow-control-and-logic/conditions
- 2: https://maestro.mobile.dev/maestro-flows/flow-control-and-logic/conditions
- 3: https://docs.maestro.dev/maestro-flows/flow-control-and-logic/wait-commands
- 4: https://docs.maestro.dev/reference/commands-available/assertvisible
- 5: https://maestro.dev/insights/dynamic-ui-testing-yaml-best-practices
The when: visible condition is evaluated once; async alerts appearing after this check will bypass the retry block.
runFlow with when: visible does not poll—it checks if the dialog is visible at the moment the command is reached. If the "Network request failed" alert appears after this point, the retry is skipped and execution continues behind the blocking modal. Add an assertVisible check for the expected post-submit state first (which polls for up to 7 seconds), then conditionally dismiss the alert with the runFlow. This ensures the alert is handled within a predictable window.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.maestro/flows/auth/login-flow.yaml around lines 92 - 104, The current retry
block uses runFlow with when: visible which only checks once and can miss async
alerts; modify the flow to first call assertVisible for the expected post-submit
state (poll up to 7s) to wait for the normal outcome, then add a runFlow that
checks/dismisses the "Network request failed" alert (using the existing runFlow
+ when: visible / tapOn "OK" / waitForAnimationToEnd / tapOn id:
"continue-button" / waitForAnimationToEnd) so the alert is handled within the
polling window; reference the existing runFlow and replace its immediate check
with an assertVisible followed by the conditional runFlow to ensure retries
execute reliably.
Biome's import sorting rule requires alphabetical order within named imports; `router` must come before `Stack`. This was blocking the previous commit that switched sign-out navigation to router.replace.
…eout
The previous approach called setTimeout(setIsLoading(false), 50) from
AppLayout's useEffect — fired ~50ms after router.replace('/auth'). On iOS,
navigation transitions take ~300ms, so the atom reset caused AppLayout to
re-render its Stack mid-transition, which could cancel the in-flight
router.replace and leave the app on the signed-out app screen rather than
reaching the auth screen.
Move the reset to a useEffect in auth/index.tsx that fires only after the
auth screen has mounted (i.e., after navigation is fully committed). This
guarantees the sign-in buttons are visible immediately after the transition
and Maestro's extendedWaitUntil can find sign-in-email-button.
…ack re-init
After router.replace('/auth') fires from AppLayout's useEffect, auth/index.tsx
mounts and immediately resets isLoadingAtom=false via its own useEffect. That
atom reset caused AppLayout to re-render without the spinner, which re-mounted
the inner (app) Stack navigator. The re-initialising Stack navigator re-registered
with React Navigation while the root navigator was still committing the replace,
overriding the in-flight navigation and landing the user back on Trips/Profile
instead of the auth screen (confirmed by CI failure screenshot).
Fix: latch hasNavigatedToAuthRef=true before dispatching router.replace. Include
the ref in the spinner condition so AppLayout keeps showing the spinner regardless
of isLoadingAtom changes, until the component actually unmounts after the
transition completes. auth/index.tsx still resets isLoadingAtom=false on mount
so its own sign-in buttons become visible once navigation fully commits.
Custom portal-based AlertComponent dialogs are not surfaced in XCTest's accessibility tree on iOS, so Maestro's tapOn: text: "Log Out" couldn't reach the dialog button — the "Sync in progress" dialog remained open and handleSignOut() was never called, blocking the logout E2E assertion. Switch both platforms to native Alert.alert() (already done for Android with this rationale). Remove the now-unused AlertComponent import, alertRef, and AlertMethods type import from the ListFooterComponent.
On iOS, native UIAlertController overlays the Profile screen but both the profile list "Log Out" button (index 0) and the alert action (index 1) are present in the XCTest accessibility tree. Without an index, Maestro taps the list button, which re-triggers the dialog instead of confirming sign-out. Split the sync-conflict handler into platform branches so iOS uses index: 1 (alert action) while Android continues to use text-only matching (alert covers the screen, list button unreachable).
CI network latency can delay the catalog item detail data load beyond the previous 15s limit. Screenshot confirms the app navigated to the detail screen correctly but was still showing a loading spinner at assertion time. 30s matches the timeout used for catalog-browse-flow.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
apps/expo/app/(app)/_layout.tsx (1)
17-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove
useRouter()out ofheaderLeftcallbacks.
useRouter()is being called inside screen-option callbacks (headerLeft), which are not safe hook call sites. Use the module-levelrouter(already imported) or a dedicated React component for the header button.Suggested fix
-import { type Href, router, Stack, useRouter } from 'expo-router'; +import { type Href, router, Stack } from 'expo-router'; 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> - ); - }, + headerLeft: () => ( + <Pressable + testID={TestIds.CancelTripFormButton} + onPress={() => router.back()} + className="px-2" + > + <Text className="text-primary">{t('common.cancel')}</Text> + </Pressable> + ), }); const getPackNewOptions = (t: TranslationFunction) => ({ title: t('packs.createPack'), presentation: 'modal' as const, animation: 'fade_from_bottom' as const, - headerLeft: () => { - const router = useRouter(); - return ( - <Pressable - testID={TestIds.CancelPackFormButton} - onPress={() => router.back()} - className="px-2" - > - <Text className="text-primary">{t('common.cancel')}</Text> - </Pressable> - ); - }, + headerLeft: () => ( + <Pressable + testID={TestIds.CancelPackFormButton} + onPress={() => router.back()} + className="px-2" + > + <Text className="text-primary">{t('common.cancel')}</Text> + </Pressable> + ), });#!/bin/bash set -euo pipefail # Verify the problematic pattern in this file. cat -n apps/expo/app/\(app\)/_layout.tsx | sed -n '315,372p' rg -n -C2 "headerLeft: \(\) => \{|useRouter\(" apps/expo/app/\(app\)/_layout.tsxExpected result:
useRouter(appears inside theheaderLeftcallback blocks for both trip/pack modal options.Also applies to: 322-334, 355-371
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/app/`(app)/_layout.tsx at line 17, The headerLeft callbacks currently call useRouter() (unsafe hook usage) inside the screen options for the trip and pack modal options; move the hook out by either using the already-imported module-level router (router.push, router.replace, router.back) inside those headerLeft functions or replace headerLeft with a dedicated React component (e.g., HeaderButton) that calls useRouter() at component scope and performs the navigation, then reference that component from the headerLeft; update the trip/pack modal option blocks (headerLeft) to use router or the new HeaderButton instead of calling useRouter() inline.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/expo/app/`(app)/_layout.tsx:
- Line 17: The headerLeft callbacks currently call useRouter() (unsafe hook
usage) inside the screen options for the trip and pack modal options; move the
hook out by either using the already-imported module-level router (router.push,
router.replace, router.back) inside those headerLeft functions or replace
headerLeft with a dedicated React component (e.g., HeaderButton) that calls
useRouter() at component scope and performs the navigation, then reference that
component from the headerLeft; update the trip/pack modal option blocks
(headerLeft) to use router or the new HeaderButton instead of calling
useRouter() inline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 533ac11c-8530-4fbf-a888-c1a24c3c2c87
📒 Files selected for processing (4)
.maestro/flows/auth/logout-flow.yaml.maestro/flows/catalog/catalog-item-detail-flow.yamlapps/expo/app/(app)/(tabs)/profile/index.tsxapps/expo/app/(app)/_layout.tsx
Maestro log from run 25218877925 proved that index:1 for text 'Log Out' landed on resource-id=sign-out-button (y=706, the profile list button), re-triggering the dialog. The UIAlertController action is at index 0 in XCTest because foreground window elements appear before main-window ones. Remove explicit index so tapOn defaults to 0 (the alert action). Add waitForAnimationToEnd inside the conditional so the native alert is fully settled before the tap fires, fixing a separate timing issue seen in run 25214840568 where the conditional fired before the alert presented.
iOS 'Save Password?' prompt can appear while the create-pack modal is animating, dismissing or blocking the modal. After handling 'Not Now', re-tap the create-pack-button if pack-name-input is not yet visible so we don't fail on the field lookup. Also replace the bare tapOn wait with an explicit extendedWaitUntil so the form has time to be in the XCTest accessibility tree before we interact with it.
With snapshotKeyHonorModalViews:false, the XCTest snapshot includes both the main window and alert window elements, with main-window elements appearing first. Both no-index and explicit index values were landing on resource-id=sign-out-button (y=706, the profile list button). Switch to rightOf:text:"Cancel" selector so we unambiguously target the UIAlertController "Log Out" action, which sits horizontally adjacent to the "Cancel" button (y≈430). "Cancel" only exists inside the dialog, so this selector cannot match the profile list button. Evidence: run 25223147832 log shows tapOn:text:"Log Out" with no index hitting accessibilityText=Log Out, resource-id=sign-out-button, y=706.
Resolves conflicts in testIds.ts and PackForm.tsx.
testIds.ts: takes development's namespaced `testIds` structure;
preserves Maestro-facing string values for pack name/description inputs
('pack-name-input', 'pack-description-input') so existing Maestro
create-pack-flow selectors continue to match; adds deprecated `TestIds`
backward-compat export for components not yet migrated to the new API.
PackForm.tsx: takes development's `testID={testIds.packs.*}` props,
removing the old `containerTestID`/`containerAccessibilityLabel` pattern.
… timeouts (#2274) fix(e2e): fix iOS login testID and Android ANR failures in Maestro
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
Behavior Changes