fix(ios): Upcoming Trips selection never updates after initial render#1899
fix(ios): Upcoming Trips selection never updates after initial render#1899
Conversation
On iOS, the List component (NativeWindUI/SectionList) was not
re-rendering items when selectedTrip state changed because:
- No extraData prop to signal external state changes
- keyExtractor used array indices instead of stable trip IDs
- renderItem was an inline closure without useCallback
Fix:
- Add id field to data objects and use item.id in keyExtractor
- Add extraData={selectedTrip?.id} to force re-renders on selection
- Wrap renderItem in useCallback with [upcomingTrips, selectedTrip, t]
- Move renderItem before early return (React Rules of Hooks)
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: 10446
Overall AssessmentThe three-pronged fix ( 🔴 Issue 1 —
|
📝 WalkthroughWalkthroughThe upcoming trips list component was refactored to fix iOS-specific selection issues by extracting rendering logic into a memoized callback using Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
|
@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. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
apps/expo/app/(app)/upcoming-trips.tsx (4)
122-146: Index-based lookup risks desynchronization with list data.The
renderItemcallback retrieves the trip viaupcomingTrips[info.index], but theListdata is mapped separately with its ownidfield. If the arrays ever diverge (e.g., due to a race condition or future refactoring), the selected trip could mismatch the displayed item.Since
info.item.idis already available, look up the trip by ID for a more robust approach:♻️ Proposed fix
const renderItem = useCallback( (info: { item: { id: string; title: string; subTitle: string }; index: number }) => { - const trip = upcomingTrips[info.index]; - assertDefined(trip); + const trip = upcomingTrips.find((t) => t.id === info.item.id); + if (!trip) return null; 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 122 - 146, renderItem currently fetches the trip by index (upcomingTrips[info.index]), which can desynchronize from the list item; instead locate the trip by id using info.item.id to ensure the rendered data matches selection. Update renderItem to find the trip from upcomingTrips by matching trip.id === info.item.id (still call getTripStatus(trip, t), setSelectedTrip(trip), and compare selectedTrip?.id to trip.id) so lookups are ID-based and robust against reordering or divergence.
110-112: MemoizeupcomingTripsto stabilize theuseCallbackdependency.
upcomingTripsis recomputed on every render, which invalidates theuseCallbackmemoization even whentripshasn't changed. Wrap it inuseMemoto preserve referential stability:♻️ Proposed fix
+ import { useCallback, useEffect, useMemo, useState } from 'react'; - import { useCallback, useEffect, useState } from 'react';- const upcomingTrips = trips.filter( - (t) => !!t.startDate && new Date(t.startDate).getTime() > Date.now(), - ); + const upcomingTrips = useMemo( + () => trips.filter((t) => !!t.startDate && new Date(t.startDate).getTime() > Date.now()), + [trips], + );Also applies to: 122-146
🤖 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 - 112, upcomingTrips is being recomputed on every render which breaks referential stability for the dependent useCallback; wrap the filter into a useMemo that depends on trips (e.g. const upcomingTrips = useMemo(() => trips.filter(...), [trips])) so upcomingTrips identity is stable and the callback memoization remains effective; apply the same useMemo pattern to the other similar filtered arrays referenced by the useCallback in the same component.
190-208: Hardcoded strings bypass translations.Several user-facing strings in the Trip Summary section are not internationalized:
- Line 190:
'No location'- Line 197:
'DATES'- Line 205:
'PACK'- Line 208:
'No pack assigned'Consider replacing these with
t('...')calls for consistency with the rest of the component.🌐 Proposed fix
<Text variant="subhead" className="mt-1 text-muted-foreground"> - {selectedTrip.location?.name ?? 'No location'} + {selectedTrip.location?.name ?? t('trips.noLocation')} </Text><Text variant="footnote" className="text-muted-foreground"> - DATES + {t('trips.dates')} </Text><Text variant="footnote" className="text-muted-foreground"> - PACK + {t('trips.pack')} </Text> <Text variant="subhead" className="mt-1"> - {selectedPack ? `${selectedPack.items.length} items` : 'No pack assigned'} + {selectedPack ? t('trips.itemCount', { count: selectedPack.items.length }) : t('trips.noPackAssigned')} </Text>🤖 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 190 - 208, Replace hardcoded UI strings in the Trip Summary with translation calls: wrap 'No location', 'DATES', 'PACK', and 'No pack assigned' with the i18n function (e.g., t('No location'), t('DATES'), t('PACK'), t('No pack assigned')) where these appear in the JSX that renders selectedTrip.location, the DATES label and range (uses formatDate), and the PACK label and selectedPack fallback; ensure the translation function (t) is imported/available in this file and use the same keys/style as other strings in this component.
116-120: Consider invalidatingselectedTripwhen it leavesupcomingTrips.The current effect only sets an initial selection. If the selected trip's start date passes while the screen is open,
selectedTripbecomes stale (no longer inupcomingTrips). Consider re-validating:♻️ Proposed fix
useEffect(() => { - if (!selectedTrip && upcomingTrips.length > 0) { + const isSelectedValid = selectedTrip && upcomingTrips.some((t) => t.id === selectedTrip.id); + if (!isSelectedValid && upcomingTrips.length > 0) { setSelectedTrip(upcomingTrips[0]); } }, [upcomingTrips, selectedTrip]);🤖 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 116 - 120, The effect that currently only initializes selectedTrip should also invalidate it when it no longer appears in upcomingTrips: inside the useEffect that watches upcomingTrips and selectedTrip (the one calling setSelectedTrip), add a branch that if selectedTrip is non-null and not found in upcomingTrips (compare by the trip id or unique key), then call setSelectedTrip(upcomingTrips.length ? upcomingTrips[0] : null) so the selection is updated to the next available trip or cleared; keep the existing initialization behavior for when selectedTrip is null and upcomingTrips has items.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/expo/app/`(app)/upcoming-trips.tsx:
- Around line 122-146: renderItem currently fetches the trip by index
(upcomingTrips[info.index]), which can desynchronize from the list item; instead
locate the trip by id using info.item.id to ensure the rendered data matches
selection. Update renderItem to find the trip from upcomingTrips by matching
trip.id === info.item.id (still call getTripStatus(trip, t),
setSelectedTrip(trip), and compare selectedTrip?.id to trip.id) so lookups are
ID-based and robust against reordering or divergence.
- Around line 110-112: upcomingTrips is being recomputed on every render which
breaks referential stability for the dependent useCallback; wrap the filter into
a useMemo that depends on trips (e.g. const upcomingTrips = useMemo(() =>
trips.filter(...), [trips])) so upcomingTrips identity is stable and the
callback memoization remains effective; apply the same useMemo pattern to the
other similar filtered arrays referenced by the useCallback in the same
component.
- Around line 190-208: Replace hardcoded UI strings in the Trip Summary with
translation calls: wrap 'No location', 'DATES', 'PACK', and 'No pack assigned'
with the i18n function (e.g., t('No location'), t('DATES'), t('PACK'), t('No
pack assigned')) where these appear in the JSX that renders
selectedTrip.location, the DATES label and range (uses formatDate), and the PACK
label and selectedPack fallback; ensure the translation function (t) is
imported/available in this file and use the same keys/style as other strings in
this component.
- Around line 116-120: The effect that currently only initializes selectedTrip
should also invalidate it when it no longer appears in upcomingTrips: inside the
useEffect that watches upcomingTrips and selectedTrip (the one calling
setSelectedTrip), add a branch that if selectedTrip is non-null and not found in
upcomingTrips (compare by the trip id or unique key), then call
setSelectedTrip(upcomingTrips.length ? upcomingTrips[0] : null) so the selection
is updated to the next available trip or cleared; keep the existing
initialization behavior for when selectedTrip is null and upcomingTrips has
items.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 86c4bbb2-af58-4feb-bdc2-6560add62eb8
📒 Files selected for processing (1)
apps/expo/app/(app)/upcoming-trips.tsx
On iOS, tapping trips in the Upcoming Trips screen had no visual effect — the first trip stayed highlighted indefinitely. Android was unaffected.
Root cause: Three compounding issues caused
List(NativeWindUISectionList) to skip re-rendering items whenselectedTripchanged:extraData— list had no signal that external selection state changedkeyExtractor— unstable keys cause incorrect cell reuse on iOSrenderItemwithoutuseCallback— stale closure risk onselectedTripFix (
apps/expo/app/(app)/upcoming-trips.tsx):extraData={selectedTrip?.id}so the list re-renders items on selection changeid: trip.idto data items; switchkeyExtractorto(item) => item.idrenderIteminuseCallback([upcomingTrips, selectedTrip, t])— same pattern asconversations.tsxrenderItembefore the early return (Rules of Hooks compliance)Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.
Summary by CodeRabbit