[WEB-2555] fix: add "mark all as read" in the notifications header#5770
[WEB-2555] fix: add "mark all as read" in the notifications header#5770
Conversation
WalkthroughThe changes involve modifications to two components: Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 4
🧹 Outside diff range and nitpick comments (1)
web/core/components/workspace-notifications/sidebar/header/options/menu-option/root.tsx (1)
15-15: LGTM: Type definition updated for flexibility.The change to make
workspaceSlugoptional in theTNotificationHeaderMenuOptiontype is appropriate, given that it's no longer directly used within the component.Consider adding a brief comment explaining why
workspaceSlugis optional, to improve code maintainability. For example:type TNotificationHeaderMenuOption = { // Optional: workspaceSlug is not directly used in this component but may be required by parent components workspaceSlug?: string; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- web/core/components/workspace-notifications/sidebar/header/options/menu-option/root.tsx (2 hunks)
- web/core/components/workspace-notifications/sidebar/header/options/root.tsx (4 hunks)
🧰 Additional context used
🔇 Additional comments (5)
web/core/components/workspace-notifications/sidebar/header/options/menu-option/root.tsx (4)
5-5: LGTM: Import addition is consistent with component changes.The addition of the
Checkicon import aligns with the modifications made to the component's functionality, as mentioned in the AI summary.
Line range hint
1-85: Summary: Changes align with PR objectives and improve component structure.The modifications to this file successfully simplify the
NotificationHeaderMenuOptioncomponent by removing unused functionality and props. These changes appear to support the PR objective of relocating the "Mark all as read" button to the notifications header.Key points:
- Removal of unused
workspaceSlugprop and related functionality.- Simplification of the component structure.
- Removal of event tracking related to marking all notifications as read.
The changes are well-implemented and improve the overall structure of the component. However, ensure that the "Mark all as read" functionality has been properly implemented in the new location as per the PR objectives.
7-7: LGTM: New UI component import added.The addition of the
PopoverMenuimport from "@plane/ui" is appropriate for the changes made to the component.Please ensure that the
PopoverMenucomponent is properly utilized within this file. You can verify this by running the following command:✅ Verification successful
Action Required: Verify the Usage of
PopoverMenuComponentPlease run the following command to ensure that the
PopoverMenucomponent is utilized withinroot.tsx:
Verification Successful:
PopoverMenuComponent Usage ConfirmedThe
PopoverMenucomponent is properly utilized within theNotificationHeaderMenuOptioncomponent inroot.tsx.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of PopoverMenu component # Expected: The PopoverMenu component should be used in the NotificationHeaderMenuOption component rg --type typescript 'PopoverMenu' web/core/components/workspace-notifications/sidebar/header/options/menu-option/root.tsxLength of output: 157
Script:
#!/bin/bash # Description: Verify the usage of PopoverMenu component # Expected: The PopoverMenu component should be used in the NotificationHeaderMenuOption component rg 'PopoverMenu' web/core/components/workspace-notifications/sidebar/header/options/menu-option/root.tsxLength of output: 413
28-30: LGTM: Component simplified as per requirements.The removal of unused props, the
useEventTrackerhook, and thehandleMarkAllNotificationsAsReadfunction aligns with the goal of simplifying the component.Please confirm that the functionality for marking all notifications as read has been implemented elsewhere or is no longer required. You can verify this by searching for similar functionality in other components:
✅ Verification successful
Please run the following shell script to search for "mark all as read" functionality without specifying an unrecognized file type:
LGTM: "Mark all as read" functionality remains implemented within the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for "mark all as read" functionality in other components # Expected: Find implementation of marking all notifications as read in another component rg --type typescript 'markAllNotificationsAsRead|mark all as read' web/Length of output: 157
Script:
# #!/bin/bash # Description: Search for "mark all as read" functionality in other components # Expected: Find implementation of marking all notifications as read in another component rg 'markAllNotificationsAsRead|mark all as read' web/Length of output: 1112
web/core/components/workspace-notifications/sidebar/header/options/root.tsx (1)
78-78: Verify all usages ofNotificationHeaderMenuOptionafter prop removal.The
workspaceSlugprop has been removed fromNotificationHeaderMenuOption. Ensure that all instances where this component is used have been updated to prevent potential errors.Run the following script to locate any remaining usages passing the outdated prop:
This will help identify and update any outdated component usages.
| import { ENotificationLoader, ENotificationQueryParamType } from "@/constants/notification"; | ||
| // hooks | ||
| import { useWorkspaceNotifications } from "@/hooks/store"; | ||
| import { useEventTracker, useWorkspaceNotifications } from "@/hooks/store"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to prevent multiple calls to useWorkspaceNotifications.
You are invoking useWorkspaceNotifications() twice to destructure different values. This can be optimized by combining them into a single call to enhance performance and readability.
Apply the following change:
const { isMobile } = usePlatformOS();
-const { loader, getNotifications } = useWorkspaceNotifications();
+const { loader, getNotifications, markAllNotificationsAsRead } = useWorkspaceNotifications();
const { captureEvent } = useEventTracker();
-const { markAllNotificationsAsRead } = useWorkspaceNotifications();Also applies to: 23-24
There was a problem hiding this comment.
This makes sense, can you fix this
There was a problem hiding this comment.
@gakshita, sure, I've applied the refactoring to combine the calls to useWorkspaceNotifications() into a single call. Here's the updated code:
import { FC } from "react";
import { observer } from "mobx-react";
import { CheckCheck, RefreshCw } from "lucide-react";
import { Spinner, Tooltip } from "@plane/ui";
// components
import { NotificationFilter, NotificationHeaderMenuOption } from "@/components/workspace-notifications";
// constants
import { NOTIFICATIONS_READ } from "@/constants/event-tracker";
import { ENotificationLoader, ENotificationQueryParamType } from "@/constants/notification";
// hooks
import { useEventTracker, useWorkspaceNotifications } from "@/hooks/store";
import { usePlatformOS } from "@/hooks/use-platform-os";
type TNotificationSidebarHeaderOptions = {
workspaceSlug: string;
};
export const NotificationSidebarHeaderOptions: FC<TNotificationSidebarHeaderOptions> = observer((props) => {
const { workspaceSlug } = props;
// hooks
const { isMobile } = usePlatformOS();
const { loader, getNotifications, markAllNotificationsAsRead } = useWorkspaceNotifications();
const { captureEvent } = useEventTracker();
const refreshNotifications = async () => {
if (loader) return;
try {
await getNotifications(
workspaceSlug,
ENotificationLoader.MUTATION_LOADER,
ENotificationQueryParamType.CURRENT
);
} catch (error) {
console.error(error);
}
};
const handleMarkAllNotificationsAsRead = async () => {
// NOTE: We are using loader to prevent continuous requests when we are marking all notifications as read
if (loader) return;
try {
await markAllNotificationsAsRead(workspaceSlug);
} catch (error) {
console.error(error);
}
};
return (
<div className="relative flex justify-center items-center gap-2 text-sm">
{/* mark all notifications as read */}
<Tooltip tooltipContent="Mark all as read" isMobile={isMobile} position="bottom">
<div
className="flex-shrink-0 w-5 h-5 flex justify-center items-center overflow-hidden cursor-pointer transition-all hover:bg-custom-background-80 rounded-sm"
onClick={() => {
captureEvent(NOTIFICATIONS_READ);
handleMarkAllNotificationsAsRead();
}}
>
{loader === ENotificationLoader.MARK_ALL_AS_READY ? (
<Spinner height="14px" width="14px" />
) : (
<CheckCheck className="h-3 w-3" />
)}
</div>
</Tooltip>
{/* refetch current notifications */}
<Tooltip tooltipContent="Refresh" isMobile={isMobile} position="bottom">
<div
className="flex-shrink-0 w-5 h-5 flex justify-center items-center overflow-hidden cursor-pointer transition-all hover:bg-custom-background-80 rounded-sm"
onClick={refreshNotifications}
>
<RefreshCw
className={`h-3 w-3 ${
loader === ENotificationLoader.MUTATION_LOADER ? "animate-spin" : ""
}`}
/>
</div>
</Tooltip>
{/* notification filters */}
<NotificationFilter />
{/* notification menu options */}
<NotificationHeaderMenuOption />
</div>
);
});This change enhances performance and readability by reducing unnecessary calls to the hook.
(^‿^)
| }; | ||
|
|
||
| const handleMarkAllNotificationsAsRead = async () => { | ||
| // NOTE: We are using loader to prevent continues request when we are making all the notification to read |
There was a problem hiding this comment.
Correct the typo and improve the clarity of the comment.
The comment contains grammatical errors that might lead to confusion.
Apply this fix:
-// NOTE: We are using loader to prevent continues request when we are making all the notification to read
+// NOTE: We are using loader to prevent continuous requests when marking all notifications as read📝 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.
| // NOTE: We are using loader to prevent continues request when we are making all the notification to read | |
| // NOTE: We are using loader to prevent continuous requests when marking all notifications as read |
| await markAllNotificationsAsRead(workspaceSlug); | ||
| } catch (error) { | ||
| console.error(error); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling by notifying the user upon failure.
In the catch block, errors are logged to the console, but the user remains uninformed. Providing user feedback improves the user experience.
Consider adding a user notification:
} catch (error) {
console.error(error);
+ // Optionally, display an error message to the user
+ // showToast("Failed to mark all notifications as read.", "error");
}Committable suggestion was skipped due to low confidence.
| if (loader) return; | ||
| try { | ||
| await markAllNotificationsAsRead(workspaceSlug); | ||
| } catch (error) { | ||
| console.error(error); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure the loader state correctly prevents concurrent requests.
Currently, you're checking if (loader) return;, which might block the action if any loader is active. To prevent unintended blocking, check for the specific loader state related to marking notifications as read.
Update the condition as follows:
-if (loader) return;
+if (loader === ENotificationLoader.MARK_ALL_AS_READY) return;This ensures that only the relevant loader state prevents concurrent requests for this action.
Committable suggestion was skipped due to low confidence.
|
|
||
| export const NotificationHeaderMenuOption: FC<TNotificationHeaderMenuOption> = observer((props) => { | ||
| const { workspaceSlug } = props; | ||
| export const NotificationHeaderMenuOption: FC<TNotificationHeaderMenuOption> = observer(() => { |
There was a problem hiding this comment.
Since you removed the props you can remove its type declaration as well
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
web/core/components/workspace-notifications/sidebar/header/options/menu-option/root.tsx (1)
24-26: LGTM: Component simplification.The removal of unused props, functions, and hooks simplifies the component and aligns with the PR objectives of relocating the "Mark all as read" functionality.
Consider removing the
TNotificationHeaderMenuOptiontype declaration since it's no longer used in the component props.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- web/core/components/workspace-notifications/sidebar/header/options/menu-option/root.tsx (2 hunks)
- web/core/components/workspace-notifications/sidebar/header/options/root.tsx (4 hunks)
🧰 Additional context used
🔇 Additional comments (8)
web/core/components/workspace-notifications/sidebar/header/options/menu-option/root.tsx (4)
5-5: LGTM: Import reordering.The reordering of imported icons improves code readability and maintains consistency.
7-7: LGTM: Import reordering.The reordering of imported components improves code readability and maintains consistency.
12-12: LGTM: Import reordering.The reordering of the
useWorkspaceNotificationsimport improves code readability and maintains consistency.
Line range hint
1-78: Overall assessment: Changes align with PR objectives.The modifications to the
NotificationHeaderMenuOptioncomponent successfully simplify its structure by removing unused functionality related to marking all notifications as read. This aligns well with the PR objective of relocating the "Mark all as read" button to the notifications header.The changes include:
- Removal of the
workspaceSlugprop and associated functionality.- Elimination of the
useEventTrackerhook andhandleMarkAllNotificationsAsReadfunction.- Simplification of the component's props and internal logic.
These alterations contribute to a more focused and maintainable component, supporting the overall goal of improving the user interface for notification management.
web/core/components/workspace-notifications/sidebar/header/options/root.tsx (4)
3-4: LGTM: Import changes are appropriate.The new imports from 'lucide-react' and '@plane/ui' are necessary for the added UI elements. The NOTIFICATIONS_READ constant and useEventTracker hook are correctly imported for the new functionality.
Also applies to: 8-8, 11-11
46-61: LGTM: UI changes for "Mark all as read" functionality.The new "Mark all as read" button with its tooltip and loading state is well-implemented and aligns with the PR objectives. The use of Spinner and CheckCheck icons provides clear visual feedback to the user.
22-23: 🛠️ Refactor suggestionOptimize hook usage.
The useWorkspaceNotifications hook is called twice in the component. To improve performance and readability, consider combining these calls.
Apply the following change:
-const { loader, getNotifications } = useWorkspaceNotifications(); +const { loader, getNotifications, markAllNotificationsAsRead } = useWorkspaceNotifications(); const { captureEvent } = useEventTracker(); -const { markAllNotificationsAsRead } = useWorkspaceNotifications();Likely invalid or redundant comment.
77-77: Verify the impact of removing workspaceSlug prop.The workspaceSlug prop has been removed from the NotificationHeaderMenuOption component. Ensure that this change doesn't break any functionality in the NotificationHeaderMenuOption component.
Run the following script to check for any remaining uses of workspaceSlug in the NotificationHeaderMenuOption component:
| const handleMarkAllNotificationsAsRead = async () => { | ||
| // NOTE: We are using loader to prevent continues request when we are making all the notification to read | ||
| if (loader) return; | ||
| try { | ||
| await markAllNotificationsAsRead(workspaceSlug); | ||
| } catch (error) { | ||
| console.error(error); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refine loader state check for better concurrency control.
The current loader check might prevent the action if any loader is active. Consider checking for the specific loader state related to marking notifications as read.
Update the condition as follows:
-if (loader) return;
+if (loader === ENotificationLoader.MARK_ALL_AS_READY) return;This ensures that only the relevant loader state prevents concurrent requests for this action.
📝 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 handleMarkAllNotificationsAsRead = async () => { | |
| // NOTE: We are using loader to prevent continues request when we are making all the notification to read | |
| if (loader) return; | |
| try { | |
| await markAllNotificationsAsRead(workspaceSlug); | |
| } catch (error) { | |
| console.error(error); | |
| } | |
| }; | |
| const handleMarkAllNotificationsAsRead = async () => { | |
| // NOTE: We are using loader to prevent continues request when we are making all the notification to read | |
| if (loader === ENotificationLoader.MARK_ALL_AS_READY) return; | |
| try { | |
| await markAllNotificationsAsRead(workspaceSlug); | |
| } catch (error) { | |
| console.error(error); | |
| } | |
| }; |
🛠️ Refactor suggestion
Enhance error handling with user feedback.
Currently, errors are only logged to the console. Consider providing feedback to the user when an error occurs.
Add user notification in the catch block:
} catch (error) {
console.error(error);
+ // TODO: Implement a user-facing error notification
+ // For example: showToast("Failed to mark all notifications as read. Please try again.", "error");
}This improvement will enhance the user experience by keeping them informed of any issues.
Committable suggestion was skipped due to low confidence.
4 participants
[WEB-2555]
This PR aims to relocate the "Mark all as read" button from the dropdown to the header in the notifications header.
Previous State:
Screen.Recording.2024-10-08.at.1.06.15.PM.mov
New State:
Note: The below video is recorded on 3G speed using dev-tools
Screen.Recording.2024-10-08.at.1.07.00.PM.mov
Summary by CodeRabbit
New Features
Bug Fixes
Refactor