Refactor withMembershipScope

* Add script to resolve existing collection index collisions

* Remove debug logging

---------

Co-authored-by: Tom Moor <tom.moor@gmail.com>

plugins/notion/server/utils/NotionConverter.ts

@@ -288,7 +288,7 @@ export class NotionConverter {
       if (item.mention.type === "link_mention") {
         return {
           type: "text",
-          text: item.plain_text,
+          text: item.plain_text || item.mention.link_mention.href,
           marks: [
             {
               type: "link",
@@ -302,7 +302,7 @@ export class NotionConverter {
       if (item.mention.type === "link_preview") {
         return {
           type: "text",
-          text: item.plain_text,
+          text: item.plain_text || item.mention.link_preview.url,
           marks: [
             {
               type: "link",
@@ -314,14 +314,14 @@ export class NotionConverter {
         };
       }
 
-      if (!item.plain_text) {
-        return undefined;
+      if (item.plain_text) {
+        return {
+          type: "text",
+          text: item.plain_text,
+        };
       }
 
-      return {
-        type: "text",
-        text: item.plain_text,
-      };
+      return undefined;
     }
 
     if (item.type === "equation") {
@@ -336,20 +336,20 @@ export class NotionConverter {
       };
     }
 
-    if (!item.text.content) {
-      return undefined;
+    if (item.text.content) {
+      return {
+        type: "text",
+        text: item.text.content,
+        marks: [
+          ...mapAttrs(),
+          ...(item.text.link
+            ? [{ type: "link", attrs: { href: item.text.link.url } }]
+            : []),
+        ].filter(Boolean),
+      };
     }
 
-    return {
-      type: "text",
-      text: item.text.content,
-      marks: [
-        ...mapAttrs(),
-        ...(item.text.link
-          ? [{ type: "link", attrs: { href: item.text.link.url } }]
-          : []),
-      ].filter(Boolean),
-    };
+    return undefined;
   }
 
   private static rich_text_to_plaintext(item: RichTextItemResponse) {

server/migrations/20250327062414-resolve-collection-index-collisions.js

@@ -0,0 +1,29 @@
+"use strict";
+
+const { execFileSync } = require("child_process");
+const path = require("path");
+
+/** @type {import('sequelize-cli').Migration} */
+module.exports = {
+  async up() {
+    if (
+      process.env.NODE_ENV === "test" ||
+      process.env.DEPLOYMENT === "hosted"
+    ) {
+      return;
+    }
+
+    const scriptName = path.basename(__filename);
+    const scriptPath = path.join(
+      process.cwd(),
+      "build",
+      `server/scripts/${scriptName}`
+    );
+
+    execFileSync("node", [scriptPath], { stdio: "inherit" });
+  },
+
+  async down() {
+    // noop
+  },
+};

server/models/Document.ts

@@ -632,9 +632,13 @@ class Document extends ArchivableModel<
     return uniq(membershipUserIds);
   }
 
-  static withMembershipScope(userId: string, options?: FindOptions<Document>) {
+  static withMembershipScope(
+    userId: string,
+    options?: FindOptions<Document> & { includeDrafts?: boolean }
+  ) {
     return this.scope([
-      "defaultScope",
+      options?.includeDrafts ? "withDrafts" : "defaultScope",
+      "withoutState",
       {
         method: ["withViews", userId],
       },

server/models/helpers/SearchHelper.ts

@@ -182,16 +182,16 @@ export default class SearchHelper {
       },
     ];
 
-    return Document.withMembershipScope(user.id)
-      .scope("withDrafts")
-      .findAll({
-        where,
-        subQuery: false,
-        order: [["updatedAt", "DESC"]],
-        include,
-        offset,
-        limit,
-      });
+    return Document.withMembershipScope(user.id, {
+      includeDrafts: true,
+    }).findAll({
+      where,
+      subQuery: false,
+      order: [["updatedAt", "DESC"]],
+      include,
+      offset,
+      limit,
+    });
   }
 
   public static async searchCollectionsForUser(
@@ -264,14 +264,12 @@ export default class SearchHelper {
 
       // Final query to get associated document data
       const [documents, count] = await Promise.all([
-        Document.withMembershipScope(user.id)
-          .scope("withDrafts")
-          .findAll({
-            where: {
-              teamId: user.teamId,
-              id: map(results, "id"),
-            },
-          }),
+        Document.withMembershipScope(user.id, { includeDrafts: true }).findAll({
+          where: {
+            teamId: user.teamId,
+            id: map(results, "id"),
+          },
+        }),
         results.length < limit && offset === 0
           ? Promise.resolve(results.length)
           : countQuery,

server/routes/api/documents/documents.ts

@@ -535,14 +535,14 @@ router.post(
       delete where.updatedAt;
     }
 
-    const documents = await Document.withMembershipScope(user.id)
-      .scope("withDrafts")
-      .findAll({
-        where,
-        order: [[sort, direction]],
-        offset: ctx.state.pagination.offset,
-        limit: ctx.state.pagination.limit,
-      });
+    const documents = await Document.withMembershipScope(user.id, {
+      includeDrafts: true,
+    }).findAll({
+      where,
+      order: [[sort, direction]],
+      offset: ctx.state.pagination.offset,
+      limit: ctx.state.pagination.limit,
+    });
     const data = await Promise.all(
       documents.map((document) => presentDocument(ctx, document))
     );

server/routes/api/groupMemberships/groupMemberships.ts

@@ -58,13 +58,13 @@ router.post(
     const documentIds = memberships
       .map((p) => p.documentId)
       .filter(Boolean) as string[];
-    const documents = await Document.withMembershipScope(userId)
-      .scope("withDrafts")
-      .findAll({
-        where: {
-          id: documentIds,
-        },
-      });
+    const documents = await Document.withMembershipScope(userId, {
+      includeDrafts: true,
+    }).findAll({
+      where: {
+        id: documentIds,
+      },
+    });
 
     const groups = uniqBy(
       memberships.map((membership) => membership.group),

server/scripts/20250327062414-resolve-collection-index-collisions.ts

@@ -0,0 +1,127 @@
+import "./bootstrap";
+import fractionalIndex from "fractional-index";
+import { Sequelize, Transaction } from "sequelize";
+import { Collection, Team } from "@server/models";
+import { sequelize } from "@server/storage/database";
+
+const limit = 100;
+
+class CollectionIndexCollisionResolver {
+  private teamId: string;
+  private currDuplicateIndex: string | null = null;
+  private currDuplicateGroup: Collection[] = [];
+  private resolvedCollisionsCount: number = 0;
+
+  constructor(teamId: string) {
+    this.teamId = teamId;
+  }
+
+  public async process() {
+    await sequelize.transaction(async (transaction) => {
+      await this.processPage(0, transaction);
+      // edge case of last batch
+      await this.resolveDuplicates({ transaction });
+    });
+  }
+
+  private async processPage(
+    page: number,
+    transaction: Transaction
+  ): Promise<void> {
+    console.log(
+      `Resolve collection index collisions for team ${this.teamId}… page ${page}`
+    );
+
+    const collections = await Collection.unscoped().findAll({
+      where: { teamId: this.teamId },
+      attributes: ["id", "index"],
+      limit,
+      offset: page * limit,
+      order: [
+        Sequelize.literal('"collection"."index" collate "C"'), // ensure duplicates are in sequential order
+        ["updatedAt", "DESC"], // fallback as a tie breaker
+      ],
+      lock: Transaction.LOCK.UPDATE,
+      transaction,
+    });
+
+    if (!collections.length) {
+      return;
+    }
+
+    let idx = 0;
+
+    while (idx < collections.length) {
+      const collection = collections[idx];
+
+      if (collection.index === this.currDuplicateIndex) {
+        // still in the same duplicate group.
+        this.currDuplicateGroup.push(collection);
+      } else {
+        // current collection index is different from the previous one; resolve duplicates, if applicable.
+        await this.resolveDuplicates({
+          nextCollection: collection,
+          transaction,
+        });
+        // reset the duplicate index and group.
+        this.currDuplicateIndex = collection.index;
+        this.currDuplicateGroup = [collection];
+      }
+
+      idx++;
+    }
+
+    return collections.length === limit
+      ? this.processPage(page + 1, transaction)
+      : undefined;
+  }
+
+  private async resolveDuplicates({
+    nextCollection,
+    transaction,
+  }: {
+    nextCollection?: Collection;
+    transaction: Transaction;
+  }) {
+    if (this.currDuplicateGroup.length <= 1) {
+      // no action needed when there aren't more than 1 item in a group.
+      return;
+    }
+
+    let prevIndex = this.currDuplicateGroup[0].index;
+    const endIndex = nextCollection?.index ?? null;
+
+    // First collection in a duplicate group can retain its index.
+    for (let idx = 1; idx < this.currDuplicateGroup.length; idx++) {
+      const collection = this.currDuplicateGroup[idx];
+      const newIndex = fractionalIndex(prevIndex, endIndex);
+
+      console.log(`New index for collection ${collection.id} = ${newIndex}`);
+
+      collection.index = newIndex;
+      await collection.save({ silent: true, hooks: false, transaction });
+
+      prevIndex = newIndex;
+    }
+
+    this.resolvedCollisionsCount += this.currDuplicateGroup.length - 1;
+  }
+}
+
+export default async function main(exit = false) {
+  await Team.findAllInBatches<Team>({ batchLimit: 5 }, async (teams) => {
+    for (const team of teams) {
+      const resolver = new CollectionIndexCollisionResolver(team.id);
+      await resolver.process();
+    }
+  });
+
+  if (exit) {
+    process.exit(0);
+  }
+}
+
+// In the test suite we import the script rather than run via node CLI
+if (process.env.NODE_ENV !== "test") {
+  void main(true);
+}

server/test/fixtures/notion-page-with-empty-text-nodes.json

@@ -191,7 +191,7 @@
             "code": false,
             "color": "default"
           },
-          "plain_text": "http://github.com/outline/",
+          "plain_text": "",
           "href": "http://github.com/outline/"
         }
       ],
@@ -506,4 +506,4 @@
       "color": "default"
     }
   }
-]
+]

shared/editor/queries/table.ts

@@ -6,6 +6,30 @@ import {
   selectedRect,
 } from "prosemirror-tables";
 
+/**
+ * Checks if the current selection is a column selection.
+ * @param state The editor state.
+ * @returns True if the selection is a column selection, false otherwise.
+ */
+export function isColSelection(state: EditorState): boolean {
+  if (state.selection instanceof CellSelection) {
+    return state.selection.isColSelection();
+  }
+  return false;
+}
+
+/**
+ * Checks if the current selection is a row selection.
+ * @param state The editor state.
+ * @returns True if the selection is a row selection, false otherwise.
+ */
+export function isRowSelection(state: EditorState): boolean {
+  if (state.selection instanceof CellSelection) {
+    return state.selection.isRowSelection();
+  }
+  return false;
+}
+
 export function getColumnIndex(state: EditorState): number | undefined {
   if (state.selection instanceof CellSelection) {
     if (state.selection.isColSelection()) {
@@ -62,13 +86,18 @@ export function getCellsInRow(index: number) {
   };
 }
 
+/**
+ * Check if a specific column is selected in the editor.
+ *
+ * @param state The editor state
+ * @param index The index of the column to check
+ * @returns Boolean indicating if the column is selected
+ */
 export function isColumnSelected(index: number) {
   return (state: EditorState): boolean => {
-    if (state.selection instanceof CellSelection) {
-      if (state.selection.isColSelection()) {
-        const rect = selectedRect(state);
-        return rect.left <= index && rect.right > index;
-      }
+    if (isColSelection(state)) {
+      const rect = selectedRect(state);
+      return rect.left <= index && rect.right > index;
     }
 
     return false;
@@ -106,28 +135,42 @@ export function isHeaderEnabled(
   return true;
 }
 
+/**
+ * Check if a specific row is selected in the editor.
+ *
+ * @param state The editor state
+ * @param index The index of the row to check
+ * @returns Boolean indicating if the row is selected
+ */
 export function isRowSelected(index: number) {
   return (state: EditorState): boolean => {
-    if (state.selection instanceof CellSelection) {
-      if (state.selection.isRowSelection()) {
-        const rect = selectedRect(state);
-        return rect.top <= index && rect.bottom > index;
-      }
+    if (isRowSelection(state)) {
+      const rect = selectedRect(state);
+      return rect.top <= index && rect.bottom > index;
     }
 
     return false;
   };
 }
 
+/**
+ * Check if an entire table is selected in the editor.
+ *
+ * @param state The editor state
+ * @returns Boolean indicating if the table is selected
+ */
 export function isTableSelected(state: EditorState): boolean {
-  const rect = selectedRect(state);
+  if (state.selection instanceof CellSelection) {
+    const rect = selectedRect(state);
 
-  return (
-    rect.top === 0 &&
-    rect.left === 0 &&
-    rect.bottom === rect.map.height &&
-    rect.right === rect.map.width &&
-    !state.selection.empty &&
-    state.selection instanceof CellSelection
-  );
+    return (
+      rect.top === 0 &&
+      rect.left === 0 &&
+      rect.bottom === rect.map.height &&
+      rect.right === rect.map.width &&
+      !state.selection.empty
+    );
+  }
+
+  return false;
 }