build: Enable live reloading using web-ext, esbuild, and chokidar#738
build: Enable live reloading using web-ext, esbuild, and chokidar#738
Conversation
WalkthroughConsolidates and modernizes build/start tooling: introduces watchable esbuild contexts and asset watcher, replaces child_process launcher with a web-ext-based extension runner, moves manifest/version logic to async utils, updates path roots and types, and adjusts README/scripts and .gitignore entries. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant Build as build() (esbuild ctx)
participant Assets as copyAndWatchAssets()
participant GenMan as generateInstallManifest()
participant WebExt as web-ext runner
participant Zotero as Zotero App
Dev->>Build: invoke build({sourcemap, watch})
Build->>Assets: copyAndWatchAssets()/copyAssets()
Assets-->>Build: assets ready / watcher returned
Build->>GenMan: generateInstallManifest()
GenMan-->>Build: manifest.json written
Build-->>Dev: contexts watching / CleanupFunction
Dev->>WebExt: run(sourceDir=buildDir, options)
WebExt->>Zotero: launch with profile & args
Note over Build,WebExt: on rebuild -> WebExt.reload()
sequenceDiagram
autonumber
participant Script as Script
participant Ver as getVersion()
participant FS as version.json
participant Pkg as package.json
Script->>Ver: await getVersion()
alt version.json exists
Ver->>FS: read version.json
FS-->>Ver: version
else
Ver->>Pkg: read package.json metadata
Ver->>Ver: computeVersion()
Ver->>FS: write version.json
end
Ver-->>Script: version string
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Build Bot✅ Successful build ⬇️ Download: notero-1.2.2-852 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/generate-fluent-types.mts (1)
23-27: import.meta.filename is Node 22‑only; switch to fileURLToPathPrevents runtime errors on Node 18/20 and Windows.
+import { fileURLToPath } from 'node:url'; ... -const tsContent = `// This file is generated by ${relativeToRoot(import.meta.filename)} +const thisFile = fileURLToPath(import.meta.url); +const tsContent = `// This file is generated by ${relativeToRoot(thisFile)} export type FluentMessageId = ${messageIds.map((id) => ` | '${id}'`).join('\n')}; `;scripts/create-xpi.mts (1)
19-21: Ensure output directory exists before writing XPIAvoids ENOENT if /xpi doesn’t exist on clean clones/CI.
const zip = new AdmZip(); zip.addLocalFolder(buildDir); +fs.ensureDirSync(xpiDir); zip.writeZip(xpiPath);
🧹 Nitpick comments (12)
.gitignore (1)
1-7: Root‑anchored ignores look good; also ignore the xpi output dirSince artifacts are written to /xpi, ignore the directory to avoid empty dir churn.
/node_modules /src/locale/fluent-types.ts /zotero.config.json /.env /.eslintcache +/xpiscripts/utils/version.mts (2)
46-48: Guard against missing GITHUB_RUN_NUMBERAvoids producing versions like
x.y.z-undefined.-function getPrereleaseVersion(baseVersion: string): string { - return `${baseVersion}-${process.env.GITHUB_RUN_NUMBER}`; -} +function getPrereleaseVersion(baseVersion: string): string { + const run = process.env.GITHUB_RUN_NUMBER ?? '0'; + return `${baseVersion}-${run}`; +}
42-44: os.userInfo() may throw in containers; add a safe fallbackImproves robustness in CI and minimal container images.
-function getLocalVersion(): string { - return `${getPatchBumpVersion()}-${os.userInfo().username}.${os.hostname()}`; -} +function getLocalVersion(): string { + let user = 'unknown'; + try { + user = os.userInfo().username; + } catch {} + return `${getPatchBumpVersion()}-${user}.${os.hostname()}`; +}package.json (1)
94-96: web-ext@8.10.0 is compatible with CI Node; TS types are present — consider adding package.json "engines"
- web-ext (^8.10.0) resolves to 8.10.0 (package-lock.json); web-ext lists engines.node ">=18.0.0".
- CI uses .node-version = 24.3.0 and actions/setup-node@v4 with node-version-file: '.node-version' (.github/workflows/build.yml, release.yml) — CI Node satisfies web-ext requirement.
- scripts/start.mts imports from 'web-ext' and types/web-ext.d.ts is present — TypeScript typings available.
- package.json has no "engines" field; optional: add "engines": { "node": ">=18" } to document/enforce runtime (or keep relying on .node-version + workflows).
scripts/utils/copy-assets.mts (2)
7-8: tests not fully ignored; nested files still copied.Your regex only matches trailing “…/tests” or “.DS_Store”, so files inside “tests” are included. Tighten the pattern to ignore the directory anywhere in the path (both POSIX and Windows).
Apply this diff:
-const IGNORED_PATHS = /(\.DS_Store|__tests__)$/; +const IGNORED_PATHS = /(?:^|[\\/])(?:\.DS_Store|__tests__)(?:[\\/]|$)/;Also applies to: 31-41
28-46: Asset changes don’t trigger rebuild/reload.
copyAssets()is only invoked once before esbuild watch starts. Changes to non‑TS(X)/JSON assets won’t be recopied, so web‑ext won’t reload. Consider watching assets and re‑runningcopyAssets()on change.I can sketch a
chokidarwatcher wired into the build flow if you want.README.md (1)
412-416: Add minimum Node version requirement near start instructions.Docs now depend on web‑ext and ESM features (TLA, JSON import attributes). Add a brief note (e.g., “Requires Node ≥ 20.x”) to reduce setup friction.
Also applies to: 418-424, 430-431
scripts/build.mts (1)
30-39: Consider watching/copying assets during dev.Even with esbuild watch, asset edits (images/css/html) won’t be recopied. Option: integrate a small asset watcher calling
copyAssets()and maybe touch a file to trigger web‑ext reload.scripts/start.mts (2)
68-86: Fix Windows default Zotero path fallback.Zotero 7 installs to “Program Files” (64‑bit). Prefer probing both locations.
Apply this diff:
- if (process.platform === 'win32') { - return 'C:/Program Files (x86)/Zotero/Zotero.exe'; - } + if (process.platform === 'win32') { + const candidates = [ + 'C:/Program Files/Zotero/Zotero.exe', + 'C:/Program Files (x86)/Zotero/Zotero.exe', + ]; + const found = candidates.find((p) => fs.existsSync(p)); + assert.ok(found, 'Invalid path to Zotero'); + return found!; + }
125-156: Ensure log directory exists and guard stream errors.If
logFile’s directory is missing,createWriteStreamfails. AddensureDirSync, and log stream errors.Apply this diff:
const { logFile, preserveLog } = config.zotero || {}; if (!logFile) return; + fs.ensureDirSync(path.dirname(logFile)); @@ - const logStream = fs.createWriteStream(logFile, { flags }); + const logStream = fs.createWriteStream(logFile, { flags }); + logStream.on('error', (err) => { + console.warn(`Log stream error: ${err?.message ?? err}`); + });scripts/generate-update-manifest.mts (1)
59-60: Document the JSON-based update.rdf for legacy compatibility
Thexpi.updateURLin package.json points toupdates.json, so copying JSON intoupdate.rdfis intentional. Add a brief comment in scripts/generate-update-manifest.mts explaining that update.rdf contains the same JSON manifest for legacy consumers.types/web-ext.d.ts (1)
84-87: Consider using empty interfaces without eslint-disable comments.The ESLint disable comments for empty object types might be unnecessary. Empty interfaces are valid and commonly used in TypeScript for extensibility points.
Apply this diff to remove the ESLint disable comments:
// Docs command types - // eslint-disable-next-line @typescript-eslint/no-empty-object-type export interface DocsOptions { // No specific options documented for docs command } // Dump config command types - // eslint-disable-next-line @typescript-eslint/no-empty-object-type export interface DumpConfigOptions extends BaseOptions { // Inherits from BaseOptions }Also applies to: 90-93
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
.gitignore(1 hunks)README.md(1 hunks)package.json(1 hunks)scripts/build.mts(1 hunks)scripts/create-xpi.mts(1 hunks)scripts/generate-fluent-types.mts(1 hunks)scripts/generate-install-manifest.mts(0 hunks)scripts/generate-update-manifest.mts(2 hunks)scripts/start.mts(4 hunks)scripts/utils/copy-assets.mts(1 hunks)scripts/utils/generate-install-manifest.mts(1 hunks)scripts/utils/paths.mts(1 hunks)scripts/utils/version.mts(1 hunks)types/web-ext.d.ts(1 hunks)zotero.config.example.json(0 hunks)
💤 Files with no reviewable changes (2)
- scripts/generate-install-manifest.mts
- zotero.config.example.json
🧰 Additional context used
🧬 Code graph analysis (8)
scripts/build.mts (2)
scripts/utils/generate-install-manifest.mts (1)
generateInstallManifest(12-36)scripts/utils/copy-assets.mts (1)
copyAssets(28-46)
scripts/create-xpi.mts (2)
scripts/utils/paths.mts (1)
buildDir(5-5)scripts/utils/version.mts (1)
getVersion(13-27)
scripts/generate-fluent-types.mts (1)
scripts/copy-assets.mts (2)
filter(31-37)removeEmptyDirectories(11-26)
scripts/utils/generate-install-manifest.mts (2)
scripts/utils/paths.mts (2)
buildDir(5-5)relativeToRoot(10-10)scripts/utils/version.mts (1)
getVersion(13-27)
scripts/utils/version.mts (2)
scripts/utils/paths.mts (2)
genDir(6-6)relativeToRoot(10-10)scripts/version.mts (4)
getVersion(15-26)getPatchBumpVersion(36-38)getLocalVersion(28-30)getPrereleaseVersion(32-34)
scripts/utils/copy-assets.mts (1)
scripts/utils/paths.mts (3)
srcDir(7-7)buildDir(5-5)relativeToRoot(10-10)
scripts/start.mts (4)
scripts/utils/version.mts (1)
getVersion(13-27)scripts/build.mts (1)
build(13-44)types/web-ext.d.ts (2)
cmd(172-172)MultiExtensionRunner(119-121)scripts/utils/paths.mts (1)
buildDir(5-5)
scripts/generate-update-manifest.mts (1)
scripts/utils/version.mts (1)
getVersion(13-27)
🔇 Additional comments (13)
scripts/generate-fluent-types.mts (1)
6-6: Import path update: LGTMscripts/utils/generate-install-manifest.mts (2)
5-5: JSON import useswithattribute; confirm Node baseline or switch toassert
with { type: 'json' }is newer; Node 20 LTS widely supportsassert { type: 'json' }. Either set engines to Node ≥22 or useassertfor broader support.-import pkg from '../../package.json' with { type: 'json' }; +import pkg from '../../package.json' assert { type: 'json' };
22-23: Double‑check manifest_version for Zotero 7/8Zotero typically uses MV2 today; please confirm MV2 is still required/valid for your target (7.0/8.0.*).
scripts/create-xpi.mts (2)
14-15: Top‑level await for version: LGTM
7-7: JSON import attribute: align with Node baselineSame concern as elsewhere—prefer
assertunless Node ≥22 is enforced.-import pkg from '../package.json' with { type: 'json' }; +import pkg from '../package.json' assert { type: 'json' };scripts/utils/version.mts (1)
13-27: Overall versioning flow: LGTMLazy read/write of gen/version.json and logging are clear and predictable.
scripts/build.mts (2)
17-24: Consider bundling bootstrap or documenting reasons not to.
src/bootstrap.tsis built withoutbundle: true. If it imports local modules, missing bundling could break in Zotero’s environment. If intentional, add a comment; otherwise setbundle: true.
46-52: Use a Node‑portable main‑module check or require Node ≥24.import.meta.main was merged into Node.js (PR #57804, merged May 26, 2025); a TypeScript .ts/.mts edge case was later fixed (PR #58661). (github.com)
If you need to support Node <24 or older tooling, replace the check with a portable guard, e.g.:
import { pathToFileURL } from 'node:url';
const isMain = import.meta.url === pathToFileURL(process.argv[1] ?? '').href;
if (isMain) { /* ... */ }. (2ality.com)Likely an incorrect or invalid review comment.
scripts/start.mts (2)
90-99: Good: build before start with sourcemaps.Nice flow; ensures manifest/assets exist and enables reload after rebuild.
105-115: Cleanup hook correctly disposes build context.Solid use of
registerCleanupto stop the watcher.Also applies to: 117-121
scripts/generate-update-manifest.mts (1)
45-53: Validate updates.json schema for Zotero fields — confirm version stringsapplications.zotero is correct for Zotero 7 (gecko is only needed for Zotero 6). Confirm pkg.xpi.zoteroMinVersion and pkg.xpi.zoteroMaxVersion emit Zotero‑compatible strings (e.g., strict_min_version like "6.999" to allow Zotero 7 betas; strict_max_version like "7.0.*" to limit to a tested minor series).
Location: scripts/generate-update-manifest.mts lines 45–53.types/web-ext.d.ts (2)
1-173: LGTM! Comprehensive and well-structured TypeScript declarations.The type definitions provide excellent coverage of the web-ext API with proper interface hierarchies, clear documentation, and thoughtful organization. The structure effectively captures the command-based architecture and extension runner lifecycle.
167-168: Keep the eslint-disable — rule enforced globallyeslint.config.mjs sets "import/no-default-export" to "error"; matching inline disables exist (types/components/virtualized-table.d.ts, vitest.config.mts). The disable in types/web-ext.d.ts is consistent and should remain.
| import { genDir, relativeToRoot } from './utils/paths.mts'; | ||
| import { getVersion } from './utils/version.mts'; |
There was a problem hiding this comment.
🧩 Analysis chain
Confirm Node runtime supports import attributes and TLA.
This relies on JSON import attributes (with) and top‑level await. Ensure the project enforces a Node version that supports both (e.g., engines field and/or .nvmrc).
Also applies to: 18-18
🏁 Script executed:
#!/bin/bash
# Verify Node engine and .nvmrc
jq -r '.engines.node // "missing engines.node"' package.json || true
[ -f .nvmrc ] && cat .nvmrc || echo "missing .nvmrc"
# Grep for ' with { type: \"json\" }' usage
rg -nP "with\s*{\s*type:\s*'json'\s*}" -g '!node_modules/**'Length of output: 184
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== package.json (full) ==="
if [ -f package.json ]; then
jq -r '.' package.json || cat package.json
else
echo "no package.json"
fi
echo
echo "=== package.json: type, engines.node, packageManager, volta ==="
if [ -f package.json ]; then
jq -r '.type // "(no type)"' package.json || true
jq -r '.engines.node // "(missing engines.node)"' package.json || true
jq -r '.packageManager // "(no packageManager)"' package.json || true
jq -r '.volta.node // "(no volta.node)"' package.json || true
fi
echo
echo "=== .nvmrc ==="
if [ -f .nvmrc ]; then cat .nvmrc; else echo "missing .nvmrc"; fi
echo
echo "=== .tool-versions ==="
[ -f .tool-versions ] && cat .tool-versions || echo "missing .tool-versions"
echo
echo "=== scripts/generate-update-manifest.mts (first 200 lines) ==="
if [ -f scripts/generate-update-manifest.mts ]; then sed -n '1,200p' scripts/generate-update-manifest.mts; else echo "missing scripts/generate-update-manifest.mts"; fi
echo
echo "=== Searching for import assertions and JSON imports ==="
rg -n --hidden -uu -S "assert\\s*\\{\\s*type\\s*:\\s*['\\\"]json['\\\"]\\s*\\}" --glob '!node_modules/**' || true
rg -n --hidden -uu -S "with\\s*\\{\\s*type\\s*:\\s*['\\\"]json['\\\"]\\s*\\}" --glob '!node_modules/**' || true
rg -n --hidden -uu -S "import\\s+[^;]+\\.json" --glob '!node_modules/**' || true
echo
echo "=== Searching for await in .mts/.mjs/.ts ==="
rg -n --hidden -uu -S --glob '!node_modules/**' --glob '**/*.mts' '\\bawait\\b' -C3 || true
rg -n --hidden -uu -S --glob '!node_modules/**' --glob '**/*.mjs' '\\bawait\\b' -C3 || true
rg -n --hidden -uu -S --glob '!node_modules/**' --glob '**/*.ts' '\\bawait\\b' -C3 || true
echo
echo "=== Searching for Node version config in CI/workflows ==="
ls -la .github/workflows || true
rg -n --hidden -uu -S "actions/setup-node|node-version" .github || true
echo
echo "=== Searching for engines in other package.json files ==="
rg -n --hidden -uu -S '"engines"' --glob '!node_modules/**' || true
echo
echo "=== Searching for tsx/ts-node/typescript in deps ==="
if [ -f package.json ]; then
jq -r '((.devDependencies // {}) + (.dependencies // {})) | keys[]' package.json | rg -n 'tsx|ts-node|typescript|esbuild|tsup|esbuild-runner|tsx|@swc' || true
fi
echo
echo "=== Done ==="Length of output: 6685
Require Node >= 18.20.0 (import attributes 'with')
scripts/generate-update-manifest.mts uses import ... with { type: 'json' } and top-level await; import attributes (with) are available from Node 18.20.0, and top-level await was unflagged in Node 14.8.0. (nodejs.org)
- Add an engines.node entry (e.g. "node": ">=18.20.0") to package.json and/or commit a .nvmrc or .node-version set to 18.20.0.
- CI references '.node-version' in .github/workflows/* (release.yml, build.yml) — ensure that file exists or pin node-version directly in those workflows.
- Location: scripts/generate-update-manifest.mts (uses JSON import attribute + TLA).
🤖 Prompt for AI Agents
In scripts/generate-update-manifest.mts around lines 8-9 it uses import
attributes (with { type: 'json' }) which requires Node >= 18.20.0; add a
engines.node entry in package.json (e.g. "node": ">=18.20.0") and commit a
runtime pin file (.nvmrc or .node-version) containing 18.20.0, and/or update
.github/workflows/* to pin node-version directly to 18.20.0 so CI and local
developers run the required Node version; ensure the chosen approach is
consistent (package.json engines + .nvmrc/.node-version and CI references that
file or CI uses the same pinned version).
There was a problem hiding this comment.
I'm already using a .node-version file.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for .node-version file and its content
echo "=== Checking for .node-version file ==="
if [ -f .node-version ]; then
echo "✓ .node-version exists"
echo "Content:"
cat .node-version
echo ""
else
echo "✗ .node-version not found"
fi
# Also check for .nvmrc as alternative
echo "=== Checking for .nvmrc file ==="
if [ -f .nvmrc ]; then
echo "✓ .nvmrc exists"
echo "Content:"
cat .nvmrc
echo ""
else
echo "✗ .nvmrc not found"
fi
# Search for any Node version files
echo "=== Searching for Node version files ==="
find . -maxdepth 2 -name ".node-version" -o -name ".nvmrc" -o -name ".tool-versions" | head -10
# Check if the files are in the current working directory
echo "=== Current directory listing (showing hidden files) ==="
ls -la | grep -E "(node|nvmrc)"
Length of output: 311
@dvanoni You're absolutely right! I can confirm that .node-version exists and specifies Node 24.3.0, which fully supports both import attributes (with { type: 'json' }) and top-level await used in the code. My apologies for the confusion - your setup is perfectly adequate for the features being used in scripts/generate-update-manifest.mts.
The Node version requirement is well satisfied, so there's no action needed here.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| import path from 'node:path'; | ||
|
|
||
| export const rootDir = path.join(import.meta.dirname, '..'); | ||
| export const rootDir = path.join(import.meta.dirname, '../..'); |
There was a problem hiding this comment.
Avoid Node 22‑only import.meta.dirname; use fileURLToPath for broader Node support
import.meta.dirname requires Node ≥22. Use fileURLToPath to work on Node 18/20/22.
+import { fileURLToPath } from 'node:url';
export const rootDir = path.join(import.meta.dirname, '../..');
+const __dirname = path.dirname(fileURLToPath(import.meta.url));
+export const rootDir = path.join(__dirname, '../..');Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In scripts/utils/paths.mts around line 3, the code uses import.meta.dirname
which only works on Node 22+; replace it with the Node-compatible pattern using
fileURLToPath(import.meta.url) to compute __dirname and then join with '../..'.
Import fileURLToPath from 'url' (and keep path import) and use
path.join(fileURLToPath(import.meta.url), '..', '..') or compute a dirname
variable via path.dirname(fileURLToPath(import.meta.url)) before joining.
There was a problem hiding this comment.
Since we're using other new features of Node, we require version 24.3.0 or higher.
There was a problem hiding this comment.
@dvanoni Thank you for the clarification! Since the project requires Node 24.3.0 or higher, using import.meta.dirname is perfectly appropriate and my compatibility suggestion is unnecessary.
✏️ Learnings added
Learnt from: dvanoni
PR: dvanoni/notero#738
File: scripts/utils/paths.mts:3-3
Timestamp: 2025-09-22T14:47:36.288Z
Learning: The notero project requires Node.js version 24.3.0 or higher, so modern Node.js features like import.meta.dirname can be used without compatibility concerns.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| const versionModule = (await import(versionJsonPath, { | ||
| with: { type: 'json' }, | ||
| })) as { default: string }; | ||
| const version = versionModule.default; | ||
| console.log(`Found ${relativeToRoot(versionJsonPath)} with ${version}`); |
There was a problem hiding this comment.
Dynamic JSON import: use file URL and assert for cross‑platform safety
Absolute Windows paths can break import(). Convert to file:// and use assert.
- const versionModule = (await import(versionJsonPath, {
- with: { type: 'json' },
- })) as { default: string };
+ import { pathToFileURL } from 'node:url';
+ const url = pathToFileURL(versionJsonPath).href;
+ const versionModule = (await import(url, {
+ assert: { type: 'json' },
+ })) as { default: string };📝 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.
| const versionModule = (await import(versionJsonPath, { | |
| with: { type: 'json' }, | |
| })) as { default: string }; | |
| const version = versionModule.default; | |
| console.log(`Found ${relativeToRoot(versionJsonPath)} with ${version}`); | |
| import { pathToFileURL } from 'node:url'; | |
| const url = pathToFileURL(versionJsonPath).href; | |
| const versionModule = (await import(url, { | |
| assert: { type: 'json' }, | |
| })) as { default: string }; | |
| const version = versionModule.default; | |
| console.log(`Found ${relativeToRoot(versionJsonPath)} with ${version}`); |
🤖 Prompt for AI Agents
In scripts/utils/version.mts around lines 15 to 19, the dynamic JSON import uses
a plain filesystem path which can break on Windows; convert the path to a
file:// URL and use the import assertion form. Create a file URL from
versionJsonPath (e.g. via pathToFileURL(versionJsonPath).href), then call
import(fileUrl, { assert: { type: 'json' } }) and read the default export; keep
the existing cast to { default: string } and update the log call to use the
imported value.
There was a problem hiding this comment.
Do we need to convert to file URLs when using version 24.3.0 of Node?
There was a problem hiding this comment.
@dvanoni Yes, you still need to convert to file URLs when using Node.js 24.3.0.
The current Node.js v24.8.0 documentation explicitly states: "Given the differences between URL and path resolution (such as percent encoding details), it is recommended to use url.pathToFileURL when importing a path."
The Windows compatibility issues that were reported in GitHub issue #31710 from 2020 (showing "ERR_UNSUPPORTED_ESM_URL_SCHEME" errors) haven't been fundamentally resolved because they stem from how Windows absolute paths are interpreted as URLs. The issue affects both static and dynamic ES imports.
Even recent examples from Stack Overflow show developers still using pathToFileURL to convert absolute paths like "C:\Users\sparky\example\plugins\plugin1.js" to proper file URLs like "file:///C:/Users/sparky/example/plugins/plugin1.js" for cross-platform compatibility.
So the original suggestion remains valid - using pathToFileURL ensures cross-platform compatibility:
import { pathToFileURL } from 'node:url';
const url = pathToFileURL(versionJsonPath).href;
const versionModule = (await import(url, {
with: { type: 'json' },
})) as { default: string };This approach works reliably across all platforms, including Windows, in Node.js 24.3.0 and later versions.
🐰💭
There was a problem hiding this comment.
Got it. Thanks! Is this only necessary with the await import syntax, or do I need this in all places where I use with { type: 'json' }?
There was a problem hiding this comment.
🧩 Analysis chain
@dvanoni The file URL conversion is only necessary for dynamic imports (await import() syntax) that use absolute filesystem paths.
Here are the different scenarios:
✅ No conversion needed:
-
Static imports with relative paths (like in your code):
import pkg from '../../package.json' with { type: 'json' };
These work fine across all platforms because the module resolver handles the path resolution at compile time.
-
Dynamic imports with relative paths:
await import('./config.json', { with: { type: 'json' } })
Also work fine because relative paths don't trigger Windows path interpretation issues.
- Dynamic imports with absolute filesystem paths (like in your
getVersion()function):// This can break on Windows await import('/absolute/path/to/file.json', { with: { type: 'json' } }) // Convert to file URL first import { pathToFileURL } from 'node:url'; const url = pathToFileURL(absolutePath).href; await import(url, { with: { type: 'json' } })
The issue isn't with the with { type: 'json' } syntax itself - it's specifically about how Windows interprets absolute filesystem paths in dynamic imports. Static imports and relative paths in dynamic imports work fine without conversion.
🏁 Script executed:
#!/bin/bash
# Search for all occurrences of 'with { type: 'json' }' in the codebase
rg -n "with \{ type: 'json' \}" --type=ts --type=js -C2
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for 'with' and 'type' and 'json' patterns more broadly
rg -n "with.*type.*json" --type=ts --type=js -C2
# Also search for import assertions with different spacing
rg -n "with.*{.*type.*:.*['\"]json['\"].*}" --type=ts --type=js -C2
# Search for any import statements with json
rg -n "import.*\.json" --type=ts --type=js -C2
Length of output: 122
🏁 Script executed:
#!/bin/bash
# Search for import statements with json files
rg -n "import.*\.json" -C2
# Search for with type json with proper escaping
rg -n 'with.*type.*json' -C2
# Search specifically for the pattern we saw in the file
rg -n 'type.*json' -C2
# Look for all import patterns in the scripts directory
find scripts -name "*.mts" -exec grep -n "import" {} \; -print
Length of output: 9744
@dvanoni Perfect! Now I can give you a complete answer based on your actual codebase.
Looking at your code, you have both patterns:
✅ These static imports are fine as-is (no conversion needed):
// In create-xpi.mts, generate-update-manifest.mts:
import pkg from '../package.json' with { type: 'json' };
// In generate-install-manifest.mts, version.mts:
import pkg from '../../package.json' with { type: 'json' };// In version.mts - this uses an absolute path (versionJsonPath)
const versionModule = (await import(versionJsonPath, {
with: { type: 'json' },
})) as { default: string };The key difference is that versionJsonPath is constructed as path.join(genDir, 'version.json'), which creates an absolute filesystem path. This is the one that can break on Windows.
So to answer your question directly: You only need the file URL conversion for the dynamic import in getVersion(). All your static imports with relative paths work fine across all platforms without any changes.
The with { type: 'json' } syntax itself isn't the problem - it's specifically the combination of dynamic import + absolute filesystem paths that needs the conversion.
| } | ||
|
|
||
| export interface RunDependencies { | ||
| getValidatedManifest?: (sourceDir: string) => Promise<unknown>; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify web-ext's getValidatedManifest function signature.
The getValidatedManifest function signature should be confirmed against the actual web-ext implementation to ensure accuracy.
🌐 Web query:
web-ext getValidatedManifest function signature TypeScript
💡 Result:
There are no official TypeScript types in web-ext, but in the web-ext API the getValidatedManifest option is a function that should return the (validated) manifest object — synchronous or Promise. A practical TS signature you can use:
type GetValidatedManifest = () => any | Promise;
and in options:
getValidatedManifest?: GetValidatedManifest;
References: example usage in web-ext docs/npm pages. [1][2]
Sources
- web-ext npm package docs (example using getValidatedManifest). [1]
- web-ext README / skypack example (shows getValidatedManifest: () => ({ name: ..., version: ... })). [2]
Update getValidatedManifest signature to match web-ext API
web-ext's getValidatedManifest is a zero-argument function that may return the manifest synchronously or a Promise — update types/web-ext.d.ts (line 55) to:
getValidatedManifest?: () => unknown | Promise;
🤖 Prompt for AI Agents
In types/web-ext.d.ts around line 55, the getValidatedManifest signature
currently expects a string argument and a Promise return; change it to a
zero-argument function returning either a synchronous value or a Promise by
replacing the line with: getValidatedManifest?: () => unknown |
Promise<unknown>; this aligns the type with the web-ext API.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (13)
package.json (2)
76-76: Add watcher stability to avoid copying partially written files.Since chokidar v4 is introduced, consider enabling awaitWriteFinish in your watcher to prevent copying half-written assets during saves.
You can apply this in scripts/utils/copy-assets.mts (Line 21):
- const watcher = watch(srcDir, { ignored: IGNORE_PATTERNS, persistent }); + const watcher = watch(srcDir, { + ignored: IGNORE_PATTERNS, + persistent, + awaitWriteFinish: { stabilityThreshold: 200, pollInterval: 50 }, + });
96-97: Pin web-ext to a stable minor and add a lint script.web-ext CLIs occasionally ship breaking changes; consider pinning to a minor (e.g., ~8.10.0) and adding a script to catch manifest issues in CI.
- "web-ext": "^8.10.0" + "web-ext": "~8.10.0"Add in scripts:
+ "xpi:lint": "web-ext lint --source-dir build"Optionally add engines to enforce Node ≥24.3.0:
"homepage": "https://github.com/dvanoni/notero", + "engines": { "node": ">=24.3.0" },Would you like a small CI step added to run xpi:lint during verify?
scripts/utils/version.mts (1)
25-29: Avoid sync file I/O in async code path.Replace outputJsonSync with async outputJson to keep the event loop responsive during builds.
- fs.outputJsonSync(versionJsonPath, version); + await fs.outputJson(versionJsonPath, version);scripts/utils/copy-assets.mts (3)
21-21: Add awaitWriteFinish to prevent partial copies.Helps on editors that save via temp files/chunked writes.
- const watcher = watch(srcDir, { ignored: IGNORE_PATTERNS, persistent }); + const watcher = watch(srcDir, { + ignored: IGNORE_PATTERNS, + persistent, + awaitWriteFinish: { stabilityThreshold: 200, pollInterval: 50 }, + });
24-31: Prefer async fs operations in hot path.Synchronous copy blocks the event loop during frequent saves.
- .on('add', (srcPath) => { + .on('add', async (srcPath) => { const destPath = srcPathToBuildPath(srcPath); console.log(`Copying asset: ${relativeToRoot(srcPath)}`); - fs.copySync(srcPath, destPath); + await fs.copy(srcPath, destPath); })
55-64: Use async fs for change/unlink; handle missing targets gracefully.Reduces blocking and makes removals tolerant to races.
- .on('change', (srcPath) => { + .on('change', async (srcPath) => { const destPath = srcPathToBuildPath(srcPath); console.log(`Copying updated asset: ${relativeToRoot(srcPath)}`); - fs.copySync(srcPath, destPath); + await fs.copy(srcPath, destPath); }) - .on('unlink', (srcPath) => { + .on('unlink', async (srcPath) => { const destPath = srcPathToBuildPath(srcPath); console.log(`Removing deleted asset: ${relativeToRoot(srcPath)}`); - fs.removeSync(destPath); + await fs.remove(destPath); });scripts/start.mts (4)
44-46: Consider manifest validation or surfacing lint errors early.Reading JSON is fine; optionally run web‑ext lint first and reuse its parsed manifest to fail fast on schema errors.
Would you like me to wire xpi:lint here and show errors before launching Zotero?
105-115: web-ext run options: register cleanup via async to ensure orderly shutdown.If registerCleanup awaits promises, drop void to avoid racing shutdown.
- if (cleanup) { - extensionRunner.registerCleanup(() => { - void cleanup(); - }); - } + if (cleanup) { + extensionRunner.registerCleanup(async () => { + await cleanup(); + }); + }
126-157: Harden logging: ensure directory exists and add newlines.Prevents EINVAL on missing dirs and improves readability.
- const flags = preserveLog ? 'a' : 'w'; - const logStream = fs.createWriteStream(logFile, { flags }); + const flags = preserveLog ? 'a' : 'w'; + await fs.ensureDir(path.dirname(logFile)); + const logStream = fs.createWriteStream(logFile, { flags }); @@ - const writeToLog = (data: unknown) => { + const writeToLog = (data: unknown) => { const timestamp = new Date().toISOString(); - logStream.write(`[${timestamp}] ${String(data)}`); + logStream.write(`[${timestamp}] ${String(data)}\n`); };
67-86: Prefer 64-bit Program Files path on Windows
UseC:/Program Files/Zotero/Zotero.exewith a fallback toC:/Program Files (x86)/Zotero/Zotero.exe:- if (process.platform === 'win32') { - return 'C:/Program Files (x86)/Zotero/Zotero.exe'; - } + if (process.platform === 'win32') { + const x64 = 'C:/Program Files/Zotero/Zotero.exe'; + const x86 = 'C:/Program Files (x86)/Zotero/Zotero.exe'; + return fs.existsSync(x64) ? x64 : x86; + }scripts/build.mts (3)
13-15: Single source of truth for build output dir.Avoid hardcoding 'build' here; reuse buildDir from utils/paths.mts to prevent drift with other scripts.
-import esbuild, { type Plugin } from 'esbuild'; +import esbuild, { type Plugin } from 'esbuild'; +import { buildDir } from './utils/paths.mts'; @@ -const OUTDIR = 'build'; -const TARGET = 'firefox115'; +const TARGET = 'firefox115'; @@ - outdir: OUTDIR, + outdir: buildDir, @@ - outdir: OUTDIR, + outdir: buildDir,Also applies to: 33-39, 47-51
66-74: Ensure cleanup runs fully even if one disposer throws.Try/finally each step so one failure doesn’t skip the rest.
- const cleanup: CleanupFunction = async () => { - console.log('Stopping build watcher'); - await bootstrapContext.dispose(); - await contentContext.dispose(); - await cleanupAssetWatcher(); - }; + const cleanup: CleanupFunction = async () => { + console.log('Stopping build watcher'); + const tasks = [ + bootstrapContext.dispose().catch((e) => console.warn('bootstrap dispose failed:', e)), + contentContext.dispose().catch((e) => console.warn('content dispose failed:', e)), + cleanupAssetWatcher().catch((e) => console.warn('asset watcher cleanup failed:', e)), + ]; + await Promise.all(tasks); + };
76-91: Add SIGTERM handling for graceful shutdown
Extend the cleanup promise in scripts/build.mts to listen forSIGTERMalongsideSIGINT.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.gitignore(1 hunks)package.json(3 hunks)scripts/build.mts(1 hunks)scripts/start.mts(4 hunks)scripts/utils/copy-assets.mts(1 hunks)scripts/utils/version.mts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .gitignore
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-22T14:47:36.288Z
Learnt from: dvanoni
PR: dvanoni/notero#738
File: scripts/utils/paths.mts:3-3
Timestamp: 2025-09-22T14:47:36.288Z
Learning: The notero project requires Node.js version 24.3.0 or higher, so modern Node.js features like import.meta.dirname can be used without compatibility concerns.
Applied to files:
scripts/utils/version.mts
🧬 Code graph analysis (4)
scripts/start.mts (4)
scripts/utils/version.mts (1)
getVersion(14-29)scripts/build.mts (1)
build(27-74)types/web-ext.d.ts (2)
cmd(172-172)MultiExtensionRunner(119-121)scripts/utils/paths.mts (1)
buildDir(5-5)
scripts/build.mts (2)
scripts/utils/generate-install-manifest.mts (1)
generateInstallManifest(12-36)scripts/utils/copy-assets.mts (2)
copyAssets(45-49)copyAndWatchAssets(51-76)
scripts/utils/version.mts (1)
scripts/utils/paths.mts (2)
genDir(6-6)relativeToRoot(10-10)
scripts/utils/copy-assets.mts (1)
scripts/utils/paths.mts (3)
buildDir(5-5)srcDir(7-7)relativeToRoot(10-10)
🔇 Additional comments (3)
package.json (1)
7-7: Script addition looks good; aligns with watchable build flow.No issues. This pairs correctly with scripts/build.mts watch mode.
scripts/utils/version.mts (2)
14-21: Dynamic JSON import via file URL: LGTM.Correct use of pathToFileURL + JSON modules for cross‑platform dynamic import. Nice logging.
48-54: Nullish-coalesce GITHUB_RUN_NUMBER in getPrereleaseVersion
Useconst run = process.env.GITHUB_RUN_NUMBER ?? 'local'; return `${baseVersion}-${run}`;to prevent
1.2.3-undefinedsuffixes. GitHub Actions always providesGITHUB_RUN_NUMBER(so CI tags stay numeric), and local builds usegetLocalVersion, so the'local'fallback won’t conflict.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
package.json(4 hunks)scripts/build.mts(1 hunks)scripts/utils/version.mts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-22T14:47:36.288Z
Learnt from: dvanoni
PR: dvanoni/notero#738
File: scripts/utils/paths.mts:3-3
Timestamp: 2025-09-22T14:47:36.288Z
Learning: The notero project requires Node.js version 24.3.0 or higher, so modern Node.js features like import.meta.dirname can be used without compatibility concerns.
Applied to files:
scripts/utils/version.mtspackage.json
🧬 Code graph analysis (2)
scripts/utils/version.mts (1)
scripts/utils/paths.mts (2)
genDir(6-6)relativeToRoot(10-10)
scripts/build.mts (3)
scripts/utils/generate-install-manifest.mts (1)
generateInstallManifest(12-36)scripts/utils/paths.mts (1)
buildDir(5-5)scripts/utils/copy-assets.mts (2)
copyAssets(45-49)copyAndWatchAssets(51-76)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/utils/version.mts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-22T14:47:36.288Z
Learnt from: dvanoni
PR: dvanoni/notero#738
File: scripts/utils/paths.mts:3-3
Timestamp: 2025-09-22T14:47:36.288Z
Learning: The notero project requires Node.js version 24.3.0 or higher, so modern Node.js features like import.meta.dirname can be used without compatibility concerns.
Applied to files:
scripts/utils/version.mts
🧬 Code graph analysis (1)
scripts/utils/version.mts (1)
scripts/utils/paths.mts (2)
genDir(6-6)relativeToRoot(10-10)
dvanoni
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/utils/version.mts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-22T14:47:36.288Z
Learnt from: dvanoni
PR: dvanoni/notero#738
File: scripts/utils/paths.mts:3-3
Timestamp: 2025-09-22T14:47:36.288Z
Learning: The notero project requires Node.js version 24.3.0 or higher, so modern Node.js features like import.meta.dirname can be used without compatibility concerns.
Applied to files:
scripts/utils/version.mts
🧬 Code graph analysis (1)
scripts/utils/version.mts (2)
scripts/utils/paths.mts (2)
genDir(6-6)relativeToRoot(10-10)scripts/version.mts (4)
getVersion(15-26)getPatchBumpVersion(36-38)getLocalVersion(28-30)getPrereleaseVersion(32-34)
| export async function getVersion(): Promise<string> { | ||
| const versionJsonExists = await fs.pathExists(versionJsonPath); | ||
|
|
||
| function getVersion(): string { | ||
| if (versionJsonExists) { | ||
| const version = (await fs.readJson(versionJsonPath)) as string; | ||
| console.log(`Found ${relativeToRoot(versionJsonPath)} with ${version}`); | ||
| return version; | ||
| } | ||
|
|
||
| const version = computeVersion(); | ||
| console.log(`Writing ${relativeToRoot(versionJsonPath)} with ${version}`); | ||
| await fs.outputJson(versionJsonPath, version); | ||
| return version; |
There was a problem hiding this comment.
Version cache never refreshes after package.json bump
Line 17 returns the cached gen/version.json unconditionally. After the first run, getVersion() stops recomputing, so if we bump package.json (or the GH env changes), subsequent builds keep serving the stale version string unless the developer manually deletes gen/version.json. That breaks the whole “live reload” promise because the new bundle keeps the old version. Please recompute when the derived version diverges from the cached value and overwrite the file accordingly.
- if (versionJsonExists) {
- const version = (await fs.readJson(versionJsonPath)) as string;
- console.log(`Found ${relativeToRoot(versionJsonPath)} with ${version}`);
- return version;
- }
-
- const version = computeVersion();
- console.log(`Writing ${relativeToRoot(versionJsonPath)} with ${version}`);
- await fs.outputJson(versionJsonPath, version);
- return version;
+ if (versionJsonExists) {
+ const cachedVersion = (await fs.readJson(versionJsonPath)) as string;
+ const computedVersion = computeVersion();
+
+ if (cachedVersion === computedVersion) {
+ console.log(`Found ${relativeToRoot(versionJsonPath)} with ${cachedVersion}`);
+ return cachedVersion;
+ }
+
+ console.log(
+ `Updating ${relativeToRoot(versionJsonPath)} from ${cachedVersion} to ${computedVersion}`,
+ );
+ await fs.outputJson(versionJsonPath, computedVersion);
+ return computedVersion;
+ }
+
+ const version = computeVersion();
+ console.log(`Writing ${relativeToRoot(versionJsonPath)} with ${version}`);
+ await fs.outputJson(versionJsonPath, version);
+ return version;📝 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.
| export async function getVersion(): Promise<string> { | |
| const versionJsonExists = await fs.pathExists(versionJsonPath); | |
| function getVersion(): string { | |
| if (versionJsonExists) { | |
| const version = (await fs.readJson(versionJsonPath)) as string; | |
| console.log(`Found ${relativeToRoot(versionJsonPath)} with ${version}`); | |
| return version; | |
| } | |
| const version = computeVersion(); | |
| console.log(`Writing ${relativeToRoot(versionJsonPath)} with ${version}`); | |
| await fs.outputJson(versionJsonPath, version); | |
| return version; | |
| export async function getVersion(): Promise<string> { | |
| const versionJsonExists = await fs.pathExists(versionJsonPath); | |
| if (versionJsonExists) { | |
| const cachedVersion = (await fs.readJson(versionJsonPath)) as string; | |
| const computedVersion = computeVersion(); | |
| if (cachedVersion === computedVersion) { | |
| console.log(`Found ${relativeToRoot(versionJsonPath)} with ${cachedVersion}`); | |
| return cachedVersion; | |
| } | |
| console.log( | |
| `Updating ${relativeToRoot(versionJsonPath)} from ${cachedVersion} to ${computedVersion}`, | |
| ); | |
| await fs.outputJson(versionJsonPath, computedVersion); | |
| return computedVersion; | |
| } | |
| const version = computeVersion(); | |
| console.log(`Writing ${relativeToRoot(versionJsonPath)} with ${version}`); | |
| await fs.outputJson(versionJsonPath, version); | |
| return version; | |
| } |
🤖 Prompt for AI Agents
In scripts/utils/version.mts around lines 13 to 25, the function currently
returns the cached gen/version.json unconditionally which prevents updates after
package.json or env changes; change the logic to read the cached version,
compute the current derived version, compare them, and if they differ overwrite
gen/version.json (with a log) and return the newly computed version; keep the
fast-path when they match (log and return cached), and ensure you await
computeVersion() if it is async and use fs.outputJson to persist the new value.
There was a problem hiding this comment.
We don't update the version during development, so we don't need to worry about live reloading it. I don't think this concern is applicable.
There was a problem hiding this comment.
@dvanoni, thank you for the clarification! You're absolutely right - if the version doesn't change during development, then the caching behavior works perfectly for your workflow. I understand that this concern isn't applicable to your development practices.
ᵔᴥᵔ
✏️ Learnings added
Learnt from: dvanoni
PR: dvanoni/notero#738
File: scripts/utils/version.mts:13-25
Timestamp: 2025-09-28T00:43:33.442Z
Learning: In the notero project, versions are not updated during development, so version caching in getVersion() doesn't need to handle live updates of package.json version changes during development workflow.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
1 participant
Summary by CodeRabbit
Documentation
Chores
Tests