Skip to content
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.

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

[sync] Add syncing with link to relation #633

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
6,767 changes: 6,767 additions & 0 deletions pnpm-lock.yaml

Large diffs are not rendered by default.

10 changes: 10 additions & 0 deletions scripts/build.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,16 @@ const builds: (BuildOptions & { entryPoints: [string] })[] = [
sourcemap,
target,
},
{
bundle: true,
entryPoints: ['src/content/prefs/dialog.tsx'],
external: ['components/*', 'react', 'react-dom'],
format: 'iife',
globalName: 'notero',
outdir: 'build/content/prefs',
sourcemap,
target,
},
];

Promise.all(
Expand Down
10 changes: 10 additions & 0 deletions shell.nix
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{ pkgs ? import <nixpkgs> {} }:

pkgs.mkShell {

buildInputs = [
pkgs.nodejs
pkgs.pnpm
];

}
11 changes: 11 additions & 0 deletions src/bootstrap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ function log(msg: string) {
Zotero.debug(`${LOG_PREFIX}${msg}`);
Zotero.log(`${LOG_PREFIX}${msg}`, 'info');
}
// eslint-disable-next-line @typescript-eslint/no-unused-vars
let chromeHandle;

/**
*
Expand Down Expand Up @@ -40,7 +42,16 @@ async function startup(
) {
log(`Starting v${version}`);

Zotero.log(rootURI);
Services.scriptloader.loadSubScript(rootURI + 'content/notero.js');
// @ts-expect-error - `Zotero.Notero` is not defined in the Zotero namespace
const aomStartup = Components.classes[
'@mozilla.org/addons/addon-manager-startup;1'
].getService(Components.interfaces.amIAddonManagerStartup);
const manifestURI = Services.io.newURI(rootURI + 'manifest.json');
chromeHandle = aomStartup.registerChrome(manifestURI, [
['content', 'notero', rootURI + 'content/'],
]);
Comment on lines +47 to +54
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add error handling for chrome registration.

The chrome registration process should include error handling to ensure proper initialization.

-  const aomStartup = Components.classes[
-    '@mozilla.org/addons/addon-manager-startup;1'
-  ].getService(Components.interfaces.amIAddonManagerStartup);
-  const manifestURI = Services.io.newURI(rootURI + 'manifest.json');
-  chromeHandle = aomStartup.registerChrome(manifestURI, [
-    ['content', 'notero', rootURI + 'content/'],
-  ]);
+  try {
+    const aomStartup = Components.classes[
+      '@mozilla.org/addons/addon-manager-startup;1'
+    ].getService(Components.interfaces.amIAddonManagerStartup);
+    const manifestURI = Services.io.newURI(rootURI + 'manifest.json');
+    chromeHandle = aomStartup.registerChrome(manifestURI, [
+      ['content', 'notero', rootURI + 'content/'],
+    ]);
+    log('Chrome registration successful');
+  } catch (error) {
+    log(`Chrome registration failed: ${error}`);
+    throw error;
+  }

Committable suggestion skipped: line range outside the PR's diff.


await Zotero.Notero?.startup({ pluginID: id, rootURI, version });
}
Expand Down
1 change: 1 addition & 0 deletions src/content/prefs/collection-sync-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { getNoteroPref, NoteroPref, setNoteroPref } from './notero-pref';
export type CollectionSyncConfig = {
notionOptionID?: string;
syncEnabled: boolean;
associatedLink?: string;
};

export type CollectionSyncConfigsRecord = Record<
Expand Down
72 changes: 72 additions & 0 deletions src/content/prefs/dialog.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
import { isFullPage } from '@notionhq/client';

import { getGlobalNotero, getXULElementById } from '../utils';
import { setMenuItems } from '../utils/elements';

import { getNoteroPref, NoteroPref } from './notero-pref';

type DialogArguments = {
accepted: boolean;
associatedLink: string;
syncEnabled: boolean;
};

class Dialog {
private collectionDialogContainer!: XUL.XULElement;
private associatedLinkElement!: XUL.MenuListElement;
private syncEnabledElement!: XUL.CheckboxElement;
private params!: DialogArguments;

public async init(): Promise<void> {
await Zotero.uiReadyPromise;

/* eslint-disable @typescript-eslint/no-non-null-assertion */
this.params = window.arguments![0]! as DialogArguments;
Comment on lines +23 to +24
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider safer type assertions.

The non-null assertion on window.arguments could lead to runtime errors. Consider adding proper type checking.

-    this.params = window.arguments![0]! as DialogArguments;
+    if (!window.arguments?.[0]) {
+      throw new Error('Dialog initialized without arguments');
+    }
+    this.params = window.arguments[0] as DialogArguments;
📝 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
/* eslint-disable @typescript-eslint/no-non-null-assertion */
this.params = window.arguments![0]! as DialogArguments;
/* eslint-disable @typescript-eslint/no-non-null-assertion */
if (!window.arguments?.[0]) {
throw new Error('Dialog initialized without arguments');
}
this.params = window.arguments[0] as DialogArguments;


/* eslint-disable @typescript-eslint/no-non-null-assertion */
this.collectionDialogContainer = getXULElementById(
'notero-collection-dialog',
)!;
this.associatedLinkElement = getXULElementById('notero-associatedLink')!;

const notero = getGlobalNotero();
const pref = getNoteroPref(NoteroPref.linkedCollectionID);
if (pref) {
const client = await notero.getNotionClient();
const res = await client.databases.query({ database_id: pref });
const results = res.results
.map((result: (typeof res.results)[0]) => {
if (isFullPage(result)) {
return {
value: result.id,
label: Object.values(result.properties).find(
(prop) => prop.type === 'title',
)?.title[0]!.plain_text,
};
}
})
.filter((item) => item != undefined);
setMenuItems(this.associatedLinkElement, results);
}

this.associatedLinkElement.value = this.params.associatedLink;
this.syncEnabledElement = getXULElementById('notero-syncEnabled')!;
this.syncEnabledElement.checked = this.params.syncEnabled;
document.addEventListener('dialogaccept', () => {
return this.accept();
});
}
accept(): boolean {
// @ts-expect-error dataOut is not defined in the type
window.arguments![0]!.dataOut = {
associatedLink: this.associatedLinkElement.value,
syncEnabled: this.syncEnabledElement.checked,
accepted: true,
};
return true;
}
Comment on lines +59 to +67
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve type safety in the accept method.

