Review fixes

2026-09-22 13:39:35 +00:00 · 2025-09-05 22:03:57 -04:00
parent 3350121635
commit ad42fb4721
12 changed files with 226 additions and 169 deletions
--- a/backend/src/ee/services/access-approval-request/access-approval-request-service.ts
+++ b/backend/src/ee/services/access-approval-request/access-approval-request-service.ts
@@ -69,7 +69,7 @@ type TSecretApprovalRequestServiceFactoryDep = {
  projectSlackConfigDAL: Pick<TProjectSlackConfigDALFactory, "getIntegrationDetailsByProject">;
  microsoftTeamsService: Pick<TMicrosoftTeamsServiceFactory, "sendNotification">;
  projectMicrosoftTeamsConfigDAL: Pick<TProjectMicrosoftTeamsConfigDALFactory, "getIntegrationDetailsByProject">;
-  notificationService: Pick<TNotificationServiceFactory, "createUserNotification">;
+  notificationService: Pick<TNotificationServiceFactory, "createUserNotifications">;
 };

 export const accessApprovalRequestServiceFactory = ({
@@ -279,15 +279,15 @@ export const accessApprovalRequestServiceFactory = ({
        }
      });

-      for await (const approver of approverUsers) {
-        await notificationService.createUserNotification({
+      await notificationService.createUserNotifications(
+        approverUsers.map((approver) => ({
          userId: approver.id,
          type: NotificationType.ACCESS_APPROVAL_REQUEST,
          title: "Access Approval Request",
          body: `**${requesterFullName}** (${requestedByUser.email}) has requested ${isTemporary ? "temporary" : "permanent"} access to **${secretPath}** in the **${envSlug}** environment for project **${project.name}**.`,
          link: approvalPath
-        });
-      }
+        }))
+      );

      await smtpService.sendMail({
        recipients: approverUsers.filter((approver) => approver.email).map((approver) => approver.email!),
@@ -406,7 +406,8 @@ export const accessApprovalRequestServiceFactory = ({

      const requesterFullName = `${requestedByUser.firstName} ${requestedByUser.lastName}`;
      const editorFullName = `${editedByUser.firstName} ${editedByUser.lastName}`;
-      const approvalUrl = `${cfg.SITE_URL}/projects/secret-management/${project.id}/approval`;
+      const approvalPath = `/projects/secret-management/${project.id}/approval`;
+      const approvalUrl = `${cfg.SITE_URL}${approvalPath}`;

      await triggerWorkflowIntegrationNotification({
        input: {
@@ -437,27 +438,43 @@ export const accessApprovalRequestServiceFactory = ({
        }
      });

-      await smtpService.sendMail({
-        recipients: policy.approvers
-          .filter((approver) => Boolean(approver.email) && approver.userId !== editedByUser.id)
-          .map((approver) => approver.email!),
-        subjectLine: "Access Approval Request Updated",
-        substitutions: {
-          projectName: project.name,
-          requesterFullName,
-          requesterEmail: requestedByUser.email,
-          isTemporary: true,
-          expiresIn: msFn(ms(temporaryRange || ""), { long: true }),
-          secretPath,
-          environment: envSlug,
-          permissions: accessTypes,
-          approvalUrl,
-          editNote,
-          editorFullName,
-          editorEmail: editedByUser.email
-        },
-        template: SmtpTemplates.AccessApprovalRequestUpdated
-      });
+      await notificationService.createUserNotifications(
+        policy.approvers
+          .filter((approver) => Boolean(approver.userId) && approver.userId !== editedByUser.id)
+          .map((approver) => ({
+            userId: approver.userId!,
+            type: NotificationType.ACCESS_APPROVAL_REQUEST_UPDATED,
+            title: "Access Approval Request Updated",
+            body: `**${editorFullName}** (${editedByUser.email}) has updated the access request submitted by **${requesterFullName}** (${requestedByUser.email}) for **${secretPath}** in the **${envSlug}** environment for project **${project.name}**.`,
+            link: approvalPath
+          }))
+      );
+
+      const recipients = policy.approvers
+        .filter((approver) => Boolean(approver.email) && approver.userId !== editedByUser.id)
+        .map((approver) => approver.email!);
+
+      if (recipients.length > 0) {
+        await smtpService.sendMail({
+          recipients,
+          subjectLine: "Access Approval Request Updated",
+          substitutions: {
+            projectName: project.name,
+            requesterFullName,
+            requesterEmail: requestedByUser.email,
+            isTemporary: true,
+            expiresIn: msFn(ms(temporaryRange || ""), { long: true }),
+            secretPath,
+            environment: envSlug,
+            permissions: accessTypes,
+            approvalUrl,
+            editNote,
+            editorFullName,
+            editorEmail: editedByUser.email
+          },
+          template: SmtpTemplates.AccessApprovalRequestUpdated
+        });
+      }

      return approvalRequest;
    });
--- a/backend/src/queue/queue-service.ts
+++ b/backend/src/queue/queue-service.ts
@@ -318,7 +318,7 @@ export type TQueueJobTypes = {
  };
  [QueueName.UserNotification]: {
    name: QueueJobs.UserNotification;
-    payload: TCreateUserNotificationDTO;
+    payload: { notifications: TCreateUserNotificationDTO[] };
  };
 };

--- a/backend/src/server/routes/v1/notification-router.ts
+++ b/backend/src/server/routes/v1/notification-router.ts
@@ -64,15 +64,23 @@ export const registerNotificationRouter = async (server: FastifyZodProvider) =>
  });

  server.route({
-    url: "/user/:notificationId/mark-as-read",
+    url: "/user/:notificationId",
    config: {
      rateLimit: writeLimit
    },
-    method: "POST",
+    method: "PATCH",
    schema: {
      params: z.object({
        notificationId: z.string()
-      })
+      }),
+      body: z.object({
+        isRead: z.boolean()
+      }),
+      response: {
+        200: z.object({
+          notification: UserNotificationsSchema
+        })
+      }
    },
    onRequest: verifyAuth([AuthMode.JWT]),
    handler: async (req) => {
@@ -80,10 +88,13 @@ export const registerNotificationRouter = async (server: FastifyZodProvider) =>
        throw new UnauthorizedError({ message: "This endpoint can only be accessed by users" });
      }

-      await server.services.notification.markUserNotificationAsRead({
+      const notification = await server.services.notification.updateUserNotification({
        notificationId: req.params.notificationId,
-        userId: req.auth.userId
+        userId: req.auth.userId,
+        ...req.body
      });
+
+      return { notification };
    }
  });

--- a/backend/src/services/notification/notification-queue.ts
+++ b/backend/src/services/notification/notification-queue.ts
@@ -4,43 +4,36 @@ import { TCreateUserNotificationDTO } from "./notification-types";
 import { TUserNotificationDALFactory } from "./user-notification-dal";

 type TNotificationQueueServiceFactoryDep = {
-  userNotificationDAL: Pick<TUserNotificationDALFactory, "create">;
+  userNotificationDAL: Pick<TUserNotificationDALFactory, "batchInsert">;
  queueService: TQueueServiceFactory;
 };

 export type TNotificationQueueServiceFactory = {
-  pushUserNotification: (data: TCreateUserNotificationDTO) => Promise<void>;
+  pushUserNotifications: (data: TCreateUserNotificationDTO[]) => Promise<void>;
 };

 export const notificationQueueServiceFactory = async ({
  userNotificationDAL,
  queueService
 }: TNotificationQueueServiceFactoryDep): Promise<TNotificationQueueServiceFactory> => {
-  const pushUserNotification = async (data: TCreateUserNotificationDTO) => {
-    await queueService.queuePg(QueueJobs.UserNotification, data);
+  const pushUserNotifications = async (data: TCreateUserNotificationDTO[]) => {
+    await queueService.queuePg(QueueJobs.UserNotification, { notifications: data });
  };

  await queueService.startPg(
    QueueJobs.UserNotification,
    async ([job]) => {
-      const { userId, type, title, body, link } = job.data as TCreateUserNotificationDTO;
-
-      await userNotificationDAL.create({
-        userId,
-        type,
-        title,
-        body,
-        link
-      });
+      const { notifications } = job.data as { notifications: TCreateUserNotificationDTO[] };
+      await userNotificationDAL.batchInsert(notifications);
    },
    {
-      batchSize: 100,
-      workerCount: 5,
-      pollingIntervalSeconds: 2
+      batchSize: 1,
+      workerCount: 2,
+      pollingIntervalSeconds: 1
    }
  );

  return {
-    pushUserNotification
+    pushUserNotifications
  };
 };
--- a/backend/src/services/notification/notification-service.ts
+++ b/backend/src/services/notification/notification-service.ts
@@ -29,8 +29,8 @@ export const notificationServiceFactory = ({
    return notifications;
  };

-  const createUserNotification = async (data: TCreateUserNotificationDTO) => {
-    return notificationQueue.pushUserNotification(data);
+  const createUserNotifications = async (data: TCreateUserNotificationDTO[]) => {
+    return notificationQueue.pushUserNotifications(data);
  };

  const deleteUserNotification = async ({ userId, notificationId }: { userId: string; notificationId: string }) => {
@@ -47,23 +47,34 @@ export const notificationServiceFactory = ({
    await userNotificationDAL.markAllNotificationsAsRead(userId);
  };

-  const markUserNotificationAsRead = async ({ userId, notificationId }: { userId: string; notificationId: string }) => {
-    await userNotificationDAL.update(
+  const updateUserNotification = async ({
+    userId,
+    notificationId,
+    isRead
+  }: {
+    userId: string;
+    notificationId: string;
+    isRead: boolean;
+  }) => {
+    const [updatedNotification] = await userNotificationDAL.update(
      {
        id: notificationId,
        userId
      },
      {
-        isRead: true
+        isRead
      }
    );
+
+    if (!updatedNotification) throw new NotFoundError({ message: "Notification not found" });
+    return updatedNotification;
  };

  return {
    listUserNotifications,
-    createUserNotification,
+    createUserNotifications,
    deleteUserNotification,
    markUserNotificationsAsRead,
-    markUserNotificationAsRead
+    updateUserNotification
  };
 };
--- a/backend/src/services/notification/notification-types.ts
+++ b/backend/src/services/notification/notification-types.ts
@@ -1,5 +1,6 @@
 export enum NotificationType {
-  ACCESS_APPROVAL_REQUEST = "access-approval-request"
+  ACCESS_APPROVAL_REQUEST = "access-approval-request",
+  ACCESS_APPROVAL_REQUEST_UPDATED = "access-approval-request-updated"
 }

 export interface TCreateUserNotificationDTO {
--- a/backend/src/services/notification/user-notification-dal.ts
+++ b/backend/src/services/notification/user-notification-dal.ts
@@ -21,7 +21,7 @@ export const userNotificationDALFactory = (db: TDbClient) => {
      userId,
      startDate,
      endDate,
-      limit = 10000,
+      limit = 1000,
      offset = 0
    }: {
      userId: string;
--- a/frontend/src/hooks/api/notifications/mutations.tsx
+++ b/frontend/src/hooks/api/notifications/mutations.tsx
@@ -23,17 +23,21 @@ export const useMarkAllNotificationsAsRead = () => {
  });
 };

-export const useMarkNotificationAsRead = () => {
+export const useUpdateNotification = () => {
  const queryClient = useQueryClient();
  return useMutation({
-    mutationFn: async (notificationId: string) => {
-      await apiRequest.post(`/api/v1/notifications/user/${notificationId}/mark-as-read`);
+    mutationFn: async ({ notificationId, isRead }: { notificationId: string; isRead: boolean }) => {
+      const { data } = await apiRequest.patch<{ notification: TUserNotification }>(
+        `/api/v1/notifications/user/${notificationId}`,
+        { isRead }
+      );
+      return data.notification;
    },
-    onSuccess: (_, notificationId) => {
+    onSuccess: (updatedNotification) => {
      queryClient.setQueryData<TUserNotification[]>(notificationKeys.list(), (oldData) => {
        if (!oldData) return oldData;
        return oldData.map((notification) =>
-          notification.id === notificationId ? { ...notification, isRead: true } : notification
+          notification.id === updatedNotification.id ? updatedNotification : notification
        );
      });
    }
--- a/frontend/src/hooks/api/notifications/queries.tsx
+++ b/frontend/src/hooks/api/notifications/queries.tsx
@@ -20,6 +20,6 @@ export const useGetMyNotifications = () => {
      );
      return notifications;
    },
-    refetchInterval: 10 * 1000 // Poll every 10 seconds
+    refetchInterval: 30 * 1000 // Poll every 30 seconds
  });
 };
--- a/frontend/src/layouts/OrganizationLayout/components/NavBar/Navbar.tsx
+++ b/frontend/src/layouts/OrganizationLayout/components/NavBar/Navbar.tsx
@@ -1,6 +1,6 @@
-import { useMemo, useState } from "react";
+import { useState } from "react";
 import { faGithub, faSlack } from "@fortawesome/free-brands-svg-icons";
-import { faBell, faCircleQuestion, faUserCircle } from "@fortawesome/free-regular-svg-icons";
+import { faCircleQuestion, faUserCircle } from "@fortawesome/free-regular-svg-icons";
 import {
  faArrowUpRightFromSquare,
  faBook,
@@ -25,7 +25,6 @@ import SecurityClient from "@app/components/utilities/SecurityClient";
 import {
  BreadcrumbContainer,
  Button,
-  ContentLoader,
  DropdownMenu,
  DropdownMenuContent,
  DropdownMenuItem,
@@ -43,19 +42,13 @@ import { useToggle } from "@app/hooks";
 import { useGetOrganizations, useLogoutUser, workspaceKeys } from "@app/hooks/api";
 import { authKeys, selectOrganization } from "@app/hooks/api/auth/queries";
 import { MfaMethod } from "@app/hooks/api/auth/types";
-import {
-  useDeleteNotification,
-  useMarkAllNotificationsAsRead,
-  useMarkNotificationAsRead
-} from "@app/hooks/api/notifications/mutations";
-import { useGetMyNotifications } from "@app/hooks/api/notifications/queries";
 import { getAuthToken } from "@app/hooks/api/reactQuery";
 import { SubscriptionPlan } from "@app/hooks/api/types";
 import { AuthMethod } from "@app/hooks/api/users/types";
 import { navigateUserToOrg } from "@app/pages/auth/LoginPage/Login.utils";

 import { ServerAdminsPanel } from "../ServerAdminsPanel/ServerAdminsPanel";
-import { Notification } from "./Notification";
+import { NotificationDropdown } from "./NotificationDropdown";

 const getPlan = (subscription: SubscriptionPlan) => {
  if (subscription.groups) return "Enterprise";
@@ -127,16 +120,6 @@ export const Navbar = () => {
  const router = useRouter();
  const queryClient = useQueryClient();

-  const { data: notifications, isLoading } = useGetMyNotifications();
-  const { mutate: markAllAsRead } = useMarkAllNotificationsAsRead();
-  const { mutate: markNotificationAsRead } = useMarkNotificationAsRead();
-  const { mutate: deleteNotification } = useDeleteNotification();
-
-  const unreadCount = useMemo(
-    () => notifications?.filter((n) => !n.isRead).length || 0,
-    [notifications]
-  );
-
  const location = useLocation();
  const matches = useRouterState({ select: (s) => s.matches.at(-1)?.context });
  const breadcrumbs = matches && "breadcrumbs" in matches ? matches.breadcrumbs : undefined;
@@ -377,86 +360,7 @@ export const Navbar = () => {
          )}
        </DropdownMenuContent>
      </DropdownMenu>
-      <DropdownMenu modal={false}>
-        <DropdownMenuTrigger>
-          <div className="relative border border-r-0 border-mineshaft-500 px-2.5 py-1 hover:bg-mineshaft-600">
-            <FontAwesomeIcon icon={faBell} className="text-mineshaft-200" />
-            {unreadCount > 0 && (
-              <span className="absolute -right-1 -top-1 flex h-4 min-w-4 items-center justify-center rounded-full bg-yellow-400 px-1 text-[10px] text-black">
-                {unreadCount > 99 ? "99+" : unreadCount}
-              </span>
-            )}
-          </div>
-        </DropdownMenuTrigger>
-        <DropdownMenuContent
-          align="end"
-          side="bottom"
-          className="mt-3 flex h-[550px] w-[400px] overflow-hidden rounded-lg"
-        >
-          <div className="flex w-full flex-col">
-            <div className="flex items-center justify-between border-b border-mineshaft-500 px-4 py-2">
-              <span className="text-xl font-semibold text-white">Notifications</span>
-              <button
-                type="button"
-                className="text-xs font-medium text-mineshaft-300 hover:text-primary-400 disabled:pointer-events-none disabled:opacity-50"
-                onClick={(e) => {
-                  e.preventDefault();
-                  markAllAsRead();
-                }}
-                disabled={unreadCount === 0}
-              >
-                Mark all as read
-              </button>
-            </div>
-            <div className="flex h-full w-full overflow-auto">
-              {isLoading && (
-                <div className="flex h-full w-full items-center justify-center">
-                  <ContentLoader className="pointer-events-none" lottieClassName="size-10" />
-                </div>
-              )}
-              {!isLoading && notifications?.length === 0 && (
-                <div className="flex h-full w-full flex-col items-center justify-center">
-                  <FontAwesomeIcon icon={faBell} size="3x" className="text-mineshaft-400" />
-                  <span className="mt-4 text-sm text-mineshaft-300">No new notifications</span>
-                  <span className="text-xs text-mineshaft-400">
-                    We&apos;ll let you know when something important happens.
-                  </span>
-                </div>
-              )}
-              {!isLoading && notifications && notifications.length > 0 && (
-                <div className="flex w-full flex-col">
-                  {notifications.map((notification) => (
-                    <div
-                      role="button"
-                      tabIndex={0}
-                      key={notification.id}
-                      onClick={() => {
-                        if (!notification.isRead) {
-                          markNotificationAsRead(notification.id);
-                        }
-                        if (notification.link) {
-                          router.navigate({ to: notification.link });
-                        }
-                      }}
-                      onKeyDown={(e) => {
-                        if (e.key !== "Enter") return;
-                        if (!notification.isRead) {
-                          markNotificationAsRead(notification.id);
-                        }
-                        if (notification.link) {
-                          router.navigate({ to: notification.link });
-                        }
-                      }}
-                    >
-                      <Notification notification={notification} onDelete={deleteNotification} />
-                    </div>
-                  ))}
-                </div>
-              )}
-            </div>
-          </div>
-        </DropdownMenuContent>
-      </DropdownMenu>
+      <NotificationDropdown />
      <DropdownMenu modal={false}>
        <DropdownMenuTrigger asChild>
          <div className="rounded-r-md border border-mineshaft-500 px-2.5 py-1 hover:bg-mineshaft-600">
--- a/frontend/src/layouts/OrganizationLayout/components/NavBar/Notification.tsx
+++ b/frontend/src/layouts/OrganizationLayout/components/NavBar/Notification.tsx
@@ -26,9 +26,9 @@ export const Notification = ({ notification, onDelete }: Props) => {
          {!notification.isRead && (
            <FontAwesomeIcon icon={faCircle} className="mt-1.5 size-2 text-yellow-400" />
          )}
-          <Tooltip content={notification.title} delayDuration={300}>
-            <span className="overflow-hidden text-ellipsis whitespace-nowrap font-medium leading-5 text-mineshaft-100">
-              <Markdown>{notification.title}</Markdown>
+          <Tooltip content={<Markdown>{notification.title}</Markdown>} delayDuration={300}>
+            <span className="overflow-hidden text-ellipsis whitespace-nowrap text-sm font-medium leading-5 text-mineshaft-100">
+              <Markdown components={{ p: "span" }}>{notification.title}</Markdown>
            </span>
          </Tooltip>
          <span className="ml-auto mt-px whitespace-nowrap text-xs text-mineshaft-400">
@@ -36,7 +36,7 @@ export const Notification = ({ notification, onDelete }: Props) => {
          </span>
        </div>
        {notification.body && (
-          <span className="max-w-[350px] text-sm text-mineshaft-300">
+          <span className="max-w-[350px] text-xs text-mineshaft-300">
            <Markdown>{notification.body}</Markdown>
          </span>
        )}
--- a/frontend/src/layouts/OrganizationLayout/components/NavBar/NotificationDropdown.tsx
+++ b/frontend/src/layouts/OrganizationLayout/components/NavBar/NotificationDropdown.tsx
@@ -0,0 +1,116 @@
+import { useMemo } from "react";
+import { faBell } from "@fortawesome/free-solid-svg-icons";
+import { FontAwesomeIcon } from "@fortawesome/react-fontawesome";
+import { useRouter } from "@tanstack/react-router";
+
+import {
+  ContentLoader,
+  DropdownMenu,
+  DropdownMenuContent,
+  DropdownMenuTrigger
+} from "@app/components/v2";
+import {
+  useDeleteNotification,
+  useMarkAllNotificationsAsRead,
+  useUpdateNotification
+} from "@app/hooks/api/notifications/mutations";
+import { useGetMyNotifications } from "@app/hooks/api/notifications/queries";
+
+import { Notification } from "./Notification";
+
+export const NotificationDropdown = () => {
+  const router = useRouter();
+
+  const { data: notifications, isLoading } = useGetMyNotifications();
+  const { mutate: markAllAsRead } = useMarkAllNotificationsAsRead();
+  const { mutate: updateNotification } = useUpdateNotification();
+  const { mutate: deleteNotification } = useDeleteNotification();
+
+  const unreadCount = useMemo(
+    () => notifications?.filter((n) => !n.isRead).length || 0,
+    [notifications]
+  );
+
+  return (
+    <DropdownMenu modal={false}>
+      <DropdownMenuTrigger>
+        <div className="relative border border-r-0 border-mineshaft-500 px-2.5 py-1 hover:bg-mineshaft-600">
+          <FontAwesomeIcon icon={faBell} className="text-mineshaft-200" />
+          {unreadCount > 0 && (
+            <span className="absolute -right-1 -top-1 flex h-4 min-w-4 items-center justify-center rounded-full bg-yellow-400 px-1 text-[10px] text-black">
+              {unreadCount > 99 ? "99+" : unreadCount}
+            </span>
+          )}
+        </div>
+      </DropdownMenuTrigger>
+      <DropdownMenuContent
+        align="end"
+        side="bottom"
+        className="mt-3 flex h-[550px] w-[400px] overflow-hidden rounded-lg"
+      >
+        <div className="flex w-full flex-col">
+          <div className="flex items-center justify-between border-b border-mineshaft-500 px-4 py-2">
+            <span className="font-semibold text-white">Notifications</span>
+            <button
+              type="button"
+              className="text-xs font-medium text-mineshaft-300 hover:text-primary-400 disabled:pointer-events-none disabled:opacity-50"
+              onClick={(e) => {
+                e.preventDefault();
+                markAllAsRead();
+              }}
+              disabled={unreadCount === 0}
+            >
+              Mark all as read
+            </button>
+          </div>
+          <div className="flex h-full w-full overflow-auto">
+            {isLoading && (
+              <div className="flex h-full w-full items-center justify-center">
+                <ContentLoader className="pointer-events-none" lottieClassName="size-10" />
+              </div>
+            )}
+            {!isLoading && notifications?.length === 0 && (
+              <div className="flex h-full w-full flex-col items-center justify-center">
+                <FontAwesomeIcon icon={faBell} size="3x" className="text-mineshaft-400" />
+                <span className="mt-4 text-sm text-mineshaft-300">No new notifications</span>
+                <span className="text-xs text-mineshaft-400">
+                  We&apos;ll let you know when something important happens.
+                </span>
+              </div>
+            )}
+            {!isLoading && notifications && notifications.length > 0 && (
+              <div className="flex w-full flex-col">
+                {notifications.map((notification) => (
+                  <div
+                    role="button"
+                    tabIndex={0}
+                    key={notification.id}
+                    onClick={() => {
+                      if (!notification.isRead) {
+                        updateNotification({ notificationId: notification.id, isRead: true });
+                      }
+                      if (notification.link) {
+                        router.navigate({ to: notification.link });
+                      }
+                    }}
+                    onKeyDown={(e) => {
+                      if (e.key !== "Enter") return;
+                      if (!notification.isRead) {
+                        updateNotification({ notificationId: notification.id, isRead: true });
+                      }
+                      if (notification.link) {
+                        router.navigate({ to: notification.link });
+                      }
+                    }}
+                  >
+                    <Notification notification={notification} onDelete={deleteNotification} />
+                  </div>
+                ))}
+              </div>
+            )}
+          </div>
+        </div>
+      </DropdownMenuContent>
+    </DropdownMenu>
+  );
+};