fix: Users subscribed to document and collection may be notified twice

fix: Create notifications in transaction

app/components/Sidebar/components/DocumentLink.tsx

@@ -148,7 +148,12 @@ function InnerDocumentLink(
   const color = document?.color || node.color;
 
   // Draggable
-  const [{ isDragging }, drag] = useDragDocument(node, depth, document);
+  const [{ isDragging }, drag] = useDragDocument(
+    node,
+    depth,
+    document,
+    isEditing
+  );
 
   // Drop to re-parent
   const parentRef = React.useRef<HTMLDivElement>(null);
@@ -270,6 +275,8 @@ function InnerDocumentLink(
           <div ref={dropToReparent}>
             <DropToImport documentId={node.id} activeClassName="activeDropZone">
               <SidebarLink
+                // @ts-expect-error react-router type is wrong, string component is fine.
+                component={isEditing ? "div" : undefined}
                 expanded={hasChildren ? isExpanded : undefined}
                 onDisclosureClick={handleDisclosureClick}
                 onClickIntent={handlePrefetch}
@@ -285,6 +292,7 @@ function InnerDocumentLink(
                   <EditableTitle
                     title={title}
                     onSubmit={handleTitleChange}
+                    isEditing={isEditing}
                     onEditing={setIsEditing}
                     canUpdate={canUpdate}
                     maxLength={DocumentValidation.maxTitleLength}

app/components/Sidebar/components/NavLink.tsx

@@ -39,6 +39,7 @@ export interface Props extends React.AnchorHTMLAttributes<HTMLAnchorElement> {
   location?: Location;
   strict?: boolean;
   to: LocationDescriptor;
+  component?: React.ComponentType;
   onBeforeClick?: () => void;
 }
 
@@ -146,17 +147,22 @@ const NavLink = ({
     setPreActive(undefined);
   }, [currentLocation]);
 
+  const handleKeyDown = React.useCallback(
+    (event: React.KeyboardEvent<HTMLAnchorElement>) => {
+      if (["Enter", " "].includes(event.key)) {
+        navigateTo();
+        event.currentTarget?.blur();
+      }
+    },
+    [navigateTo]
+  );
+
   return (
     <Link
       key={isActive ? "active" : "inactive"}
       ref={linkRef}
       onClick={handleClick}
-      onKeyDown={(event) => {
-        if (["Enter", " "].includes(event.key)) {
-          navigateTo();
-          event.currentTarget?.blur();
-        }
-      }}
+      onKeyDown={handleKeyDown}
       aria-current={(isActive && ariaCurrent) || undefined}
       className={className}
       style={style}

app/components/Sidebar/hooks/useDragAndDrop.tsx

@@ -166,11 +166,13 @@ export function useDropToReorderStar(getIndex?: () => string) {
  * @param node The NavigationNode model to drag.
  * @param depth The depth of the node in the sidebar.
  * @param document The related Document model.
+ * @param isEditing Whether the sidebar item is currently being edited.
  */
 export function useDragDocument(
   node: NavigationNode,
   depth: number,
-  document?: Document
+  document?: Document,
+  isEditing?: boolean
 ) {
   const icon = document?.icon || node.icon || node.emoji;
   const color = document?.color || node.color;
@@ -188,7 +190,7 @@ export function useDragDocument(
         icon: icon ? <Icon value={icon} color={color} /> : undefined,
         collectionId: document?.collectionId || "",
       } as DragObject),
-    canDrag: () => !!document?.isActive,
+    canDrag: () => !!document?.isActive && !isEditing,
     collect: (monitor) => ({
       isDragging: monitor.isDragging(),
     }),

plugins/github/server/github.ts

@@ -206,12 +206,12 @@ export class GitHub {
       );
       const { data } = await client.requestResource(resource);
       if (!data) {
-        return;
+        return { error: "Resource not found" };
       }
       return { ...data, type: resource.type };
     } catch (err) {
       Logger.warn("Failed to fetch resource from GitHub", err);
-      return;
+      return { error: err.message || "Unknown error" };
     }
   };
 }

plugins/iframely/server/iframely.ts

@@ -1,6 +1,6 @@
 import { JSONObject, UnfurlResourceType } from "@shared/types";
 import Logger from "@server/logging/Logger";
-import { UnfurlSignature } from "@server/types";
+import { UnfurlError, UnfurlSignature } from "@server/types";
 import fetch from "@server/utils/fetch";
 import env from "./env";
 
@@ -10,7 +10,7 @@ class Iframely {
   public static async requestResource(
     url: string,
     type = "oembed"
-  ): Promise<JSONObject | undefined> {
+  ): Promise<JSONObject | UnfurlError> {
     const isDefaultHost = env.IFRAMELY_URL === this.defaultUrl;
 
     // Cloud Iframely requires /api path, while self-hosted does not.
@@ -25,7 +25,7 @@ class Iframely {
       return await res.json();
     } catch (err) {
       Logger.error(`Error fetching data from Iframely for url: ${url}`, err);
-      return;
+      return { error: err.message || "Unknown error" };
     }
   }
 
@@ -36,7 +36,9 @@ class Iframely {
    */
   public static unfurl: UnfurlSignature = async (url: string) => {
     const data = await Iframely.requestResource(url);
-    return { ...data, type: UnfurlResourceType.OEmbed };
+    return "error" in data // In addition to our custom UnfurlError, sometimes iframely returns error in the response body.
+      ? ({ error: data.error } as UnfurlError)
+      : { ...data, type: UnfurlResourceType.OEmbed };
   };
 }
 

server/models/helpers/NotificationHelper.test.ts

@@ -251,6 +251,10 @@ describe("NotificationHelper", () => {
         userId: subscribedUser.id,
         collectionId: document.collectionId!,
       });
+      await buildSubscription({
+        userId: subscribedUser.id,
+        documentId: document.id,
+      });
 
       const recipients =
         await NotificationHelper.getDocumentNotificationRecipients({

server/models/helpers/NotificationHelper.ts

@@ -1,4 +1,5 @@
 import uniq from "lodash/uniq";
+import uniqBy from "lodash/uniqBy";
 import { Op } from "sequelize";
 import {
   NotificationEventType,
@@ -187,7 +188,6 @@ export default class NotificationHelper {
       });
     } else {
       const subscriptions = await Subscription.findAll({
-        attributes: ["userId"],
         where: {
           userId: {
             [Op.ne]: actorId,
@@ -206,17 +206,19 @@ export default class NotificationHelper {
         ],
       });
 
-      recipients = subscriptions.map((s) => s.user);
+      recipients = uniqBy(
+        subscriptions.map((s) => s.user),
+        (user) => user.id
+      );
     }
 
-    recipients = recipients.filter((recipient) =>
-      recipient.subscribedToEventType(notificationType)
-    );
-
     const filtered = [];
 
     for (const recipient of recipients) {
-      if (recipient.isSuspended) {
+      if (
+        recipient.isSuspended ||
+        !recipient.subscribedToEventType(notificationType)
+      ) {
         continue;
       }
 

server/queues/tasks/CommentCreatedNotificationsTask.ts

@@ -85,16 +85,21 @@ export default class CommentCreatedNotificationsTask extends BaseTask<CommentEve
       )
     ).filter((recipient) => !userIdsMentioned.includes(recipient.id));
 
-    for (const recipient of recipients) {
-      await Notification.create({
-        event: NotificationEventType.CreateComment,
-        userId: recipient.id,
-        actorId: comment.createdById,
-        teamId: document.teamId,
-        commentId: comment.id,
-        documentId: document.id,
-      });
-    }
+    await sequelize.transaction(async (transaction) => {
+      for (const recipient of recipients) {
+        await Notification.create(
+          {
+            event: NotificationEventType.CreateComment,
+            userId: recipient.id,
+            actorId: comment.createdById,
+            teamId: document.teamId,
+            commentId: comment.id,
+            documentId: document.id,
+          },
+          { transaction }
+        );
+      }
+    });
   }
 
   public get options() {

server/routes/api/urls/urls.test.ts

@@ -20,7 +20,7 @@ jest.mock("dns", () => ({
 
 jest
   .spyOn(Iframely, "requestResource")
-  .mockImplementation(() => Promise.resolve(undefined));
+  .mockImplementation(() => Promise.resolve({}));
 
 const server = getTestServer();
 

server/routes/api/urls/urls.ts

@@ -98,11 +98,12 @@ router.post(
     }
 
     for (const plugin of plugins) {
-      const data = await plugin.value.unfurl(url, actor);
-      if (data) {
-        if ("error" in data) {
+      const unfurl = await plugin.value.unfurl(url, actor);
+      if (unfurl) {
+        if (unfurl.error) {
           return (ctx.response.status = 204);
         } else {
+          const data = unfurl as Unfurl;
           await CacheHelper.setData(
             CacheHelper.getUnfurlKey(actor.teamId, url),
             data,

server/types.ts

@@ -578,10 +578,12 @@ export type CollectionJSONExport = {
 
 export type Unfurl = { [x: string]: JSONValue; type: UnfurlResourceType };
 
+export type UnfurlError = { error: string };
+
 export type UnfurlSignature = (
   url: string,
   actor?: User
-) => Promise<Unfurl | void>;
+) => Promise<Unfurl | UnfurlError | undefined>;
 
 export type UninstallSignature = (integration: Integration) => Promise<void>;
 

shared/editor/lib/code.ts

@@ -279,7 +279,7 @@ export const codeLanguages: Record<string, CodeLanguage> = {
     loader: () => import("refractor/lang/typescript").then((m) => m.default),
   },
   vb: {
-    lang: "vb",
+    lang: "vbnet",
     label: "Visual Basic",
     loader: () => import("refractor/lang/vbnet").then((m) => m.default),
   },