Skip to content

[WEB-2555] fix: add "mark all as read" in the notifications header #5770

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pushya22 merged 2 commits into from
Oct 8, 2024
Merged

[WEB-2555] fix: add "mark all as read" in the notifications header #5770

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d9eb215
move mark all as read to header and remove it from dropdown
sharma01ketan Oct 8, 2024
c863eb0
made recommended changes
sharma01ketan Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 5 additions & 39 deletions web/core/components/workspace-notifications/sidebar/header/options/menu-option/root.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,14 @@

import { FC, ReactNode } from "react";
import { observer } from "mobx-react";
import { Check, CheckCheck, CheckCircle, Clock } from "lucide-react";
import { Check, CheckCircle, Clock } from "lucide-react";
import { TNotificationFilter } from "@plane/types";
import { ArchiveIcon, PopoverMenu, Spinner } from "@plane/ui";
import { ArchiveIcon, PopoverMenu } from "@plane/ui";
// components
import { NotificationMenuOptionItem } from "@/components/workspace-notifications";
// constants
import { NOTIFICATIONS_READ } from "@/constants/event-tracker";
import { ENotificationLoader } from "@/constants/notification";
// hooks
import { useEventTracker, useWorkspaceNotifications } from "@/hooks/store";

type TNotificationHeaderMenuOption = {
workspaceSlug: string;
};
import { useWorkspaceNotifications } from "@/hooks/store";

export type TPopoverMenuOptions = {
key: string;
Expand All @@ -27,44 +21,16 @@ export type TPopoverMenuOptions = {
onClick?: (() => void) | undefined;
};

export const NotificationHeaderMenuOption: FC<TNotificationHeaderMenuOption> = observer((props) => {
const { workspaceSlug } = props;
export const NotificationHeaderMenuOption = observer(() => {
// hooks
const { captureEvent } = useEventTracker();
const { loader, filters, updateFilters, updateBulkFilters, markAllNotificationsAsRead } = useWorkspaceNotifications();
const { filters, updateFilters, updateBulkFilters } = useWorkspaceNotifications();

const handleFilterChange = (filterType: keyof TNotificationFilter, filterValue: boolean) =>
updateFilters(filterType, filterValue);

const handleBulkFilterChange = (filter: Partial<TNotificationFilter>) => updateBulkFilters(filter);

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 popoverMenuOptions: TPopoverMenuOptions[] = [
{
key: "menu-mark-all-read",
type: "menu-item",
label: "Mark all as read",
isActive: true,
prependIcon: <CheckCheck className="h-3 w-3" />,
appendIcon: loader === ENotificationLoader.MARK_ALL_AS_READY ? <Spinner height="14px" width="14px" /> : undefined,
onClick: () => {
captureEvent(NOTIFICATIONS_READ);
handleMarkAllNotificationsAsRead();
},
},
{
key: "menu-divider",
type: "divider",
},
{
key: "menu-unread",
type: "menu-item",
Expand Down
39 changes: 34 additions & 5 deletions web/core/components/workspace-notifications/sidebar/header/options/root.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import { FC } from "react";
import { observer } from "mobx-react";
import { RefreshCw } from "lucide-react";
import { Tooltip } from "@plane/ui";
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 { useWorkspaceNotifications } from "@/hooks/store";
import { useEventTracker, useWorkspaceNotifications } from "@/hooks/store";
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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

Copy link
Collaborator

@gakshita gakshita Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense, can you fix this

Copy link
Contributor

@coderabbitai coderabbitai bot Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

(^‿^)

import { usePlatformOS } from "@/hooks/use-platform-os";

type TNotificationSidebarHeaderOptions = {
Expand All @@ -18,7 +19,8 @@ export const NotificationSidebarHeaderOptions: FC<TNotificationSidebarHeaderOpti
const { workspaceSlug } = props;
// hooks
const { isMobile } = usePlatformOS();
const { loader, getNotifications } = useWorkspaceNotifications();
const { loader, getNotifications, markAllNotificationsAsRead } = useWorkspaceNotifications();
const { captureEvent } = useEventTracker();

const refreshNotifications = async () => {
if (loader) return;
Expand All @@ -29,8 +31,35 @@ export const NotificationSidebarHeaderOptions: FC<TNotificationSidebarHeaderOpti
}
};

const handleMarkAllNotificationsAsRead = async () => {
// NOTE: We are using loader to prevent continues request when we are making all the notification to read
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
// 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

if (loader) return;
try {
await markAllNotificationsAsRead(workspaceSlug);
} catch (error) {
console.error(error);
}
Comment on lines +38 to +41
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

};
Comment on lines +36 to +42
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

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.

Comment on lines +34 to +42
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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.


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
Expand All @@ -45,7 +74,7 @@ export const NotificationSidebarHeaderOptions: FC<TNotificationSidebarHeaderOpti
<NotificationFilter />

{/* notification menu options */}
<NotificationHeaderMenuOption workspaceSlug={workspaceSlug} />
<NotificationHeaderMenuOption />
</div>
);
});