Fix: Error page on second trip edit attempt (Android)

[Copilot]

• Fix crash in EditTripScreen by replacing assertDefined(trip) with a loading state (same pattern as TripDetailScreen)
• Initialize tripLocationStore with the trip's current location on edit screen mount to prevent stale location data and fix location data loss bug
• Split useEffect — cleanup now runs in its own empty-deps effect (unmount only), preventing any risk of clearing in-progress location edits on sync-induced re-runs
• Add proper "not found" fallback using tripsSyncState.isLoaded — shows Back button for stale/deleted trip IDs instead of an infinite spinner

Original prompt

---

This section details on the original issue you should resolve

<issue_title>Android - Error page appears on second edit attempt (Trips tab)</issue_title>
<issue_description>Description

• An error page appears after using the Edit function for a trip in the Trips tab. The first edit attempt works successfully, but the error consistently occurs on the second edit attempt for the same trip.

Steps to Reproduce

• Go to the Trips tab
• Select an existing trip
• Click Edit
• Modify any field
• Save → ✅ Works as expected
• Click Edit again on the same trip
• ❌ Redirects to error page

Screenshots / Screen Recordings
https://github.com/user-attachments/assets/542a5a20-fb28-4807-9ec6-5c7321cebcb6

Expected Behavior
The trip should update successfully every time and return to the trip details or trips list view.

Actual Behavior
The second edit attempt results in an error page.

Environment

• Platform: Android
• Environment: (dev/staging/prod)

Notes

• First edit works normally
• Issue occurs consistently on second attempt</issue_description>

Comments on the Issue (you are @copilot in this section)

@andrew-bierman @copilot Assigned for POC development. Please investigate and create a proof of concept implementation.

• Fixes Android - Error page appears on second edit attempt (Trips tab) #1852

---

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Summary by CodeRabbit

• Improvements
  • Added a visual loading indicator when opening the edit trip screen, providing clear feedback to users during the trip data initialization and preparation process.
  • Enhanced trip state management with improved data handling to ensure robust app stability, effective error prevention, and an overall more reliable and seamless trip editing experience.

[andrew-bierman]

@coderabbitai Please review this PR for code quality, best practices, and potential issues.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get the PR diff and changed files
gh pr view 1898 --repo PackRat-AI/PackRat --json files,title,body,additions,deletions
gh pr diff 1898 --repo PackRat-AI/PackRat

Length of output: 5006

---

🏁 Script executed:

#!/bin/bash
# Read surrounding files for context
fd "TripDetailScreen" --type f | xargs cat
fd "tripLocationStore" --type f | xargs cat
fd "useTripDetailsFromStore" --type f | xargs cat
cat apps/expo/features/trips/screens/EditTripScreen.tsx

Length of output: 10103

---

@andrew-bierman: The fix is well-targeted and follows existing patterns correctly. Here's a full review:

---

✅ What's done well

• The ActivityIndicator + SafeAreaView loading state exactly mirrors the TripDetailScreen pattern — good consistency.
• Seeding tripLocationStore from trip.location on mount fixes the silent data-loss bug cleanly.
• The trip?.id dependency is correctly chosen: since !trip shows a spinner (not the form), the user cannot pick a new location while the trip is transiently undefined. This means the cleanup → re-seed cycle on transient undefined is safe — no in-progress location data is actually lost.
• Retaining assertDefined(effectiveId) is correct — a missing route param is a programmer error, not a runtime loading state.

---

⚠️ Issues to address

1. Perpetual loading spinner on invalid/deleted trip IDs

If effectiveId never resolves to a valid trip (e.g., the trip was deleted remotely, or the ID is stale), !trip will show the ActivityIndicator forever with no way out. The same issue exists in TripDetailScreen, but it's worth fixing here to avoid a silent UX deadlock.

// Consider adding a timeout-based fallback or a "not found" state
const [timedOut, setTimedOut] = useState(false);

useEffect(() => {
  if (trip) return;
  const timer = setTimeout(() => setTimedOut(true), 8000); // 8 s timeout
  return () => clearTimeout(timer);
}, [trip]);

if (!trip) {
  return (
    <SafeAreaView className="flex-1 bg-background items-center justify-center">
      {timedOut ? (
        <Text className="text-muted-foreground">{t('trips.notFound')}</Text>
      ) : (
        <ActivityIndicator />
      )}
    </SafeAreaView>
  );
}

2. biome-ignore suppression can be avoided with two effects

The suppression is well-explained, but the intent is cleaner with two separate effects:

// Effect 1: seed on mount / trip change — exhaustive deps are satisfied
useEffect(() => {
  if (trip?.location !== undefined) {
    tripLocationStore.set(trip.location ?? null);
  }
}, [trip?.id, trip?.location]); // location included here won't re-init during in-progress picks
                                 // because id changes only on trip switch

// Effect 2: cleanup on unmount only
useEffect(() => {
  return () => { tripLocationStore.set(null); };
}, []);

Alternatively, if the current single-effect approach is kept, the biome-ignore comment is at least clear and sufficient.

3. useTripDetailsFromStore uses @ts-ignore (pre-existing)

// `@ts-ignore`: Safe because Legend-State uses Proxy
const trip_ = tripsStore[id].get();

This is not introduced here, but a typed helper (e.g., (tripsStore as Record<string, ...>)[id]) would be safer than suppressing the error globally.

---

🔍 Minor observations

