Repository navigation
feat: reuse a source across notebooks from a library - #217
Merged
Merged
Conversation
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
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.
What does this PR do?
A source is now a library-level entity and a notebook holds a membership of it, so one imported paper can be used by two notebooks without being stored or embedded twice.
library_sourcesholds the durable snapshot: canonical text, parse structure, original URI and the library-owned copy of the file.documentsstays the per-notebook membership. Existing rows keep their ids, so every recorded citation, excerpt and retrieval scope keeps resolving.Why?
Sources belonged to exactly one notebook, so reading the same paper in two contexts meant importing it twice, storing it twice and embedding it twice — with the two copies drifting as soon as one was re-indexed. This is the storage and indexing half of #99 (the part that had to be decided before any UI existed); the design is recorded in
docs/adr/0002-library-source-reuse.md.Two answers the issue asked for, in writing:
e5-smalland B something else? The membership landspendingand says indexing is required. Nothing is embedded automatically, and no existing vectors are cleared.Related issue
Fixes #99
What changed?
0023_sleepy_luckman): newlibrary_sourcestable, nullabledocuments.source_id, and a(notebook_id, source_id)unique index. The migration backfills one snapshot per existing document, one-to-one — unrelated sources are deliberately not merged by title or path.src/main/services/librarySources.ts(new, Electron-free): list, attach and permanent-delete, plus the synchronous clone of blocks, chunks, chunk↔block mappings, embedding metadata, vectors and full-text rows.resolveReuse()is the single reuse decision, shared by the listing and the attach, so what the picker promises is what attaching does.KnowledgeService: snapshots are created after parsing/content is known and before indexing; file refresh is copy-on-write so a shared snapshot's text and page offsets can never move under another notebook's citations;deleteDocumentis now detach-only.deleteNotebook: no longer unlinks files, which a shared snapshot may still own.knowledge:list-library-sources,knowledge:attach-library-source,knowledge:delete-library-source; a "From library" picker that distinguishes ready-to-reuse from needs-indexing; deletion copy changed to "Remove from notebook"; EN and zh-CN strings. The source-add menu now only disables the import actions that need an embedding model, not the whole menu.docs/architecture.md.How was this tested?
npm run typecheck,npm test(555 tests),npm run build,npm run check:design,npm run check:version.npm run build:unpack+npm run smoke:packaged— 31 checks pass, including two new packaged-service checks that drive the realKnowledgeService: import into notebook A, attach to B with an embedding backend that throws if called, retrieve from B, compare page/span provenance, read the reused source throughknownote-doc://, then refresh A (copy-on-write, A's shared file unchanged and B's text unchanged), remove A's membership (B still retrieves), delete notebook A (file intact), and confirm that permanent deletion is refused while attached and without confirmation.test/librarySourceReuse.test.ts(real SQLite + sqlite-vec: clone with preserved provenance, empty-target space adoption, space mismatch → pending, incompatible target vectors untouched, idempotent attach, incomplete donor coverage → pending, half-refreshed donor not reused, clone writes FTS rows, unparsed snapshot visible but unusable, listing and attaching agree),test/librarySourceBackfill.test.ts(runs the real migration statements, including idempotency),test/librarySourceGuards.test.ts(source-level direction checks, now parsed with the TypeScript AST so they inspect method bodies rather than an object return type),test/librarySourceWatch.test.ts(watcher ignores snapshot-only memberships),test/librarySourceUi.test.ts(IPC validation + picker/locale contracts).window.api.knowledge.{listLibrarySources,attachLibrarySource,deleteLibrarySource,deleteDocument}are all present in the packaged preload, the renderer mounts, and the packaged artifact contains both locales' new strings. This environment has no GUI screenshot tooling (headless WSL, noscrot/xwd/import), so the screenshots below are omitted rather than fabricated; the UI itself was verified by typecheck, ESLint, the build and the packaged-artifact checks above.Screenshots / recordings
Not available in this environment (no screenshot tooling); see the verification note above.
Checklist
npm run typecheckpasses.npm run buildpasses.Desktop / build changes
npm run build:unpackpasses.npm run smoke:packagedpasses.Known limitations
pendingand needs an explicit re-index.#95limitation): a crash during replacement, not a failure before it, can leave a membership without a complete index.