Conversation
- Add GET /api/notifications/trip-reminders endpoint that generates smart notification messages based on days until trip (7, 3, 1 day and day-of) including pack item counts and device charging alerts - Add useTripNotifications hook in Expo app to fetch trip reminders - Add TripNotificationsList component with priority-colored cards - Update notifications settings screen to display trip reminders - Add i18n translation keys for notification strings 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. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 749 🏁 Script executed: Length of output: 20797 🏁 Script executed: Length of output: 8511
Review: Smart Pre-Trip Notifications POC🔴 Critical Issues1.
|
| # | Severity | Location | Issue |
|---|---|---|---|
| 1 | 🔴 Critical | notifications/index.ts |
Math.ceil — morning_of branch never fires |
| 2 | 🔴 Critical | notifications/index.ts |
N+1 query for pack item counts |
| 3 | 🔴 Critical | notifications/index.ts |
No DB-level date filter |
| 4 | 🔴 Critical | i18n/types.ts |
notifications.* keys not in TranslationKey |
| 5 | 🟡 Medium | notifications.tsx |
refresh unconnected to pull-to-refresh / retry |
| 6 | 🟡 Medium | TripNotificationsList.tsx |
Nested ScrollView with scrollEnabled={false} |
| 7 | 🟡 Medium | en.json |
Several i18n keys defined but unused |
| 8 | 🟡 Medium | notifications/index.ts |
orderBy missing explicit direction |
| 9 | 🔵 Low | TripNotificationsList.tsx |
Unsafe as cast for icon name |
| 10 | 🔵 Low | useTripNotifications.ts |
cancelled flag instead of AbortController |
|
@copilot CodeRabbit has provided review feedback on this PR. Please review and address the comments. |
- Fix morning_of notification bug: use calendar-date Math.round instead
of Math.ceil to correctly detect day-of-trip (fixes same-day trips)
- Fix N+1 query: batch-fetch all pack item counts with single inArray
query instead of per-trip SELECT inside loop
- Add DB-level date filter (gte/lte) to trips query; remove JS filter
- Add explicit asc() direction to orderBy per codebase convention
- Add notifications.* keys to TranslationKey union type in i18n/types.ts
- Wire refresh() to pull-to-refresh RefreshControl on notifications screen
- Pass onRetry prop to TripNotificationsList for error state retry action
- Replace nested ScrollView scrollEnabled={false} with plain View
- Remove 7 unused i18n keys from en.json
- Type NOTIFICATION_ICON map with MaterialIconName from @roninoss/icons;
replace 4 invalid icon names (package-variant, flag-checkered,
check-all, battery-charging) with valid equivalents; fix error/empty
state icons (alert-circle-outline, bell-check-outline)
- Replace cancelled boolean flag with AbortController + axios.isCancel
Co-authored-by: andrew-bierman <94939237+andrew-bierman@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview 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:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
This PR implements a "Smart pre-trip notifications" proof-of-concept that adds a new /api/notifications/trip-reminders endpoint and surfaces the resulting notifications in the existing notifications settings screen.
Changes:
- New API endpoint that batch-fetches upcoming trips within a 30-day window and generates categorized notifications (week reminder, 3-day, day-before, morning-of, pack-progress, device-charging) for trips within 7 days
- New
useTripNotificationshook with AbortController-based cancellation and arefreshKeypattern for pull-to-refresh - New
TripNotificationsListcomponent and integration into the notifications settings screen withRefreshControl
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
packages/api/src/routes/notifications/index.ts |
New API route: batch-queries trips and pack items, generates time-based notification objects |
packages/api/src/routes/index.ts |
Registers the new notifications route under the protected router |
apps/expo/lib/i18n/types.ts |
Adds notifications.* keys to the TranslationKeys type union |
apps/expo/lib/i18n/locales/en.json |
Adds English strings for all new notifications.* keys; also adds "trip reminders" to the notificationsFootnote |
apps/expo/features/trips/hooks/useTripNotifications.ts |
Data-fetching hook; uses AbortController for cleanup and refreshKey to trigger re-fetches |
apps/expo/features/trips/hooks/index.ts |
Re-exports the new hook |
apps/expo/features/trips/components/TripNotificationsList.tsx |
Pure display component for rendering notification cards; handles loading, error, and empty states |
apps/expo/app/(app)/(tabs)/profile/notifications.tsx |
Integrates the hook and component into the notifications screen with RefreshControl |
You can also share your feedback on Copilot code review. Take the survey.
| {notifications.map((notification) => ( | ||
| <Pressable | ||
| key={`${notification.tripId}-${notification.type}`} | ||
| onPress={() => router.push(`/trips/${notification.tripId}`)} |
There was a problem hiding this comment.
The navigation path router.push('/trips/${notification.tripId}') is incorrect. The actual Expo Router route for a trip detail screen is /trip/[id] (singular "trip"), as evidenced by apps/expo/app/(app)/trip/[id]/index.tsx and how other components navigate (e.g. TripListScreen.tsx uses { pathname: '/trip/[id]', params: { id: trip.id } }). This will cause navigation to fail at runtime. The push call should use { pathname: '/trip/[id]', params: { id: notification.tripId } } instead.
| const now = new Date(); | ||
| const thirtyDaysFromNow = new Date(now.getTime() + 30 * 24 * 60 * 60 * 1000); | ||
|
|
||
| // Use calendar-date noon as "today" boundary to avoid timezone edge cases |
There was a problem hiding this comment.
The inline comment on line 78 says "Use calendar-date noon as 'today' boundary" but the code constructs a Date at midnight (00:00:00) with no time component — not noon. The comment is misleading and should read "Use calendar-date midnight as 'today' boundary" (or similar).
| notificationsRoutes.openapi(getTripRemindersRoute, async (c) => { | ||
| const auth = c.get('user'); | ||
| const db = createDb(c); | ||
|
|
||
| try { | ||
| const now = new Date(); | ||
| const thirtyDaysFromNow = new Date(now.getTime() + 30 * 24 * 60 * 60 * 1000); | ||
|
|
||
| // Use calendar-date noon as "today" boundary to avoid timezone edge cases | ||
| const todayStart = new Date(now.getFullYear(), now.getMonth(), now.getDate()); | ||
|
|
||
| // Fetch upcoming trips filtered at DB level (no full table scan) | ||
| const futureTrips = await db.query.trips.findMany({ | ||
| where: and( | ||
| eq(trips.userId, auth.userId), | ||
| eq(trips.deleted, false), | ||
| gte(trips.startDate, todayStart), | ||
| lte(trips.startDate, thirtyDaysFromNow), | ||
| ), | ||
| with: { pack: true }, | ||
| orderBy: (t) => [asc(t.startDate)], | ||
| }); | ||
|
|
||
| // Batch-fetch pack item counts to avoid N+1 queries | ||
| const packIds = futureTrips.map((t) => t.packId).filter((id): id is string => !!id); | ||
| const allPackItems = packIds.length | ||
| ? await db | ||
| .select({ packId: packItems.packId, id: packItems.id }) | ||
| .from(packItems) | ||
| .where(and(inArray(packItems.packId, packIds), eq(packItems.deleted, false))) | ||
| : []; | ||
|
|
||
| const itemCountByPackId = allPackItems.reduce<Record<string, number>>((acc, item) => { | ||
| acc[item.packId] = (acc[item.packId] ?? 0) + 1; | ||
| return acc; | ||
| }, {}); | ||
|
|
||
| const notifications: z.infer<typeof TripNotificationSchema>[] = []; | ||
|
|
||
| for (const trip of futureTrips) { | ||
| if (!trip.startDate) continue; | ||
|
|
||
| const startDate = new Date(trip.startDate); | ||
| const startDateIso = startDate.toISOString(); | ||
|
|
||
| // Compare calendar dates (year/month/day only) to handle time-of-day correctly | ||
| const tripCalendarDate = new Date( | ||
| startDate.getFullYear(), | ||
| startDate.getMonth(), | ||
| startDate.getDate(), | ||
| ); | ||
| const daysUntilTrip = Math.round( | ||
| (tripCalendarDate.getTime() - todayStart.getTime()) / (1000 * 60 * 60 * 24), | ||
| ); | ||
|
|
||
| const packItemCount = trip.packId ? (itemCountByPackId[trip.packId] ?? 0) : 0; | ||
| const packSuffix = (count: number) => `item${count !== 1 ? 's' : ''}`; | ||
|
|
||
| if (daysUntilTrip === 0) { | ||
| // Morning of the trip | ||
| notifications.push({ | ||
| tripId: trip.id, | ||
| tripName: trip.name, | ||
| startDate: startDateIso, | ||
| daysUntilTrip: 0, | ||
| type: 'morning_of', | ||
| title: `Today's the day — ${trip.name}!`, | ||
| message: trip.packId | ||
| ? `Have a great trip! Double-check your ${packItemCount} packed ${packSuffix(packItemCount)} before heading out.` | ||
| : "Have a great trip! Make sure you haven't forgotten anything.", | ||
| priority: 'high', | ||
| }); | ||
| } else if (daysUntilTrip === 1) { | ||
| // Day before | ||
| notifications.push({ | ||
| tripId: trip.id, | ||
| tripName: trip.name, | ||
| startDate: startDateIso, | ||
| daysUntilTrip: 1, | ||
| type: 'day_before', | ||
| title: `${trip.name} is tomorrow!`, | ||
| message: trip.packId | ||
| ? `Final pack check — you have ${packItemCount} ${packSuffix(packItemCount)} in your pack. Don't forget to charge your devices tonight!` | ||
| : "Final preparations! Don't forget to charge your phone, headlamp, and power bank.", | ||
| priority: 'high', | ||
| }); | ||
|
|
||
| // Add device charging alert for day before | ||
| notifications.push({ | ||
| tripId: trip.id, | ||
| tripName: trip.name, | ||
| startDate: startDateIso, | ||
| daysUntilTrip: 1, | ||
| type: 'device_charging', | ||
| title: 'Charge your devices tonight', | ||
| message: | ||
| 'Plug in your phone, headlamp, GPS, and power bank before bed so they are ready for tomorrow.', | ||
| priority: 'medium', | ||
| }); | ||
| } else if (daysUntilTrip <= 3) { | ||
| // 2–3 days before: packing reminder | ||
| notifications.push({ | ||
| tripId: trip.id, | ||
| tripName: trip.name, | ||
| startDate: startDateIso, | ||
| daysUntilTrip, | ||
| type: 'three_day_reminder', | ||
| title: `${trip.name} starts in ${daysUntilTrip} days`, | ||
| message: trip.packId | ||
| ? `Time to start packing! You have ${packItemCount} ${packSuffix(packItemCount)} in your pack list. Review and start gathering your gear.` | ||
| : "Time to start packing! Review your gear list and start gathering everything you'll need.", | ||
| priority: 'medium', | ||
| }); | ||
| } else if (daysUntilTrip <= 7) { | ||
| // 4–7 days before: week reminder | ||
| notifications.push({ | ||
| tripId: trip.id, | ||
| tripName: trip.name, | ||
| startDate: startDateIso, | ||
| daysUntilTrip, | ||
| type: 'week_reminder', | ||
| title: `${trip.name} is coming up`, | ||
| message: trip.packId | ||
| ? `Your trip is ${daysUntilTrip} days away. Review your pack list with ${packItemCount} ${packSuffix(packItemCount)} and make sure you have everything.` | ||
| : `Your trip is ${daysUntilTrip} days away. Now is a great time to review your gear list!`, | ||
| priority: 'low', | ||
| }); | ||
|
|
||
| // Add a separate pack progress card when pack has items | ||
| if (trip.packId && packItemCount > 0) { | ||
| notifications.push({ | ||
| tripId: trip.id, | ||
| tripName: trip.name, | ||
| startDate: startDateIso, | ||
| daysUntilTrip, | ||
| type: 'pack_progress', | ||
| title: 'Check your packing progress', | ||
| message: `You have ${packItemCount} ${packSuffix(packItemCount)} in your pack for ${trip.name}. Go through your checklist to make sure everything is ready.`, | ||
| priority: 'low', | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return c.json( | ||
| { | ||
| notifications, | ||
| upcomingTripsCount: futureTrips.length, | ||
| }, | ||
| 200, | ||
| ); | ||
| } catch (error) { | ||
| console.error('Error fetching trip reminders:', error); | ||
| return c.json({ error: 'Failed to fetch trip reminders' }, 500); | ||
| } | ||
| }); | ||
|
|
||
| export { notificationsRoutes }; |
There was a problem hiding this comment.
The new GET /notifications/trip-reminders route has no corresponding test file, while every other route in the API has one (e.g. test/packs.test.ts, test/trips pattern doesn't exist in test dir but similar coverage for packs, catalog, auth, guides, weather, etc.). A test file covering at minimum: authentication requirement, successful response structure, and a case with upcoming trips would be consistent with the existing test suite.
Fix navigation path from /trips/ to /trip/ to match the actual Expo Router route at app/(app)/trip/[id]. Fix inline comment that incorrectly said "noon" when the code constructs a midnight boundary. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
/api/notifications/trip-remindersuseTripNotificationshook in Expo appTripNotificationsListcomponentOriginal 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.