• ActivityIndicator has no size or color props — consistent with TripDetailScreen, but consider using a themed color (e.g., color={colors.primary}) for visual polish.
• TripDetailScreen uses as Trip to bypass the undefined return type, then still guards with if (!trip). The new code correctly avoids the type assertion entirely — this is the better pattern.

---

Summary: The core fix is correct, well-motivated, and consistent with existing patterns. The main actionable items are the perpetual-loading edge case (#1) and the optional biome-ignore refactor (#2).

[andrew-bierman]

@copilot CodeRabbit has reviewed this PR and provided feedback. Please address the review comments and push updates. Once you've made the changes, the monitor will ask CodeRabbit to re-review.

[andrew-bierman]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Warning

Rate limit exceeded

@andrew-bierman has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 0 minutes and 17 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b54d97c8-f248-4400-bd0e-49ef001597c1

📥 Commits

Reviewing files that changed from the base of the PR and between 566762a and 921a0c3.

📒 Files selected for processing (1)

• apps/expo/features/trips/screens/EditTripScreen.tsx

📝 Walkthrough

Walkthrough

Modified EditTripScreen to synchronize trip location data with a store via useEffect, added loading state handling with ActivityIndicator, and improved lifecycle management by clearing store state on component cleanup or trip changes.

Changes

Cohort / File(s)	Summary
Trip Edit State Management apps/expo/features/trips/screens/EditTripScreen.tsx	Added useEffect hook to synchronize trip location with tripLocationStore on trip ID changes, implemented loading state with ActivityIndicator while trip data loads, and added proper cleanup to clear store state.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A store that syncs, a state so clean,
No more ghosts from scenes between,
With effects that flow and cleanup neat,
Second edits now complete! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main fix: resolving the error page that appears on the second trip edit attempt on Android.
Linked Issues check	✅ Passed	The PR directly addresses issue #1852 by replacing assertDefined with a loading state to prevent crashes during async trip resolution, and seeding/resetting tripLocationStore to fix location data loss.
Out of Scope Changes check	✅ Passed	All changes are scoped to fixing the EditTripScreen crash and location storage issues identified in #1852; no unrelated modifications present.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch copilot/fix-edit-error-trips-tab

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

apps/expo/features/trips/screens/EditTripScreen.tsx (1)

5-5: Use the Expo app UI wrappers for this loading state.

This screen is under apps/expo/**, but the loading UI is built from raw react-native primitives. Please switch this branch to the app’s @packrat-ai/nativewindui components for consistency with the rest of the Expo surface. As per coding guidelines, apps/expo/**/*.ts{,x}: Use @packrat-ai/nativewindui components for consistent cross-platform UI in the Expo app.

Also applies to: 28-30

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/expo/features/trips/screens/EditTripScreen.tsx` at line 5, Replace raw
react-native UI primitives in EditTripScreen.tsx with the Expo app's UI
wrappers: remove imports of ActivityIndicator and SafeAreaView from
'react-native' and instead import and use the equivalent components from
'@packrat-ai/nativewindui' (replace usages of ActivityIndicator and SafeAreaView
in the file and specifically the loading block around lines 28-30). Update the
JSX to use the nativewindui equivalents wherever ActivityIndicator and
SafeAreaView are referenced so the screen uses the consistent Expo app UI
components.

🤖 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/features/trips/screens/EditTripScreen.tsx`:
- Around line 26-32: The current early return treats any falsy trip as loading;
instead detect a stale/missing trip by checking that effectiveId exists but the
fetch is not loading/has errored and trip is null, then replace the spinner
block in EditTripScreen with a non-loading fallback UI: show a user-friendly
message that the trip was not found and provide actions (e.g., a Back button or
Create New Trip) wired to the existing navigation methods, while keeping the
ActivityIndicator for the true loading state (when isLoading/isFetching is
true). Ensure you reference and use the same variables used in this component
(trip and effectiveId and the component's navigation handlers) so the fallback
only appears for deleted/stale IDs and not during actual loading.
- Around line 17-24: The current useEffect with dependency [trip?.id] sets
tripLocationStore but its cleanup runs on every dependency change; change this
by removing the cleanup from that effect (keep only the logic that calls
tripLocationStore.set(trip.location ?? null) inside the effect keyed by
trip?.id) and add a second useEffect with an empty dependency array whose sole
purpose is to return a cleanup that clears tripLocationStore
(tripLocationStore.set(null)) on unmount; reference the useEffect that currently
touches tripLocationStore, the trip variable, and tripLocationStore to locate
and split the logic in EditTripScreen.

---

Nitpick comments:
In `@apps/expo/features/trips/screens/EditTripScreen.tsx`:
- Line 5: Replace raw react-native UI primitives in EditTripScreen.tsx with the
Expo app's UI wrappers: remove imports of ActivityIndicator and SafeAreaView
from 'react-native' and instead import and use the equivalent components from
'@packrat-ai/nativewindui' (replace usages of ActivityIndicator and SafeAreaView
in the file and specifically the loading block around lines 28-30). Update the
JSX to use the nativewindui equivalents wherever ActivityIndicator and
SafeAreaView are referenced so the screen uses the consistent Expo app UI
components.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f984bf39-4cf0-448e-8907-543fce142902

📥 Commits

Reviewing files that changed from the base of the PR and between 3f94763 and 566762a.

📒 Files selected for processing (1)

• apps/expo/features/trips/screens/EditTripScreen.tsx

[andrew-bierman]

@copilot Please address the CodeRabbit review feedback on this PR (2 inline comments).

[andrew-bierman]

@copilot Reminder: please address the CodeRabbit review feedback on this PR.

[andrew-bierman]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.