implement notifications UI #202
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several changes to the notification system within the application. The Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (8)
web/src/components/notifications/item.ts (1)
3-12: LGTM: Well-structuredNotificationItemtype. Consider adding JSDoc comments.The
NotificationItemtype is well-defined and covers all necessary properties for a comprehensive notification system. The use of appropriate types for each property enhances type safety and clarity.Suggestions for improvement:
- Consider adding JSDoc comments to describe each property, especially for
sourceanditem, to provide more context on their usage.- You might want to consider using a more specific type for
url, such as a customURLtype or a string literal type with URL validation, to ensure valid URLs are used.Would you like me to provide an example of how to add JSDoc comments to this type definition?
web/src/components/site/Navigation/components/Toolbar.tsx (1)
27-27: LGTM: NotificationsMenu component addedThe
NotificationsMenucomponent is correctly placed within the authenticated user's view, which aligns with the PR objective of implementing a notifications UI.A couple of suggestions:
- Verify that the
status="unread"prop is being used correctly in theNotificationsMenucomponent.- Consider adding an
aria-labelto the component for improved accessibility.Would you like me to provide an example of how to add the
aria-label?web/src/components/site/Navigation/Actions/Notifications.tsx (3)
8-11: Constants are well-defined and exported correctly.The use of constants for notification-related values improves maintainability and readability. Great job!
Consider using an object to group these related constants:
export const Notifications = { ID: "notifications", Route: "/notifications", Label: "Notifications", Icon: <BellIcon />, };This approach can make it easier to import and use these values together in other parts of the application.
13-38: Function components are well-implemented.Both
NotificationActionandNotificationsMenuItemare correctly implemented and follow React best practices. The use of TypeScript for prop typing inNotificationActionis commendable.For
NotificationAction, consider using the optional chaining operator for a more concise conditional rendering:export function NotificationAction({ hideLabel, ...props }: AnchorProps & LinkButtonStyleProps) { return ( <IconButton {...props}> {NotificationsIcon} {!hideLabel && <> <span>{NotificationsLabel}</span></>} </IconButton> ); }This change eliminates the need for nested JSX elements and improves readability.
1-38: Overall, excellent implementation of notification components!This file demonstrates good use of React and TypeScript practices. The code is well-structured, readable, and maintainable. The use of constants for shared values and the separation of concerns between the
NotificationActionandNotificationsMenuItemcomponents are particularly commendable.As the notification system grows, consider creating a dedicated
notificationsfolder within thecomponentsdirectory to house all notification-related components and utilities. This will help maintain a clean and scalable architecture as the feature expands.web/src/components/notifications/NotificationCardList.tsx (1)
28-28: Capitalize the message when there are no notifications.For better user experience and consistency, consider capitalizing the message to "No notifications."
Apply this diff:
- <styled.p color="fg.muted">no notifications.</styled.p> + <styled.p color="fg.muted">No notifications.</styled.p>web/src/components/notifications/useNotifications.ts (2)
76-77: SimplifyfilterStatusFunctionThe
filterStatusfunction can be simplified for better readability by directly returning the filtered function.Apply this diff to simplify the function:
const filterStatus = (s: NotificationStatus) => - filter<Notification>((n) => n.status === s); + (notifications: Notification[]) => notifications.filter((n) => n.status === s);And update its usage accordingly.
Also applies to: 79-79
23-32: Remove Redundant SWR OptionsThe
revalidateIfStaleandrevalidateOnReconnectoptions are set to their default values (true). Explicitly setting default values is unnecessary and can be omitted for cleaner code.Apply this diff to remove redundant options:
swr: { fallbackData: props.initialData, - revalidateIfStale: true, - revalidateOnReconnect: true, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- web/src/app/(dashboard)/notifications/page.tsx (1 hunks)
- web/src/components/notifications/NotificationCardList.tsx (1 hunks)
- web/src/components/notifications/NotificationsMenu.tsx (1 hunks)
- web/src/components/notifications/item.ts (1 hunks)
- web/src/components/notifications/useNotifications.ts (1 hunks)
- web/src/components/site/Navigation/Actions/Notifications.tsx (1 hunks)
- web/src/components/site/Navigation/components/Toolbar.tsx (2 hunks)
- web/src/screens/notifications/NotificationScreen.tsx (1 hunks)
🧰 Additional context used
🪛 Biome
web/src/components/notifications/NotificationsMenu.tsx
[error] 76-76: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (10)
web/src/components/notifications/item.ts (2)
1-1: LGTM: Import statement is correct and necessary.The import statement correctly brings in the required types
DatagraphItemandProfileReferencefrom the OpenAPI schema. These types are used in theNotificationItemtype definition, ensuring type consistency across the application.
1-12: Overall, excellent implementation of theNotificationItemtype.This file successfully introduces a well-structured and comprehensive
NotificationItemtype, which will enhance type safety and clarity in the notification system. The type definition covers all necessary properties and integrates well with existing types from the OpenAPI schema.The code is clean, concise, and follows good TypeScript practices. With the suggested minor improvements in documentation, this implementation will provide a solid foundation for handling notifications throughout the application.
web/src/components/site/Navigation/components/Toolbar.tsx (2)
10-10: LGTM: New import for NotificationsMenuThe import statement for the
NotificationsMenucomponent is correctly added and follows the project's import conventions.
Line range hint
1-38: Summary: Notifications feature successfully integratedThe changes to the
Toolbarcomponent effectively introduce the notifications feature to the UI. TheNotificationsMenuis appropriately placed within the authenticated user's view, and the implementation is consistent with the PR objectives. The existing structure and logic of the component have been preserved, ensuring that the new feature integrates smoothly with the current functionality.web/src/components/site/Navigation/Actions/Notifications.tsx (1)
1-6: Imports look good!The necessary icons, UI components, and types are imported correctly. There are no unused imports, which is great for maintaining a clean codebase.
web/src/app/(dashboard)/notifications/page.tsx (2)
5-5: LGTM!Converting the
Pagefunction to anasyncfunction is appropriate for handling asynchronous data fetching.
7-7: LGTM!Data fetching with
await notificationList()is correctly implemented and appropriately wrapped in atry-catchblock.web/src/components/notifications/useNotifications.ts (2)
45-46: Confirm Error Handling BehaviorEnsure that the
handlefunction properly handles errors thrown within the asynchronous operation. IfnotificationUpdatefails, the error should be caught and managed to provide feedback to the user.Please confirm that the
handlefunction includes error handling logic, or consider wrapping the operation in atry-catchblock:handle(async () => { + try { await notificationUpdate(id, { status }); // Rest of the code... + } catch (error) { + // Handle the error appropriately + } });
84-85: Validatecreated_atFormatWhen creating a new
Dateobject fromn.created_at, ensure thatn.created_atis a valid date string. An invalid date could lead to unexpected behavior.Run the following script to verify that all
created_atfields are valid ISO date strings:web/src/components/notifications/NotificationsMenu.tsx (1)
121-127: Well-implementedNotificationAvatarcomponentThe
NotificationAvatarcomponent correctly checks for the existence of a notification source and renders the appropriate avatar or fallback icon. This ensures a consistent and intuitive user experience.
| } catch (e) { | ||
| return <Unready error={e} />; |
There was a problem hiding this comment.
Ensure error handling doesn't expose sensitive information
Passing the error object directly to the <Unready> component might expose sensitive information to the user. It's recommended to log the error internally and display a user-friendly message instead.
Apply this diff to prevent sensitive error information from being exposed:
- return <Unready error={e} />;
+ console.error(e); // Log the error for debugging purposes
+ return <Unready error="An unexpected error occurred." />;📝 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.
| } catch (e) { | |
| return <Unready error={e} />; | |
| } catch (e) { | |
| console.error(e); // Log the error for debugging purposes | |
| return <Unready error="An unexpected error occurred." />; |
| const showingArchived = status === NotificationStatus.read; | ||
|
|
||
| export function NotificationScreen() { | ||
| return ( | ||
| <Box p="4"> | ||
| <Box> | ||
| <Mailbox width="4em" height="auto" /> | ||
| </Box> | ||
|
|
||
| <Box> | ||
| <Heading size="sm">Notifications</Heading> | ||
| <p>You have no notifications.</p> | ||
| </Box> | ||
| </Box> | ||
| <LStack> | ||
| <HStack w="full" justify="space-between"> | ||
| <LStack> | ||
| <styled.h1 fontWeight="bold">Notifications</styled.h1> | ||
|
|
||
| <Switch | ||
| size="sm" | ||
| checked={showingArchived} | ||
| onClick={handlers.handleToggleStatus} | ||
| > | ||
| Archived | ||
| </Switch> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align terminology between "Read" and "Archived" notifications
In the code, status === NotificationStatus.read is used to determine if archived notifications are being displayed (showingArchived), while the UI label reads "Archived". This may cause confusion if "Read" and "Archived" are not strictly equivalent. To enhance clarity, consider standardizing the terminology used in both the code and the UI.
| <Switch | ||
| size="sm" | ||
| checked={showingArchived} | ||
| onClick={handlers.handleToggleStatus} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use onChange instead of onClick for the Switch component
The Switch component currently uses the onClick event handler. For components like switches or checkboxes, the onChange event is more appropriate and ensures better accessibility and consistency across the application.
Apply this change:
<Switch
size="sm"
checked={showingArchived}
- onClick={handlers.handleToggleStatus}
+ onChange={handlers.handleToggleStatus}
>
Archived
</Switch>📝 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.
| <Switch | |
| size="sm" | |
| checked={showingArchived} | |
| onClick={handlers.handleToggleStatus} | |
| > | |
| <Switch | |
| size="sm" | |
| checked={showingArchived} | |
| onChange={handlers.handleToggleStatus} | |
| > | |
| Archived | |
| </Switch> |
| notification: NotificationItem; | ||
| onMove: (id: string, status: NotificationStatus) => void; |
There was a problem hiding this comment.
Inconsistent return type of onMove in StatusControl props.
The onMove function is typed to return void in StatusControl, but in NotificationCardList, it returns a Promise<void>. This inconsistency might lead to type errors or unexpected behavior.
Apply this diff to correct the return type:
onMove: (id: string, status: NotificationStatus) => void;Change to:
- onMove: (id: string, status: NotificationStatus) => void;
+ onMove: (id: string, status: NotificationStatus) => Promise<void>;📝 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.
| notification: NotificationItem; | |
| onMove: (id: string, status: NotificationStatus) => void; | |
| notification: NotificationItem; | |
| onMove: (id: string, status: NotificationStatus) => Promise<void>; |
| return notification.isRead ? ( | ||
| <Button | ||
| variant="ghost" | ||
| size="sm" | ||
| title="Mark as unread" | ||
| onClick={handleChangeStatus} | ||
| > | ||
| <InboxArrowDownIcon /> | ||
| </Button> | ||
| ) : ( | ||
| <Button | ||
| variant="ghost" | ||
| size="sm" | ||
| title="Mark as read" | ||
| onClick={handleChangeStatus} | ||
| > | ||
| <ArchiveBoxIcon /> | ||
| </Button> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor StatusControl component to reduce code duplication.
The conditional rendering of <Button> components in StatusControl duplicates code with minor differences. Refactoring can improve readability and maintainability.
Apply this diff to simplify the return statement:
}
- return notification.isRead ? (
- <Button
- variant="ghost"
- size="sm"
- title="Mark as unread"
- onClick={handleChangeStatus}
- >
- <InboxArrowDownIcon />
- </Button>
- ) : (
- <Button
- variant="ghost"
- size="sm"
- title="Mark as read"
- onClick={handleChangeStatus}
- >
- <ArchiveBoxIcon />
- </Button>
- );
+ const isRead = notification.isRead;
+ const buttonTitle = isRead ? "Mark as unread" : "Mark as read";
+ const IconComponent = isRead ? InboxArrowDownIcon : ArchiveBoxIcon;
+
+ return (
+ <Button
+ variant="ghost"
+ size="sm"
+ title={buttonTitle}
+ onClick={handleChangeStatus}
+ >
+ <IconComponent />
+ </Button>
+ );
}📝 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.isRead ? ( | |
| <Button | |
| variant="ghost" | |
| size="sm" | |
| title="Mark as unread" | |
| onClick={handleChangeStatus} | |
| > | |
| <InboxArrowDownIcon /> | |
| </Button> | |
| ) : ( | |
| <Button | |
| variant="ghost" | |
| size="sm" | |
| title="Mark as read" | |
| onClick={handleChangeStatus} | |
| > | |
| <ArchiveBoxIcon /> | |
| </Button> | |
| ); | |
| const isRead = notification.isRead; | |
| const buttonTitle = isRead ? "Mark as unread" : "Mark as read"; | |
| const IconComponent = isRead ? InboxArrowDownIcon : ArchiveBoxIcon; | |
| return ( | |
| <Button | |
| variant="ghost" | |
| size="sm" | |
| title={buttonTitle} | |
| onClick={handleChangeStatus} | |
| > | |
| <IconComponent /> | |
| </Button> | |
| ); |
| import { filter, flow, map } from "lodash/fp"; | ||
|
|
||
| import { handle } from "@/api/client"; | ||
| import { notificationUpdate } from "@/api/openapi-client/categories"; |
There was a problem hiding this comment.
Import from Incorrect Module
The notificationUpdate function is imported from "@/api/openapi-client/categories", which may not be the correct module. Since it's related to notifications, consider importing it from "@/api/openapi-client/notifications" to maintain consistency and clarity.
Apply this diff to fix the import statement:
-import { notificationUpdate } from "@/api/openapi-client/categories";
+import { notificationUpdate } from "@/api/openapi-client/notifications";📝 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 { notificationUpdate } from "@/api/openapi-client/categories"; | |
| import { notificationUpdate } from "@/api/openapi-client/notifications"; |
| async function handleMarkAs(id: string, status: NotificationStatus) { | ||
| handle(async () => { | ||
| await notificationUpdate(id, { status }); | ||
|
|
||
| if (data) { | ||
| const newList = { | ||
| ...data, | ||
| notifications: data.notifications.map((n) => { | ||
| if (n.id === id) { | ||
| return { ...n, status }; | ||
| } | ||
| return n; | ||
| }), | ||
| }; | ||
|
|
||
| await mutate(newList); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Ensure Fresh Data When Mutating State
In the handleMarkAs function, using the data object from the closure may lead to stale data if the cache has been updated elsewhere. This could cause inconsistencies when updating the notification list.
Use the functional form of mutate to ensure you're working with the latest data from the cache:
async function handleMarkAs(id: string, status: NotificationStatus) {
handle(async () => {
await notificationUpdate(id, { status });
- if (data) {
- const newList = {
- ...data,
- notifications: data.notifications.map((n) => {
+ await mutate((currentData) => {
+ if (!currentData) return currentData;
+ return {
+ ...currentData,
+ notifications: currentData.notifications.map((n) => {
if (n.id === id) {
return { ...n, status };
}
return n;
}),
- };
-
- await mutate(newList);
- }
+ };
+ }, false);
});
}This approach ensures that you're updating the most recent data.
📝 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.
| async function handleMarkAs(id: string, status: NotificationStatus) { | |
| handle(async () => { | |
| await notificationUpdate(id, { status }); | |
| if (data) { | |
| const newList = { | |
| ...data, | |
| notifications: data.notifications.map((n) => { | |
| if (n.id === id) { | |
| return { ...n, status }; | |
| } | |
| return n; | |
| }), | |
| }; | |
| await mutate(newList); | |
| } | |
| }); | |
| } | |
| async function handleMarkAs(id: string, status: NotificationStatus) { | |
| handle(async () => { | |
| await notificationUpdate(id, { status }); | |
| await mutate((currentData) => { | |
| if (!currentData) return currentData; | |
| return { | |
| ...currentData, | |
| notifications: currentData.notifications.map((n) => { | |
| if (n.id === id) { | |
| return { ...n, status }; | |
| } | |
| return n; | |
| }), | |
| }; | |
| }, false); | |
| }); | |
| } |
| function mapToItem(n: Notification): NotificationItem { | ||
| const content = getNotificationContent(n); | ||
| const createdAt = new Date(n.created_at); | ||
| const title = n.source?.handle ?? "System"; | ||
| const isRead = n.status === "read"; | ||
|
|
||
| return { | ||
| id: n.id, | ||
| createdAt, | ||
| title, | ||
| description: content.description, | ||
| url: content.url, | ||
| isRead, | ||
| source: n.source, | ||
| item: n.item, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Handle Undefined Content in mapToItem
The getNotificationContent(n) function might return undefined if the n.event does not match any of the specified cases. In mapToItem, accessing content.description and content.url will cause an error if content is undefined.
To prevent potential runtime errors, consider adding a default case in getNotificationContent to handle unexpected event values:
function getNotificationContent(n: Notification) {
switch (n.event) {
case "thread_reply":
return { description: "replied to your post", url: `/t/${n.item?.slug}` };
case "post_like":
return { description: "liked your post", url: `/t/${n.item?.slug}` };
case "profile_mention":
return { description: "mentioned you", url: `/t/${n.item?.slug}` };
case "follow":
return { description: "followed you", url: `/m/${n.source?.handle}` };
+ default:
+ return { description: "performed an action", url: "#" };
}
}Alternatively, handle the possibility of content being undefined in mapToItem:
const content = getNotificationContent(n);
+if (!content) {
+ // Handle undefined content, e.g., log an error or provide default values
+ return null;
+}Committable suggestion was skipped due to low confidence.
| handlers.handleMarkAs(notification.id, "read") | ||
| } | ||
| > |
There was a problem hiding this comment.
Ensure handleMarkAs executes before navigation
The onClick handler attached to the <Link> component may not execute as expected because the navigation might occur before handlers.handleMarkAs(notification.id, "read") completes. This could result in the notification not being marked as read.
Consider modifying the code to prevent the default navigation and programmatically navigate after marking the notification as read:
<Link
className={hstack({
w: "full",
justify: "space-between",
})}
href={notification.url}
- onClick={() =>
+ onClick={(e) => {
+ e.preventDefault();
+ handlers.handleMarkAs(notification.id, "read").then(() => {
+ router.push(notification.url);
+ });
+ }}
>Ensure that router is imported from next/router:
import { useRouter } from 'next/router';
// Inside your component:
const router = useRouter();Alternatively, you might consider moving the onClick handler to the <Menu.Item> component or handling the mark-as-read action on the destination page.
| </Center> | ||
| ) : ( | ||
| notifications.map((notification) => ( | ||
| <Menu.Item value={notification.id} height="auto" py="1"> |
There was a problem hiding this comment.
Add a key prop to items rendered in the list
When rendering a list with .map(), each item should have a unique key prop to help React identify which items have changed, are added, or are removed. The <Menu.Item> component in the notifications.map() function is missing the key prop.
Apply this diff to fix the issue:
notifications.map((notification) => (
- <Menu.Item value={notification.id} height="auto" py="1">
+ <Menu.Item key={notification.id} value={notification.id} height="auto" py="1">📝 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.
| <Menu.Item value={notification.id} height="auto" py="1"> | |
| <Menu.Item key={notification.id} value={notification.id} height="auto" py="1"> |
🧰 Tools
🪛 Biome
[error] 76-76: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
Southclaws
force-pushed
the
notifications
branch
from
bf168e2 to
27a3679
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
web/src/app/(dashboard)/notifications/page.tsx (1)
6-9: LGTM: Asynchronous data fetching implemented correctly.The try block successfully fetches notification data and passes it to the
NotificationScreencomponent.Consider adding a loading state to improve user experience:
export default async function Page() { + const [isLoading, setIsLoading] = useState(true); try { + setIsLoading(true); const { data } = await notificationList(); + setIsLoading(false); - return <NotificationScreen initialData={data} />; + return <NotificationScreen initialData={data} isLoading={isLoading} />; } catch (e) { + setIsLoading(false); return <Unready error={e} />; } }This change would allow the
NotificationScreencomponent to show a loading indicator while data is being fetched.web/src/components/notifications/NotificationsMenu.tsx (1)
126-126: Addaria-labelto the icon for accessibilityConsider adding an
aria-labelto theCog6ToothIconto improve accessibility for users who rely on screen readers.Apply this diff to enhance accessibility:
- return <Cog6ToothIcon width="1rem" />; + return <Cog6ToothIcon width="1rem" aria-label="System Notification" />;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- web/src/app/(dashboard)/notifications/page.tsx (1 hunks)
- web/src/components/notifications/NotificationCardList.tsx (1 hunks)
- web/src/components/notifications/NotificationsMenu.tsx (1 hunks)
- web/src/components/notifications/item.ts (1 hunks)
- web/src/components/notifications/useNotifications.ts (1 hunks)
- web/src/components/site/Navigation/Actions/Notifications.tsx (1 hunks)
- web/src/components/site/Navigation/components/Toolbar.tsx (2 hunks)
- web/src/screens/notifications/NotificationScreen.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- web/src/components/notifications/item.ts
- web/src/components/notifications/useNotifications.ts
- web/src/components/site/Navigation/Actions/Notifications.tsx
- web/src/components/site/Navigation/components/Toolbar.tsx
🧰 Additional context used
🪛 Biome
web/src/components/notifications/NotificationsMenu.tsx
[error] 76-76: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (13)
web/src/app/(dashboard)/notifications/page.tsx (3)
1-3: LGTM: Imports are appropriate and well-organized.The imports are relevant to the component's functionality and follow a logical order (API, components, screens).
5-5: LGTM: Function signature updated correctly.The
Pagefunction is now correctly declared asasync, which is necessary for usingawaitinside the function body. This change aligns with the asynchronous data fetching implemented in the function.
10-12:⚠️ Potential issueImprove error handling to prevent exposure of sensitive information.
The current implementation passes the error object directly to the
Unreadycomponent, which could potentially expose sensitive information to the user.As mentioned in a previous review, it's recommended to log the error internally and display a user-friendly message instead. Please apply the following change:
} catch (e) { - return <Unready error={e} />; + console.error(e); // Log the error for debugging purposes + return <Unready error="An unexpected error occurred while fetching notifications." />; }This change ensures that sensitive error details are logged for debugging while presenting a user-friendly message to the end-user.
web/src/screens/notifications/NotificationScreen.tsx (4)
3-17: LGTM: Imports and type definition are well-structuredThe new imports and the
Propstype definition are appropriate for the refactored component. The use ofNotificationListResultas the type forinitialDatainPropsensures type safety when passing data to the component.
61-66: LGTM: NotificationScreen component structureThe
NotificationScreencomponent is well-structured. It effectively uses theuseNotificationScreenhook and handles the loading state appropriately with theUnreadyBanner. The error handling is implemented correctly.
87-90: LGTM: NotificationCardList implementationThe
NotificationCardListcomponent is correctly implemented with the appropriate props. Thenotificationsdata andonMovehandler are passed down, which should provide the necessary functionality for displaying and interacting with the notifications.
77-83: 🛠️ Refactor suggestionUse
onChangeinstead ofonClickfor the Switch componentFor better accessibility and consistency, it's recommended to use
onChangeinstead ofonClickfor form components like switches.Apply this change:
<Switch size="sm" checked={showingArchived} - onClick={handlers.handleToggleStatus} + onChange={handlers.handleToggleStatus} > Archived </Switch>Likely invalid or redundant comment.
web/src/components/notifications/NotificationCardList.tsx (5)
1-17: LGTM: Import statements are appropriate and concise.The import statements are well-organized and include all necessary components and utilities for the implemented functionality. There are no unused imports, which is good for maintaining clean code.
19-22: LGTM: Type definitions are correct and address previous inconsistency.The
Propstype is well-defined for theNotificationCardListcomponent. TheonMovefunction is correctly typed to returnPromise<void>, which resolves the inconsistency mentioned in the past review comment.
24-56: LGTM: NotificationCardList component is well-implemented.The component effectively handles both empty and non-empty notification lists. The conditional rendering for the empty state and the mapping of notifications to Card components are implemented correctly. The use of styled components for layout and styling enhances readability and maintainability.
58-72: LGTM: NotificationSource component is concise and effective.The component efficiently handles the rendering of either a MemberBadge or a system message based on the presence of a source. The conditional logic is clear and the component properly handles both cases.
74-107: 🛠️ Refactor suggestionConsider refactoring StatusControl to reduce code duplication.
The current implementation of StatusControl contains duplicated code for rendering the button with different icons and titles. This duplication can be reduced to improve readability and maintainability.
Consider applying the following refactor:
- return notification.isRead ? ( - <Button - variant="ghost" - size="sm" - title="Mark as unread" - onClick={handleChangeStatus} - > - <InboxArrowDownIcon /> - </Button> - ) : ( - <Button - variant="ghost" - size="sm" - title="Mark as read" - onClick={handleChangeStatus} - > - <ArchiveBoxIcon /> - </Button> - ); + const isRead = notification.isRead; + const buttonTitle = isRead ? "Mark as unread" : "Mark as read"; + const IconComponent = isRead ? InboxArrowDownIcon : ArchiveBoxIcon; + + return ( + <Button + variant="ghost" + size="sm" + title={buttonTitle} + onClick={handleChangeStatus} + > + <IconComponent /> + </Button> + );This refactor reduces code duplication and makes the component more maintainable.
web/src/components/notifications/NotificationsMenu.tsx (1)
18-119: Component implementation looks solidThe
NotificationsMenucomponent is well-structured and follows best practices. The logic for handling notifications and the UI presentation is appropriately managed.🧰 Tools
🪛 Biome
[error] 76-76: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
| const [status, setStatus] = useQueryState<NotificationStatus>("status", { | ||
| defaultValue: "unread", | ||
| parse(v: string) { | ||
| switch (v) { | ||
| case "read": | ||
| return NotificationStatus.read; | ||
| default: | ||
| return NotificationStatus.unread; | ||
| } | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling invalid status values explicitly
The parse function in useQueryState currently defaults to NotificationStatus.unread for any value that isn't "read". To improve robustness and debuggability, consider handling invalid statuses explicitly.
Here's a suggested improvement:
parse(v: string) {
switch (v) {
case "read":
return NotificationStatus.read;
case "unread":
return NotificationStatus.unread;
default:
console.warn(`Invalid notification status: ${v}`);
return NotificationStatus.unread;
}
}| function handleToggleStatus() { | ||
| setStatus( | ||
| status === NotificationStatus.unread | ||
| ? NotificationStatus.read | ||
| : NotificationStatus.unread, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider generalizing handleToggleStatus for future flexibility
The current implementation of handleToggleStatus only toggles between unread and read states. If additional notification statuses are introduced in the future, this function may need to be updated.
Consider a more flexible approach:
function handleToggleStatus() {
const statusOrder = [NotificationStatus.unread, NotificationStatus.read];
const currentIndex = statusOrder.indexOf(status);
const nextIndex = (currentIndex + 1) % statusOrder.length;
setStatus(statusOrder[nextIndex]);
}This implementation allows for easy addition of new statuses in the future by simply updating the statusOrder array.
|
|
||
| const { notifications } = data; | ||
|
|
||
| const showingArchived = status === NotificationStatus.read; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider aligning terminology between code and UI
The variable showingArchived is determined by status === NotificationStatus.read, but the UI label says "Archived". This might cause confusion if "Read" and "Archived" are not strictly equivalent.
Consider standardizing the terminology. For example:
const showingRead = status === NotificationStatus.read;And update the UI label to "Read" instead of "Archived" if appropriate.
No description provided.