Conversation
This comment has been minimized.
This comment has been minimized.
|
Warning Review limit reached
Next review available in: 19 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 Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR changes call-note keyboard avoidance and updates notification detail initialization and inbox row interaction handling, including selection, long-press, navigation, and memoized row rendering. ChangesCall note keyboard handling
Notification interaction updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 4
🧹 Nitpick comments (2)
src/components/notifications/NotificationInbox.tsx (2)
37-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the required
React.FCsignature.
NotificationRowis a new typed component but is not declared asReact.FC<NotificationRowProps>. As per coding guidelines, “Utilize React.FC for defining functional components with props.”🤖 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` around lines 37 - 38, Update the NotificationRow component declaration to use the required React.FC<NotificationRowProps> type while preserving its existing React.memo wrapping and behavior.Source: Coding guidelines
241-253: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve the notification item type at the mapping boundary.
Line 242 uses
any, so malformed SDK payloads can bypass theNotificationPayloadcontract. Typeitemfrom the notifications collection (or the Novu SDK model) and retain thereferenceTypeunion. As per coding guidelines, “Never useanytype; use precise types and interfaces with TypeScript strict mode enabled.”🤖 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` around lines 241 - 253, Replace the any type in renderItem with the precise notification collection or Novu SDK item type, and ensure the mapping to NotificationPayload preserves the referenceType union without unsafe widening. Keep the existing field mapping unchanged while enforcing the item shape under strict TypeScript.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/components/calls/__tests__/call-notes-modal-new.test.tsx`:
- Around line 69-71: Update the KeyboardAvoidingView mock to use a precise props
interface instead of any, including children, behavior, and
keyboardVerticalOffset, and forward those props to the rendered View. Add or
update the Jest assertions to verify behavior="padding" and
keyboardVerticalOffset={0} on the keyboard-avoiding wrapper.
In `@src/components/notifications/NotificationDetail.tsx`:
- Around line 31-32: Update the notification-open flow around NotificationDetail
so selecting an unread notification invokes the Novu SDK mark-as-read operation
and refreshes the inbox state, ensuring NotificationRow no longer derives it as
unread. Preserve already-read behavior and update the detail-open test to verify
the read-status update flow.
In `@src/components/notifications/NotificationInbox.tsx`:
- Around line 232-237: Update handleNavigateToReference to route each supported
referenceType to its referenced call, message, or note using the existing
navigation utilities, and invoke onClose only after navigation succeeds. Remove
the console.log call; if diagnostics are still needed, use the shared logger
from `@/lib/logging`.
- Around line 166-174: Update handleNotificationLongPress so the event handler
determines whether selection mode is currently inactive, then performs
setSelectedNotificationIds and setIsSelectionMode directly rather than nesting
the former inside the setIsSelectionMode updater. Keep the updater pure and
preserve selecting the pressed notification only when entering selection mode.
---
Nitpick comments:
In `@src/components/notifications/NotificationInbox.tsx`:
- Around line 37-38: Update the NotificationRow component declaration to use the
required React.FC<NotificationRowProps> type while preserving its existing
React.memo wrapping and behavior.
- Around line 241-253: Replace the any type in renderItem with the precise
notification collection or Novu SDK item type, and ensure the mapping to
NotificationPayload preserves the referenceType union without unsafe widening.
Keep the existing field mapping unchanged while enforcing the item shape under
strict TypeScript.
🪄 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 Plus
Run ID: 94bae7fd-16c0-47c0-89ac-488e88d0b6a0
📒 Files selected for processing (4)
src/components/calls/__tests__/call-notes-modal-new.test.tsxsrc/components/calls/call-notes-modal.tsxsrc/components/notifications/NotificationDetail.tsxsrc/components/notifications/NotificationInbox.tsx
| KeyboardAvoidingView: ({ children }: any) => { | ||
| const { View } = require('react-native'); | ||
| return <View testID="keyboard-sticky-view">{children}</View>; | ||
| return <View testID="keyboard-avoiding-view">{children}</View>; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Type and forward the keyboard wrapper props.
Line 69 uses prohibited any, and the mock drops behavior and keyboardVerticalOffset, so tests cannot detect regressions in this PR’s keyboard-avoidance configuration. Define a props interface, forward the props to View, and assert behavior="padding" and keyboardVerticalOffset={0}.
As per coding guidelines, “Never use any type; use precise types and interfaces with TypeScript strict mode enabled,” and generate Jest tests that validate generated components.
🤖 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/__tests__/call-notes-modal-new.test.tsx` around lines 69
- 71, Update the KeyboardAvoidingView mock to use a precise props interface
instead of any, including children, behavior, and keyboardVerticalOffset, and
forward those props to the rendered View. Add or update the Jest assertions to
verify behavior="padding" and keyboardVerticalOffset={0} on the
keyboard-avoiding wrapper.
Source: Coding guidelines
| <FlatList | ||
| data={filteredNotes} | ||
| renderItem={renderNote} | ||
| keyExtractor={(item) => item.CallNoteId} |
There was a problem hiding this comment.
Render inefficiency caused by the inline arrow function assigned to keyExtractor, which creates a new function reference on every render cycle. Extract this function definition outside the render method to prevent unnecessary re-renders.
Kody rule violation: Avoid using .bind() or arrow functions in JSX props
Prompt for LLM
File src/components/calls/call-notes-modal.tsx:
Line 136:
Render inefficiency caused by the inline arrow function assigned to `keyExtractor`, which creates a new function reference on every render cycle. Extract this function definition outside the render method to prevent unnecessary re-renders.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
This comment has been minimized.
This comment has been minimized.
| } else { | ||
| // Mark unread notifications as read via the Novu SDK; the SDK | ||
| // optimistically updates its cache, refreshing inbox state. | ||
| notification.markAsRead?.(); |
There was a problem hiding this comment.
Unhandled promise rejection propagates when notification.markAsRead?.() invokes a Novu SDK method without awaiting or calling .catch(). Wrap the call in a try/catch block with await or append .catch((e) => logger.error('markAsRead failed', { err: e })) to handle network or auth failures.
Kody rule violation: Handle async operations with proper error handling
Prompt for LLM
File src/components/notifications/NotificationInbox.tsx:
Line 164:
Unhandled promise rejection propagates when `notification.markAsRead?.()` invokes a Novu SDK method without awaiting or calling `.catch()`. Wrap the call in a try/catch block with `await` or append `.catch((e) => logger.error('markAsRead failed', { err: e }))` to handle network or auth failures.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| } else { | ||
| // Mark unread notifications as read via the Novu SDK; the SDK | ||
| // optimistically updates its cache, refreshing inbox state. | ||
| notification.markAsRead?.(); |
There was a problem hiding this comment.
Unhandled network call rejection occurs when notification.markAsRead?.() triggers a Novu backend request without a try/catch wrapper. Wrap the call in try { await notification.markAsRead?.(); } catch (e) { logger.error('Failed to mark notification as read', { err: e }); } to add context and map to application-level errors.
Kody rule violation: Add try-catch blocks for external calls
Prompt for LLM
File src/components/notifications/NotificationInbox.tsx:
Line 164:
Unhandled network call rejection occurs when `notification.markAsRead?.()` triggers a Novu backend request without a try/catch wrapper. Wrap the call in `try { await notification.markAsRead?.(); } catch (e) { logger.error('Failed to mark notification as read', { err: e }); }` to add context and map to application-level errors.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| referenceId: item.payload?.referenceId, | ||
| referenceType: item.payload?.referenceType, | ||
| metadata: item.payload?.metadata, | ||
| markAsRead: !item.isRead && typeof item.read === 'function' ? () => item.read() : undefined, |
There was a problem hiding this comment.
Unhandled promise rejection occurs when notification.markAsRead?.() (line 164) invokes the async Novu SDK call () => item.read() (line 261) without awaiting or catching the returned promise. Wrap the invocation in handleNotificationPress or attach a catch handler to surface a toast and prevent local 'read' state desync on network or auth errors.
markAsRead: !item.isRead && typeof item.read === 'function' ? async () => { try { await item.read(); } catch (e) { logger.warn({ message: 'Failed to mark notification as read', context: { error: e } }); } } : undefined,Prompt for LLM
File src/components/notifications/NotificationInbox.tsx:
Line 261:
Unhandled promise rejection occurs when `notification.markAsRead?.()` (line 164) invokes the async Novu SDK call `() => item.read()` (line 261) without awaiting or catching the returned promise. Wrap the invocation in `handleNotificationPress` or attach a catch handler to surface a toast and prevent local 'read' state desync on network or auth errors.
Suggested Code:
markAsRead: !item.isRead && typeof item.read === 'function' ? async () => { try { await item.read(); } catch (e) { logger.warn({ message: 'Failed to mark notification as read', context: { error: e } }); } } : undefined,
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| </View> | ||
| const handleNavigateToReference = React.useCallback( | ||
| (referenceType: string, referenceId: string) => { | ||
| if (referenceType === 'call') { |
There was a problem hiding this comment.
Magic string detected where the string literal 'call' represents a value from a finite set of notification reference types but is inlined directly. Define an enum like NotificationReferenceType or a const tuple and compare against NotificationReferenceType.Call to prevent typos and ease maintenance.
Kody rule violation: Use enums instead of magic strings
Prompt for LLM
File src/components/notifications/NotificationInbox.tsx:
Line 239:
Magic string detected where the string literal `'call'` represents a value from a finite set of notification reference types but is inlined directly. Define an enum like `NotificationReferenceType` or a const tuple and compare against `NotificationReferenceType.Call` to prevent typos and ease maintenance.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| const handleNavigateToReference = React.useCallback( | ||
| (referenceType: string, referenceId: string) => { | ||
| if (referenceType === 'call') { | ||
| router.push(`/call/${referenceId}`); |
There was a problem hiding this comment.
Hardcoded route path detected where /call/${referenceId} is inlined directly. Centralize route names as constants using a route helper like ROUTES.CALL(referenceId) or buildRoute('call', referenceId) to ensure consistent referencing and single-point updates.
Kody rule violation: Centralize string constants
Prompt for LLM
File src/components/notifications/NotificationInbox.tsx:
Line 240:
Hardcoded route path detected where `/call/${referenceId}` is inlined directly. Centralize route names as constants using a route helper like `ROUTES.CALL(referenceId)` or `buildRoute('call', referenceId)` to ensure consistent referencing and single-point updates.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| referenceId?: string; | ||
| referenceType?: 'call' | 'message' | 'status' | 'note' | 'other'; | ||
| metadata?: Record<string, any>; | ||
| markAsRead?: () => void; |
There was a problem hiding this comment.
Separation of concerns violation found where the NotificationPayload interface mixes pure data with executable behavior via the markAsRead callback function. Remove the callback function from the payload interface to prevent serialization issues across network boundaries, and handle the markAsRead action separately in the consuming UI component or a dedicated service.
Kody rule violation: Separate UI logic from business logic
Prompt for LLM
File src/types/notification.ts:
Line 11:
Separation of concerns violation found where the `NotificationPayload` interface mixes pure data with executable behavior via the `markAsRead` callback function. Remove the callback function from the payload interface to prevent serialization issues across network boundaries, and handle the `markAsRead` action separately in the consuming UI component or a dedicated service.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
| try { | ||
| await item.read(); | ||
| } catch (error) { | ||
| logger.warn({ message: 'Failed to mark notification as read', context: { error } }); |
There was a problem hiding this comment.
Incomplete warning logs prevent tracing failed notification read operations. Rule [3] mandates appending the operation name and relevant identifiers, such as notificationId: item.id, to the structured context object.
Kody rule violation: Include error context in structured logs
Prompt for LLM
File src/components/notifications/NotificationInbox.tsx:
Line 267:
Incomplete warning logs prevent tracing failed notification read operations. Rule [3] mandates appending the operation name and relevant identifiers, such as `notificationId: item.id`, to the structured `context` object.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
2 participants
Description
This PR addresses fixes and improvements to the Call Notes modal and Notification Inbox components.
Call Notes Modal
Replaced
KeyboardStickyViewwithKeyboardAvoidingViewto wrap the entire modal content (header, search bar, notes list, and footer) instead of just the footer. This provides more consistent keyboard handling behavior where the full view adjusts when the keyboard appears, rather than only sticking the input section to the keyboard.Notification Inbox
Refactored the notification list for improved rendering performance:
NotificationRowcomponent with a custom comparison function to prevent unnecessary re-rendershandleNotificationPress,handleNotificationLongPress,toggleNotificationSelection,handleNavigateToReference, andrenderItem) inuseCallbackto stabilize references and reduce re-rendersNotification Detail
Removed the entrance animation (slide and fade-in) and the automatic read-marking behavior (via Novu
useNotificationsrefetch) that occurred when opening an unread notification. The detail panel now appears immediately without animation.