Fix iOS Upcoming Trips selection not updating#1891
Conversation
- Add extraData={selectedTripId} to List so iOS re-renders items on selection change
- Store selectedTripId (string) instead of full trip object to avoid stale refs
- Memoize upcomingTrips with useMemo to prevent unnecessary effect runs
- Use trip id as keyExtractor for stable React reconciliation
- Include id in data items for the list
Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
|
@coderabbitai Please review this PR for code quality, best practices, and potential issues. |
🧩 Analysis chain🏁 Script executed: Length of output: 3125
✅ What's done well
🔴 Potential Bug — Stale
|
| Severity | Issue |
|---|---|
| 🔴 Bug | Stale selectedTripId not cleared when selected trip disappears from list |
| 🟡 Robustness | renderItem index-based lookup on upcomingTrips |
| 🟡 Minor | selectedTrip .find() not memoized |
The stale ID issue (#1) is the only one worth addressing before merge; the others are code quality improvements that could be done as follow-ups.
✅ 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.
📝 WalkthroughWalkthroughThe change refactors the Upcoming Trips screen to use ID-based trip selection instead of object-based selection, addressing an iOS-specific bug where trip selection wasn't updating properly. The selectedTripId state replaces the selectedTrip state, with the selected trip computed from the upcomingTrips list using find(). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 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)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/expo/app/(app)/upcoming-trips.tsx (1)
148-159: Memoize the actualdataprop and stop re-reading rows by index.
upcomingTripsis memoized, butupcomingTrips.map(...)still creates a newdataarray every render, andrenderItemstill couples each row back toupcomingTrips[info.index]. That weakens the referential-stability fix and keeps rows dependent on list ordering. Prefer a memoized list item shape that includes the fields needed for rendering, then useinfo.itemas the source of truth.♻️ Suggested direction
+ const listData = useMemo( + () => + upcomingTrips.map((trip) => ({ + id: trip.id, + trip, + title: trip.name, + subTitle: `${trip.location?.name ?? t('trips.unknown')} • ${formatDate( + trip.startDate, + )} to ${formatDate(trip.endDate)}`, + })), + [upcomingTrips, t], + ); + <List - data={upcomingTrips.map((trip) => ({ - id: trip.id, - title: trip.name, - subTitle: `${trip.location?.name ?? t('trips.unknown')} • ${formatDate( - trip.startDate, - )} to ${formatDate(trip.endDate)}`, - }))} + data={listData} extraData={selectedTripId} keyExtractor={(item) => item.id} renderItem={(info) => { - const trip = upcomingTrips[info.index]; - assertDefined(trip); + const { trip } = info.item; const { status, completion } = getTripStatus(trip, t);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/app/`(app)/upcoming-trips.tsx around lines 148 - 159, upcomingTrips.map(...) is rebuilt every render and renderItem reads rows by index; memoize the mapped data (e.g. const memoizedData = useMemo(() => upcomingTrips.map(t => ({ id: t.id, title: t.name, subTitle: `${t.location?.name ?? t('trips.unknown')} • ${formatDate(t.startDate)} to ${formatDate(t.endDate)}` , raw: t } ), [upcomingTrips, formatDate, t]), pass memoizedData as data and update renderItem to use info.item (and no longer reference upcomingTrips[info.index] or call assertDefined on that index) while keeping keyExtractor as item => item.id so rows are stable and decoupled from list ordering).
🤖 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)/upcoming-trips.tsx:
- Around line 119-123: The effect for selecting a default trip only handles the
initial empty selection; update it to also reset when the currently selected id
is no longer present in upcomingTrips or when the list repopulates. Inside the
existing useEffect (referencing selectedTripId, upcomingTrips,
setSelectedTripId), add a branch that checks if selectedTripId is non-null and
not found via upcomingTrips.some(t => t.id === selectedTripId) and in that case
call setSelectedTripId(upcomingTrips[0]?.id ?? undefined); also keep the
existing branch that sets the first id when nothing is selected and
upcomingTrips.length > 0, and ensure you still clear selection
(setSelectedTripId(undefined)) when upcomingTrips is empty.
- Around line 110-113: The upcomingTrips useMemo is using Date.now() inside its
filter but only depends on trips, so the list won’t update as time passes;
update the logic in upcomingTrips (the useMemo block) to either remove
memoization and compute the filter each render, or add a time-based trigger
(e.g., a ticking state or useInterval) as an additional dependency so new
Date(t.startDate).getTime() > now is re-evaluated; reference the upcomingTrips
constant and the useMemo call and adjust accordingly.
---
Nitpick comments:
In `@apps/expo/app/`(app)/upcoming-trips.tsx:
- Around line 148-159: upcomingTrips.map(...) is rebuilt every render and
renderItem reads rows by index; memoize the mapped data (e.g. const memoizedData
= useMemo(() => upcomingTrips.map(t => ({ id: t.id, title: t.name, subTitle:
`${t.location?.name ?? t('trips.unknown')} • ${formatDate(t.startDate)} to
${formatDate(t.endDate)}` , raw: t } ), [upcomingTrips, formatDate, t]), pass
memoizedData as data and update renderItem to use info.item (and no longer
reference upcomingTrips[info.index] or call assertDefined on that index) while
keeping keyExtractor as item => item.id so rows are stable and decoupled from
list ordering).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82c740e3-6c1d-4f09-8fff-ca51decd7ea2
📒 Files selected for processing (1)
apps/expo/app/(app)/upcoming-trips.tsx
| const upcomingTrips = useMemo( | ||
| () => trips.filter((t) => !!t.startDate && new Date(t.startDate).getTime() > Date.now()), | ||
| [trips], | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/expo/app/\(app\)/upcoming-trips.tsx | head -130 | tail -40Repository: PackRat-AI/PackRat
Length of output: 1442
🏁 Script executed:
wc -l apps/expo/app/\(app\)/upcoming-trips.tsxRepository: PackRat-AI/PackRat
Length of output: 104
🏁 Script executed:
cat -n apps/expo/app/\(app\)/upcoming-trips.tsx | sed -n '115,180p'Repository: PackRat-AI/PackRat
Length of output: 2674
Avoid memoizing a Date.now()-based filter with only trips in the dependency array.
This freezes upcomingTrips until the trips reference changes. A trip will remain in the "upcoming" list after its start time passes if the screen stays open. Either re-evaluate the filter on each render, or include a time-based dependency (note: using Date.now() directly in dependencies is not recommended; consider a timer or refactoring the time check to the point of use).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/expo/app/`(app)/upcoming-trips.tsx around lines 110 - 113, The
upcomingTrips useMemo is using Date.now() inside its filter but only depends on
trips, so the list won’t update as time passes; update the logic in
upcomingTrips (the useMemo block) to either remove memoization and compute the
filter each render, or add a time-based trigger (e.g., a ticking state or
useInterval) as an additional dependency so new Date(t.startDate).getTime() >
now is re-evaluated; reference the upcomingTrips constant and the useMemo call
and adjust accordingly.
| useEffect(() => { | ||
| if (!selectedTrip && upcomingTrips.length > 0) { | ||
| setSelectedTrip(upcomingTrips[0]); | ||
| if (!selectedTripId && upcomingTrips.length > 0) { | ||
| setSelectedTripId(upcomingTrips[0]?.id); | ||
| } | ||
| }, [upcomingTrips, selectedTrip]); | ||
| }, [upcomingTrips, selectedTripId]); |
There was a problem hiding this comment.
Reset the selection when the current id is no longer present.
This effect only handles the initial “nothing selected yet” case. If the selected trip is removed, or the list empties and later repopulates, selectedTripId stays stale and the summary can disappear until the user taps again.
💡 Suggested fix
useEffect(() => {
- if (!selectedTripId && upcomingTrips.length > 0) {
- setSelectedTripId(upcomingTrips[0]?.id);
- }
+ if (!upcomingTrips.length) {
+ setSelectedTripId(undefined);
+ return;
+ }
+
+ const stillSelected = selectedTripId
+ ? upcomingTrips.some((trip) => trip.id === selectedTripId)
+ : false;
+
+ if (!stillSelected) {
+ setSelectedTripId(upcomingTrips[0].id);
+ }
}, [upcomingTrips, selectedTripId]);📝 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.
| useEffect(() => { | |
| if (!selectedTrip && upcomingTrips.length > 0) { | |
| setSelectedTrip(upcomingTrips[0]); | |
| if (!selectedTripId && upcomingTrips.length > 0) { | |
| setSelectedTripId(upcomingTrips[0]?.id); | |
| } | |
| }, [upcomingTrips, selectedTrip]); | |
| }, [upcomingTrips, selectedTripId]); | |
| useEffect(() => { | |
| if (!upcomingTrips.length) { | |
| setSelectedTripId(undefined); | |
| return; | |
| } | |
| const stillSelected = selectedTripId | |
| ? upcomingTrips.some((trip) => trip.id === selectedTripId) | |
| : false; | |
| if (!stillSelected) { | |
| setSelectedTripId(upcomingTrips[0].id); | |
| } | |
| }, [upcomingTrips, selectedTripId]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/expo/app/`(app)/upcoming-trips.tsx around lines 119 - 123, The effect
for selecting a default trip only handles the initial empty selection; update it
to also reset when the currently selected id is no longer present in
upcomingTrips or when the list repopulates. Inside the existing useEffect
(referencing selectedTripId, upcomingTrips, setSelectedTripId), add a branch
that checks if selectedTripId is non-null and not found via upcomingTrips.some(t
=> t.id === selectedTripId) and in that case call
setSelectedTripId(upcomingTrips[0]?.id ?? undefined); also keep the existing
branch that sets the first id when nothing is selected and upcomingTrips.length
> 0, and ensure you still clear selection (setSelectedTripId(undefined)) when
upcomingTrips is empty.
|
@copilot CodeRabbit has provided review feedback on this PR. Please review and address the comments. |
- Remove useMemo from upcomingTrips: computed fresh each render to avoid stale Date.now() snapshot when a trip's start time passes while open - Fix useEffect to reset selectedTripId when selected trip is removed from list, and clear selection when list empties - Embed trip object in data items; use info.item.trip in renderItem to eliminate index-based upcomingTrips[info.index] lookup - Remove unused assertDefined import - Use optional chaining on upcomingTrips[0]?.id for safety Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
Addressed all CodeRabbit feedback in commit
|
|
@copilot Please address the CodeRabbit review feedback on this PR (2 inline comments). |
|
@copilot Reminder: please address the 2 CodeRabbit inline review comments on this PR. |
Both inline comments were already addressed in commit
GitHub marks the first thread as outdated; the second thread's suggested fix matches the code already in place. |
Both inline comments are already addressed in commit
|
|
@copilot Please address the 2 CodeRabbit inline review comments on this PR. |
extraData, ID-based state, stablekeyExtractor)useMemofromupcomingTripsto avoid staleDate.now()snapshotuseEffectto reset selection when selected trip is removed or list emptiestripobject indataitems; useinfo.item.tripinrenderItem(no index lookup)assertDefinedimportupcomingTrips[0]?.idfor safetyOriginal prompt
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.
Summary by CodeRabbit