Conversation
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a routed POI detail screen and full-screen map modal, updates call and gallery interactions, fixes fixed-size header controls, adds contact and message state handling, replaces notification confirmation UI, and revises date parsing and formatting. ChangesPOI and map flows
Call and shared UI behavior
State-backed UI flows
Date utility handling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CallDetail
participant StaticMap
participant FullScreenMapModal
participant Mapbox
CallDetail->>StaticMap: render map preview with onPress
StaticMap->>CallDetail: report tap
CallDetail->>FullScreenMapModal: open with coordinates
FullScreenMapModal->>Mapbox: render interactive map
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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.
Actionable comments posted: 5
🧹 Nitpick comments (4)
src/stores/messages/store.ts (1)
228-237: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHandle the floating promise from
setItem.
setItemis anasyncfunction. SincemarkMessageReadis synchronous and returnsvoid, the promise returned bysetItemis unhandled. If the underlying storage or serialization fails, it will lead to an unhandled promise rejection which could crash the app.Consider appending
.catch()to log or swallow the error safely.♻️ Proposed refactor
markMessageRead: (messageId: string) => { const { readMessageIds } = get(); if (readMessageIds.has(messageId)) { return; } const next = new Set(readMessageIds); next.add(messageId); set({ readMessageIds: next }); - setItem<string[]>('messages_read_ids', Array.from(next)); + setItem<string[]>('messages_read_ids', Array.from(next)).catch(console.error); },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/stores/messages/store.ts` around lines 228 - 237, Update markMessageRead so the promise returned by setItem is explicitly handled without changing the method’s synchronous void contract. Append a catch path that safely logs or swallows persistence errors, while preserving the existing readMessageIds update behavior.src/app/(app)/poi/[id].tsx (1)
161-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the static map preview tappable.
Because
StaticMaphas all map gestures disabled to allow for smooth page scrolling, tapping the map itself currently does nothing. Passing theonPresshandler will add a transparent overlay that makes the map intuitively trigger the map view.👆 Proposed fix
- <StaticMap latitude={poi.Latitude} longitude={poi.Longitude} address={poi.Address || title} zoom={15} height={220} showUserLocation={true} /> + <StaticMap latitude={poi.Latitude} longitude={poi.Longitude} address={poi.Address || title} zoom={15} height={220} showUserLocation={true} onPress={handleViewOnMap} />🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/`(app)/poi/[id].tsx at line 161, Update the StaticMap usage in the POI detail view to pass an onPress handler that opens the map view for the current POI, while preserving the existing map properties and gesture-disabled behavior.src/components/notifications/NotificationInbox.tsx (1)
321-321: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMemoize
extraDatato prevent unnecessary re-renders.Passing an inline object literal to
extraDatacreates a new reference on every render, causingFlashListto bypass itsPureComponentoptimizations and re-render all items unnecessarily whenever the parent component renders (e.g., when toggling the confirmation modal).As per coding guidelines, "Minimize useEffect/useState usage and avoid heavy computations during render" and "Optimize FlatList".
Define the object using
useMemoin the component body:const listExtraData = React.useMemo( () => ({ isSelectionMode, selectedNotificationIds }), [isSelectionMode, selectedNotificationIds] );⚡ Proposed fix for `extraData`
- extraData={{ isSelectionMode, selectedNotificationIds }} + extraData={listExtraData}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/notifications/NotificationInbox.tsx` at line 321, Memoize the FlashList extraData object in NotificationInbox using useMemo, with isSelectionMode and selectedNotificationIds as dependencies, then pass the stable listExtraData value to extraData instead of an inline object.Source: Coding guidelines
src/components/calls/call-images-modal.tsx (1)
607-607: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
useCallbackfor event handlers.As per coding guidelines, avoid anonymous functions and define callbacks with
useCallbackfor event handlers to prevent unnecessary re-renders.
src/components/calls/call-images-modal.tsx#L607-L607: Extract the inline anonymous function into a named callback usinguseCallback(e.g.,const handleLayout = useCallback((e) => setListWidth(e.nativeEvent.layout.width), []);).src/components/calls/call-images-modal.tsx#L514-L519: WraphandleMomentumScrollEndinuseCallbackand include its dependencies (e.g.,[listWidth, validImages.length]).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/calls/call-images-modal.tsx` at line 607, The event handlers in src/components/calls/call-images-modal.tsx at lines 607-607 and 514-519 should use stable callbacks: extract the inline onLayout handler near the Box into a named useCallback callback, and wrap handleMomentumScrollEnd in useCallback with listWidth and validImages.length as dependencies.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/`(app)/poi/[id].tsx:
- Around line 35-42: Update PoiDetailHeader to use useSafeAreaInsets from
react-native-safe-area-context, and apply the returned top inset as additional
top padding on the header while preserving its existing background and content
layout.
In `@src/app/call/`[id]/index.tsx:
- Around line 757-759: Update the FullScreenMapModal render condition to depend
only on non-null latitude and longitude, removing isMapModalOpen from the outer
check. Keep passing isMapModalOpen through isOpen so the modal remains mounted
and can perform its exit animation.
In `@src/components/notifications/NotificationInbox.tsx`:
- Around line 341-361: Extract the inline modal-close callbacks in the
NotificationInbox component into a memoized handleCloseConfirmModal useCallback
that sets showDeleteConfirmModal to false. Reuse this callback for RNModal’s
onRequestClose and the cancel Button’s onPress, while leaving confirmBulkDelete
unchanged.
In `@src/lib/utils.ts`:
- Around line 155-163: Update the timezone suffix regex in the timestamp parsing
block to allow hour-only offsets such as +04 or -04, while continuing to accept
offsets with minutes in both +0400 and +04:00 forms. Keep valid timezone-bearing
timestamps routed through the native Date parser before the manual parsing path.
In `@src/stores/contacts/store.ts`:
- Around line 79-88: Update fetchContactDetails so its success and error state
updates only apply when the requested contactId is still the active contact ID.
Recheck the store’s current selected contact before setting
selectedContactDetails or isDetailsLoading, preventing stale requests from
overwriting the newer selection while preserving the existing fallback behavior.
---
Nitpick comments:
In `@src/app/`(app)/poi/[id].tsx:
- Line 161: Update the StaticMap usage in the POI detail view to pass an onPress
handler that opens the map view for the current POI, while preserving the
existing map properties and gesture-disabled behavior.
In `@src/components/calls/call-images-modal.tsx`:
- Line 607: The event handlers in src/components/calls/call-images-modal.tsx at
lines 607-607 and 514-519 should use stable callbacks: extract the inline
onLayout handler near the Box into a named useCallback callback, and wrap
handleMomentumScrollEnd in useCallback with listWidth and validImages.length as
dependencies.
In `@src/components/notifications/NotificationInbox.tsx`:
- Line 321: Memoize the FlashList extraData object in NotificationInbox using
useMemo, with isSelectionMode and selectedNotificationIds as dependencies, then
pass the stable listExtraData value to extraData instead of an inline object.
In `@src/stores/messages/store.ts`:
- Around line 228-237: Update markMessageRead so the promise returned by setItem
is explicitly handled without changing the method’s synchronous void contract.
Append a catch path that safely logs or swallows persistence errors, while
preserving the existing readMessageIds update behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ac8d884a-e1c3-4e1f-986e-8e34ed8d8052
📒 Files selected for processing (20)
src/app/(app)/poi/[id].tsxsrc/app/(app)/poi/_layout.tsxsrc/app/call/[id]/index.tsxsrc/app/poi/[id].tsxsrc/components/calls/call-card.tsxsrc/components/calls/call-detail-menu.tsxsrc/components/calls/call-images-modal.tsxsrc/components/common/__tests__/header-back-button.test.tsxsrc/components/common/header-back-button.tsxsrc/components/contacts/contact-details-sheet.tsxsrc/components/maps/__tests__/full-screen-map-modal.test.tsxsrc/components/maps/full-screen-map-modal.tsxsrc/components/maps/static-map.tsxsrc/components/messages/message-card.tsxsrc/components/notifications/NotificationInbox.tsxsrc/components/notifications/__tests__/NotificationInbox.test.tsxsrc/lib/__tests__/utils-date.test.tssrc/lib/utils.tssrc/stores/contacts/store.tssrc/stores/messages/store.ts
💤 Files with no reviewable changes (1)
- src/app/poi/[id].tsx
| const PoiDetailHeader: React.FC<{ title: string; onBack: () => void }> = ({ title, onBack }) => ( | ||
| <HStack className="items-center bg-white px-2 pb-2 pt-1 dark:bg-gray-900" space="sm"> | ||
| <HeaderBackButton onPress={onBack} /> | ||
| <Text className="flex-1 text-lg font-bold text-gray-900 dark:text-white" numberOfLines={1}> | ||
| {title} | ||
| </Text> | ||
| </HStack> | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add safe area padding to prevent the header from overlapping the device status bar.
Because this screen runs in a layout with headerShown: false, the custom PoiDetailHeader will sit flush against the top edge of the screen, causing its content to overlap with the iOS notch or Android status bar.
Use useSafeAreaInsets to apply top padding so the header's background color extends to the edge while the content is safely pushed down.
📱 Proposed fix
First, add the import at the top of the file:
import { useSafeAreaInsets } from 'react-native-safe-area-context';Then, apply the insets to the header:
-const PoiDetailHeader: React.FC<{ title: string; onBack: () => void }> = ({ title, onBack }) => (
- <HStack className="items-center bg-white px-2 pb-2 pt-1 dark:bg-gray-900" space="sm">
- <HeaderBackButton onPress={onBack} />
- <Text className="flex-1 text-lg font-bold text-gray-900 dark:text-white" numberOfLines={1}>
- {title}
- </Text>
- </HStack>
-);
+const PoiDetailHeader: React.FC<{ title: string; onBack: () => void }> = ({ title, onBack }) => {
+ const insets = useSafeAreaInsets();
+ return (
+ <HStack className="items-center bg-white px-2 pb-2 dark:bg-gray-900" space="sm" style={{ paddingTop: insets.top + 4 }}>
+ <HeaderBackButton onPress={onBack} />
+ <Text className="flex-1 text-lg font-bold text-gray-900 dark:text-white" numberOfLines={1}>
+ {title}
+ </Text>
+ </HStack>
+ );
+};📝 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.
| const PoiDetailHeader: React.FC<{ title: string; onBack: () => void }> = ({ title, onBack }) => ( | |
| <HStack className="items-center bg-white px-2 pb-2 pt-1 dark:bg-gray-900" space="sm"> | |
| <HeaderBackButton onPress={onBack} /> | |
| <Text className="flex-1 text-lg font-bold text-gray-900 dark:text-white" numberOfLines={1}> | |
| {title} | |
| </Text> | |
| </HStack> | |
| ); | |
| import { useSafeAreaInsets } from 'react-native-safe-area-context'; | |
| const PoiDetailHeader: React.FC<{ title: string; onBack: () => void }> = ({ title, onBack }) => { | |
| const insets = useSafeAreaInsets(); | |
| return ( | |
| <HStack className="items-center bg-white px-2 pb-2 dark:bg-gray-900" space="sm" style={{ paddingTop: insets.top + 4 }}> | |
| <HeaderBackButton onPress={onBack} /> | |
| <Text className="flex-1 text-lg font-bold text-gray-900 dark:text-white" numberOfLines={1}> | |
| {title} | |
| </Text> | |
| </HStack> | |
| ); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/`(app)/poi/[id].tsx around lines 35 - 42, Update PoiDetailHeader to
use useSafeAreaInsets from react-native-safe-area-context, and apply the
returned top inset as additional top padding on the header while preserving its
existing background and content layout.
| {isMapModalOpen && coordinates.latitude != null && coordinates.longitude != null ? ( | ||
| <FullScreenMapModal isOpen={isMapModalOpen} onClose={() => setIsMapModalOpen(false)} latitude={coordinates.latitude} longitude={coordinates.longitude} address={call.Address} zoom={15} showUserLocation={true} /> | ||
| ) : null} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep FullScreenMapModal mounted to allow its exit animation.
When isMapModalOpen evaluates to false, the short-circuit instantly unmounts the <FullScreenMapModal> component. This abruptly destroys the native Modal inside it, skipping its closing slide animation entirely.
By removing isMapModalOpen from this outer condition, the component remains mounted and delegates visibility control to the isOpen prop, enabling smooth exit transitions.
🎞️ Proposed fix
- {isMapModalOpen && coordinates.latitude != null && coordinates.longitude != null ? (
+ {coordinates.latitude != null && coordinates.longitude != null ? (
<FullScreenMapModal isOpen={isMapModalOpen} onClose={() => setIsMapModalOpen(false)} latitude={coordinates.latitude} longitude={coordinates.longitude} address={call.Address} zoom={15} showUserLocation={true} />
) : null}📝 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.
| {isMapModalOpen && coordinates.latitude != null && coordinates.longitude != null ? ( | |
| <FullScreenMapModal isOpen={isMapModalOpen} onClose={() => setIsMapModalOpen(false)} latitude={coordinates.latitude} longitude={coordinates.longitude} address={call.Address} zoom={15} showUserLocation={true} /> | |
| ) : null} | |
| {coordinates.latitude != null && coordinates.longitude != null ? ( | |
| <FullScreenMapModal isOpen={isMapModalOpen} onClose={() => setIsMapModalOpen(false)} latitude={coordinates.latitude} longitude={coordinates.longitude} address={call.Address} zoom={15} showUserLocation={true} /> | |
| ) : null} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/call/`[id]/index.tsx around lines 757 - 759, Update the
FullScreenMapModal render condition to depend only on non-null latitude and
longitude, removing isMapModalOpen from the outer check. Keep passing
isMapModalOpen through isOpen so the modal remains mounted and can perform its
exit animation.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
1 participant
Summary by CodeRabbit