Implement Trips feature (behind feature flag)#1912
Conversation
|
It looks like your comment got cut off — could you share what you'd like me to look at or change? Happy to address any feedback! |
|
It looks like your comment may have been submitted without any text. Could you share what you'd like me to look at or change? |
|
@coderabbitai Please review this PR for code quality, best practices, and potential issues. |
|
✅ Actions performedFull review triggered. |
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
📝 WalkthroughWalkthroughAdds local search/filtering to TripListScreen, shows formatted trip date ranges in TripCard, tightens upcoming-trip date parsing with parseLocalDate, re-exports trips hooks/types, updates a modal translation key to Changes
Sequence DiagramsequenceDiagram
participant User
participant TripListScreen
participant useMemo
participant FlatList
participant TripCard
User->>TripListScreen: type in search bar (onChangeText)
TripListScreen->>TripListScreen: set searchValue
TripListScreen->>useMemo: compute filteredTrips(trips, searchValue)
useMemo-->>TripListScreen: return filteredTrips
TripListScreen->>FlatList: update data = filteredTrips
FlatList->>TripCard: render trip item
TripCard->>TripCard: formatLocalDate(startDate / endDate)
TripCard-->>User: display trip with formatted dates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/expo/features/trips/components/TripCard.tsx (1)
12-16: Consider locale-aware date formatting for better UX.The current implementation extracts the raw
YYYY-MM-DDdate portion, which works but may not be ideal for all users. Consider usingtoLocaleDateString()for locale-aware formatting:♻️ Optional: Locale-aware date formatting
function formatTripDate(dateString?: string): string { if (!dateString) return '—'; - const datePart = dateString.split('T')[0]; - return datePart ?? '—'; + try { + return new Date(dateString).toLocaleDateString(); + } catch { + return '—'; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/features/trips/components/TripCard.tsx` around lines 12 - 16, The current formatTripDate function returns the raw YYYY-MM-DD chunk; update it to parse the input into a Date and use Date.prototype.toLocaleDateString (optionally with a locale or Intl.DateTimeFormat options) to produce a locale-aware string, ensure you handle undefined/invalid inputs by returning '—', and guard against timezone/parsing issues (e.g., use new Date(dateString) and check isNaN(date.getTime()) before calling toLocaleDateString) so formatTripDate always returns a safe, localized string.apps/expo/features/trips/screens/TripListScreen.tsx (1)
53-66: Empty state may confuse users when search yields no results.The
ListEmptyComponentalways shows "No trips yet" with a create button, but this same message appears when a search returns no matches. Consider differentiating between "no trips exist" vs "no search results":♻️ Suggested differentiation for empty states
const renderEmptyState = () => { + // Differentiate between no trips and no search results + if (searchValue.trim() && trips.length > 0) { + return ( + <View className="flex-1 items-center justify-center p-8"> + <Text className="text-muted-foreground">{t('trips.noSearchResults')}</Text> + </View> + ); + } return ( <View className="flex-1 items-center justify-center p-8"> <View className="mb-4 rounded-full bg-muted p-4">This requires adding a
trips.noSearchResultstranslation key.Also applies to: 91-91, 98-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/expo/features/trips/screens/TripListScreen.tsx` around lines 53 - 66, The empty-state UI in renderEmptyState (used as ListEmptyComponent) always shows the "no trips yet" message and create button, which is misleading when a search yields zero matches; update renderEmptyState to accept a parameter or read the current search/filter state (e.g., a prop or local state like searchQuery) and conditionally render two variants: one for "no trips exist" using t('trips.noTripsYet') and create button that calls handleCreateTrip, and another for "no search results" that shows t('trips.noSearchResults') without suggesting creation; add the new translation key trips.noSearchResults and ensure the component locations referenced in the diff use the updated renderEmptyState signature or conditional logic.
🤖 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/features/trips/components/TripCard.tsx`:
- Around line 12-16: The current formatTripDate function returns the raw
YYYY-MM-DD chunk; update it to parse the input into a Date and use
Date.prototype.toLocaleDateString (optionally with a locale or
Intl.DateTimeFormat options) to produce a locale-aware string, ensure you handle
undefined/invalid inputs by returning '—', and guard against timezone/parsing
issues (e.g., use new Date(dateString) and check isNaN(date.getTime()) before
calling toLocaleDateString) so formatTripDate always returns a safe, localized
string.
In `@apps/expo/features/trips/screens/TripListScreen.tsx`:
- Around line 53-66: The empty-state UI in renderEmptyState (used as
ListEmptyComponent) always shows the "no trips yet" message and create button,
which is misleading when a search yields zero matches; update renderEmptyState
to accept a parameter or read the current search/filter state (e.g., a prop or
local state like searchQuery) and conditionally render two variants: one for "no
trips exist" using t('trips.noTripsYet') and create button that calls
handleCreateTrip, and another for "no search results" that shows
t('trips.noSearchResults') without suggesting creation; add the new translation
key trips.noSearchResults and ensure the component locations referenced in the
diff use the updated renderEmptyState signature or conditional logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6dbab0a4-5422-4a3d-955f-237824e48805
📒 Files selected for processing (4)
apps/expo/app/(app)/_layout.tsxapps/expo/features/trips/components/TripCard.tsxapps/expo/features/trips/index.tsapps/expo/features/trips/screens/TripListScreen.tsx
|
@copilot CodeRabbit has provided review feedback on this PR. Please address the comments and suggestions. Once you've made the necessary changes, I'll ask CodeRabbit to re-review. |
Addressed both CodeRabbit suggestions in commit
|
|
@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.
|
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 `@apps/expo/features/trips/components/TripCard.tsx`:
- Around line 12-17: formatTripDate currently uses new Date(dateString) which
treats 'YYYY-MM-DD' as UTC and causes off-by-one-day for users west of UTC;
update formatTripDate to detect pure date-only strings (e.g.
/^\d{4}-\d{2}-\d{2}$/) and construct a local Date via new Date(year, monthIndex,
day) (parsing year/month/day from the string) before calling
toLocaleDateString(); apply the same fix to the analogous function in
UpcomingTripsTile.tsx so both parse date-only values as local dates instead of
relying on Date's UTC parsing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e65851f0-98c4-433e-9f7f-e4b7e3b3892b
📒 Files selected for processing (3)
apps/expo/features/trips/components/TripCard.tsxapps/expo/features/trips/screens/TripListScreen.tsxapps/expo/lib/i18n/locales/en.json
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/expo/features/trips/screens/TripListScreen.tsx
| function formatTripDate(dateString?: string): string { | ||
| if (!dateString) return '—'; | ||
| const date = new Date(dateString); | ||
| if (Number.isNaN(date.getTime())) return '—'; | ||
| return date.toLocaleDateString(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Trip type definitions under apps/expo/features/trips ==="
fd 'types\.ts$' apps/expo/features/trips -x sh -c 'echo "--- $1 ---"; sed -n "1,220p" "$1"' sh {}
echo
echo "=== startDate/endDate definitions and transformations ==="
rg -n -C3 '\b(startDate|endDate)\b' apps/expo/features/tripsRepository: PackRat-AI/PackRat
Length of output: 9006
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Searching for formatDate function definition ==="
rg -n -B2 -A8 'function formatDate|const formatDate|export.*formatDate' apps/expo/features/trips
echo
echo "=== Checking date picker date value handling ==="
rg -n -B3 -A3 'setShowStartPicker|setShowEndPicker|onDateChange|getSelectedDate' apps/expo/features/trips/components/TripForm.tsx
echo
echo "=== Full TripForm.tsx date picker section ==="
sed -n '213,280p' apps/expo/features/trips/components/TripForm.tsx
echo
echo "=== Searching for date utilities/helpers ==="
fd -e ts -e tsx -name '*date*' -o -name '*time*' apps/expo/features/tripsRepository: PackRat-AI/PackRat
Length of output: 7228
Fix timezone handling for date-only strings in formatTripDate.
Line 14 parses YYYY-MM-DD date strings directly with new Date(), which treats them as UTC midnight. For users west of UTC, toLocaleDateString() on line 16 will then display the previous day. The form stores dates as YYYY-MM-DD (confirmed in TripForm.tsx lines 235, 270), so this timezone shift is a real issue.
Replace with local-date parsing:
Corrected formatTripDate implementation
function formatTripDate(dateString?: string): string {
if (!dateString) return '—';
- const date = new Date(dateString);
+ const dateOnlyMatch = /^(\d{4})-(\d{2})-(\d{2})$/.exec(dateString);
+ const date = dateOnlyMatch
+ ? new Date(
+ Number(dateOnlyMatch[1]),
+ Number(dateOnlyMatch[2]) - 1,
+ Number(dateOnlyMatch[3]),
+ )
+ : new Date(dateString);
if (Number.isNaN(date.getTime())) return '—';
return date.toLocaleDateString();
}Note: UpcomingTripsTile.tsx line 21 has the same pattern and should be updated as well.
📝 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.
| function formatTripDate(dateString?: string): string { | |
| if (!dateString) return '—'; | |
| const date = new Date(dateString); | |
| if (Number.isNaN(date.getTime())) return '—'; | |
| return date.toLocaleDateString(); | |
| } | |
| function formatTripDate(dateString?: string): string { | |
| if (!dateString) return '—'; | |
| const dateOnlyMatch = /^(\d{4})-(\d{2})-(\d{2})$/.exec(dateString); | |
| const date = dateOnlyMatch | |
| ? new Date( | |
| Number(dateOnlyMatch[1]), | |
| Number(dateOnlyMatch[2]) - 1, | |
| Number(dateOnlyMatch[3]), | |
| ) | |
| : new Date(dateString); | |
| if (Number.isNaN(date.getTime())) return '—'; | |
| return date.toLocaleDateString(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/expo/features/trips/components/TripCard.tsx` around lines 12 - 17,
formatTripDate currently uses new Date(dateString) which treats 'YYYY-MM-DD' as
UTC and causes off-by-one-day for users west of UTC; update formatTripDate to
detect pure date-only strings (e.g. /^\d{4}-\d{2}-\d{2}$/) and construct a local
Date via new Date(year, monthIndex, day) (parsing year/month/day from the
string) before calling toLocaleDateString(); apply the same fix to the analogous
function in UpcomingTripsTile.tsx so both parse date-only values as local dates
instead of relying on Date's UTC parsing.
✅ Actions performedReview triggered.
|
…display, and search filtering Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
…ated empty states Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
… bug Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
…ition - Extract duplicated local-date parsing logic from TripCard.tsx and UpcomingTripsTile.tsx into a shared dateUtils.ts utility (parseLocalDate and formatLocalDate), addressing CodeRabbit review feedback - Fix TripInStore type that was using Omit<Trip, 'trips'> where Trip has no 'trips' property, making the Omit a no-op
…ocalDate Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
- TripCard: Handle optional startDate/endDate with proper null checks, only show arrow separator when both dates exist - UpcomingTripsTile: Compare against start-of-today instead of current time so same-day trips are included in upcoming filter - TripListScreen: Trim search query before lowercasing to avoid missed matches from trailing whitespace
The trips feature should ship disabled by default until it's ready for release. Consumers can opt-in by toggling this flag.
andrew-bierman
force-pushed
the
copilot/implement-trips-feature
branch
from
dc409f3 to
9861f13
Compare
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. |
Deploying packrat-landing with
|
| Latest commit: |
98aa5ca
|
| Status: | ⚡️ Build in progress... |
#1912 added featureFlags.enableTrips and wired the tab trigger and home tiles to hide when the flag is off, but the underlying route files still rendered their screens. That meant deep links such as packrat://trip/new or packrat://trip/:id rendered trip UI even with the kill switch off. Adds a Redirect fallback to each app-router entry point: - app/(app)/trip/[id]/index.tsx - app/(app)/trip/new.tsx - app/(app)/upcoming-trips.tsx - app/(app)/(tabs)/trips/index.tsx Feature components under features/trips/* are intentionally left alone — the gating happens at the route level.
Implement Trips feature (behind feature flag)
#1912 added featureFlags.enableTrips and wired the tab trigger and home tiles to hide when the flag is off, but the underlying route files still rendered their screens. That meant deep links such as packrat://trip/new or packrat://trip/:id rendered trip UI even with the kill switch off. Adds a Redirect fallback to each app-router entry point: - app/(app)/trip/[id]/index.tsx - app/(app)/trip/new.tsx - app/(app)/upcoming-trips.tsx - app/(app)/(tabs)/trips/index.tsx Feature components under features/trips/* are intentionally left alone — the gating happens at the route level.
formatTripDateto usetoLocaleDateString()with invalid-date guard (TripCard.tsx)trips.noSearchResultstranslation key (en.json)YYYY-MM-DDstrings as local dates inTripCard.tsxandUpcomingTripsTile.tsx?? ''for optionaldescription/location.namebefore.includes()(TripListScreen.tsx)parseLocalDate: reject dates that normalize (e.g.2024-02-31→ March) by comparing constructed date fields back to original year/month/day (dateUtils.ts)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
New Features
Bug Fixes