Repository navigation
feat(reader): render sources through a PDF-first reader contract - #116
Merged
Merged
Conversation
The document viewer was a `<pre>` of concatenated chunk text: overlap duplicated at every boundary, the parser's cleaned form instead of the source, no page, no selection mapping, and no way to "jump to page 5". This replaces it with a reader surface that renders whatever structure a source has. - **One contract, two implementations (#71).** `ReaderHandle` exposes `openAt({documentId, page?, blockId?, startOffset?, endOffset?})` and `getSelection()`. PDF is the full implementation (continuous pages, zoom, a pdfjs text layer for real text selection, a block overlay); every other format uses a text reader. #72 and #73 talk to the contract, never to a format. - **Bytes by id, never by path.** `knownote-doc://docs/<documentId>` is resolved against `documents.localFilePath` in the main process. The renderer never builds a filesystem path and an unknown id is a 404. The scheme is registered before app-ready and CORS-reflected only to the app's own origins. - **Blocks travel with the source.** A new `knowledge:get-document-blocks` IPC returns page / char span / normalized bbox, so `openAt` can scroll to a page and highlight the paragraph instead of re-deriving geometry from chunk text. - **Selection maps back to a source location.** The DOM-selection → anchor mapping is a pure function (`anchor.ts`) and unit-tested, because it is what #73 will build excerpts on. Verification: typecheck, 143 tests (10 new), `npm run build`, `build:unpack`, `smoke:packaged` (19 checks, including a new `knownote-doc://` byte-serving and 404 check that runs in the real main process). Not verified here (no GUI): actual page rendering, selectable text, page navigation/zoom, `openAt` landing, and highlighting are Manual QA in the PR. Refs #71
mrsibe
added a commit
that referenced
this pull request
Sep 25, 2026
…e reverse dependency (#126) * docs(arch): record the seams v1.4 actually produced, and close the one reverse dependency Closes #60. This was an acceptance audit before it was anything else: after #66–#73 the seams the issue planned had mostly already formed, and two of its four acceptance items were satisfied. The work is therefore a documentation update plus **one narrow code fix** — not the architecture refactor the title suggests. **Acceptance outcome** | Item | Before | | --- | --- | | `architecture.md` names the three seams, their implementations, and the IPC → service → db call path | **Partial** — the Retrieval seam was documented, the Document and Application seams were never named, and no call path appeared anywhere | | Every v1.4 module sits behind one of the seams | **Met**, now recorded | | No new service reaches `getDatabase()` from outside the Document/Application layers | **One instance** — fixed below | | The document lists the seams considered and rejected | **Not met** — added | **The seams are named as they exist, not as the issue guessed.** #60 proposed `DocumentParser`, `SourceStore` and `CitationService`. None of the three exists, and none was needed: - parsing is `IDocumentLoader` (already an interface, five implementations) plus `FileParserService` for dispatch, so `DocumentParser` would be duplicate abstraction; - there is no storage consumer independent enough for `SourceStore` — the provenance helpers take the database handle explicitly and `KnowledgeService` owns Document persistence orchestration; - `CitationService` was never needed because citation parsing, resolution and source-anchor mapping are stateless and shared by main *and* renderer; pure functions in `src/shared/utils/` are the right shape. Application is documented as an **architectural boundary, not a class**, so nobody goes looking for the `ApplicationService` that this document would otherwise imply. **The one code fix.** `src/main/protocol/documentProtocol.ts` (added by #116) queried `documents.localFilePath` itself, which made an Electron plumbing module depend on the database from outside both permitted layers — the only such instance in v1.4 code. Writing it off as a "known exception" would mean accepting a fresh architectural exception at the moment this issue closes, and the fix is cheap, so it is fixed: - `KnowledgeService.getDocumentLocalFilePath(documentId): string | null` — a narrow query rather than reusing `getDocument()`. The handler needs "a readable file for this id", not a whole `Document` row, so handing it the row would widen the dependency surface for nothing. No `SourceStore`-style repository was invented for it; it is one method on the existing Document seam. - `registerDocumentProtocolHandler()` now takes the resolver, so the protocol layer imports no database at all. The dependency direction is now `protocol → Document layer → getDatabase() → Drizzle/SQLite`. - `src/main/index.ts` and the packaged smoke test both inject it. The main-process wiring reads `knowledgeService` lazily because it is constructed later in startup; that is noted at the call site rather than reordering startup for it. **A guard, not a comment.** `test/architectureBoundary.test.ts` fails if any file under `src/main/protocol/` imports the database, Drizzle, or the schema, or calls `getDatabase()` — with a self-check so it cannot pass by scanning nothing. Verified by injecting the import back: the guard reports `documentProtocol.ts imports ../db` and fails. **Accuracy note recorded in the document.** Three handlers (`noteHandlers`, `notebookHandlers`, part of `chatHandlers`) still call `db/queries` instead of a service. That predates v1.4 and #60's non-goals forbid removing a legacy path before a real feature exercises the replacement, so it is documented as a known legacy exception rather than silently implied to be clean. No handler calls `getDatabase()` itself. Verification: `npm run typecheck` (all three projects), `npm test` (261 pass, 3 new for the boundary guard), `npm run check:design`, `npm run lint` (0 errors, baseline unchanged), `npx prettier --check`, and `npm run build:unpack && npm run smoke:packaged` **executed** — 20/20 checks, including `knownote-doc:// serves an owned document and 404s an unknown id` through the newly injected resolver, which is the regression that matters for this change. Not touched, per the issue's non-goals: `KnowledgeService`'s structure, `SourceStore`, a `CitationService`, directory layout, the Model seam (#44 owns it), and the legacy `db/queries` handlers. * test(arch): match the whole db subtree in the protocol boundary guard The matcher only caught an exact `../db` plus the `/db` and `/db/schema` suffixes, so `../db/queries` passed the scan - even though `docs/architecture.md` records `db/queries` as exactly the kind of database-reaching path this guard exists to catch. A guard that misses a case its own documentation names is worse than no guard: it reports an invariant as enforced while leaving it open. `/(^|\/)db(?:\/|$)/` treats the whole db subtree as the boundary, covering `../db`, `../db/schema`, `../db/queries` and `../../db/foo`. It still does not fire on `db-utils` or `@shared/dbx`, and a table-driven test pins both directions so the rule cannot quietly become either leaky or noisy. Verified by injecting the exact miss back: `import { getNotesByNotebook } from '../db/queries'` in documentProtocol.ts now fails with `documentProtocol.ts imports ../db/queries`, and passes again once restored. The seam work itself is unchanged: the narrow KnowledgeService query, the injected resolver and the architecture document are as reviewed.
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.
What does this PR do?
Replaces the chunk-concatenation
<pre>document viewer with a reader surface that renders the source's own structure. PDF gets continuous page rendering, zoom, a real text layer and a block overlay; every other format uses a text reader behind the same contract. This is the spike-approved path (rendererpdfjs-dist+?urlworker bundles cleanly; the main-processDOMMatrixissue does not affect the renderer).Why?
The old viewer duplicated overlap at every chunk boundary, showed the parser's cleaned text rather than the PDF, had no page or selection mapping, and made "jump to page 5" impossible. #72 (citation click) and #73 (excerpt to note) need a format-agnostic
openAt/getSelectionseam to build on.Related issue
Fixes #71
What changed?
src/shared/types/source.ts+src/shared/utils/documentUrl.ts—ReaderAnchor/ReaderSelection/ReaderHandle,SourceBlock, and theknownote-doc://docs/<id>URL shape.src/main/protocol/documentProtocol.ts— privileged scheme + a handler that servesdocuments.localFilePathby id; unknown ids are 404. CORS is reflected only for the app's own origins.src/main/services/KnowledgeService.ts—getDocumentBlocksis exposed over a newknowledge:get-document-blocksIPC (page / span / normalized bbox).src/renderer/.../source/reader/—SourceReader(chooser),PdfSourceReader(canvas + pdfjsTextLayer+ highlight overlay + page/zoom toolbar),TextSourceReader(same contract viaRange), andanchor.ts(the pure selection→location mapping).SourcePanel'sDocumentViewerPanelnow mounts the reader.connect-src knownote-doc:,worker-src 'self' blob:.smokeTestgains a real-main-process check: an owned document serves its bytes over the protocol and an unknown id is a 404.docs/architecture.mddocuments the reader seam.Design notes
SourceReaderis the only place that branches on format;openAt/getSelectionare identical for both implementations, so [Feat] Citation click → open source at page/block with highlight #72/[Feat] Selection → excerpt to Note, anchored back to its source #73 never branch.How was this tested?
npm run typecheck— passes.npm test— 143 tests pass (10 new: URL parsing/round-trip, block-at-offset, block-at-point with page + smallest-area selection, selection→anchor, block rect).npm run check:design— passes.npm run build— passes; the pdfjs worker is emitted as its own asset.npm run build:unpackandnpm run smoke:packaged— pass (19 checks), including the newknownote-doc://protocol check.Manual QA (not verifiable headlessly — please run in a real Electron window)
These are the #71 acceptance items that need a human at a display. I am not claiming they pass:
Esc/ back returns to the document list.knownote-doc://docs/<id>resolves (covered automatically, but worth an eyeball in DevTools).Screenshots / recordings
Required for UI changes — please add before/after here when running the manual QA.
Checklist
npm run typecheckpasses.npm run buildpasses.Desktop / build changes
npm run build:unpackpasses.npm run smoke:packagedpasses.