The current implementation uses @ts-expect-error and unsafe type assertions. Consider extending the window type or using a safer approach.

+interface DialogWindow extends Window {
+  arguments: [{
+    dataOut?: DialogArguments;
+  } & DialogArguments];
+}
+
 accept(): boolean {
-    // @ts-expect-error dataOut is not defined in the type
-    window.arguments![0]!.dataOut = {
+    (window as DialogWindow).arguments[0].dataOut = {
       associatedLink: this.associatedLinkElement.value,
       syncEnabled: this.syncEnabledElement.checked,
       accepted: true,
     };
     return true;
   }

Committable suggestion skipped: line range outside the PR's diff.

}

module.exports = {
dialog: new Dialog(),
};
48 changes: 48 additions & 0 deletions src/content/prefs/dialog.xhtml
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
<?xml version="1.0"?>
<?xml-stylesheet href="chrome://global/skin/" type="text/css"?> <?xml-stylesheet
href="chrome://zotero/skin/zotero.css" type="text/css"?> <?xml-stylesheet
href="chrome://zotero-platform/content/zotero.css" type="text/css"?>

<window
type="child"
xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
xmlns:html="http://www.w3.org/1999/xhtml"
style="width: 100px; height: 100px"
buttons="cancel,accept"
Comment on lines +10 to +11
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Adjust window dimensions

The hardcoded window dimensions (100x100) seem too small for the content. Consider:

  1. Using more appropriate dimensions
  2. Making the window resizable
  3. Using min/max dimensions instead of fixed size
-  style="width: 100px; height: 100px"
+  style="min-width: 400px; min-height: 200px"
📝 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
style="width: 100px; height: 100px"
buttons="cancel,accept"
style="min-width: 400px; min-height: 200px"
buttons="cancel,accept"

onload="notero.dialog.init();"
>
Comment on lines +12 to +13
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Remove duplicate initialization

The notero.dialog.init() function is called twice - both in window and dialog onload. Remove one of the calls to prevent potential initialization issues.

   buttons="cancel,accept"
