feat: run notes import in worker#292
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds mutex-backed SQLiteTransaction support and transaction-aware query blocking; refactors controllers to accept raw SQLiteDB and use wrapSQLite(this.db); introduces a Comlink-based Notes importer worker with deep-object transfer handling and batch import processing; updates tests and vault hooks to use managedDb.get(). ChangesDatabase Transaction System and Controller Wiring
Notes Importer Worker System and Deep Serialization
Sequence Diagram (Notes import flow) sequenceDiagram
participant UI
participant NotesImporterWorker
participant WorkerModule
participant NotesImporter
participant TagsRegistry
participant NotesRegistry
UI->>NotesImporterWorker: import(files, {config, options})
NotesImporterWorker->>WorkerModule: spawn & wrap via Comlink
NotesImporterWorker->>WorkerModule: api.import(deps, ComlinkHostFS(files), {config, options})
WorkerModule->>NotesImporter: run import logic (parse, batch, upload)
NotesImporter->>TagsRegistry: add tags (returnIfExist)
NotesImporter->>NotesRegistry: updateBatch(notes)
NotesImporter-->>WorkerModule: finish
WorkerModule-->>NotesImporterWorker: resolve
NotesImporterWorker-->>UI: resolve
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/app/src/core/features/notes/history/NoteVersions.ts (1)
97-107:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCompare against the latest version per note, not one global latest row.
The
lastsubquery usesORDER BY ... LIMIT 1without partitioning bynote_id, so it returns only one row globally. For batch input, unchanged notes can be treated as missing and re-snapshotted repeatedly.💡 Suggested fix
- LEFT JOIN ( - SELECT v.title, v.text, v.note_id - FROM note_versions v - -- Order by monotonic "rowid" in case of timestamp collisions - ORDER BY v.created_at DESC, rowid DESC - LIMIT 1 - ) last ON last.note_id = n.id + LEFT JOIN ( + SELECT note_id, title, text + FROM ( + SELECT + v.note_id, + v.title, + v.text, + ROW_NUMBER() OVER ( + PARTITION BY v.note_id + ORDER BY v.created_at DESC, v.rowid DESC + ) AS rn + FROM note_versions v + WHERE v.note_id IN (${qb.values(noteIds)}) + ) ranked + WHERE rn = 1 + ) last ON last.note_id = n.id🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/core/features/notes/history/NoteVersions.ts` around lines 97 - 107, The subquery aliased as last in NoteVersions.ts selects a single global latest row from note_versions (ORDER BY ... LIMIT 1) instead of the latest row per note_id; change it to select the latest version for each note_id (e.g. use a window function like ROW_NUMBER() OVER (PARTITION BY v.note_id ORDER BY v.created_at DESC, rowid DESC) and then filter for row_number = 1, or use DISTINCT ON (v.note_id) if your DB supports it) so that the JOIN compares each note against its own latest version when using qb.values(noteIds) and workspace filtering.
🧹 Nitpick comments (3)
packages/app/src/core/storage/interop/import/index.ts (3)
287-350: 💤 Low valueConsider abortion responsiveness within batches.
The abortion check (Line 288) is only performed at the start of each batch. If
abortSignalis triggered while processing a batch (e.g., after 10 of 100 notes), the remaining notes in that batch will still be processed. This delays the abortion response and may result in unnecessary work, especially with large batch sizes.For improved responsiveness, consider checking
abortSignalwithin the per-note processing loop or reducing the defaultbatchSizefor more frequent checks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/core/storage/interop/import/index.ts` around lines 287 - 350, The batch loop only calls checkForAbortion() once per batch; to make abortion responsive, call checkForAbortion() inside each per-note handler in the notesSlice.map (e.g., at the start of the Promise.resolve().then(...) body and again before heavy ops like replaceUrls, before pushing into updates, or before attachmentsRegistry.set) so the handler throws immediately when aborted; this ensures Promise.all short-circuits with an exception and the batch stops processing further notes. Use the existing checkForAbortion() helper (or abortSignal.aborted) within the per-note async function in the notesSlice.map and throw to propagate cancellation up to the outer loop.
226-245: ⚡ Quick winConsider skipping tag attachment when no tags are present.
Currently, even when
tagNamesToAttachis empty, the code still callsgetTagIds([])and pushes an entry with an empty tags array totagsToAttach. According to the commit summary, one optimization was to "skip tag attachment when none provided."♻️ Proposed optimization
const tagIds = await this.getTagIds(tagNamesToAttach); - tagsToAttach.push({ - noteId, - tags: tagIds, - }); + if (tagIds.length > 0) { + tagsToAttach.push({ + noteId, + tags: tagIds, + }); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/core/storage/interop/import/index.ts` around lines 226 - 245, The code currently calls getTagIds(tagNamesToAttach) and pushes a tagsToAttach entry even when tagNamesToAttach is empty; update the block around convertPathToTag/tagNamesToAttach so that you only call this.getTagIds(...) and push into tagsToAttach when tagNamesToAttach.length > 0 (use the existing variables tagNamesToAttach, noteDirPath, convertPathToTag and noteId to determine and guard this), thereby skipping both the unnecessary getTagIds([]) call and the empty entry in tagsToAttach when no tags are present.
164-177: ⚡ Quick winGuard against YAML node remaining in output if parent/index are missing.
If
parentorindexareundefined, the YAML node won't be removed from the AST butnoteMetawill still be parsed. This would result in the YAML frontmatter appearing in the final note content. Whileunist-util-visitshould always provide parent/index when traversing withremark-parse, a defensive check could prevent unexpected output.🛡️ Proposed defensive handling
visit(mdTree, (node, index, parent) => { if (node.type === 'yaml') { const rawMeta = parseYaml(node.value); noteMeta = RawNoteMetaScheme.parse(rawMeta); // Remove header node with meta data if (parent && index !== undefined) { parent.children.splice(index, 1); + } else { + console.warn('YAML node missing parent or index - frontmatter not removed from content'); } return EXIT; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/core/storage/interop/import/index.ts` around lines 164 - 177, The YAML frontmatter visit block can leave the YAML node in the AST if parent or index are undefined; update the visit callback (the visit/mdTree handler that calls parseYaml and RawNoteMetaScheme.parse) to defensively remove or neutralize the YAML node when parent/index are missing: if parent && index are present keep the existing parent.children.splice(index,1) path, otherwise clear the node by replacing its value/type (e.g. set node.value = '' and node.type = 'text' or similar neutralization) so the frontmatter won't appear in output; optionally emit a debug/warn (processLogger or console) indicating a malformed AST when parent/index are absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/app/src/core/database/sqlite/SQLiteDatabase.ts`:
- Around line 106-108: The transaction overload in SQLiteDatabase.ts wrongly
declares a return type of Promise<void>; change the signature of transaction<T
extends unknown>(cb: (tx: SQLiteTransaction) => Promise<T>): Promise<T> so it
returns the callback result type T (matching the interface in index.ts and tests
in SQLiteDatabase.test.ts); update the declaration for the overload in the
SQLiteDatabase class (the transaction method) to return Promise<T> so the
implementation that returns cb(...) .finally(transaction.close) correctly
preserves and types the callback's return value.
In `@packages/app/src/core/database/sqlite/SQLiteDatabaseWorker.ts`:
- Around line 35-37: The transaction overload signature currently declares
Promise<void> but should propagate the callback's generic return type; update
the declaration of transaction<T extends unknown>(cb: (tx: SQLiteTransaction) =>
Promise<T>): Promise<void> to return Promise<T> instead so the overload matches
the interface in index.ts and preserves the callback's return value (ensure you
change the return type for the transaction generic overload in
SQLiteDatabaseWorker to Promise<T>).
In
`@packages/app/src/core/features/notes/controller/__tests__/NotesController.bench.ts`:
- Around line 32-33: The benchmark opens a SQLite handle with openSQLite(...)
returning managedDB but never closes it; update the benchmark teardown (the
afterEach/afterAll or the teardown function used in these tests) to call
managedDB.close() (or the appropriate close/destroy method on the managedDB
object) after the run so resources are released; locate where managedDB is
created (the lines with openSQLite and managedDB.get()) and add a teardown hook
to invoke managedDB.close() and handle any errors.
In `@packages/app/src/core/features/notes/controller/index.ts`:
- Around line 111-114: The JSDoc above updateBatch is misleadingly describing a
single-note update; update the comment for the updateBatch(notes:
NoteContentUpdateInfo[]): Promise<void> method to state that it updates multiple
notes in a single operation (e.g., "Update multiple notes by their unique ids"
or similar), mention that it accepts an array of NoteContentUpdateInfo and
returns a Promise<void>, and remove any singular "note" wording copied from
update().
In `@packages/app/src/core/features/notes/history/NoteVersions.ts`:
- Around line 58-69: batchSnapshot currently builds SQL with qb.values(noteIds)
even when noteIds is an empty array which produces invalid IN (...) clauses; add
an early guard at the start of NoteVersions.batchSnapshot to check if
noteIds.length === 0 and return an appropriate empty result (or
Promise.resolve([]) / no-op) instead of proceeding, and apply the same guard
wherever qb.values(noteIds) is used in this file (the other query-building sites
around the same logic) to avoid constructing IN () queries; ensure callers
relying on the return shape still receive the expected empty structure.
In `@packages/app/src/core/features/tags/controller/TagsController.ts`:
- Around line 333-372: The method setAttachedTagsInTransaction must await the
transaction and stop manually issuing BEGIN/COMMIT/ROLLBACK inside the
transaction callback: change the call to await this.db.transaction(...) so the
function waits for completion and any errors propagate to the caller, and remove
the explicit db.query(qb.sql`BEGIN`), db.query(qb.sql`COMMIT`) and
db.query(qb.sql`ROLLBACK`) calls inside the transaction callback (leave only the
DELETE, INSERT logic using wrapSQLite(tx) and qb.* helpers). After the awaited
transaction completes, call this.onChanged('noteTags'); do not catch and rethrow
transaction errors inside the callback — let the transaction helper handle
rollback and error propagation.
In `@packages/app/src/core/storage/interop/import/index.ts`:
- Around line 300-346: The batch import must run atomically: wrap the parallel
URL-replace/attachments work (the Promise.all over notesSlice that calls
replaceUrls and attachmentsRegistry.set), the subsequent
notesRegistry.updateBatch(updates), and noteVersions.batchSnapshot(noteIds)
inside one database transaction so all three operations commit or all are rolled
back; replicate the approach used by setAttachedTagsInTransaction (or the
project’s transaction helper) to obtain a transactional context, perform the
Promise.all and then call attachmentsRegistry.set, notesRegistry.updateBatch,
and noteVersions.batchSnapshot within that same transaction, and ensure any
thrown error causes a rollback rather than leaving attachments persisted while
note content/versions fail.
---
Outside diff comments:
In `@packages/app/src/core/features/notes/history/NoteVersions.ts`:
- Around line 97-107: The subquery aliased as last in NoteVersions.ts selects a
single global latest row from note_versions (ORDER BY ... LIMIT 1) instead of
the latest row per note_id; change it to select the latest version for each
note_id (e.g. use a window function like ROW_NUMBER() OVER (PARTITION BY
v.note_id ORDER BY v.created_at DESC, rowid DESC) and then filter for row_number
= 1, or use DISTINCT ON (v.note_id) if your DB supports it) so that the JOIN
compares each note against its own latest version when using qb.values(noteIds)
and workspace filtering.
---
Nitpick comments:
In `@packages/app/src/core/storage/interop/import/index.ts`:
- Around line 287-350: The batch loop only calls checkForAbortion() once per
batch; to make abortion responsive, call checkForAbortion() inside each per-note
handler in the notesSlice.map (e.g., at the start of the
Promise.resolve().then(...) body and again before heavy ops like replaceUrls,
before pushing into updates, or before attachmentsRegistry.set) so the handler
throws immediately when aborted; this ensures Promise.all short-circuits with an
exception and the batch stops processing further notes. Use the existing
checkForAbortion() helper (or abortSignal.aborted) within the per-note async
function in the notesSlice.map and throw to propagate cancellation up to the
outer loop.
- Around line 226-245: The code currently calls getTagIds(tagNamesToAttach) and
pushes a tagsToAttach entry even when tagNamesToAttach is empty; update the
block around convertPathToTag/tagNamesToAttach so that you only call
this.getTagIds(...) and push into tagsToAttach when tagNamesToAttach.length > 0
(use the existing variables tagNamesToAttach, noteDirPath, convertPathToTag and
noteId to determine and guard this), thereby skipping both the unnecessary
getTagIds([]) call and the empty entry in tagsToAttach when no tags are present.
- Around line 164-177: The YAML frontmatter visit block can leave the YAML node
in the AST if parent or index are undefined; update the visit callback (the
visit/mdTree handler that calls parseYaml and RawNoteMetaScheme.parse) to
defensively remove or neutralize the YAML node when parent/index are missing: if
parent && index are present keep the existing parent.children.splice(index,1)
path, otherwise clear the node by replacing its value/type (e.g. set node.value
= '' and node.type = 'text' or similar neutralization) so the frontmatter won't
appear in output; optionally emit a debug/warn (processLogger or console)
indicating a malformed AST when parent/index are absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 596532c9-ce3f-480e-b3f0-4fcda359795f
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpackages/app/src/core/storage/interop/import/__snapshots__/NotesImporter.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (38)
packages/app/package.jsonpackages/app/scripts/webpack/app.tspackages/app/src/__tests__/utils/vaultContext.tspackages/app/src/core/database/sqlite/SQLiteDatabase.test.tspackages/app/src/core/database/sqlite/SQLiteDatabase.tspackages/app/src/core/database/sqlite/SQLiteDatabase.worker.tspackages/app/src/core/database/sqlite/SQLiteDatabaseWorker.tspackages/app/src/core/database/sqlite/index.tspackages/app/src/core/database/sqlite/utils/wrapDB.tspackages/app/src/core/features/attachments/AttachmentsController.tspackages/app/src/core/features/files/FilesController.tspackages/app/src/core/features/notes/bin/DeletedNotesController.test.tspackages/app/src/core/features/notes/controller/NotesController.filters.test.tspackages/app/src/core/features/notes/controller/NotesController.test.tspackages/app/src/core/features/notes/controller/NotesController.tspackages/app/src/core/features/notes/controller/NotesTextIndexer.test.tspackages/app/src/core/features/notes/controller/__tests__/NotesController.bench.tspackages/app/src/core/features/notes/controller/index.tspackages/app/src/core/features/notes/history/NoteVersions.tspackages/app/src/core/features/tags/controller/TagsController.tspackages/app/src/core/features/workspaces/WorkspacesController.test.tspackages/app/src/core/features/workspaces/WorkspacesController.tspackages/app/src/core/storage/interop/import/NotesImporter.test.tspackages/app/src/core/storage/interop/import/NotesImporter.worker.tspackages/app/src/core/storage/interop/import/NotesImporterWorker.tspackages/app/src/core/storage/interop/import/deepSerialize.tspackages/app/src/core/storage/interop/import/index.tspackages/app/src/features/App/Settings/sections/WorkspaceSettings.tsxpackages/app/src/features/App/Vault/index.tsxpackages/app/src/features/App/Vault/services/useBinService.tsxpackages/app/src/features/App/Vault/services/useFilesIntegrityService.tsxpackages/app/src/features/App/Vaults/hooks/useVaultContainers.tspackages/app/src/features/App/Workspace/WorkspaceErrorScreen.tsxpackages/app/src/features/App/Workspace/useWorkspace.tspackages/app/src/features/MainScreen/WorkspacesPanel/WorkspaceCreatePopup.tsxpackages/app/src/features/MainScreen/WorkspacesPanel/useWorkspacesList.tsxpackages/app/src/hooks/notes/useNotesImport.tsxwords.txt
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/app/src/features/App/Settings/sections/ImportAndExport.tsx (1)
89-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDisable export while import is in progress.
Line 91 only disables export during export activity, not during import, so users can export mid-import and potentially capture incomplete state.
Suggested fix
<Button size="sm" - isDisabled={notesExport.progress !== null} + isDisabled={notesExport.progress !== null || importProgress !== null} onClick={async () => { + if (importProgress !== null) return; await notesExport.exportNotes( buildFileName(workspaceData?.name, 'backup'), ); }} >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/app/src/features/App/Settings/sections/ImportAndExport.tsx` around lines 89 - 99, The export Button currently disables only when notesExport.progress !== null, allowing exports during an import; update the disabled condition on the Button in ImportAndExport.tsx to also check the import progress (e.g., disable when notesImport?.progress !== null OR notesExport.progress !== null) so that exports are blocked while an import is in progress; keep the onClick calling notesExport.exportNotes(buildFileName(workspaceData?.name, 'backup')) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/app/src/locales/en/settings.json`:
- Line 244: Update the English locale string for the "directory" key in
settings.json to read "Import a directory with Markdown files" (replace the
current "Import directory with Markdown files") so the label is grammatically
correct; locate the "directory" property in the JSON (key: "directory") and
change its value accordingly.
---
Outside diff comments:
In `@packages/app/src/features/App/Settings/sections/ImportAndExport.tsx`:
- Around line 89-99: The export Button currently disables only when
notesExport.progress !== null, allowing exports during an import; update the
disabled condition on the Button in ImportAndExport.tsx to also check the import
progress (e.g., disable when notesImport?.progress !== null OR
notesExport.progress !== null) so that exports are blocked while an import is in
progress; keep the onClick calling
notesExport.exportNotes(buildFileName(workspaceData?.name, 'backup')) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b288d28e-82c4-4f12-bbed-ee1860a68e35
📒 Files selected for processing (3)
packages/app/src/features/App/Settings/sections/ImportAndExport.tsxpackages/app/src/hooks/notes/useNotesImport.tsxpackages/app/src/locales/en/settings.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/app/src/hooks/notes/useNotesImport.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #125
Key changes
TODO
Summary by CodeRabbit
New Features
Chores