fix(sync): Support changing destination Notion database (#604)

* refactor: Rename `syncNote` to `syncNoteItem`

* refactor: Extract `syncRegularItem` to its own file

* refactor: Replace `SyncJob` class with functional approach

* refactor: Move `ItemSyncError` class to `errors` directory

* chore(zed): Add tasks for testing individual file and symbol

* chore(zed): Simplify tasks by removing `args` field

* refactor: Extract `normalizeID` Notion utility function

* fix(sync): Recreate Notion pages when found in a different database

* fix(sync): Reset synced notes when page ID changes

Fixes #241

.zed/tasks.json

@@ -2,9 +2,9 @@
 [
   {
     "label": "Build",
-    "command": "npm",
+    "command": "npm run build",
     // rest of the parameters are optional
-    "args": ["run", "build"],
+    // "args": [],
     // Env overrides for the command, will be appended to the terminal's environment from the settings.
     // "env": { "foo": "bar" },
     // Current working directory to spawn the command into, defaults to current project root.
@@ -20,38 +20,45 @@
   },
   {
     "label": "Build with source maps",
-    "command": "npm",
-    "args": ["run", "build", "--", "--sourcemap"],
+    "command": "npm run build -- --sourcemap",
     "reveal": "never"
   },
   {
     "label": "Generate Fluent types",
-    "command": "npm",
-    "args": ["run", "generate-fluent-types"],
+    "command": "npm run generate-fluent-types",
     "reveal": "never"
   },
   {
     "label": "Start Zotero",
-    "command": "npm",
-    "args": ["run", "start"],
+    "command": "npm run start",
     "reveal": "never"
   },
   {
     "label": "Start Zotero Beta",
-    "command": "npm",
-    "args": ["run", "start:beta"],
+    "command": "npm run start:beta",
     "reveal": "never"
   },
   {
-    "label": "Test",
-    "command": "npm",
-    "args": ["run", "test"],
+    "label": "Test all (once)",
+    "command": "npm run test",
     "reveal": "always"
   },
   {
-    "label": "Test watch",
-    "command": "npm",
-    "args": ["run", "test:watch"],
+    "label": "Test all (watch)",
+    "command": "npm run test:watch",
     "reveal": "always"
+  },
+  {
+    "label": "Test $ZED_STEM",
+    "command": "npx vitest related",
+    "args": ["\"$ZED_RELATIVE_FILE\""],
+    "reveal": "always"
+  },
+  {
+    "label": "Test $ZED_SYMBOL",
+    "command": "npx vitest",
+    "args": ["\"$ZED_RELATIVE_FILE\"", "--testNamePattern", "\"$ZED_SYMBOL\""],
+    "reveal": "always",
+    "tags": ["ts-test", "tsx-test"]
   }
 ]

src/content/data/__tests__/item-data.spec.ts

@@ -1,7 +1,11 @@
 import { describe, expect, it } from 'vitest';
 
-import { createZoteroItemMock } from '../../../../test/utils';
-import { getSyncedNotesFromAttachment } from '../item-data';
+import { createZoteroItemMock, mockZoteroPrefs } from '../../../../test/utils';
+import { NoteroPref, setNoteroPref } from '../../prefs/notero-pref';
+import {
+  getSyncedNotesFromAttachment,
+  saveNotionLinkAttachment,
+} from '../item-data';
 
 describe('getSyncedNotesFromAttachment', () => {
   it('loads expected data when synced notes are saved in original format', () => {
@@ -50,3 +54,49 @@ describe('getSyncedNotesFromAttachment', () => {
     });
   });
 });
+
+describe('saveNotionLinkAttachment', () => {
+  it('preserves synced notes when page ID does not change', async () => {
+    mockZoteroPrefs();
+    setNoteroPref(NoteroPref.syncNotes, true);
+    const pageURL =
+      'notion://www.notion.so/page-00000000000000000000000000000000';
+    const syncedNotes =
+      '<pre id="notero-synced-notes">{"existing":"notes"}</pre>';
+    const item = createZoteroItemMock();
+    const attachment = createZoteroItemMock();
+    item.getAttachments.mockReturnValue([attachment.id]);
+    attachment.getField.calledWith('url').mockReturnValue(pageURL);
+    attachment.getNote.mockReturnValue(syncedNotes);
+
+    await saveNotionLinkAttachment(item, pageURL);
+
+    // eslint-disable-next-line @typescript-eslint/unbound-method
+    expect(attachment.setNote).toHaveBeenCalledWith(
+      expect.stringContaining(syncedNotes),
+    );
+  });
+
+  it('resets synced notes when page ID changes', async () => {
+    mockZoteroPrefs();
+    setNoteroPref(NoteroPref.syncNotes, true);
+    const oldPageURL =
+      'notion://www.notion.so/old-page-00000000000000000000000000000000';
+    const newPageURL =
+      'notion://www.notion.so/new-page-77777777777777777777777777777777';
+    const syncedNotes =
+      '<pre id="notero-synced-notes">{"existing":"notes"}</pre>';
+    const item = createZoteroItemMock();
+    const attachment = createZoteroItemMock();
+    item.getAttachments.mockReturnValue([attachment.id]);
+    attachment.getField.calledWith('url').mockReturnValue(oldPageURL);
+    attachment.getNote.mockReturnValue(syncedNotes);
+
+    await saveNotionLinkAttachment(item, newPageURL);
+
+    // eslint-disable-next-line @typescript-eslint/unbound-method
+    expect(attachment.setNote).toHaveBeenCalledWith(
+      expect.stringContaining('<pre id="notero-synced-notes">{}</pre>'),
+    );
+  });
+});

src/content/data/item-data.ts

@@ -49,9 +49,13 @@ export async function saveNotionLinkAttachment(
     await Zotero.Items.erase(attachmentIDs);
   }
 
-  let attachment = attachments.length ? attachments[0] : null;
+  let attachment = attachments[0];
+  let pageIDChanged = false;
 
   if (attachment) {
+    const currentURL = attachment.getField('url');
+    pageIDChanged =
+      !currentURL || getPageIDFromURL(currentURL) !== getPageIDFromURL(appURL);
     attachment.setField('url', appURL);
   } else {
     attachment = await Zotero.Attachments.linkFromURL({
@@ -64,7 +68,8 @@ export async function saveNotionLinkAttachment(
     });
   }
 
-  updateNotionLinkAttachmentNote(attachment);
+  const syncedNotes = pageIDChanged ? {} : undefined;
+  updateNotionLinkAttachmentNote(attachment, syncedNotes);
 
   await attachment.saveTx();
 }
@@ -157,7 +162,7 @@ export async function saveSyncedNote(
 
 function updateNotionLinkAttachmentNote(
   attachment: Zotero.Item,
-  syncedNotes?: Required<SyncedNotes>,
+  syncedNotes?: SyncedNotes,
 ) {
   let note = `
 <h2 style="background-color: #ff666680;">Do not modify or delete!</h2>

src/content/errors/ItemSyncError.ts

@@ -0,0 +1,11 @@
+export class ItemSyncError extends Error {
+  public readonly item: Zotero.Item;
+  public readonly name = 'ItemSyncError';
+
+  public constructor(cause: unknown, item: Zotero.Item) {
+    super(`Failed to sync item with ID ${item.id} due to ${String(cause)}`, {
+      cause,
+    });
+    this.item = item;
+  }
+}

src/content/errors/index.ts

@@ -1,2 +1,3 @@
+export { ItemSyncError } from './ItemSyncError';
 export { LocalizableError } from './LocalizableError';
 export { MissingPrefError } from './MissingPrefError';

src/content/prefs/preferences.tsx

@@ -7,6 +7,7 @@ import type { createRoot } from 'react-dom/client';
 import { FluentMessageId } from '../../locale/fluent-types';
 import { LocalizableError } from '../errors';
 import { getNotionClient } from '../sync/notion-client';
+import { normalizeID } from '../sync/notion-utils';
 import {
   createXULElement,
   getLocalizedErrorMessage,
@@ -157,14 +158,13 @@ class Preferences {
       const databases = await this.retrieveNotionDatabases();
 
       menuItems = databases.map<MenuItem>((database) => {
-        const idWithoutDashes = database.id.replace(/-/g, '');
         const title = database.title.map((t) => t.plain_text).join('');
         const icon =
           database.icon?.type === 'emoji' ? database.icon.emoji : null;
 
         return {
           label: icon ? `${icon} ${title}` : title,
-          value: idWithoutDashes,
+          value: normalizeID(database.id),
         };
       });
 

src/content/sync/__tests__/sync-note.spec.ts → src/content/sync/__tests__/sync-note-item.spec.ts

@@ -1,7 +1,7 @@
 import { APIErrorCode, APIResponseError, type Client } from '@notionhq/client';
-import {
+import type {
+  AppendBlockChildrenResponse,
   PartialBlockObjectResponse,
-  type AppendBlockChildrenResponse,
 } from '@notionhq/client/build/src/api-endpoints';
 import { describe, expect, it, vi } from 'vitest';
 import { mockDeep, objectContainsValue } from 'vitest-mock-extended';
@@ -13,7 +13,7 @@ import {
   getSyncedNotes,
   saveSyncedNote,
 } from '../../data/item-data';
-import { syncNote } from '../sync-note';
+import { syncNoteItem } from '../sync-note-item';
 
 vi.mock('../../data/item-data');
 
@@ -74,13 +74,13 @@ function setup({ syncedNotes }: { syncedNotes: SyncedNotes }) {
   return { noteItem, notion, regularItem };
 }
 
-describe('syncNote', () => {
+describe('syncNoteItem', () => {
   it('throws an error when note has no parent', async () => {
     const { noteItem, notion } = setup({ syncedNotes: {} });
     noteItem.isTopLevelItem.mockReturnValue(true);
     noteItem.topLevelItem = noteItem;
 
-    await expect(syncNote(notion, noteItem)).rejects.toThrow(
+    await expect(() => syncNoteItem(noteItem, notion)).rejects.toThrow(
       'Cannot sync note without a parent item',
     );
   });
@@ -89,15 +89,15 @@ describe('syncNote', () => {
     const { noteItem, notion } = setup({ syncedNotes: {} });
     vi.mocked(getNotionPageID).mockReturnValue(undefined);
 
-    await expect(syncNote(notion, noteItem)).rejects.toThrow(
+    await expect(() => syncNoteItem(noteItem, notion)).rejects.toThrow(
       'Cannot sync note because its parent item is not synced',
     );
   });
 
   it('creates a container block when item does not already have one', async () => {
     const { noteItem, notion } = setup({ syncedNotes: {} });
 
-    await syncNote(notion, noteItem);
+    await syncNoteItem(noteItem, notion);
 
     expect(notion.blocks.children.append).toHaveBeenCalledWith({
       block_id: fakePageID,
@@ -108,7 +108,7 @@ describe('syncNote', () => {
   it('saves the containerBlockID to the regular item', async () => {
     const { noteItem, notion, regularItem } = setup({ syncedNotes: {} });
 
-    await syncNote(notion, noteItem);
+    await syncNoteItem(noteItem, notion);
 
     expect(saveSyncedNote).toHaveBeenCalledWith(
       regularItem,
@@ -124,7 +124,7 @@ describe('syncNote', () => {
         syncedNotes: { containerBlockID: fakeContainerID },
       });
 
-      await syncNote(notion, noteItem);
+      await syncNoteItem(noteItem, notion);
 
       expect(notion.blocks.children.append).not.toHaveBeenCalledWith({
         block_id: fakePageID,
@@ -142,7 +142,7 @@ describe('syncNote', () => {
         .mockRejectedValueOnce(objectNotFoundError)
         .mockResolvedValueOnce(createResponseMock({ id: fakeNoteBlockID }));
 
-      await syncNote(notion, noteItem);
+      await syncNoteItem(noteItem, notion);
 
       expect(notion.blocks.children.append).toHaveBeenCalledWith({
         block_id: fakePageID,
@@ -160,7 +160,7 @@ describe('syncNote', () => {
       .calledWith(objectContainsValue(fakeContainerID))
       .mockRejectedValue(new Error('Failed to append children'));
 
-    await expect(() => syncNote(notion, noteItem)).rejects.toThrow();
+    await expect(() => syncNoteItem(noteItem, notion)).rejects.toThrow();
 
     expect(saveSyncedNote).toHaveBeenCalledWith(
       regularItem,
@@ -179,7 +179,7 @@ describe('syncNote', () => {
       .calledWith(objectContainsValue(fakeNoteBlockID))
       .mockRejectedValue(new Error('Failed to append children'));
 
-    await expect(() => syncNote(notion, noteItem)).rejects.toThrow();
+    await expect(() => syncNoteItem(noteItem, notion)).rejects.toThrow();
 
     expect(saveSyncedNote).toHaveBeenCalledWith(
       regularItem,