BA-2274: native notifications list #225
Conversation
Ronan-Fernandes
requested review from
anicioalexandre,
deboracosilveira and
priscilladeroode
as code owners
|
WalkthroughThis update introduces a comprehensive set of new React Native components for the notifications module and complementary additions to the design system. New components include a notification bell with a badge, various notification item types (comment, reply, reaction), empty state displays, mark-all-as-read functionality, and a complete notifications list with Relay integration and subscription support. Additionally, new design system components for avatars (with placeholders) and badges along with associated types and styling files have been added. Changes
Sequence Diagram(s)sequenceDiagram
participant NB as NotificationBellWithBadge
participant Relay as NotificationsPopoverQuery
participant Sub as useNotificationsSubscription
participant Badge as Badge Component
NB->>Relay: Execute lazy load query for notifications
Relay-->>NB: Return user notification data
NB->>Sub: Subscribe to real-time notification updates
Sub-->>NB: Stream updated notification info
NB->>Badge: Render Badge if notificationsUnreadCount > 0
sequenceDiagram
participant U as User
participant NL as NotificationsList
participant Relay as useLazyLoadQuery / Pagination Fragment
participant Sub as useNotificationsSubscription
participant NI as NotificationItem
U->>NL: Opens notifications list
NL->>Relay: Fetch initial notifications
Relay-->>NL: Return notifications data
NL->>Sub: Subscribe to notification events
Sub-->>NL: Streaming notification updates
NL->>NI: Render/update notification items accordingly
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
packages/components/modules/notifications/native/NotificationBellWithBadge/types.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
packages/components/modules/notifications/native/NotificationsList/EmptyState/index.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
packages/components/modules/notifications/native/NotificationBellWithBadge/index.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/components/.eslintrc.js
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Ronan-Fernandes
changed the base branch from
master
to
BA-2279-mobile-settings-menu
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently, please check "Code review limits" under "Moderation" settings.
Actionable comments posted: 7
🧹 Nitpick comments (37)
packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/types.ts (1)
1-6: Clean interface definition for notification headers.The interface properly defines all required properties for rendering notification headers with appropriate types.
Consider standardizing
actorNameto use just one null pattern (eithernullorundefinedbut not both) for consistency, though this is a minor suggestion.packages/design-system/components/native/avatars/AvatarWithPlaceholder/types.ts (1)
1-5: Well-defined interface with appropriate optional properties.The interface properly defines all properties as optional with good type definitions. Consider adding JSDoc comments to document the purpose of each property for better developer experience.
Add JSDoc comments to improve the developer experience:
+/** + * Props for the AvatarWithPlaceholder component + */ export interface AvatarWithPlaceholderProps { + /** Size of the avatar in pixels */ size?: number + /** Source URL for the avatar image */ imgSource?: string | null + /** Accessible text description of the avatar */ alt?: string }packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/types.ts (1)
6-9: Consider separating props interface from component type definition.The current approach extends
FC<PropsWithChildren>directly, which mixes component type definition with props definition. This could be improved for clarity.-export interface NotificationContentProps extends FC<PropsWithChildren> { +export interface NotificationContentComponentProps { + children?: React.ReactNode; +} + +export interface NotificationContentProps extends FC<NotificationContentComponentProps> { Header: typeof NotificationHeader Body: typeof NotificationBody }packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/index.tsx (2)
8-14: Consider removing unnecessary View wrapper.The
Viewcomponent wrapping theAvatarWithPlaceholderdoesn't seem to add any styling or functionality. Unless there's a specific layout requirement, consider simplifying by removing the wrapper.const NotificationAvatar: FC<NotificationAvatarProps> = ({ actorAvatar, actorName }) => ( - <View> <AvatarWithPlaceholder imgSource={actorAvatar} alt={actorName ?? `Notification's user avatar`} /> - </View> )
12-12: Use consistent apostrophe style in string literals.The string uses a straight apostrophe in "Notification's". Consider using a consistent style (either straight or curly) across the codebase for string literals.
packages/components/modules/notifications/native/NotificationBellWithBadge/styles.ts (1)
5-17: Well-structured theme-based styling approachThe styles are properly organized and make good use of the theme for consistent styling. The badge positioning and appearance align well with standard notification badge patterns.
Consider specifying fontWeight as a string ('700' or 'bold') rather than a number for better cross-platform consistency:
- fontWeight: 700, + fontWeight: '700',packages/components/modules/notifications/common/graphql/subscriptions/useNotificationsSubscription.ts (1)
35-38: Improved error handling with additional contextThe enhanced error logging provides better context for debugging subscription issues, which is a good improvement.
Consider these improvements to further enhance error handling:
- Make the log message more descriptive about the specific subscription
- Use a more specific type than
anyfor errors- Consider adding error recovery or propagation mechanisms
- onError: (error: any) => { - console.log('running on error before console.error') - console.error('useNotificationsSubscription', error) + onError: (error: Error) => { + console.log('Notifications subscription error occurred') + console.error('useNotificationsSubscription failed:', error) + // Consider adding error recovery logic or propagating the error },packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationRoot/index.tsx (1)
7-11: Clean and well-structured container componentThe NotificationRoot component follows good React patterns as a simple container component that provides consistent styling for notification content.
Consider these minor improvements:
- Memoize styles creation to avoid recreating them on every render
- Add accessibility attributes to the container View
const NotificationRoot: FC<PropsWithChildren> = ({ children }) => { - const styles = createStyles() + const styles = useMemo(() => createStyles(), []) - return <View style={styles.notificationRootContainer}>{children}</View> + return <View + style={styles.notificationRootContainer} + accessible + accessibilityRole="none" + >{children}</View> }Remember to import useMemo from React if implementing this suggestion.
packages/design-system/layouts/native/index.ts (1)
3-5: Consistent addition of new layout exportsThe exports for AppbarNavigationLayout follow the same pattern as existing layout components, maintaining consistency in the design system's API.
For consistent spacing in the file, consider removing the extra blank line after the AppbarNavigationLayout exports:
export { default as AppbarNavigationLayout } from './AppbarNavigationLayout' export type * from './AppbarNavigationLayout/types' - export { default as BottomNavigationLayout } from './BottomNavigationLayout'packages/components/modules/notifications/native/NotificationsList/types.ts (1)
5-9: Good interface design for component customization.The NotificationsListProps interface provides flexible customization options with optional components. Consider renaming the
NotificationItemPropsproperty to something likenotificationItemPropsoritemPropsto avoid confusion with the type name.export interface NotificationsListProps { EmptyState?: FC NotificationItem?: FC<NotificationItemProps> - NotificationItemProps?: Partial<NotificationItemProps> + notificationItemProps?: Partial<NotificationItemProps> }packages/design-system/components/native/icons/CloseIcon/index.tsx (1)
8-30: Well-implemented icon component with proper theme integration.The CloseIcon component is correctly implemented with:
- Appropriate default values for props
- Proper theme color handling
- Correct SVG structure with accessibility considerations
One suggestion would be to consider adding an accessibility label prop to improve screen reader support.
Consider enhancing accessibility by adding an aria-label or accessibilityLabel prop:
const CloseIcon: FC<SvgIconProps> = ({ isActive = false, color, width = '24', height = '25', + accessibilityLabel = 'Close', ...props }) => { const { colors } = useTheme() const defaultColor = color ?? colors.object.low const svgColor = isActive ? colors.primary.high : defaultColor return ( <Svg width={width} height={height} viewBox="0 0 24 25" color={svgColor} + accessibilityLabel={accessibilityLabel} {...props} >packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/index.tsx (1)
21-36: Good notification header implementation with a minor optimization opportunity.The component has a clean structure with appropriate text styling and badge indication for unread notifications. However, there's an empty View wrapping the Badge component that could be simplified.
Consider simplifying the badge rendering by removing the unnecessary wrapping View:
- <View>{unread && <Badge size={10} style={styles.badge} />}</View> + {unread && <Badge size={10} style={styles.badge} />}packages/design-system/components/native/icons/ChevronIcon/index.tsx (1)
1-47: Well-structured and flexible icon component!The ChevronIcon component implements a reusable, customizable chevron that supports multiple directions through rotation. The code follows good practices including:
- Clear prop definitions with sensible defaults
- Theme integration for consistent styling
- Direction-based rotation using a mapping object
- Proper prop spreading for additional SVG attributes
Consider adding accessibility attributes like
accessibilityRole="image"andaccessibilityLabelto improve screen reader support.packages/design-system/components/native/avatars/ClickableAvatar/index.tsx (1)
10-35: Well-implemented ClickableAvatar component with clear conditional renderingThe component correctly handles both normal and fallback states, with appropriate default values for dimensions. The use of Pressable for touch handling follows React Native best practices.
Consider adding accessibility properties to enhance the component:
<Pressable onPress={onPress}> + <View + style={styles.container} + accessibilityRole="button" + accessible={true} + accessibilityLabel="Avatar" + > - <View style={styles.container}> <Image source={imageSource} style={styles.image} /> </View> </Pressable>packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentCreated/index.tsx (1)
9-10: Consider a cleaner approach to access typenameThe eslint rule is being disabled to access
__typename. If this is from GraphQL-generated types, consider adding a utility function that extracts type names without triggering the linting rule, or configure eslint to allow this pattern specifically for GraphQL types.- // eslint-disable-next-line no-underscore-dangle - const message = `added a comment to your ${notification.target?.__typename.toLowerCase()}` + const targetType = notification.target?.type || notification.target?.__typename?.toLowerCase() || 'item' + const message = `added a comment to your ${targetType}`packages/components/modules/notifications/native/NotificationBellWithBadge/index.tsx (2)
25-29: Add error handling for GraphQL queryThe GraphQL query using
useLazyLoadQuerydoesn't include any error handling. If the query fails, the component might break.Consider adding proper error handling:
const { me } = useLazyLoadQuery<NotificationsPopoverQueryType>( NotificationsPopoverQuery, {}, { fetchPolicy: 'store-and-network' }, ) + + // Show fallback UI if query fails + if (!me) { + return ( + <View> + <BellIcon width="24" height="24" /> + </View> + ) + }
36-41: Add accessibility attributes to notification bellThe notification bell lacks accessibility attributes, which would help users with screen readers.
return ( - <View> - <BellIcon width="24" height="24" /> + <View accessibilityRole="button" accessibilityLabel="Notifications"> + <BellIcon width="24" height="24" accessibilityRole="image" /> {!!user?.notificationsUnreadCount && ( - <Badge style={styles.badge}>{user?.notificationsUnreadCount}</Badge> + <Badge + style={styles.badge} + accessibilityLabel={`${user.notificationsUnreadCount} unread notifications`} + > + {user.notificationsUnreadCount} + </Badge> )} </View>packages/design-system/layouts/native/BottomNavigationLayout/index.tsx (1)
19-24: Optimize useLayoutEffect dependenciesThe
useLayoutEffecthas dependencies that could cause unnecessary re-renders.Consider memoizing the headerShown and swipeEnabled values:
+ const shouldShowHeader = !routesWithoutHeader?.includes(pathname) && isPathnameInTabs + useLayoutEffect(() => { navigation.setOptions({ - headerShown: !routesWithoutHeader?.includes(pathname) && isPathnameInTabs, - swipeEnabled: !routesWithoutHeader?.includes(pathname) && isPathnameInTabs, + headerShown: shouldShowHeader, + swipeEnabled: shouldShowHeader, }) - }, [navigation, pathname, isPathnameInTabs, routesWithoutHeader]) + }, [navigation, shouldShowHeader])packages/components/modules/notifications/native/NotificationsList/NotificationItem/ReactionCreated/index.tsx (1)
10-11: Improve TypeScript type access patternDirect access to
__typenamewith an eslint-disable comment suggests a type issue.Consider creating a type guard or helper function instead of using eslint-disable:
- // eslint-disable-next-line no-underscore-dangle - const message = `liked your ${notification.target?.__typename.toLowerCase()}` + const targetType = notification.target?.type || notification.target?.__typename?.toLowerCase() || 'content' + const message = `liked your ${targetType}`packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentReply/index.tsx (2)
9-10: Improve TypeScript type access patternSimilar to the ReactionCreated component, the direct access to
__typenameshould be improved.- // eslint-disable-next-line no-underscore-dangle - const message = `added a comment to your ${notification.target?.__typename.toLowerCase()}` + const targetType = notification.target?.type || notification.target?.__typename?.toLowerCase() || 'content' + const message = `added a comment to your ${targetType}`
25-25: Add null check for actionObject bodyThe component should check if actionObject body exists before rendering it.
- <Notification.Content.Body content={notification.actionObject?.body} /> + {notification.actionObject?.body && ( + <Notification.Content.Body content={notification.actionObject.body} /> + )}packages/components/modules/notifications/native/NotificationsList/NotificationItem/index.tsx (1)
10-36: Well-implemented NotificationItem component with good separation of concernsThe component is well-structured with clear responsibility separation between displaying the notification and handling the "mark as read" functionality.
Consider these potential improvements:
- Wrap the component with React.memo for better performance in lists
- Add error handling for the mutation
- Consider adding a loading state during mutation execution
-const NotificationItem: FC<NotificationItemProps> = ({ +const NotificationItem: FC<NotificationItemProps> = React.memo(({ notification: notificationRef, NotificationItemRenderer = DefaultNotificationItemRenderer, -}) => { +}) => { const notification = useFragment(NotificationItemFragment, notificationRef) - const [commitMutation] = useNotificationsMarkAsRead() + const [commitMutation, isInFlight] = useNotificationsMarkAsRead() const markAsRead = () => { if (notification.unread) { commitMutation({ variables: { input: { read: true, notificationIds: [notification.id], }, }, + onError: (error) => { + console.error('Failed to mark notification as read:', error) + }, }) } } return ( - <Pressable onPress={markAsRead}> + <Pressable onPress={markAsRead} disabled={isInFlight}> <NotificationItemRenderer notification={notification} /> </Pressable> ) -} +})packages/components/modules/navigations/web/NavMini/__tests__/NavMini.cy.tsx (2)
108-156: Test the toggle button click interaction directlyThe current test calls setSettings directly rather than testing the actual click interaction of the toggle button, which doesn't fully test the component's behavior.
Consider modifying the test to actually click the toggle button:
it('expands to vertical layout when toggle button is clicked', () => { const setSettings = cy.stub().as('setSettings') const initialSettings = createTheme().settings cy.mount( <ThemeProvider {...createTheme()}> <NavMini navData={mockNavData} openNav={false} onCloseNav={cy.stub()} LogoIcon={LogoIcon} settings={initialSettings} setSettings={setSettings} /> </ThemeProvider>, ) cy.findByRole('navigation') .should('be.visible') .and(($el) => { const width = parseInt($el.css('width')) expect(width).to.be.lte(88) }) - setSettings({ themeLayout: 'vertical' }) + // Find and click the toggle button to expand + cy.get('button.MuiIconButton-root').click() cy.get('@setSettings').should('have.been.calledWith', { themeLayout: 'vertical', }) // ...rest of the test })
63-184: Well-structured tests with good coverageThe tests cover important aspects of the NavMini component including rendering, accessibility, and positioning.
Consider extracting the common setup code to reduce duplication:
+ const mountNavMini = (settings = createTheme().settings, setSettings = cy.stub()) => { + cy.mount( + <ThemeProvider {...createTheme()}> + <NavMini + navData={mockNavData} + openNav={false} + onCloseNav={cy.stub()} + LogoIcon={LogoIcon} + settings={settings} + setSettings={setSettings} + /> + </ThemeProvider>, + ) + } // Then in your tests: it('displays a compact navigation with accessible icons and labels', () => { - cy.mount(...) + mountNavMini() // assertions... })packages/components/modules/navigations/web/NavigationLayout/MainContainer/__tests__/MainContainer.cy.tsx (1)
79-105: Consistent testing approach for mini and vertical layouts.Both tests follow the same pattern to verify width constraints, making the tests consistent and maintainable. However, since they're testing the same property (width: 1000px), consider combining them or parameterizing the test to reduce duplication.
Consider using a parameterized test approach to reduce duplication between the mini and vertical layout tests since they're checking the same property value:
['mini', 'vertical'].forEach((layout) => { it(`positions content correctly in ${layout} layout`, () => { cy.mount( <ThemeProvider {...createTheme(layout as ThemeLayout)}> <MainContainer> <div role="main" aria-label="Test Content"> Test Content </div> </MainContainer> </ThemeProvider>, ) cy.findByRole('main', { name: 'Test Content' }).parent().should('have.css', 'width', '1000px') }) })packages/design-system/components/native/appbars/AppBar/index.tsx (2)
22-28: Consider adding accessibility labels to the back button.
Conditionally rendering the back button is good, but adding an accessibility label (e.g.,accessibilityLabel="Go back") might improve screen reader support.
33-39: Mirroring accessibility for the close button.
Similarly, consider adding an accessibility label (e.g.,accessibilityLabel="Close") to better support assistive technologies.packages/components/modules/notifications/native/NotificationsList/index.tsx (4)
36-40: Infinite scroll reminder.
ATODOcomment is present for infinite scrolling. Happy to assist if you need a code snippet or approach for Relay pagination.
49-51: Refetch approach is clear.
Re-invoking withfetchPolicy: 'network-only'ensures up-to-date notifications. Consider also providing a loading indicator.
56-56: Notifications divider & read status enhancement.
You have a TODO. Let me know if you'd like assistance integrating design patterns for separators or highlighting unread items.
66-74:renderContentlogic is well-organized.
Empty vs list states are nicely separated. Make sure yourFlatListincludes akeyExtractorif needed for stable item IDs.packages/components/modules/navigations/web/NavHorizontal/__tests__/NavHorizontal.cy.tsx (2)
35-66: Mock data approach is straightforward.
All icons are textual placeholders, which is fine for test clarity. Make sure users understand these are not real icons.
124-158: Theme mode testing is comprehensive.
Good job verifying color changes in light and dark modes. Consider verifying keyboard navigation for advanced accessibility coverage.packages/components/modules/navigations/web/__shared__/NavSectionHorizontal/__tests__/NavSectionHorizontal.cy.tsx (1)
112-129: Improve hover test assertion.The current test assumes the background color after mouseout is the same as during hover, which might not accurately test the hover state transition.
Consider capturing the initial background color before hover for comparison:
-cy.findByRole('link', { name: 'Dashboard' }) - .find('.MuiListItemButton-root') - .trigger('mouseover') - .should('have.css', 'background-color') - .then((backgroundColor) => { - cy.findByRole('link', { name: 'Dashboard' }) - .find('.MuiListItemButton-root') - .trigger('mouseout') - .should('have.css', 'background-color', backgroundColor) - }) +cy.findByRole('link', { name: 'Dashboard' }) + .find('.MuiListItemButton-root') + .should('have.css', 'background-color') + .then((initialColor) => { + cy.findByRole('link', { name: 'Dashboard' }) + .find('.MuiListItemButton-root') + .trigger('mouseover') + .should('have.css', 'background-color') + .should('not.eq', initialColor) + .then(() => { + cy.findByRole('link', { name: 'Dashboard' }) + .find('.MuiListItemButton-root') + .trigger('mouseout') + .should('have.css', 'background-color', initialColor) + }) + })packages/design-system/components/native/icons/LogoutIcon/index.tsx (1)
8-45: The LogoutIcon implementation looks solid but could benefit from accessibility improvements.The SVG icon implementation follows good patterns with theming integration and proper props handling. Good use of the theme system for dynamic coloring based on the active state.
Consider adding accessibility attributes to make this icon screen reader friendly:
return ( <Svg width={width} height={height} viewBox="0 0 18 17" color={svgColor} fill="none" {...props} + accessible={true} + accessibilityLabel="Logout" + accessibilityRole="image" > <Path d="M9.83333 8.61589H1.5" stroke="currentColor" strokeWidth="1.5" strokeLinecap="round" strokeLinejoin="round" /> <Path d="M4.41667 5.69922L1.5 8.61589L4.41667 11.5326" stroke="currentColor" strokeWidth="1.5" strokeLinecap="round" strokeLinejoin="round" /> <Path d="M9 1.11523C13.1421 1.11523 16.5 4.4731 16.5 8.61523C16.5 12.7574 13.1421 16.1152 9 16.1152" stroke="currentColor" strokeWidth="1.5" strokeLinecap="round" strokeLinejoin="round" /> </Svg> )Also, adding a testID prop would help with automated testing:
return ( <Svg width={width} height={height} viewBox="0 0 18 17" color={svgColor} fill="none" + testID="logout-icon" {...props} >packages/design-system/components/native/icons/BellIcon/index.tsx (2)
8-38: Well-implemented BellIcon component with consistent pattern.The implementation follows the same pattern as other icon components, with appropriate theming integration. The code structure is clean and follows React best practices.
As with the LogoutIcon, consider enhancing accessibility for screen readers:
return ( <Svg width={width} height={height} viewBox="0 0 20 21" color={svgColor} fill="none" {...props} + accessible={true} + accessibilityLabel="Notifications" + accessibilityRole="image" + testID="bell-icon" > <Path d="M11.6674 17.2852C11.2738 17.81 10.656 18.1188 10 18.1188C9.34399 18.1188 8.72625 17.81 8.33264 17.2852" stroke="currentColor" strokeWidth="1.5" strokeLinecap="round" strokeLinejoin="round" /> <Path d="M4.99796 11.018V8.11536C4.99796 5.35279 7.23747 3.11328 10 3.11328V3.11328C11.3267 3.11328 12.599 3.64028 13.537 4.57836C14.4751 5.51643 15.0021 6.78873 15.0021 8.11536V11.018H14.9983L16.1455 12.166C16.6539 12.6748 16.8058 13.4398 16.5303 14.1042C16.2549 14.7687 15.6064 15.2018 14.8871 15.2016H5.113C4.39373 15.2018 3.74518 14.7687 3.46973 14.1042C3.19429 13.4398 3.34618 12.6748 3.85459 12.166L5.00173 11.018" stroke="currentColor" strokeWidth="1.5" strokeLinecap="round" strokeLinejoin="round" /> </Svg> )
1-40: Ensure consistent memoization across icon components.Both BellIcon and LogoutIcon components might benefit from memoization to prevent unnecessary re-renders, especially if they're used in performance-sensitive areas like lists or frequently updating components.
Consider wrapping the component with React.memo:
import { FC } from 'react' + import React from 'react' import Svg, { Path } from 'react-native-svg' import { useTheme } from '../../../../providers/native' import { SvgIconProps } from '../types' const BellIcon: FC<SvgIconProps> = ({ isActive = false, color, width = '20', height = '21', ...props }) => { // component implementation } - export default BellIcon + export default React.memo(BellIcon)This optimization should be applied consistently across all icon components in your design system to maintain code uniformity.
🛑 Comments failed to post (7)
packages/design-system/components/native/views/View/index.tsx (1)
9-9:
⚠️ Potential issue❓ Verification inconclusive
Changed default behavior for keyboard dismissal.
The default value of
dismissKeyboardhas been changed fromtruetofalse, which means keyboard dismissal will no longer happen by default when a View is pressed. While this provides more explicit control, this is a breaking change that might affect existing implementations.Consider:
- Adding a deprecation warning for a transition period
- Including this change prominently in release notes
- Verifying all dependent components handle this change correctly
🏁 Script executed:
#!/bin/bash # Check for usage of View component without explicit dismissKeyboard prop rg -p "import.*View.*from.*design-system" -A 10 | rg "<View" | rg -v "dismissKeyboard"Length of output: 293
ACTION REQUIRED: Verify Updated Default for Keyboard Dismissal in View
We've confirmed that the default value for the
dismissKeyboardprop is nowfalse, whereas it was previouslytrue. The recent usage scan shows several instances of the<View>component where thedismissKeyboardprop isn’t explicitly set—indicating these components now rely on the new default behavior. This breaking change could inadvertently affect behavior in parts of the codebase that depended on automatic keyboard dismissal.Recommendations:
- Deprecation Warning: Consider adding a deprecation warning to alert developers during a transition period.
- Release Notes: Ensure this breaking change is prominently documented in the release notes.
- Usage Audit: Manually verify or update dependent components (e.g., those found in the grep output where
<View>is used without adismissKeyboardprop) to ensure they handle this change as expected.packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/index.tsx (2)
1-7:
⚠️ Potential issueMissing FC import from React.
The FC type is used in the component but not imported from React.
-import { View } from '@baseapp-frontend/design-system/components/native/views' +import React, { FC } from 'react' +import { View } from '@baseapp-frontend/design-system/components/native/views'📝 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.import React, { FC } from 'react' import { View } from '@baseapp-frontend/design-system/components/native/views' import NotificationBody from './NotificationBody' import NotificationHeader from './NotificationHeader' import { createStyles } from './styles' import { NotificationContentProps } from './types'
8-12: 🛠️ Refactor suggestion
⚠️ Potential issueFix component type definition and optimize styles creation.
The component incorrectly uses the props type as its own type. Additionally, creating styles on every render is inefficient.
-const NotificationContent: NotificationContentProps = ({ children }) => { - const styles = createStyles() +const NotificationContent: FC<NotificationContentProps> = ({ children }) => { + const styles = React.useMemo(() => createStyles(), []) return <View style={styles.notificationContentContainer}>{children}</View> }Alternatively, for even better performance, move styles creation outside the component:
+const styles = createStyles() + -const NotificationContent: NotificationContentProps = ({ children }) => { - const styles = createStyles() +const NotificationContent: FC<NotificationContentProps> = ({ children }) => { return <View style={styles.notificationContentContainer}>{children}</View> }📝 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 NotificationContent: FC<NotificationContentProps> = ({ children }) => { const styles = React.useMemo(() => createStyles(), []) return <View style={styles.notificationContentContainer}>{children}</View> }const styles = createStyles() const NotificationContent: FC<NotificationContentProps> = ({ children }) => { return <View style={styles.notificationContentContainer}>{children}</View> }packages/design-system/components/native/avatars/AvatarWithPlaceholder/index.tsx (1)
18-20:
⚠️ Potential issueFix Avatar.Image source implementation
The current implementation passes a function to the
sourceprop of Avatar.Image, but this component expects an ImageSourcePropType object (like{uri: 'https://...'}) instead of a function or a React component.Replace with:
- const renderImage = () => <Image source={{ uri: imgSource }} alt={alt} /> - - return <Avatar.Image size={size} source={renderImage} /> + return <Avatar.Image size={size} source={{ uri: imgSource }} />If custom Image component usage is required, consider a different approach to integrate it with Avatar.Image.
📝 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.return <Avatar.Image size={size} source={{ uri: imgSource }} />packages/design-system/layouts/native/BottomNavigationLayout/index.tsx (1)
11-11: 🛠️ Refactor suggestion
Add default value for routesWithoutHeader
The
routesWithoutHeaderprop is used with optional chaining, but no default value is provided.-const BottomNavigationLayout: FC<BottomNavigationLayoutProps> = ({ tabs, routesWithoutHeader }) => { +const BottomNavigationLayout: FC<BottomNavigationLayoutProps> = ({ tabs, routesWithoutHeader = [] }) => {📝 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 BottomNavigationLayout: FC<BottomNavigationLayoutProps> = ({ tabs, routesWithoutHeader = [] }) => {packages/components/modules/notifications/native/NotificationsList/NotificationItem/ReactionCreated/index.tsx (1)
13-28: 🛠️ Refactor suggestion
Add null checks for notification data
The component doesn't handle the case when notification data is incomplete.
return ( <Notification.Root> <Notification.Avatar - actorAvatar={notification.actor?.avatar?.url} - actorName={notification.actor?.fullName} + actorAvatar={notification.actor?.avatar?.url || undefined} + actorName={notification.actor?.fullName || 'Unknown user'} /> <NotificationContent> <NotificationContent.Header message={message} - timestamp={formatRelativeTime(notification.timestamp)} - actorName={notification.actor?.fullName} + timestamp={notification.timestamp ? formatRelativeTime(notification.timestamp) : ''} + actorName={notification.actor?.fullName || 'Unknown user'} unread={notification.unread} /> - <NotificationContent.Body content={notification.target?.body} /> + {notification.target?.body && ( + <NotificationContent.Body content={notification.target.body} /> + )} </NotificationContent> </Notification.Root> )📝 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.return ( <Notification.Root> <Notification.Avatar actorAvatar={notification.actor?.avatar?.url || undefined} actorName={notification.actor?.fullName || 'Unknown user'} /> <NotificationContent> <NotificationContent.Header message={message} timestamp={notification.timestamp ? formatRelativeTime(notification.timestamp) : ''} actorName={notification.actor?.fullName || 'Unknown user'} unread={notification.unread} /> {notification.target?.body && ( <NotificationContent.Body content={notification.target.body} /> )} </NotificationContent> </Notification.Root> )packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentReply/index.tsx (1)
18-26: 🛠️ Refactor suggestion
Fix inconsistency in component structure
There's an inconsistency between
CommentReplyandReactionCreatedcomponents. CommentReply usesNotification.Contentwhile ReactionCreated usesNotificationContent.- <Notification.Content> - <Notification.Content.Header + <NotificationContent> + <NotificationContent.Header message={message} timestamp={formatRelativeTime(notification.timestamp)} actorName={notification.actor?.fullName} unread={notification.unread} /> - <Notification.Content.Body content={notification.actionObject?.body} /> - </Notification.Content> + <NotificationContent.Body content={notification.actionObject?.body} /> + </NotificationContent>📝 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.<NotificationContent> <NotificationContent.Header message={message} timestamp={formatRelativeTime(notification.timestamp)} actorName={notification.actor?.fullName} unread={notification.unread} /> <NotificationContent.Body content={notification.actionObject?.body} /> </NotificationContent>
Ronan-Fernandes
force-pushed
the
BA-2274-fe-packages-rn-notifications-list
branch
from
75b91e5 to
06703cc
Compare
anicioalexandre
force-pushed
the
BA-2279-mobile-settings-menu
branch
from
a7a6312 to
42451aa
Compare
Ronan-Fernandes
force-pushed
the
BA-2274-fe-packages-rn-notifications-list
branch
from
06703cc to
9497ed7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
packages/components/modules/notifications/native/NotificationsList/NotificationItem/index.tsx (4)
16-29: Consider adding error handling for the mutation.The current implementation doesn't handle potential errors when marking notifications as read. Adding error handling would improve the user experience by providing feedback when operations fail.
- const [commitMutation] = useNotificationsMarkAsRead() + const [commitMutation, isInFlight] = useNotificationsMarkAsRead() const markAsRead = () => { if (notification.unread) { commitMutation({ variables: { input: { read: true, notificationIds: [notification.id], }, }, + onError: (error) => { + console.error('Failed to mark notification as read:', error) + // Consider adding user feedback here + }, }) } }
18-29: Memoize the markAsRead function to prevent unnecessary recreations.Using useCallback for the markAsRead function will prevent it from being recreated on every render, which can improve performance.
+import { FC, useCallback } from 'react' -import { FC } from 'react' // ... - const markAsRead = () => { + const markAsRead = useCallback(() => { if (notification.unread) { commitMutation({ variables: { input: { read: true, notificationIds: [notification.id], }, }, }) } - } + }, [notification.unread, notification.id, commitMutation])
31-35: Add accessibility properties to the Pressable component.To improve accessibility for users with screen readers, add appropriate accessibility properties to the Pressable component.
return ( - <Pressable onPress={markAsRead}> + <Pressable + onPress={markAsRead} + accessibilityRole="button" + accessibilityLabel="Mark notification as read" + accessibilityHint="Marks this notification as read"> <NotificationItemRenderer notification={notification} /> </Pressable> )
1-39: Consider implementing loading state feedback.The component doesn't show any loading state when the mutation is in progress. Adding visual feedback during the mutation would improve user experience.
This could be implemented by:
- Using the isInFlight state from the mutation hook
- Disabling the Pressable or showing a loading indicator when the mutation is in progress
- Re-enabling interaction when the mutation completes
Here's how you might implement it:
- const [commitMutation] = useNotificationsMarkAsRead() + const [commitMutation, isInFlight] = useNotificationsMarkAsRead() // ... return ( <Pressable onPress={markAsRead} + disabled={isInFlight} accessibilityRole="button" accessibilityLabel="Mark notification as read" accessibilityHint="Marks this notification as read"> <NotificationItemRenderer notification={notification} /> + {isInFlight && <ActivityIndicator size="small" />} </Pressable> )packages/components/modules/notifications/native/NotificationBellWithBadge/index.tsx (1)
21-43: Add error handling and accessibility improvements to the notification bell component.The component logic is well-structured, but consider these important additions:
- Add error handling for the GraphQL query to gracefully handle network failures
- Include loading state handling for a better user experience
- Add accessibility attributes to the bell icon
const NotificationBellWithBadge: FC = () => { const theme = useTheme() const styles = createStyles(theme) + const [isLoading, setIsLoading] = useState(true) + const [error, setError] = useState<Error | null>(null) const { me } = useLazyLoadQuery<NotificationsPopoverQueryType>( NotificationsPopoverQuery, {}, - { fetchPolicy: 'store-and-network' }, + { + fetchPolicy: 'store-and-network', + onComplete: () => setIsLoading(false), + onError: (error) => { + setIsLoading(false) + setError(error) + } + }, ) const user = useFragment<NotificationUserMenuFragment$key>(NotificationUserMenuFragment, me) useNotificationsSubscription(user?.id) + if (error) { + // Gracefully handle the error - could log to monitoring service + console.error('Error fetching notifications:', error) + } return ( <View> - <BellIcon width="24" height="24" /> + <BellIcon + width="24" + height="24" + accessibilityLabel="Notifications" + accessibilityRole="button" + /> {!!user?.notificationsUnreadCount && ( <Badge style={styles.badge}>{user?.notificationsUnreadCount}</Badge> )} </View> ) }packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/index.tsx (3)
24-26: Consider adding truncation for long actor names.The actor's name is displayed without any truncation handling, which could cause layout issues if the name is particularly long.
- <Text variant="body2" color="high" style={styles.profileName}> + <Text variant="body2" color="high" style={styles.profileName} numberOfLines={1} ellipsizeMode="tail"> {actorName ?? ''} </Text>
27-29: Consider adding truncation for message text.Similar to the actor name, the message text should handle potential overflow situations to maintain consistent layout.
- <Text variant="body2" color="high"> + <Text variant="body2" color="high" numberOfLines={1} ellipsizeMode="tail"> {message} </Text>
31-31: Add accessibility label for the unread badge.The badge indicating unread status should have an accessibility label to improve screen reader support.
- <View>{unread && <Badge size={10} style={styles.badge} />}</View> + <View>{unread && <Badge size={10} style={styles.badge} accessibilityLabel="Unread notification" />}</View>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (49)
packages/authentication/package.json(1 hunks)packages/components/modules/notifications/common/graphql/subscriptions/useNotificationsSubscription.ts(1 hunks)packages/components/modules/notifications/native/NotificationBellWithBadge/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationBellWithBadge/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/EmptyState/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/EmptyState/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentCreated/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentReply/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationRoot/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationRoot/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/NotificationItemRenderer/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/NotificationItemRenderer/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/ReactionCreated/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/types.ts(1 hunks)packages/components/modules/notifications/native/index.ts(1 hunks)packages/components/package.json(2 hunks)packages/config/package.json(1 hunks)packages/design-system/components/native/avatars/AvatarWithPlaceholder/index.tsx(1 hunks)packages/design-system/components/native/avatars/AvatarWithPlaceholder/styles.ts(1 hunks)packages/design-system/components/native/avatars/AvatarWithPlaceholder/types.ts(1 hunks)packages/design-system/components/native/avatars/index.ts(1 hunks)packages/design-system/components/native/badges/Badge/index.tsx(1 hunks)packages/design-system/components/native/badges/Badge/types.ts(1 hunks)packages/design-system/components/native/badges/index.ts(1 hunks)packages/design-system/package.json(2 hunks)packages/eslint-plugin/package.json(1 hunks)packages/graphql/package.json(1 hunks)packages/provider/package.json(1 hunks)packages/test/package.json(1 hunks)packages/utils/package.json(1 hunks)packages/wagtail/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (43)
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/styles.ts
- packages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/NotificationItemRenderer/types.ts
- packages/components/modules/notifications/native/NotificationsList/styles.ts
- packages/design-system/components/native/badges/index.ts
- packages/design-system/components/native/badges/Badge/index.tsx
- packages/components/modules/notifications/native/NotificationsList/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationRoot/index.tsx
- packages/design-system/components/native/avatars/index.ts
- packages/components/modules/notifications/common/graphql/subscriptions/useNotificationsSubscription.ts
- packages/components/modules/notifications/native/NotificationBellWithBadge/styles.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/types.ts
- packages/design-system/components/native/badges/Badge/types.ts
- packages/components/modules/notifications/native/NotificationsList/EmptyState/styles.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/styles.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationRoot/styles.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/types.ts
- packages/components/modules/notifications/native/index.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/types.ts
- packages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/index.tsx
- packages/design-system/components/native/avatars/AvatarWithPlaceholder/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/styles.ts
- packages/components/modules/notifications/native/NotificationsList/EmptyState/index.tsx
- packages/design-system/components/native/avatars/AvatarWithPlaceholder/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/NotificationItemRenderer/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentReply/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/ReactionCreated/index.tsx
- packages/provider/package.json
- packages/components/modules/notifications/native/NotificationsList/index.tsx
- packages/design-system/components/native/avatars/AvatarWithPlaceholder/styles.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/index.tsx
- packages/test/package.json
- packages/graphql/package.json
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentCreated/index.tsx
- packages/components/package.json
- packages/eslint-plugin/package.json
- packages/wagtail/package.json
- packages/config/package.json
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and Lint Packages
- GitHub Check: Component Test Packages
🔇 Additional comments (12)
packages/components/modules/notifications/native/NotificationsList/NotificationItem/index.tsx (1)
1-39: The component structure is clean and well-organized.The component follows good React patterns by:
- Clearly separating data fetching, mutation handling, and rendering
- Using functional components with hooks
- Providing flexibility through customizable renderers
- Following the principle of single responsibility
The implementation aligns well with the PR objective of creating a notification feature that displays notifications and allows users to interact with them.
packages/authentication/package.json (3)
17-22: Dependency versions updated from catalog-based to explicit versions.The transition from catalog-based versioning to explicit version numbers enhances dependency clarity and consistency across the project.
26-26: React peer dependency explicitly versioned.Explicitly defining the peer dependency version creates clearer compatibility requirements.
33-47: Consistent version updates for development dependencies.All testing and type definition packages have been updated to explicit versions, maintaining consistency across the project.
packages/utils/package.json (3)
17-31: Dependencies updated with explicit versions instead of catalog references.The package now uses explicit versions for dependencies like axios, expo packages, js-cookie, and others, which improves maintainability and predictability of builds.
34-34: React peer dependency updated to specific version.Explicitly defining the React version requirement improves compatibility management.
40-57: Dev dependencies updated to explicit versions.All development dependencies now use specific versions instead of catalog references, ensuring consistent development environments.
packages/design-system/package.json (4)
17-18: Added prepare script alongside prepublishOnly.Including both
prepublishOnlyandpreparescripts ensures the package is properly built both during publishing and when installed as a dependency.
59-88: UI component dependencies updated with explicit versions.All UI-related dependencies (MUI, Emotion, React Navigation, etc.) have been updated to use explicit version numbers instead of catalog references, which improves version control.
92-93: React peer dependencies explicitly versioned.Specifying exact versions for React and React DOM peer dependencies helps prevent compatibility issues.
96-130: Development dependencies updated with explicit versions.All development tooling (Storybook, Babel, TypeScript, PostCSS, etc.) now uses explicit versions, ensuring consistent development environments.
packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/index.tsx (1)
11-16:✅ Verification successful
Check if missing prop type validation.
The component accepts props but doesn't have validation for required props. Consider adding prop validation or default values.
🏁 Script executed:
#!/bin/bash # Check if the types file properly validates props # Find and display the types.ts file content fd "types.ts" --exec cat {} \; | rg -A 20 "NotificationHeaderProps"Length of output: 1590
Prop types are already validated via TypeScript
The
NotificationHeadercomponent correctly uses theFC<NotificationHeaderProps>definition, and theNotificationHeaderPropsinterface—located intypes.ts—properly defines all required properties. Therefore, adding additional runtime prop-type validations isn’t necessary. If defaults are required for any of the props, consider adding them directly within the component; otherwise, no changes are needed.
...ficationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/index.tsx
Show resolved
Hide resolved
...ficationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/index.tsx
Show resolved
Hide resolved
Ronan-Fernandes
force-pushed
the
BA-2274-fe-packages-rn-notifications-list
branch
3 times, most recently
from
786d937 to
408c981
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🧹 Nitpick comments (6)
packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/index.tsx (1)
8-12: Consider adding missing props or removing the wrapper View.The component wraps
AvatarWithPlaceholderin aViewwithout any styling or additional props. If the wrapper isn't being used for styling or layout purposes, consider removing it to simplify the component structure.-const NotificationAvatar: FC<NotificationAvatarProps> = ({ actorAvatar }) => ( - <View> - <AvatarWithPlaceholder imgSource={actorAvatar} /> - </View> -) +const NotificationAvatar: FC<NotificationAvatarProps> = ({ actorAvatar }) => ( + <AvatarWithPlaceholder imgSource={actorAvatar} /> +)packages/components/modules/notifications/native/NotificationsList/NotificationItem/index.tsx (2)
31-35: Add accessibility attributes to the Pressable componentThe Pressable element is missing accessibility attributes that are important for screen readers. Consider adding properties like
accessibilityRole,accessibilityLabel, andaccessibilityHint.- <Pressable onPress={markAsRead}> + <Pressable + onPress={markAsRead} + accessibilityRole="button" + accessibilityLabel={`Mark notification as read: ${notification.verb || 'Notification'}`} + accessibilityHint="Marks this notification as read"> <NotificationItemRenderer notification={notification} /> </Pressable>
16-29: Add error handling and loading state to the mutationThe mutation lacks error handling and doesn't indicate loading state to the user. Consider adding these features for a better user experience.
- const [commitMutation] = useNotificationsMarkAsRead() + const [commitMutation, isInFlight] = useNotificationsMarkAsRead() const markAsRead = () => { if (notification.unread) { commitMutation({ variables: { input: { read: true, notificationIds: [notification.id], }, }, + onError: error => { + console.error('Failed to mark notification as read:', error) + // Consider adding a toast or other user feedback + } }) } }You could then use
isInFlightto show a loading indicator or disable the Pressable while the mutation is in progress.packages/components/modules/notifications/native/NotificationsList/index.tsx (3)
53-64: Improve type safety and add keyExtractor for FlatListThe renderNotificationItem function uses
anytype, which reduces type safety. Also, the FlatList component is missing performance optimization properties.- const renderNotificationItem = (notification: any) => { + const renderNotificationItem = (notification: NotificationsListFragment['notifications']['edges'][0]['node']) => { if (!notification) return null // TODO add notifications divider and unread/Read notifications as per design return ( <NotificationItem key={`notification-${notification.id}`} notification={notification} {...NotificationItemProps} /> ) }And update the FlatList implementation:
<View> - <FlatList data={notifications} renderItem={({ item }) => renderNotificationItem(item)} /> + <FlatList + data={notifications} + renderItem={({ item }) => renderNotificationItem(item)} + keyExtractor={item => item?.id || Math.random().toString()} + initialNumToRender={5} + maxToRenderPerBatch={10} + windowSize={5} + /> </View>
42-42: Consider adding error handling for subscriptionThe useNotificationsSubscription doesn't appear to have error handling, which could lead to missed notifications if the subscription fails.
Consider handling subscription errors, for example:
- useNotificationsSubscription(me?.id) + const subscriptionError = useNotificationsSubscription(me?.id) + + React.useEffect(() => { + if (subscriptionError) { + console.error('Notification subscription error:', subscriptionError) + // Consider implementing a retry mechanism or user feedback + } + }, [subscriptionError])This assumes the hook returns an error. If it doesn't, you may need to modify the hook implementation to handle errors appropriately.
76-87: Improve layout structure and add test IDsThe component's layout could benefit from semantic structure and test IDs for automated testing.
return ( - <View> + <View testID="notifications-list-container" style={styles.container}> <View style={styles.headerContainer}> - <Text variant="h6">Notifications</Text> + <Text variant="h6" testID="notifications-header">Notifications</Text> <MarkAllAsReadButton refetch={refetchNotifications} disabled={!data?.notificationsUnreadCount} + testID="mark-all-read-button" /> </View> - {renderContent()} + <View style={styles.contentContainer} testID="notifications-content"> + {renderContent()} + </View> </View> )This change adds proper test IDs and improves the layout structure with additional container styles, which should be defined in your styles file.
🛑 Comments failed to post (2)
packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/index.tsx (1)
1-14: 💡 Verification agent
🧩 Analysis chain
Consider adding missing size prop to AvatarWithPlaceholder.
Based on the notification feature context from the PR objectives, it would be beneficial to specify a size for the avatar to ensure consistent UI. The
AvatarWithPlaceholdercomponent likely accepts a size prop that should be utilized here.
🌐 Web query:
What props are commonly required for Avatar components in React Native?💡 Result:
Avatar components in React Native commonly require props to handle image sources, sizing, fallback content, and styling customization. These props enable flexible user representation through photos, initials, or icons while ensuring consistent UI behavior across platforms.
Core Props
Image Handling
source: Specifies image URI or local resource (AvatarImagein gluestack-ui [2],sourcein React Native Elements [4])defaultSource: Fallback image when primary source fails (@kolking/react-native-avatar [3])Size & Shape
size: Accepts numeric values or predefined sizes (sm,md,lg) [2][4]radius: Controls border rounding (defaults to circular whenrounded=true) [3][4]<Avatar size={100} radius={20} rounded />Fallback Content
name: Generates initials when image unavailable (react-native-user-avatar [10])title: Text display alternative (React Native Elements [4])AvatarFallbackText: Dedicated component in gluestack-ui [2]Styling Props
color: Background color for non-image avatars [3][6]style: Custom container styling [3][4]textStyle: Initials text customization [3]Interactivity
onPress: Touch event handlers [4]badge: Status indicators with configurable position/color (@kolking/react-native-avatar [3])Advanced Features
<AvatarGroup max={4}> {/* Avatar containers */} <Avatar email="user@example.com" {/* Gravatar integration */} colorize {/* Auto-generate background from name */} icon={<UserIcon />} {/* Custom icon support */} /> </AvatarGroup>Libraries like gluestack-ui enforce iOS-specific practices like placing
AvatarFallbackTextbeforeAvatarImagefor accessibility [2], while Material UI-inspired packages offer social media integration through props likefacebookId[5]. Most solutions prioritize prop inheritance from core React Native components (View/Image/Text) for styling consistency [2][4].Citations:
- 1: https://react.dev/learn/passing-props-to-a-component
- 2: https://gluestack.io/ui/docs/components/avatar
- 3: https://www.npmjs.com/package/@kolking/react-native-avatar
- 4: https://reactnativeelements.com/docs/components/avatar
- 5: https://www.dhiwise.com/post/react-avatar-from-installation-to-advanced-customization
- 6: https://mui.com/material-ui/react-avatar/
- 7: https://reactnative.dev/docs/props
- 8: https://reactnativeelements.com/docs/3.4.2/avatar
- 9: https://legacy.reactjs.org/docs/components-and-props.html
- 10: https://www.npmjs.com/package/react-native-user-avatar
Action: Explicitly Define the Avatar Size
Based on common React Native avatar patterns and our notification design objectives, it's important to explicitly set a
sizeprop on theAvatarWithPlaceholdercomponent. This ensures consistent avatar dimensions across the UI. For example, you might update the code as follows:
- File:
packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/index.tsx- Suggestion:
<AvatarWithPlaceholder imgSource={actorAvatar} size={48} />Adjust the numeric value (or use a predefined size key) to align with your design guidelines.
packages/components/modules/notifications/native/NotificationsList/index.tsx (1)
31-41: 🛠️ Refactor suggestion
Add loading and error states for data fetching
The component doesn't handle loading or error states for the query or pagination, which could lead to a poor user experience if data fetching is slow or fails.
const options = { count: 10 } - const { me } = useLazyLoadQuery<NotificationsListQueryType>(NotificationsListQuery, options, { + const { me, error } = useLazyLoadQuery<NotificationsListQueryType>(NotificationsListQuery, options, { fetchPolicy: 'store-and-network', }) + if (error) { + return ( + <View style={styles.errorContainer}> + <Text>Failed to load notifications. Please try again later.</Text> + </View> + ) + } // TODO: handle infinite scroll - const { data, refetch } = usePaginationFragment< + const { data, refetch, isLoading } = usePaginationFragment< NotificationsListQueryType, NotificationsListFragment$key >(NotificationsListFragment, me)Then update the rendering to include loading states:
const renderContent = () => { + if (isLoading && notifications.length === 0) { + return ( + <View style={styles.loadingContainer}> + <ActivityIndicator size="large" color={theme.colors.primary} /> + </View> + ) + } + if (notifications.length === 0) return <EmptyState />Don't forget to import ActivityIndicator from react-native.
📝 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 options = { count: 10 } const { me, error } = useLazyLoadQuery<NotificationsListQueryType>( NotificationsListQuery, options, { fetchPolicy: 'store-and-network', } ) if (error) { return ( <View style={styles.errorContainer}> <Text>Failed to load notifications. Please try again later.</Text> </View> ) } // TODO: handle infinite scroll const { data, refetch, isLoading } = usePaginationFragment< NotificationsListQueryType, NotificationsListFragment$key >(NotificationsListFragment, me) const renderContent = () => { if (isLoading && notifications.length === 0) { return ( <View style={styles.loadingContainer}> <ActivityIndicator size="large" color={theme.colors.primary} /> </View> ) } if (notifications.length === 0) return <EmptyState /> // ... other rendering logic }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/index.tsx (1)
8-26: The component logic looks good, but consider handling potential errors.The implementation for marking all notifications as read is clean and straightforward. You're correctly using the mutation hook, handling loading state, and providing appropriate feedback through the button state.
Consider adding error handling to the mutation call:
commitMutation({ onCompleted: refetch, + onError: (error) => { + // Handle error appropriately, e.g., show a toast notification + console.error('Failed to mark notifications as read:', error) + }, variables: { input: { read: true, }, }, })packages/components/modules/notifications/native/NotificationsList/index.tsx (3)
31-51: Good data fetching implementation, with improvement opportunities for network reliability.The implementation for fetching and refreshing notification data using Relay is well-structured. The pagination fragment setup looks correct, and you've included a TODO for implementing infinite scroll in the future.
Consider enhancing the network handling to provide a better user experience:
- Add error handling for the query and pagination operations:
const { data, refetch, hasNext, loadNext, isLoadingNext } = usePaginationFragment< NotificationsListQueryType, NotificationsListFragment$key >(NotificationsListFragment, me) + // Handle errors from data fetching + const handleRefetchError = (error: Error) => { + console.error('Failed to refresh notifications:', error) + // Display user-friendly error message + } const refetchNotifications = () => { - refetch(options, { fetchPolicy: 'network-only' }) + refetch(options, { fetchPolicy: 'network-only' }) + .catch(handleRefetchError) }
- Implement pull-to-refresh functionality in the FlatList to allow users to manually refresh.
53-64: Type safety can be improved in the renderNotificationItem function.The
anytype for the notification parameter should be replaced with a proper type for better safety:- const renderNotificationItem = (notification: any) => { + const renderNotificationItem = (notification: NotificationsListFragment['me']['notifications']['edges'][0]['node']) => {If the full type is too complex, create a dedicated type for notifications:
type Notification = NonNullable<NotificationsListFragment['me']['notifications']['edges'][0]['node']>; const renderNotificationItem = (notification: Notification | null) => { if (!notification) return null; // ... }
66-74: EmptyState handling looks good, but FlatList implementation can be improved.The conditional rendering for empty state is well-implemented. However, the FlatList implementation can be enhanced.
Improve the FlatList implementation with these changes:
- <View> - <FlatList data={notifications} renderItem={({ item }) => renderNotificationItem(item)} /> - </View> + <FlatList + data={notifications} + renderItem={({ item }) => renderNotificationItem(item)} + keyExtractor={(item) => `notification-${item.id}`} + contentContainerStyle={styles.listContent} + showsVerticalScrollIndicator={false} + onRefresh={refetchNotifications} + refreshing={false} // Replace with a loading state variable + />This improves performance (keyExtractor), user experience (pull-to-refresh), and eliminates the need for the redundant View wrapper.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/components/__generated__/ActivityLogsFragment.graphql.tsis excluded by!**/__generated__/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (37)
packages/components/modules/notifications/native/NotificationBellWithBadge/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationBellWithBadge/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/EmptyState/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/EmptyState/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentCreated/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentReply/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationRoot/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationRoot/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/NotificationItemRenderer/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/NotificationItemRenderer/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/ReactionCreated/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/types.ts(1 hunks)packages/components/modules/notifications/native/index.ts(1 hunks)packages/design-system/components/native/avatars/AvatarWithPlaceholder/index.tsx(1 hunks)packages/design-system/components/native/avatars/AvatarWithPlaceholder/types.ts(1 hunks)packages/design-system/components/native/avatars/index.ts(1 hunks)packages/design-system/components/native/badges/Badge/index.tsx(1 hunks)packages/design-system/components/native/badges/Badge/types.ts(1 hunks)packages/design-system/components/native/badges/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (35)
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/types.ts
- packages/design-system/components/native/badges/Badge/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/NotificationItemRenderer/types.ts
- packages/design-system/components/native/avatars/AvatarWithPlaceholder/types.ts
- packages/components/modules/notifications/native/NotificationsList/styles.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/index.tsx
- packages/design-system/components/native/badges/index.ts
- packages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/styles.ts
- packages/components/modules/notifications/native/index.ts
- packages/components/modules/notifications/native/NotificationsList/EmptyState/styles.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/styles.ts
- packages/components/modules/notifications/native/NotificationsList/types.ts
- packages/components/modules/notifications/native/NotificationBellWithBadge/styles.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationRoot/styles.ts
- packages/design-system/components/native/badges/Badge/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationRoot/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentReply/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/index.tsx
- packages/design-system/components/native/avatars/AvatarWithPlaceholder/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/NotificationItemRenderer/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/ReactionCreated/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/index.tsx
- packages/components/modules/notifications/native/NotificationBellWithBadge/index.tsx
- packages/components/modules/notifications/native/NotificationsList/EmptyState/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/styles.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentCreated/index.tsx
- packages/design-system/components/native/avatars/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Component Test Packages
🔇 Additional comments (6)
packages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/index.tsx (1)
1-7: Well-structured imports and type management.Good job on organizing imports by grouping them logically (React, design system, local components) and adding proper TypeScript type declarations.
packages/components/modules/notifications/native/NotificationsList/index.tsx (5)
42-47: Good use of useMemo for notifications filtering.The use of useMemo to filter and transform the notifications data is a good optimization. This ensures the filtering only happens when the data changes, not on every render.
76-87: Well-structured component layout with proper header spacing.The overall component structure is clean with a clear separation between the header (with title and action button) and content. The disabled state for the "Mark all as read" button when no unread notifications exist is a good UX choice.
23-27: Good use of default props and component composition.Using default components for EmptyState and NotificationItem allows for flexibility and customization while providing sensible defaults. This approach enables reusability of the NotificationsList component across different parts of the application.
36-36: Consider prioritizing the TODO for infinite scroll implementation.The TODO for implementing infinite scroll should be prioritized since it's an important UX feature for notification lists, especially as they grow. The current limit of 10 notifications might not be sufficient for active users.
Consider implementing it with Relay's loadNext functionality:
// Example implementation to add const loadMoreNotifications = () => { if (hasNext && !isLoadingNext) { loadNext(10); } }; // Then in FlatList <FlatList // ...other props onEndReached={loadMoreNotifications} onEndReachedThreshold={0.5} />
56-56:❓ Verification inconclusive
Plan for implementing the unread/read notifications divider.
The TODO comment mentions adding dividers and unread/read notifications sections as per design. Consider tackling this in a follow-up PR if not already planned, as it will improve the user experience by clearly distinguishing between read and unread notifications.
You'll likely need to:
- Group notifications by read status
- Add section headers
- Implement a SectionList instead of FlatList
🏁 Script executed:
#!/bin/bash # Check if the design system has a Divider component we can use rg "Divider" --glob="*.tsx" packages/components/design-system/Length of output: 137
Action: Verify the unread/read notifications divider plan and Divider component usage
Your current TODO indicates a need to group notifications by their read status, add section headers, and switch from using a FlatList to a SectionList for better segmentation. Since the command to search for a Divider component in the expected design system directory returned an error ("No such file or directory"), please verify if an alternative Divider component exists elsewhere in the codebase or if a new Divider should be implemented to adhere to the design requirements.
- Steps to follow:
- Ensure notifications are grouped by read/unread status.
- Add section headers as per design specifications.
- Confirm whether a Divider component exists in the project. If not, plan to create one or utilize a similar UI element.
- Assess the feasibility of replacing FlatList with SectionList for better organization.
Ronan-Fernandes
force-pushed
the
BA-2274-fe-packages-rn-notifications-list
branch
from
408c981 to
1165f09
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/index.tsx (1)
33-35: 🛠️ Refactor suggestionFormat the timestamp for consistency
The timestamp is currently rendered as plain text without formatting, which may lead to inconsistent display across the application.
Consider using a date formatting utility:
- {timestamp} + {formatDate(timestamp, 'relative')}You should import a date formatting utility from your codebase, likely in
packages/utils/functions/date/index.ts.
🧹 Nitpick comments (8)
packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/index.tsx (2)
25-26: Consider more robust handling for missing actor nameWhile the null coalescing operator handles null/undefined cases, you might want to consider a more descriptive fallback than an empty string for better user experience.
- {actorName ?? ''}{' '} + {actorName || 'Unknown user'}{' '}
20-37: Add accessibility attributesThe component lacks accessibility attributes, which could make it difficult for screen reader users to understand the context.
- <View style={styles.headerContainer}> + <View + style={styles.headerContainer} + accessibilityRole="header" + accessibilityLabel={`Notification from ${actorName || 'Unknown'}: ${message}`} + >packages/components/modules/notifications/native/NotificationsList/index.tsx (3)
53-53: Avoid usinganytype for better type safety.The
renderNotificationItemfunction accepts a parameter of typeany. This reduces type safety and could lead to runtime errors.- const renderNotificationItem = (notification: any) => { + const renderNotificationItem = (notification: NotificationsListFragment['notifications']['edges'][0]['node']) => {
70-72: Add key extractor and optimize FlatList.The FlatList component is missing a
keyExtractorprop, which is important for performance. Additionally, the wrapping View might be unnecessary.- <View> - <FlatList data={notifications} renderItem={({ item }) => renderNotificationItem(item)} /> - </View> + <FlatList + data={notifications} + renderItem={({ item }) => renderNotificationItem(item)} + keyExtractor={(item) => `notification-${item.id}`} + />
31-34: Add error handling and loading state for query.The component fetches data using
useLazyLoadQuerybut doesn't handle potential errors or show a loading state while fetching notifications.Consider implementing error handling and a loading state:
const options = { count: 10 } const { me } = useLazyLoadQuery<NotificationsListQueryType>(NotificationsListQuery, options, { fetchPolicy: 'store-and-network', }) + + // Add these states for better UX + const [isLoading, setIsLoading] = useState(true); + const [error, setError] = useState<Error | null>(null); + + // ...and handle them in the renderingpackages/components/modules/notifications/native/NotificationBellWithBadge/types.ts (1)
1-3: Consider adding JSDoc comments for better documentation.The type definition is clear, but adding JSDoc comments would improve developer experience by providing context about when and why the
isActiveproperty should be used.+/** + * Props for the NotificationBellWithBadge component + * @property isActive - Determines whether the bell icon should be displayed in its active state + */ export type NotificationBellWithBadgeProps = { isActive?: boolean }packages/components/modules/notifications/native/NotificationBellWithBadge/index.tsx (2)
38-43: Enhance accessibility for the notification badge.The notification badge should include accessibility attributes to help screen readers announce the number of unread notifications.
<View> - <BellIcon width="24" height="24" isActive={isActive} /> + <BellIcon + width="24" + height="24" + isActive={isActive} + accessibilityLabel={user?.notificationsUnreadCount ? `Notifications, ${user.notificationsUnreadCount} unread` : "Notifications"} + /> {!!user?.notificationsUnreadCount && ( - <Badge style={styles.badge}> + <Badge + style={styles.badge} + accessibilityElementsHidden={true} + importantForAccessibility="no-hide-descendants" + > {user.notificationsUnreadCount >= 100 ? '99+' : user.notificationsUnreadCount} </Badge> )} </View>
39-39: Consider handling the null case more explicitly.The double negation operator
!!works, but using a more explicit check might improve code readability.- {!!user?.notificationsUnreadCount && ( + {(user?.notificationsUnreadCount ?? 0) > 0 && (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/components/__generated__/ActivityLogsFragment.graphql.tsis excluded by!**/__generated__/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (38)
packages/components/modules/notifications/native/NotificationBellWithBadge/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationBellWithBadge/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationBellWithBadge/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/EmptyState/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/EmptyState/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentCreated/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentReply/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationRoot/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationRoot/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/NotificationItemRenderer/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/NotificationItemRenderer/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/ReactionCreated/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/types.ts(1 hunks)packages/components/modules/notifications/native/index.ts(1 hunks)packages/design-system/components/native/avatars/AvatarWithPlaceholder/index.tsx(1 hunks)packages/design-system/components/native/avatars/AvatarWithPlaceholder/types.ts(1 hunks)packages/design-system/components/native/avatars/index.ts(1 hunks)packages/design-system/components/native/badges/Badge/index.tsx(1 hunks)packages/design-system/components/native/badges/Badge/types.ts(1 hunks)packages/design-system/components/native/badges/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (33)
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/styles.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/types.ts
- packages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/types.ts
- packages/components/modules/notifications/native/index.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationRoot/styles.ts
- packages/design-system/components/native/badges/Badge/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/NotificationItemRenderer/types.ts
- packages/design-system/components/native/badges/Badge/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/styles.ts
- packages/components/modules/notifications/native/NotificationsList/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/types.ts
- packages/design-system/components/native/badges/index.ts
- packages/components/modules/notifications/native/NotificationBellWithBadge/styles.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/NotificationItemRenderer/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/styles.ts
- packages/design-system/components/native/avatars/AvatarWithPlaceholder/types.ts
- packages/components/modules/notifications/native/NotificationsList/EmptyState/styles.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationRoot/index.tsx
- packages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentReply/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/types.ts
- packages/components/modules/notifications/native/NotificationsList/EmptyState/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentCreated/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/index.tsx
- packages/design-system/components/native/avatars/index.ts
- packages/design-system/components/native/avatars/AvatarWithPlaceholder/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/ReactionCreated/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Component Test Packages
- GitHub Check: Build and Lint Packages
🔇 Additional comments (8)
packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/index.tsx (4)
1-7: LGTM: Clean import structureThe imports are well-organized, separating React core imports, design system components, and local imports with appropriate spacing.
11-19: Props handling looks goodThe component correctly destructures the expected props and uses the theme hook to create dynamic styles based on the current theme.
31-32: Good conditional rendering of badgeThe badge is correctly conditionally rendered only when the notification is unread.
1-41: Unit Tests for NotificationHeader NeededThis new component would benefit from unit tests to ensure correct rendering behavior in various scenarios.
Please add unit tests to verify:
- Correct rendering of actor name, message, and timestamp
- Conditional rendering of the Badge when unread is true
- Proper handling when actorName is missing
packages/components/modules/notifications/native/NotificationsList/index.tsx (2)
44-47: Good use of useMemo for notification filtering.Filtering and mapping the notification edges using
useMemois an efficient approach that prevents unnecessary re-renders.
76-86: Well-structured layout with header and content sections.The component layout is cleanly organized with a header section containing the title and action button, followed by the content section. The conditional rendering approach for empty states is also well-implemented.
packages/components/modules/notifications/native/NotificationsList/styles.ts (1)
5-20: Clean and well-structured styling approach.The styling implementation follows React Native's best practices by:
- Using a function that accepts the theme as a parameter to create dynamic styles
- Leveraging StyleSheet.create for optimized style objects
- Employing theme-based colors for consistent styling across the app
- Using clear naming conventions for the style objects
packages/components/modules/notifications/native/NotificationBellWithBadge/index.tsx (1)
19-19:✅ Verification successful
Missing styles file reference.
The import references a './styles' file, but I don't see the corresponding styles file for this component in the review. Make sure this file exists with a
badgestyle defined, otherwise this component will fail to render correctly.
🏁 Script executed:
#!/bin/bash # Check if the styles file exists and contains the badge style if [ -f "packages/components/modules/notifications/native/NotificationBellWithBadge/styles.ts" ]; then echo "File exists. Checking for badge style definition:" grep -rn "badge" packages/components/modules/notifications/native/NotificationBellWithBadge/styles.ts else echo "ERROR: styles.ts file not found at expected location!" fiLength of output: 323
Verified Import Reference: The
styles.tsfile exists atpackages/components/modules/notifications/native/NotificationBellWithBadge/and abadgestyle is properly defined. No changes are required.
packages/components/modules/notifications/native/NotificationsList/index.tsx
Show resolved
Hide resolved
packages/components/modules/notifications/native/NotificationBellWithBadge/index.tsx
Show resolved
Hide resolved
...omponents/modules/notifications/common/graphql/subscriptions/useNotificationsSubscription.ts
Outdated
Show resolved
Hide resolved
packages/components/modules/notifications/native/NotificationBellWithBadge/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/notifications/native/NotificationBellWithBadge/styles.ts
Show resolved
Hide resolved
...ages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/index.tsx
Show resolved
Hide resolved
...ages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/index.tsx
Outdated
Show resolved
Hide resolved
...ficationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/styles.ts
Outdated
Show resolved
Hide resolved
packages/components/modules/notifications/native/NotificationBellWithBadge/styles.ts
Outdated
Show resolved
Hide resolved
packages/components/modules/notifications/native/NotificationsList/index.tsx
Outdated
Show resolved
Hide resolved
packages/components/modules/notifications/native/NotificationsList/styles.ts
Show resolved
Hide resolved
anicioalexandre
added
the
needs changes
Ronan-Fernandes
force-pushed
the
BA-2274-fe-packages-rn-notifications-list
branch
from
1165f09 to
a86111f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/index.tsx (1)
33-35: 🛠️ Refactor suggestionFormat the timestamp for consistency.
The timestamp is displayed as plain text without formatting. Use a date formatting utility to ensure consistent display across the application.
React Native date formatting libraries
🧹 Nitpick comments (6)
packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/index.tsx (1)
31-31: Consider adding a unit for the badge size.The badge size is set to 10 without specifying a unit, which might lead to inconsistencies across different device densities.
- <View>{unread && <Badge size={10} style={styles.badge} />}</View> + <View>{unread && <Badge size={10} style={styles.badge} />}</View>While React Native typically assumes pixels, explicitly documenting this in a comment would improve code clarity.
packages/components/modules/notifications/native/NotificationBellWithBadge/index.tsx (2)
29-29: Add error handling for notification subscription.The notification subscription is set up without any error handling. If the subscription fails, the user won't be notified and won't receive real-time updates.
- useNotificationsSubscription(user?.id) + try { + useNotificationsSubscription(user?.id) + } catch (error) { + // Consider logging the error or showing a fallback UI + console.error('Failed to subscribe to notifications:', error) + }
20-39: Consider adding a callback for notification bell interactions.The component doesn't include any onClick/onPress handler, which would be necessary for navigation to the notifications page when the bell is clicked.
- const NotificationBellWithBadge: FC<NotificationBellWithBadgeProps> = ({ isActive = false }) => { + const NotificationBellWithBadge: FC<NotificationBellWithBadgeProps> = ({ + isActive = false, + onPress + }) => { // ... existing code ... return ( - <View> + <View onPress={onPress}> <BellIcon width="24" height="24" isActive={isActive} /> {!!user?.notificationsUnreadCount && ( <Badge>{user.notificationsUnreadCount > 99 ? '99+' : user.notificationsUnreadCount}</Badge> )} </View> ) }This would require updating the types file to include the
onPressprop.packages/components/modules/notifications/native/NotificationsList/index.tsx (3)
33-35: Add error handling to the query.Similar to the NotificationBellWithBadge component, this query doesn't handle potential errors, which could lead to a poor user experience if the query fails.
const { me } = useLazyLoadQuery<NotificationsListQueryType>(NotificationsListQuery, options, { fetchPolicy: 'store-and-network', + networkCacheConfig: { force: true }, })
54-54: Use proper typing instead of 'any'.The
renderNotificationItemfunction usesanyfor its parameter type, which reduces type safety. Consider using a more specific type based on the notification data structure.- const renderNotificationItem = (notification: any) => { + const renderNotificationItem = (notification: NonNullable<ReturnType<typeof notifications.find>>) => {Or create a dedicated type for the notification items.
72-72: Add performance optimizations to FlatList.The FlatList implementation is missing several performance optimizations that could enhance the user experience:
- A
keyExtractorfunction to improve rendering efficiencyonEndReachedhandler for implementing the infinite scrollwindowSizeandinitialNumToRenderfor optimizing rendering<FlatList data={notifications} renderItem={({ item }) => renderNotificationItem(item)} + keyExtractor={(item) => item.id} + onEndReached={() => { + // Implement infinite scroll logic here + // For example: loadNext(10) + }} + onEndReachedThreshold={0.5} + windowSize={5} + initialNumToRender={5} + maxToRenderPerBatch={5} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/components/__generated__/ActivityLogsFragment.graphql.tsis excluded by!**/__generated__/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (38)
packages/components/modules/notifications/native/NotificationBellWithBadge/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationBellWithBadge/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/EmptyState/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/EmptyState/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentCreated/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentReply/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationRoot/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationRoot/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/NotificationItemRenderer/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/NotificationItemRenderer/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/ReactionCreated/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/types.ts(1 hunks)packages/components/modules/notifications/native/index.ts(1 hunks)packages/design-system/components/native/avatars/AvatarWithPlaceholder/index.tsx(1 hunks)packages/design-system/components/native/avatars/AvatarWithPlaceholder/types.ts(1 hunks)packages/design-system/components/native/avatars/index.ts(1 hunks)packages/design-system/components/native/badges/Badge/index.tsx(1 hunks)packages/design-system/components/native/badges/Badge/styles.ts(1 hunks)packages/design-system/components/native/badges/Badge/types.ts(1 hunks)packages/design-system/components/native/badges/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (31)
- packages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/types.ts
- packages/components/modules/notifications/native/NotificationBellWithBadge/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/styles.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/NotificationItemRenderer/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationRoot/styles.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/index.tsx
- packages/components/modules/notifications/native/NotificationsList/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/types.ts
- packages/components/modules/notifications/native/index.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/styles.ts
- packages/design-system/components/native/badges/Badge/types.ts
- packages/design-system/components/native/badges/index.ts
- packages/design-system/components/native/avatars/index.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationRoot/index.tsx
- packages/components/modules/notifications/native/NotificationsList/EmptyState/styles.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/types.ts
- packages/components/modules/notifications/native/NotificationsList/EmptyState/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/NotificationItemRenderer/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/index.tsx
- packages/components/modules/notifications/native/NotificationsList/styles.ts
- packages/design-system/components/native/avatars/AvatarWithPlaceholder/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/styles.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/ReactionCreated/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentReply/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentCreated/index.tsx
- packages/design-system/components/native/avatars/AvatarWithPlaceholder/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and Lint Packages
- GitHub Check: Component Test Packages
🔇 Additional comments (15)
packages/components/modules/notifications/native/NotificationsList/NotificationItem/types.ts (3)
1-7: Imports are well-structured and appropriate for the module.The imports bring in necessary types from React, generated GraphQL fragments, and a local module, providing a clean foundation for the type definitions in this file.
9-12: Good use of TypeScript interface with optional component injection.The
NotificationItemPropsinterface is well-designed with:
- A Relay fragment reference (
$key) for data fetching- An optional renderer component that supports dependency injection
This pattern enables flexibility in how notifications can be displayed while maintaining type safety.
14-16: Clear separation between fragment reference and data consumption.The
GenericItemPropsinterface properly uses the$datatype from the generated GraphQL fragment, following the Relay pattern where parent components use references ($key) and child components consume the actual data. This separation of concerns is a good practice.packages/design-system/components/native/badges/Badge/index.tsx (1)
1-20: LGTM! Clean badge component implementation.This Badge component is well-structured, properly wraps the react-native-paper Badge with theme-aware styling, and correctly passes props and children to the underlying component.
packages/design-system/components/native/badges/Badge/styles.ts (1)
1-15: LGTM! Badge styling is appropriately defined.The badge styles use proper absolute positioning for the overlay effect and correctly utilize theme colors and typography.
packages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/index.tsx (2)
8-19: Consider implementing optimistic updates instead of refetch.While the current implementation works, using an optimistic update instead of refetch would prevent triggering the NotificationsList suspense fallback for a smoother user experience.
As acknowledged in previous review comments, this could be addressed in a future story for both native and web versions.
21-31: LGTM! Button implementation is correct.The button has the proper "small" size as requested in previous reviews and correctly handles loading and disabled states.
packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/index.tsx (1)
1-40: Add unit tests for the NotificationHeader component.To ensure robust functionality, add unit tests covering:
- Correct rendering of actorName, message, and timestamp
- Conditional rendering of the Badge when unread is true
- Handling when actorName is missing
This aligns with the previous review comments and will help prevent regressions.
packages/components/modules/notifications/native/NotificationBellWithBadge/index.tsx (3)
21-25: Add error handling and loading state to the query.The component uses
useLazyLoadQuerywithout handling error states, which could lead to a poor user experience if the query fails. Consider usinguseQueryLoaderandusePreloadedQueryfor better performance and error handling.
34-36: The badge implementation looks good.The conditional rendering of the badge and handling of large notification counts (showing "99+" for counts over 99) is implemented correctly.
41-49: Good implementation of the suspended component.The
SuspendedNotificationBellWithBadgecomponent correctly wraps the main component in a Suspense boundary with an appropriate fallback, which aligns with the past review suggestion.packages/components/modules/notifications/native/NotificationsList/index.tsx (4)
37-41: Infinite Scroll Implementation Pending in NotificationsList ComponentThe notifications list still contains a TODO for infinite scroll without any supporting implementation. Given that the component already uses
usePaginationFragment, consider implementing the infinite scroll behavior.
91-109: Good implementation of the suspended component.The
SuspendedNotificationsListcomponent provides a smooth user experience by displaying a loading screen with the header while data is loading. This is a best practice for Suspense and aligns with the past review suggestion.
45-48: Good use of useMemo for optimizing notification filtering.The component correctly uses
useMemoto optimize the filtering and mapping of notification edges, with appropriate dependencies. This ensures the operation only runs when the notification data changes.
50-52: Refetch functionality implemented correctly.The
refetchNotificationsfunction properly uses the 'network-only' fetch policy to ensure fresh data is retrieved from the server.
packages/components/modules/notifications/native/NotificationsList/index.tsx
Show resolved
Hide resolved
...ages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/index.tsx
Show resolved
Hide resolved
Ronan-Fernandes
force-pushed
the
BA-2274-fe-packages-rn-notifications-list
branch
from
a86111f to
851d395
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/index.tsx (3)
23-30: Consider adding accessibility props for screen readers.The text elements contain important information but lack accessibility props. Screen readers might struggle to properly interpret the relationship between the actor name and message.
<View style={styles.headerTextContainer}> - <Text variant="subtitle2" color="high"> + <Text + variant="subtitle2" + color="high" + accessible={true} + accessibilityRole="text"> {actorName ?? ''}{' '} </Text> - <Text variant="body2" color="high"> + <Text + variant="body2" + color="high" + accessible={true} + accessibilityRole="text"> {message} </Text> </View>
31-31: Consider simplifying the conditional Badge rendering.The current implementation uses an empty View just to wrap a conditional Badge. If the purpose of this View is only to render the Badge conditionally, you could simplify it.
- <View>{unread && <Badge size={10} style={styles.badge} />}</View> + {unread && <Badge size={10} style={styles.badge} />}
9-9: Consider adding inline types for better code readability.While importing types from a separate file is good practice for complex interfaces, consider including a brief comment or inline definition to improve code readability and maintainability directly in this file.
import { NotificationHeaderProps } from './types' +// NotificationHeaderProps: { actorName?: string, message: string, unread?: boolean, timestamp: string }packages/components/modules/notifications/native/NotificationsList/index.tsx (2)
54-55: Add proper type definition instead of using 'any'.Using 'any' type for the notification parameter reduces type safety. Consider replacing it with the appropriate type from your GraphQL schema.
- const renderNotificationItem = (notification: any) => { + const renderNotificationItem = (notification: NotificationsListFragment['me']['notifications']['edges'][0]['node']) => {
70-73: FlatList implementation needs optimization.The current FlatList implementation is basic and could benefit from performance optimizations for handling larger lists of notifications.
<FlatList data={notifications} renderItem={({ item }) => renderNotificationItem(item)} + keyExtractor={(item) => `notification-${item.id}`} + removeClippedSubviews={true} + initialNumToRender={5} + maxToRenderPerBatch={10} + windowSize={7} + showsVerticalScrollIndicator={false} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/components/__generated__/ActivityLogsFragment.graphql.tsis excluded by!**/__generated__/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (38)
packages/components/modules/notifications/native/NotificationBellWithBadge/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationBellWithBadge/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/EmptyState/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/EmptyState/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentCreated/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentReply/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationRoot/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationRoot/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/NotificationItemRenderer/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/NotificationItemRenderer/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/ReactionCreated/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/types.ts(1 hunks)packages/components/modules/notifications/native/index.ts(1 hunks)packages/design-system/components/native/avatars/AvatarWithPlaceholder/index.tsx(1 hunks)packages/design-system/components/native/avatars/AvatarWithPlaceholder/types.ts(1 hunks)packages/design-system/components/native/avatars/index.ts(1 hunks)packages/design-system/components/native/badges/Badge/index.tsx(1 hunks)packages/design-system/components/native/badges/Badge/styles.ts(1 hunks)packages/design-system/components/native/badges/Badge/types.ts(1 hunks)packages/design-system/components/native/badges/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (35)
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/types.ts
- packages/design-system/components/native/badges/index.ts
- packages/components/modules/notifications/native/NotificationBellWithBadge/types.ts
- packages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/types.ts
- packages/components/modules/notifications/native/index.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/styles.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationRoot/styles.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/types.ts
- packages/design-system/components/native/avatars/index.ts
- packages/design-system/components/native/badges/Badge/styles.ts
- packages/design-system/components/native/badges/Badge/index.tsx
- packages/design-system/components/native/badges/Badge/types.ts
- packages/components/modules/notifications/native/NotificationsList/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/styles.ts
- packages/design-system/components/native/avatars/AvatarWithPlaceholder/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/index.tsx
- packages/components/modules/notifications/native/NotificationsList/EmptyState/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/NotificationItemRenderer/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationRoot/index.tsx
- packages/design-system/components/native/avatars/AvatarWithPlaceholder/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/styles.ts
- packages/components/modules/notifications/native/NotificationsList/styles.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentCreated/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/NotificationItemRenderer/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/index.tsx
- packages/components/modules/notifications/native/NotificationBellWithBadge/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentReply/index.tsx
- packages/components/modules/notifications/native/NotificationsList/EmptyState/styles.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/ReactionCreated/index.tsx
- packages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Component Test Packages
- GitHub Check: Build and Lint Packages
🔇 Additional comments (9)
packages/components/modules/notifications/native/NotificationsList/NotificationItem/types.ts (1)
1-16: Well-structured type definitions for notification components.This file provides clean type definitions with properly separated interfaces that will enforce type safety across the notification components. The use of GraphQL fragment types and optional renderer component follows good TypeScript and React patterns.
packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/index.tsx (3)
11-16: Good job defining the props interface and component signature clearly.The component signature is well-defined with appropriate props for a notification header. Props like
actorName,message,unread, andtimestampprovide a clear contract for using this component.
33-35: Format the timestamp for consistency.The timestamp is displayed directly without formatting. Consider using a date formatting utility to ensure consistent display.
1-40: Add unit tests for this component.Please create unit tests for this component to ensure it renders correctly with different prop combinations, especially around the conditional badge rendering and handling missing actorName.
packages/components/modules/notifications/native/NotificationsList/index.tsx (5)
37-41: TODO comment for infinite scroll still needs implementation.This TODO comment for handling infinite scroll has been previously identified in reviews but remains unaddressed. The component uses
usePaginationFragmentwhich is designed for pagination, but there's no implementation ofloadNext,hasNext, and scroll event handling to trigger loading more data.#!/bin/bash # Check if there are other infinite scroll implementations in the codebase that could be referenced rg -i "usePaginationFragment.*loadNext" --type tsx
45-48: LGTM! Good optimization with useMemo.The useMemo hook is appropriately used to prevent unnecessary recalculations when filtering and mapping the notification edges. The dependency array correctly includes only the relevant data that should trigger recalculation.
57-64: TODO for notification dividers still needs implementation.The TODO comment for adding notification dividers and distinguishing between read/unread notifications has been previously identified in reviews but remains unaddressed.
91-109: LGTM! SuspendedNotificationsList implementation.The SuspendedNotificationsList component implementation looks good. It provides a consistent loading state that matches the structure of the main component, ensuring a smooth transition when data loads.
77-88: LGTM! Clean component structure with clear separation of UI sections.The component's return statement has a clean structure with clear separation between the header container and content. The conditional rendering of content based on the notification state is handled appropriately.
Ronan-Fernandes
force-pushed
the
BA-2274-fe-packages-rn-notifications-list
branch
from
851d395 to
9e4e92d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/components/modules/notifications/native/NotificationsList/index.tsx (2)
37-41:⚠️ Potential issueImplement the infinite scroll functionality as noted in the TODO comment.
This TODO comment was already flagged in previous reviews but remains unaddressed. Since notification lists typically benefit from pagination functionality, implementing infinite scroll would improve the user experience, especially as the number of notifications grows.
Consider implementing infinite scroll using the
loadNext,hasNext, andisLoadingNextproperties available from the pagination fragment. Here's a suggested implementation:- // TODO: handle infinite scroll - const { data, refetch } = usePaginationFragment< + const { + data, + refetch, + loadNext, + hasNext, + isLoadingNext + } = usePaginationFragment< NotificationsListQueryType, NotificationsListFragment$key >(NotificationsListFragment, me)Then modify the FlatList to handle loading more items:
<FlatList data={notifications} renderItem={({ item }) => renderNotificationItem(item)} + onEndReached={() => hasNext && loadNext(10)} + onEndReachedThreshold={0.5} + ListFooterComponent={isLoadingNext ? <LoadingScreen /> : null} />
54-65:⚠️ Potential issueType the notification parameter and implement the pending divider feature.
There are two issues in this code segment:
The notification parameter is typed as
any, which reduces type safety. Use a proper type to leverage TypeScript's benefits.The TODO comment about notification dividers was flagged in previous reviews but remains unaddressed.
Apply these changes to improve the code:
- const renderNotificationItem = (notification: any) => { + const renderNotificationItem = (notification: NotificationsListFragment['me']['notifications']['edges'][0]['node']) => { if (!notification) return null - // TODO add notifications divider and unread/Read notifications as per design return ( <NotificationItem key={`notification-${notification.id}`} notification={notification} {...NotificationItemProps} /> ) }Then implement the divider and unread/read notifications functionality based on the design requirements, using something like:
// Inside renderContent function, before returning the FlatList const groupedNotifications = { unread: notifications.filter(n => !n.read), read: notifications.filter(n => n.read) }; // Then in the FlatList <FlatList data={notifications} renderItem={({ item }) => renderNotificationItem(item)} ItemSeparatorComponent={() => <View style={styles.divider} />} ListHeaderComponent={() => groupedNotifications.unread.length > 0 ? ( <View> <Text style={styles.sectionHeader}>New</Text> </View> ) : null} SectionSeparatorComponent={() => groupedNotifications.read.length > 0 && groupedNotifications.unread.length > 0 ? ( <View> <Text style={styles.sectionHeader}>Earlier</Text> </View> ) : null} />
🧹 Nitpick comments (3)
packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/index.tsx (1)
24-26: Consider handling actorName with a dedicated utility function.Instead of using the nullish coalescing operator inline, consider extracting a utility function to handle potentially undefined actor names more consistently across the application.
- <Text variant="subtitle2" color="high"> - {actorName ?? ''}{' '} - </Text> + <Text variant="subtitle2" color="high"> + {actorName ? `${actorName} ` : ''} + </Text>packages/components/modules/notifications/native/NotificationsList/index.tsx (2)
72-72: Add keyExtractor to FlatList for optimal performance.While you're providing a key in the rendered item, it's better to use the keyExtractor prop directly on the FlatList for better performance.
<FlatList data={notifications} renderItem={({ item }) => renderNotificationItem(item)} + keyExtractor={(item) => `notification-${item.id}`} />
32-36: Consider making the notification count configurable.While 10 notifications is the requirement mentioned in the PR description, making this value configurable through props would improve component reusability.
+ const NotificationsList: FC<NotificationsListProps> = ({ EmptyState = DefaultEmptyState, NotificationItem = DefaultNotificationItem, NotificationItemProps = {}, + count = 10, }) => { const theme = useTheme() const styles = createStyles(theme) - const options = { count: 10 } + const options = { count } const { me } = useLazyLoadQuery<NotificationsListQueryType>(NotificationsListQuery, options, { fetchPolicy: 'store-and-network', })Don't forget to update the types.ts file to include this new prop.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/components/__generated__/ActivityLogsFragment.graphql.tsis excluded by!**/__generated__/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (38)
packages/components/modules/notifications/native/NotificationBellWithBadge/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationBellWithBadge/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/EmptyState/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/EmptyState/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentCreated/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentReply/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationRoot/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationRoot/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/NotificationItemRenderer/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/NotificationItemRenderer/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/ReactionCreated/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/types.ts(1 hunks)packages/components/modules/notifications/native/index.ts(1 hunks)packages/design-system/components/native/avatars/AvatarWithPlaceholder/index.tsx(1 hunks)packages/design-system/components/native/avatars/AvatarWithPlaceholder/types.ts(1 hunks)packages/design-system/components/native/avatars/index.ts(1 hunks)packages/design-system/components/native/badges/Badge/index.tsx(1 hunks)packages/design-system/components/native/badges/Badge/styles.ts(1 hunks)packages/design-system/components/native/badges/Badge/types.ts(1 hunks)packages/design-system/components/native/badges/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (35)
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/NotificationItemRenderer/types.ts
- packages/components/modules/notifications/native/NotificationBellWithBadge/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/styles.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationRoot/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/styles.ts
- packages/components/modules/notifications/native/index.ts
- packages/design-system/components/native/badges/Badge/index.tsx
- packages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationRoot/styles.ts
- packages/design-system/components/native/badges/Badge/styles.ts
- packages/design-system/components/native/avatars/index.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/types.ts
- packages/design-system/components/native/badges/Badge/types.ts
- packages/design-system/components/native/badges/index.ts
- packages/components/modules/notifications/native/NotificationsList/styles.ts
- packages/design-system/components/native/avatars/AvatarWithPlaceholder/types.ts
- packages/components/modules/notifications/native/NotificationsList/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentCreated/index.tsx
- packages/design-system/components/native/avatars/AvatarWithPlaceholder/index.tsx
- packages/components/modules/notifications/native/NotificationsList/EmptyState/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/NotificationItemRenderer/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/types.ts
- packages/components/modules/notifications/native/NotificationsList/EmptyState/styles.ts
- packages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/index.tsx
- packages/components/modules/notifications/native/NotificationBellWithBadge/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentReply/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/ReactionCreated/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/styles.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Component Test Packages
- GitHub Check: Build and Lint Packages
🔇 Additional comments (7)
packages/components/modules/notifications/native/NotificationsList/NotificationItem/types.ts (1)
1-16: Well-structured type definitions for notification components.The interfaces are well-designed with clear separation of concerns:
NotificationItemPropsappropriately uses Relay's fragment reference pattern with$keysuffix for data fetchingGenericItemPropscorrectly uses the$datatype for components that render already fetched data- The optional renderer property allows for component composition and customization
This approach provides good type safety and maintainability for the notification system while following Relay best practices.
packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/index.tsx (5)
1-9: Clean organization of imports.The imports are well-organized, with React core imports first, followed by design system components, and then local imports. This follows good import organization practices.
11-19: Props destructuring and theme setup looks good.The component properly defines its props interface and properly destructures the incoming props. The theme hook is correctly used to apply theming to the component.
20-32: Well-structured UI layout with proper component nesting.The UI structure is well-organized with appropriate nesting of View and Text components. The conditional rendering of the Badge component is implemented correctly.
33-35: Action required: Format the timestamp for consistency.The timestamp is currently rendered as plain text without formatting.
Consider using a date formatting utility to ensure consistent display across the application. There should be existing date formatting utilities in the codebase (e.g., in
packages/utils/functions/date/index.ts) that you can leverage.
1-41: Unit Tests for NotificationHeader Needed.It appears that there are no existing tests validating the behavior of the NotificationHeader component.
To ensure robust functionality, please add unit tests that cover:
- Correct rendering of the
actorName,message, andtimestamp.- Conditional rendering of the
Badgeonly when theunreadprop is truthy.- Graceful handling when
actorNameis missing.This will help prevent regressions and ensure the component behaves as expected under different prop combinations.
packages/components/modules/notifications/native/NotificationsList/index.tsx (1)
91-109: Great implementation of the suspended component!The implementation of the suspended component with a similar header in the loading state creates a smoother transition for users. This matches the suggestion from the previous review and provides an excellent user experience.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/design-system/CHANGELOG.md (1)
3-8: Changelog Entry for Version 1.0.9: New Native Components AddedThe new entry documents the addition of the
BadgeandAvatarWithPlaceholdercomponents for native. This aligns with the notifications feature requirements (e.g. theNotificationBellWithBadgeand avatar rendering in notifications) and the overall expansion of the design system.
• Please ensure that related documentation (and migration guides if applicable) reflects this update.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/components/CHANGELOG.md(1 hunks)packages/components/package.json(1 hunks)packages/design-system/CHANGELOG.md(1 hunks)packages/design-system/package.json(1 hunks)packages/wagtail/CHANGELOG.md(1 hunks)packages/wagtail/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- packages/wagtail/CHANGELOG.md
- packages/design-system/package.json
- packages/components/package.json
- packages/wagtail/package.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Component Test Packages
🔇 Additional comments (1)
packages/components/CHANGELOG.md (1)
3-10: Ensure New Version Entry Is Clear and Consistent.
The new changelog entry for version 1.0.21 clearly documents the addition of theNotificationsListandNotificationBellWithBadgecomponents along with the dependency upgrade to@baseapp-frontend/design-system@1.0.9. This update aligns with the PR objectives regarding the implementation of the notifications feature. Please verify that the entry follows your team's standard formatting and that similar details in previous versions (e.g., bullet styling, indentation) remain consistent.
Ronan-Fernandes
force-pushed
the
BA-2274-fe-packages-rn-notifications-list
branch
from
5425e7d to
fb7e463
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
packages/components/modules/notifications/native/NotificationsList/NotificationItem/types.ts (1)
1-17: Well-structured type definitions for notification components.The interfaces are cleanly defined with appropriate types for a notification system. The code follows good TypeScript practices by:
- Using explicit GraphQL fragment types for type safety
- Making the renderer component optional in
NotificationItemPropsto support customization- Separating concerns between props for the container component (
NotificationItemProps) and renderer component (GenericItemProps)Consider adding JSDoc comments to document the purpose of each interface and their properties, which would improve developer experience when using these types.
import { FC } from 'react' import { NotificationItemFragment$data, NotificationItemFragment$key, } from '../../../../../__generated__/NotificationItemFragment.graphql' import { NotificationItemRendererProps } from './NotificationItemRenderer/types' +/** + * Props for the NotificationItem component that handles notification display and interaction + */ export interface NotificationItemProps { + /** Reference to the notification data that will be resolved using Relay */ notification: NotificationItemFragment$key + /** Optional custom renderer component for the notification content */ NotificationItemRenderer?: FC<NotificationItemRendererProps> } +/** + * Props for components that render the actual notification content after data is resolved + */ export interface GenericItemProps { + /** Resolved notification data containing all necessary fields for display */ notification: NotificationItemFragment$data }packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/index.tsx (2)
20-32: Consider adding accessibility attributes.The notification header contains important information, but lacks accessibility attributes. Consider adding appropriate accessibility props (like
accessibilityLabelandaccessibilityRole) to improve the experience for users with assistive technologies.- <View style={styles.headerContainer}> + <View + style={styles.headerContainer} + accessibilityRole="header" + accessibilityLabel={`Notification from ${actorName}: ${message}`} + >
11-38: Add default values for props.The component only provides a fallback for
actorNamebut not for other props. Consider adding default values for all props to make the component more robust against undefined or null values.const NotificationHeader: FC<NotificationHeaderProps> = ({ actorName, message, unread, timestamp, }) => {Could be improved to:
const NotificationHeader: FC<NotificationHeaderProps> = ({ actorName = '', message = '', unread = false, timestamp = '', }) => {packages/components/modules/notifications/native/NotificationBellWithBadge/index.tsx (2)
33-36: Consider adding accessibility attributes.
For better screen-reader compatibility, consider addingaccessibleoraccessibilityLabelto theBellIconandBadgecomponents.
41-49: Good Suspense fallback approach.
The fallback with just theBellIconis reasonable. You might also consider a small loading indicator for clarity if fetching takes noticeable time.packages/components/modules/notifications/native/NotificationsList/index.tsx (2)
54-54: Use a stronger type instead ofany.
Replacingnotification: anywith a typed interface or type alias will improve reliability and IntelliSense.- const renderNotificationItem = (notification: any) => { + interface NotificationNode { + id: string + // define additional fields as needed + } + + const renderNotificationItem = (notification: NotificationNode) => {
71-73: Specify a keyExtractor for better FlatList performance.
By default, FlatList uses indices as keys. Consider adding akeyExtractorprop for stable, unique indices.<FlatList data={notifications} renderItem={({ item }) => renderNotificationItem(item)} + keyExtractor={(item) => item?.id ?? String(Math.random())} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/components/__generated__/ActivityLogsFragment.graphql.tsis excluded by!**/__generated__/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (44)
packages/components/CHANGELOG.md(1 hunks)packages/components/modules/notifications/native/NotificationBellWithBadge/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationBellWithBadge/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/EmptyState/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/EmptyState/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentCreated/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentReply/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationRoot/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationRoot/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/NotificationItemRenderer/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/NotificationItemRenderer/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/ReactionCreated/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/NotificationItem/types.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/index.tsx(1 hunks)packages/components/modules/notifications/native/NotificationsList/styles.ts(1 hunks)packages/components/modules/notifications/native/NotificationsList/types.ts(1 hunks)packages/components/modules/notifications/native/index.ts(1 hunks)packages/components/package.json(1 hunks)packages/design-system/CHANGELOG.md(1 hunks)packages/design-system/components/native/avatars/AvatarWithPlaceholder/index.tsx(1 hunks)packages/design-system/components/native/avatars/AvatarWithPlaceholder/types.ts(1 hunks)packages/design-system/components/native/avatars/index.ts(1 hunks)packages/design-system/components/native/badges/Badge/index.tsx(1 hunks)packages/design-system/components/native/badges/Badge/styles.ts(1 hunks)packages/design-system/components/native/badges/Badge/types.ts(1 hunks)packages/design-system/components/native/badges/index.ts(1 hunks)packages/design-system/package.json(1 hunks)packages/wagtail/CHANGELOG.md(1 hunks)packages/wagtail/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (40)
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/NotificationItemRenderer/types.ts
- packages/components/package.json
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/styles.ts
- packages/design-system/components/native/badges/index.ts
- packages/design-system/components/native/avatars/AvatarWithPlaceholder/types.ts
- packages/wagtail/CHANGELOG.md
- packages/components/CHANGELOG.md
- packages/design-system/components/native/badges/Badge/types.ts
- packages/components/modules/notifications/native/NotificationsList/styles.ts
- packages/design-system/CHANGELOG.md
- packages/design-system/components/native/badges/Badge/index.tsx
- packages/design-system/package.json
- packages/components/modules/notifications/native/index.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationRoot/styles.ts
- packages/components/modules/notifications/native/NotificationsList/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/types.ts
- packages/components/modules/notifications/native/NotificationBellWithBadge/types.ts
- packages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/types.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/types.ts
- packages/design-system/components/native/avatars/AvatarWithPlaceholder/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/styles.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationAvatar/index.tsx
- packages/design-system/components/native/avatars/index.ts
- packages/components/modules/notifications/native/NotificationsList/EmptyState/styles.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/index.tsx
- packages/wagtail/package.json
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationRoot/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/NotificationItemRenderer/index.tsx
- packages/components/modules/notifications/native/NotificationsList/MarkAllAsReadButton/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentCreated/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationBody/styles.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/ReactionCreated/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/index.tsx
- packages/design-system/components/native/badges/Badge/styles.ts
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/index.tsx
- packages/components/modules/notifications/native/NotificationsList/EmptyState/index.tsx
- packages/components/modules/notifications/native/NotificationsList/NotificationItem/CommentReply/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: Component Test Packages
- GitHub Check: Build and Lint Packages
🔇 Additional comments (11)
packages/components/modules/notifications/native/NotificationsList/NotificationItem/Notification/NotificationContent/NotificationHeader/index.tsx (5)
1-9: Component imports look appropriate and well-structured.The imports are properly organized with React imports first, followed by design system components, and then local imports. This follows good practices for import organization.
11-19: Component props and theme setup look good.The NotificationHeader component is properly typed with FC and destructures its props clearly. Using useTheme for styling is a good practice for maintaining consistent theming across the application.
31-32: Badge rendering looks good.The conditional rendering of the Badge component when the notification is unread is implemented correctly.
33-35: Format the timestamp for consistency.The timestamp is currently rendered as plain text without formatting, which was previously flagged in code reviews.
#!/bin/bash # Look for date formatting utilities in the codebase rg -l "formatDate|dateFormat" --type ts
1-41: Unit Tests for NotificationHeader Needed.It appears that there are no existing tests validating the behavior of the NotificationHeader component, which was previously flagged in code reviews.
#!/bin/bash # Look for test files related to the NotificationHeader component fd --type file --extension ts --regex "NotificationHeader.*(test|spec)"packages/components/modules/notifications/native/NotificationBellWithBadge/index.tsx (2)
1-2: Looks good for client-side rendering directive.
No issues found.
21-25: Add error handling and loading state to the query.
This repeats a prior remark about the absence of error handling.packages/components/modules/notifications/native/NotificationsList/index.tsx (4)
1-7: Imports and setup look solid.
No issues found with the initial imports and theme usage.
37-37: Handle infinite scroll or remove TODO comment.
Please address or remove the TODO regarding infinite scroll to maintain code clarity.
57-57: Implement or remove this TODO for notification dividers.
Please add the dividers and distinguish read/unread states per design, or remove the TODO for clarity.
91-109: Well-structured Suspense fallback.
No issues found. This approach cleanly handles loading states for the notifications list.
anicioalexandre
changed the title
Acceptance Criteria
Given I am logged in the platform, I should see the notification bell icon in the bottom navigation bar.
Given I am on any page with a visible notification icon on the nav bar, when I click the notification icon, then a notification page should open, displaying my latest notifications.
Given the following actions have occurred then I should see a notification item in my notification list:
When the page opens, then notifications should be displayed in reverse chronological order (most recent first).
The notifications should have the following information:
Given I have no notifications, when I open the notification overlay, then I should see a message stating "You don't have notifications".
Guardrails
Implementation Rules
Design Link: https://www.figma.com/design/z5ZdVlKEMmGIX4GVnjwfxA/BaseApp---NATIVE?node-id=6801-63921&t=KZA8vyoPHbFhbhK4-0
Summary by CodeRabbit