-  onload="notero.dialog.init();"
 >
   <script>
     Services.scriptloader.loadSubScript(

Also applies to: 28-29

<script>
Services.scriptloader.loadSubScript(
'chrome://zotero/content/customElements.js',
this,
);

Services.scriptloader.loadSubScript(
'chrome://notero/content/prefs/dialog.js',
this,
);
</script>
<dialog
id="notero-collection-dialog"
title="Collection Properties"
onload="notero.dialog.init();"
>
<div>
<label>Collection Settings</label>
<grid>
<rows>
<row>
<label value="Associated Link:" />
<menulist id="notero-associatedLink">
<menupopup />
</menulist>
</row>
<row>
<label value="Sync Enabled:" />
<checkbox id="notero-syncEnabled" />
</row>
</rows>
</grid>
</div>
</dialog>
</window>
3 changes: 3 additions & 0 deletions src/content/prefs/notero-pref.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { MissingPrefError } from '../errors';

export enum NoteroPref {
collectionSyncConfigs = 'collectionSyncConfigs',
linkedCollectionID = 'linkedCollectionID',
notionDatabaseID = 'notionDatabaseID',
notionToken = 'notionToken',
pageTitleFormat = 'pageTitleFormat',
Expand Down Expand Up @@ -37,6 +38,7 @@ export const PAGE_TITLE_FORMAT_L10N_IDS: Record<

type NoteroPrefValue = Partial<{
[NoteroPref.collectionSyncConfigs]: string;
[NoteroPref.linkedCollectionID]: string;
[NoteroPref.notionDatabaseID]: string;
[NoteroPref.notionToken]: string;
[NoteroPref.pageTitleFormat]: PageTitleFormat;
Expand Down Expand Up @@ -84,6 +86,7 @@ function convertRawPrefValue<P extends NoteroPref>(

return {
[NoteroPref.collectionSyncConfigs]: stringPref,
[NoteroPref.linkedCollectionID]: stringPref,
[NoteroPref.notionDatabaseID]: stringPref,
[NoteroPref.notionToken]: stringPref,
[NoteroPref.pageTitleFormat]: pageTitleFormatPref,
Expand Down
37 changes: 12 additions & 25 deletions src/content/prefs/preferences.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,48 +4,24 @@ import React from 'react';
import ReactDOM from 'react-dom';
import type { createRoot } from 'react-dom/client';

import type { FluentMessageId } from '../../locale/fluent-types';
import type { NotionAuthManager } from '../auth';
import { LocalizableError } from '../errors';
import type { EventManager } from '../services';
import { getNotionClient } from '../sync/notion-client';
import { isNotionErrorWithCode, normalizeID } from '../sync/notion-utils';
import {
createXULElement,
getGlobalNotero,
getLocalizedErrorMessage,
getXULElementById,
logger,
} from '../utils';
import { MenuItem, setMenuItems } from '../utils/elements';

import { PAGE_TITLE_FORMAT_L10N_IDS, PageTitleFormat } from './notero-pref';
import { SyncConfigsTable } from './sync-configs-table';

type ReactDOMClient = typeof ReactDOM & { createRoot: typeof createRoot };

type MenuItem = {
disabled?: boolean;
l10nId?: FluentMessageId;
label?: string;
value: string;
};

function setMenuItems(menuList: XUL.MenuListElement, items: MenuItem[]): void {
menuList.menupopup.replaceChildren();

items.forEach(({ disabled, l10nId, label, value }) => {
const item = createXULElement(document, 'menuitem');
item.value = value;
item.disabled = Boolean(disabled);
if (l10nId) {
document.l10n.setAttributes(item, l10nId);
} else {
item.label = label || value;
}
menuList.menupopup.append(item);
});
}

class Preferences {
private eventManager!: EventManager;
private notionAuthManager!: NotionAuthManager;
Expand All @@ -54,6 +30,7 @@ class Preferences {
private notionConnectButton!: XUL.ButtonElement;
private notionUpgradeConnectionButton!: XUL.ButtonElement;
private notionDatabaseMenu!: XUL.MenuListElement;
private notionLinkedDatabaseMenu!: XUL.MenuListElement;
private notionError!: XUL.LabelElement;
private notionWorkspaceLabel!: XUL.LabelElement;
private pageTitleFormatMenu!: XUL.MenuListElement;
Expand All @@ -77,6 +54,9 @@ class Preferences {
)!;
this.notionWorkspaceLabel = getXULElementById('notero-notionWorkspace')!;
this.notionDatabaseMenu = getXULElementById('notero-notionDatabase')!;
this.notionLinkedDatabaseMenu = getXULElementById(
'notero-notionLinkedDatabase',
)!;
this.notionError = getXULElementById('notero-notionError')!;
this.pageTitleFormatMenu = getXULElementById('notero-pageTitleFormat')!;
/* eslint-enable @typescript-eslint/no-non-null-assertion */
Expand Down Expand Up @@ -133,9 +113,13 @@ class Preferences {
const syncEnabled = await document.l10n.formatValue(
'notero-preferences-sync-enabled-column',
);
const associatedLink = await document.l10n.formatValue(
'notero-preferences-associatedLink-column',
);
const columnLabels = {
collectionFullName: collection || 'Collection',
syncEnabled: syncEnabled || 'Sync Enabled',
associatedLink: associatedLink || 'Associated Link',
};

(ReactDOM as ReactDOMClient)
Expand Down Expand Up @@ -215,6 +199,7 @@ class Preferences {
let menuItems: MenuItem[] = [];

this.notionDatabaseMenu.disabled = true;
this.notionLinkedDatabaseMenu.disabled = true;

try {
const databases = await this.retrieveNotionDatabases(notion);
Expand All @@ -231,8 +216,10 @@ class Preferences {
});

this.notionDatabaseMenu.disabled = false;
this.notionLinkedDatabaseMenu.disabled = false;
} finally {
setMenuItems(this.notionDatabaseMenu, menuItems);
setMenuItems(this.notionLinkedDatabaseMenu, menuItems);
}
}

Expand Down
14 changes: 14 additions & 0 deletions src/content/prefs/preferences.xhtml
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,20 @@
<menupopup />
</menulist>
</hbox>
<hbox align="center">
<label
control="notero-notionLinkedDatabase"
data-l10n-id="notero-preferences-notion-linked-database"
/>
<menulist
id="notero-notionLinkedDatabase"
disabled="true"
native="true"
preference="extensions.notero.linkedCollectionID"
>
<menupopup />
</menulist>
</hbox>
</vbox>
<label class="notero-error" hidden="true" id="notero-notionError" />
</groupbox>
Expand Down
Loading
Loading