feat: add PDF annotation reading with preview panel and sidebar stack - #1079
Open
RobinQu wants to merge 13 commits into
Open
feat: add PDF annotation reading with preview panel and sidebar stack#1079RobinQu wants to merge 13 commits into
RobinQu wants to merge 13 commits into
Conversation
Store PDF annotations as app-managed sidecar JSON under .reflect/annotations/ (mirroring the transcript cache pattern): annotation_read/annotation_write Tauri commands with sanitized, collision-proof filenames and atomic writes, plus typed zod-validated wrappers in @reflect/core.
Open PDF annotation links (assets/*.pdf#page=N) in a resizable right panel instead of the broken system-opener path: pdf.js viewer (legacy build, isEvalSupported off for CSP) with normalized-rect highlight overlay, drag-to-create annotations persisted to the sidecar, an annotation list, and a resident note preview mode for embedded blocks (reflect://preview/open deep link). Fixes the stale #fragment bug in the asset-open path.
…d annotation actions
- pin annotation sidecar writes to the graph session generation - translate comments to English for consistency with the upstream codebase - use the shadcn DropdownMenu primitive for the annotation context menu
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis change adds an in-app PDF preview with pdf.js rendering, annotation creation and persistence, PDF deep links, preview-panel routing, PDF navigation, and resizable preview and annotation panes. ChangesPDF preview and annotation workflow
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant NoteEditor
participant PreviewPanelProvider
participant WorkspaceContent
participant PdfViewerShell
participant AnnotationStore
participant TauriFilesystem
NoteEditor->>PreviewPanelProvider: Open PDF preview target
PreviewPanelProvider->>WorkspaceContent: Publish target
WorkspaceContent->>PdfViewerShell: Load and render PDF
PdfViewerShell->>AnnotationStore: Load annotations
AnnotationStore->>TauriFilesystem: Read annotation sidecar
TauriFilesystem-->>AnnotationStore: Return sidecar JSON
AnnotationStore-->>PdfViewerShell: Provide annotations
PdfViewerShell->>AnnotationStore: Persist annotation changes
AnnotationStore->>TauriFilesystem: Atomically write sidecar
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
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: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/editor/use-asset-persistence.ts (1)
35-57: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winNormalize the path before passing it to asset consumers.
isSafeAssetSourcestrips the location suffix only for validation.resolveImageUrlstill passesassets/paper.pdf#page=3toconvertFileSrc, andresolveAssetOpenPathreturns that same non-file path toopenAssetCommand.Create one normalizer that strips the raw suffix before decoding. Use its result for validation and every returned asset path. Apply the same normalized key in
resolveFileInfo, or fragment-bearing file pills will miss the directory-listing cache.Also applies to: 160-173
🤖 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 `@apps/desktop/src/editor/use-asset-persistence.ts` around lines 35 - 57, Update stripLocationSuffix and the asset-resolution flow to normalize the raw source before decoding, then use that normalized path for isSafeAssetSource validation and every returned asset path in resolveImageUrl and resolveAssetOpenPath. Apply the same normalized key in resolveFileInfo so fragment-bearing references resolve the existing directory-listing cache entry.
🧹 Nitpick comments (19)
apps/desktop/src/components/preview/annotation-list.tsx (2)
79-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpose the selected state to assistive technology.
The selected row is indicated only by the
bg-surface-activebackground class. A screen-reader user cannot perceive which annotation is selected. Addaria-pressedto the row button so the selection is announced.♿ Proposed change
<button type="button" + aria-pressed={selectedId === item.id} className={cn(🤖 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 `@apps/desktop/src/components/preview/annotation-list.tsx` around lines 79 - 88, Add an aria-pressed attribute to the annotation row button in the item rendering, using the existing selectedId === item.id condition so assistive technology receives the current selection state.
56-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the single-character sort parameters and simplify the grouping.
Line 65 uses the destructured names
aandb. The coding guidelines forbid single-character variable names. Rename them.The grouping loop also copies the whole group array for each item, so it is O(n²) for a page with many annotations. Push into a mutable array instead.
Array.from(byPage)then replaces the manualpagesaccumulation.♻️ Proposed refactor
- const byPage = new Map<number, readonly AnnotationItem[]>() + const byPage = new Map<number, AnnotationItem[]>() for (const item of annotations) { const existing = byPage.get(item.pageIndex) - byPage.set(item.pageIndex, existing === undefined ? [item] : [...existing, item]) + if (existing === undefined) { + byPage.set(item.pageIndex, [item]) + } else { + existing.push(item) + } } - const pages: Array<[number, readonly AnnotationItem[]]> = [] - for (const [pageIndex, items] of byPage) { - pages.push([pageIndex, items]) - } - pages.sort(([a], [b]) => a - b) + const pages: Array<[number, readonly AnnotationItem[]]> = Array.from(byPage) + pages.sort(([leftPage], [rightPage]) => leftPage - rightPage)As per coding guidelines: "Never use single-character variable names."
🤖 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 `@apps/desktop/src/components/preview/annotation-list.tsx` around lines 56 - 65, In the annotation grouping logic, change the per-page accumulator to mutable arrays and append items with push instead of recreating each group via spread. Replace the manual pages array and second iteration with Array.from(byPage), and update the sort comparator in that flow to use descriptive destructured parameter names instead of a and b.Source: Coding guidelines
apps/desktop/src/components/preview/pdf-viewer-shell.test.tsx (2)
73-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the load-failure path.
Every test here makes
readAssetBinaryresolve. No test makes it reject. The failure path is where the shell's recovery behavior is weakest: see the error-state issue raised onpdf-viewer-shell.tsxLines 170-184. A test that rejectsreadAssetBinary, asserts therole="alert"message, then re-renders with a secondassetPathand asserts the new document loads would guard that fix.🤖 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 `@apps/desktop/src/components/preview/pdf-viewer-shell.test.tsx` around lines 73 - 97, Add a PdfViewerShell test that mocks readAssetBinary to reject, asserts the rendered role="alert" error message, then re-renders with a different assetPath and verifies the replacement document loads successfully. Keep the existing successful-load coverage unchanged and exercise the component’s recovery behavior after the failure.
60-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not silence all
console.warnoutput for the whole file.Line 63 replaces
console.warnfor every test in this file. Any unexpected warning from the shell, from React, or from pdf.js is then hidden, and the repository's console policy no longer applies here. The guidelines direct justified exceptions toapps/desktop/src/test-utils/allowed-console.ts.Add the pdf.js fake-worker message to the allowlist with a justification, or filter on that one message and forward everything else.
♻️ Proposed narrowing, if the allowlist entry is not preferred
- vi.spyOn(console, 'warn').mockImplementation(() => {}) + const warn = console.warn.bind(console) + vi.spyOn(console, 'warn').mockImplementation((...args: unknown[]) => { + if (typeof args[0] === 'string' && args[0].includes('Setting up fake worker')) { + return + } + warn(...args) + })As per coding guidelines: "Console warnings and errors fail tests; only shrink the allowlist in
apps/desktop/src/test-utils/allowed-console.ts, and justify any new entry."🤖 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 `@apps/desktop/src/components/preview/pdf-viewer-shell.test.tsx` around lines 60 - 66, Replace the broad console.warn mock in beforeEach with a scoped solution: add the exact pdf.js fake-worker warning to apps/desktop/src/test-utils/allowed-console.ts with a justification, or filter only that message while forwarding all other warnings. Preserve visibility of unexpected warnings from the shell, React, and pdf.js.Source: Coding guidelines
apps/desktop/src-tauri/src/fs/mod.rs (1)
573-602: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving the directory creation out of the path helper.
annotation_sidecarcreates.reflect/annotations/on every call, including reads. A read of a PDF that has no sidecar therefore creates a directory as a side effect. The write path does not need it either:stage_bytesalready callsfs::create_dir_all(dir)for the target's parent before it stages the temp file. Deriving the path without touching the filesystem keeps the helper pure and keeps read commands free of writes.♻️ Proposed refactor
let path = resolve(root, &format!(".reflect/annotations/{name}.json"))?; - if let Some(parent) = path.parent() { - fs::create_dir_all(parent)?; - } Ok(path)The existing test asserts the directory exists, so it would need to assert the parent path instead:
assert!(path.starts_with(graph.path().join(".reflect/annotations")));🤖 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 `@apps/desktop/src-tauri/src/fs/mod.rs` around lines 573 - 602, Remove directory creation from annotation_sidecar so it only derives and validates the sidecar path without filesystem writes; stage_bytes already creates the target parent for writes. Update the related test to assert the returned path is under .reflect/annotations rather than asserting the directory exists.apps/desktop/src/lib/annotations/annotation-reference.ts (1)
23-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a descriptive callback parameter.
Rename
ctocharacter.As per coding guidelines, “Never use single-character variable names.”
🤖 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 `@apps/desktop/src/lib/annotations/annotation-reference.ts` at line 23, Rename the single-character callback parameter in the title escaping expression to the descriptive name character, and update its interpolation reference accordingly. Preserve the existing replaceAll behavior.Source: Coding guidelines
apps/desktop/src/lib/annotations/annotations-store.ts (1)
70-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the exported annotation types.
Add TSDoc for
PdfAnnotationsStatusandUsePdfAnnotationsResult. Add interface-level documentation forAnnotationItemandAnnotationFile.As per coding guidelines, “Always document public APIs.”
🤖 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 `@apps/desktop/src/lib/annotations/annotations-store.ts` around lines 70 - 76, Document the exported annotation API types with TSDoc: add interface-level descriptions for AnnotationItem and AnnotationFile, and document PdfAnnotationsStatus and UsePdfAnnotationsResult. Keep the existing type shapes unchanged and ensure each public type clearly describes its purpose.Source: Coding guidelines
apps/desktop/src/lib/annotations/pdf-region-text.test.ts (1)
27-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the single-character parameter
r.The coding guidelines forbid single-character variable names in TypeScript files. Rename
rtorect.♻️ Proposed rename
- convertToViewportRectangle: (r: number[]) => [ - r[0] ?? 0, - 200 - (r[3] ?? 0), - r[2] ?? 0, - 200 - (r[1] ?? 0), - ], + convertToViewportRectangle: (rect: number[]) => [ + rect[0] ?? 0, + 200 - (rect[3] ?? 0), + rect[2] ?? 0, + 200 - (rect[1] ?? 0), + ],🤖 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 `@apps/desktop/src/lib/annotations/pdf-region-text.test.ts` around lines 27 - 32, Rename the convertToViewportRectangle parameter from r to rect and update all references within that function accordingly, preserving the existing coordinate conversion behavior.Source: Coding guidelines
apps/desktop/src/lib/resolve-note-preview-body.ts (1)
5-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAttach the doc comment to the interface and mark the fields readonly.
A blank line separates the JSDoc block from
NotePreviewBody, so editors and API doc tools do not associate them. The coding guidelines also ask for readonly fields on immutable data.♻️ Proposed refactor
-/** - * A note's preview pieces, carved from its freshest source. - */ - -export interface NotePreviewBody { +/** + * A note's preview pieces, carved from its freshest source. + */ +export interface NotePreviewBody { /** The full note source, frontmatter included. */ - source: string + readonly source: string /** The YAML text between the fences, or `null` without a frontmatter block. */ - frontmatter: string | null + readonly frontmatter: string | null /** The markdown body after the frontmatter. */ - body: string + readonly body: string }🤖 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 `@apps/desktop/src/lib/resolve-note-preview-body.ts` around lines 5 - 16, Move the JSDoc directly adjacent to the NotePreviewBody interface declaration, removing the separating blank line, and mark its source, frontmatter, and body properties readonly while preserving their existing types and documentation.Source: Coding guidelines
apps/desktop/src/components/preview/highlight-layer.tsx (1)
233-237: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the non-null assertion on
wrapper.The coding guidelines say to avoid unnecessary type assertions. Capture the created element in a
constso the closure keeps its non-null type.♻️ Proposed refactor
- wrapper = document.createElement('div') - wrapper.className = OVERLAY_CLASS - wrapper.dataset.pageNumber = String(pageNumber) - wrapper.style.cssText = + const created = document.createElement('div') + created.className = OVERLAY_CLASS + created.dataset.pageNumber = String(pageNumber) + created.style.cssText = 'position:absolute; inset:0; z-index:2; pointer-events:none; touch-action:none;' // Drag-creation lives on the wrapper so a page with no annotations can still // receive draws; the handler reads the live mode through the latest ref. - wrapper.addEventListener('pointerdown', (event) => startDrag(wrapper!, pageNumber, event, latest)) - pageElement.append(wrapper) - return wrapper + created.addEventListener('pointerdown', (event) => startDrag(created, pageNumber, event, latest)) + pageElement.append(created) + return created🤖 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 `@apps/desktop/src/components/preview/highlight-layer.tsx` around lines 233 - 237, Update the wrapper creation flow around the pointerdown handler to capture the created element in a const with a non-null type, then use that variable in startDrag and append operations. Remove the unnecessary non-null assertion on wrapper while preserving the existing event behavior.Source: Coding guidelines
apps/desktop/src/editor/note-editor.tsx (1)
414-433: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared PDF preview-target construction.
Lines 360-364 and 425-429 build the same
PreviewPanelTargetfrom aPdfLinkRef. Extract a module-level helper so the two call sites cannot diverge.♻️ Proposed refactor
+function pdfPreviewTarget(ref: PdfLinkRef): PreviewPanelTarget { + return { + kind: 'pdf', + assetPath: ref.path, + ...(ref.page !== undefined ? { page: ref.page } : {}), + } +}Then call
setPreviewPanelTarget(pdfPreviewTarget(pdfHref))in both handlers.🤖 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 `@apps/desktop/src/editor/note-editor.tsx` around lines 414 - 433, Extract the duplicated PDF preview target construction into a module-level helper named pdfPreviewTarget that accepts a PdfLinkRef and returns the corresponding PreviewPanelTarget, preserving the conditional page field. Replace the inline object construction in both handlers, including the anchor-click branch and the code around the existing construction at lines 360-364, with setPreviewPanelTarget(pdfPreviewTarget(pdfHref)).apps/desktop/src/components/context-sidebar/note-actions-section.tsx (1)
67-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReconsider the button label.
"Deepdive PDF" does not follow the verb-object pattern used by the sibling actions in this section: "Pin this note", "Lock note", "Un-pin this note".
Use "Open PDF panel" or "Browse PDF".
🤖 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 `@apps/desktop/src/components/context-sidebar/note-actions-section.tsx` at line 67, Update the label in the note-actions section’s Deepdive PDF action to use a verb-object pattern consistent with its sibling actions, choosing either “Open PDF panel” or “Browse PDF”.apps/desktop/src/providers/preview-panel-provider.test.tsx (2)
28-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the test title.
The title claims the test also covers "defaults to no-op without a provider", but the body renders with
wrapperfor every assertion and never exercises the no-provider path. The dedicated test at Line 57 covers that case.Shorten the title to "opens and closes a target".
🤖 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 `@apps/desktop/src/providers/preview-panel-provider.test.tsx` around lines 28 - 40, Update the test title in the `opens and closes a target` test to remove the untested “defaults to no-op without a provider” claim, leaving the test body unchanged.
66-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the existing
wrapperhelper.
routedis identical towrapperat Line 12. Deleteroutedand passwrappertorenderHook.♻️ Proposed change
describe('usePreviewPanel route clearing', () => { - function routed({ children }: { children: ReactNode }) { - return ( - <RouterProvider initialRoute={{ kind: 'today' }}> - <PreviewPanelProvider>{children}</PreviewPanelProvider> - </RouterProvider> - ) - } - function useHarnessWithRouter() {Then replace
{ wrapper: routed }with{ wrapper }at Lines 83 and 93.🤖 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 `@apps/desktop/src/providers/preview-panel-provider.test.tsx` around lines 66 - 72, Remove the duplicate routed helper and reuse the existing wrapper helper in the renderHook calls at the affected tests. Replace each wrapper: routed reference with wrapper while preserving the current RouterProvider and PreviewPanelProvider setup defined by wrapper.apps/desktop/src/providers/preview-panel-provider.tsx (1)
70-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the
PreviewPanelinterface to avoid a name collision.
apps/desktop/src/components/preview/preview-panel.tsxexports a React component also namedPreviewPanel. Both symbols are public and belong to the same feature area, so any file that needs both must alias one.Rename this interface to
PreviewPanelControlsorUsePreviewPanelResult.🤖 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 `@apps/desktop/src/providers/preview-panel-provider.tsx` around lines 70 - 77, Rename the exported PreviewPanel interface to PreviewPanelControls (or UsePreviewPanelResult) and update all imports, type references, and exports using the interface, while preserving the existing React component name and control API.Source: Coding guidelines
apps/desktop/src/components/preview/pdf-sidebar-block.tsx (1)
259-283: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftConsider lazy thumbnail rendering for large documents.
PdfThumbnailsmounts onePdfThumbnailper page as soon as the "Pages" section expands. Each child immediately callsdoc.getPageandpage.render. For a several-hundred-page PDF this starts that many concurrent render tasks and allocates that many canvases, all inside a scroll container where only a few are visible.The collapsed default limits the blast radius, but the cost still lands the first time a user expands the section.
Gate the render on visibility with an
IntersectionObserver, or window the list.♻️ Sketch: gate the render on visibility inside
PdfThumbnailfunction PdfThumbnail({ doc, pageNumber, onNavigate }: { ... }): ReactElement { const canvasRef = useRef<HTMLCanvasElement>(null) + const buttonRef = useRef<HTMLButtonElement>(null) + const [visible, setVisible] = useState(false) + + useEffect(() => { + const element = buttonRef.current + if (element === null) { + return + } + const observer = new IntersectionObserver((entries) => { + if (entries.some((entry) => entry.isIntersecting)) { + setVisible(true) + observer.disconnect() + } + }) + observer.observe(element) + return () => { + observer.disconnect() + } + }, []) useEffect(() => { + if (!visible) { + return + } let cancelled = false // … - }, [doc, pageNumber]) + }, [doc, pageNumber, visible])🤖 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 `@apps/desktop/src/components/preview/pdf-sidebar-block.tsx` around lines 259 - 283, Update PdfThumbnails/PdfThumbnail so thumbnails render lazily based on viewport visibility, using an IntersectionObserver or list virtualization. Avoid calling doc.getPage or page.render for offscreen pages, while preserving navigation and rendering thumbnails as they enter the scroll container’s viewport.apps/desktop/src/providers/pdf-session-provider.tsx (2)
65-67: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider guarding
clearwith session ownership.
clearresets the session unconditionally. It does not check that the caller owns the session it is clearing.Today this is safe:
PdfViewerShellis keyed onassetPathinapps/desktop/src/components/preview/preview-panel.tsx, and React runs the old tree's effect cleanup before the new tree's effects, soclear()always precedes the nextregister().That ordering is the only thing preventing a stale shell from wiping a live session. If a future change makes shells overlap, the failure is silent: the sidebar renders nothing while a PDF is open.
Accepting the owning
assetPathinclearmakes the invariant enforced rather than assumed.🛡️ Sketch
- const clear = useCallback((): void => { - setSession(EMPTY_SESSION) - }, []) + const clear = useCallback((assetPath: string): void => { + setSession((current) => (current.assetPath === assetPath ? EMPTY_SESSION : current)) + }, [])
PdfViewerShellthen callsclearSession(assetPath)in its cleanup.🤖 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 `@apps/desktop/src/providers/pdf-session-provider.tsx` around lines 65 - 67, Update the session-clearing API around clear to accept the owning assetPath, and only reset the session when that path still owns the active session. Update PdfViewerShell cleanup to call clearSession(assetPath), preserving the existing reset behavior for the matching owner while preventing stale shells from clearing a newer session.
27-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider a discriminated union for
PdfSession.All three fields are independently nullable, but they are always set together and always cleared together.
registerat Line 41 already accepts a non-null triple. Consumers must therefore narrow three fields to prove one fact, asapps/desktop/src/components/preview/pdf-sidebar-block.tsxdoes at Line 126.A union makes the invariant explicit and removes the repeated narrowing.
♻️ Sketch
-export interface PdfSession { - /** The panel's PDFViewer, for jump commands. */ - viewer: PDFViewer | null - /** The loaded PDFDocumentProxy, for outline and thumbnail reads. */ - pdfDocument: PDFDocumentProxy | null - /** The graph-relative `assets/…pdf` path the session belongs to. */ - assetPath: string | null -} - -const EMPTY_SESSION: PdfSession = { viewer: null, pdfDocument: null, assetPath: null } +/** A loaded PDF session, or the empty one while no document is published. */ +export type PdfSession = + | { readonly status: 'empty' } + | { + readonly status: 'loaded' + /** The panel's PDFViewer, for jump commands. */ + readonly viewer: PDFViewer + /** The loaded PDFDocumentProxy, for outline and thumbnail reads. */ + readonly pdfDocument: PDFDocumentProxy + /** The graph-relative `assets/…pdf` path the session belongs to. */ + readonly assetPath: string + } + +const EMPTY_SESSION: PdfSession = { status: 'empty' }
PdfSidebarBlockthen narrows once:if (session.status !== 'loaded' || session.assetPath !== assetPath) { return null }🤖 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 `@apps/desktop/src/providers/pdf-session-provider.tsx` around lines 27 - 36, Change PdfSession to a discriminated union with an explicit unloaded state and a loaded state containing non-null viewer, pdfDocument, and assetPath. Update EMPTY_SESSION and the register/clear session flows to construct the appropriate variants, then adjust consumers such as PdfSidebarBlock to narrow on the session status before accessing loaded fields.Source: Coding guidelines
apps/desktop/src/components/workspace-content.test.tsx (1)
144-176: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the document view while a PDF stays open.
The new tests cover
view === 'pdf'with a PDF target and the closed-preview case. No test coversview === 'document'whilepreviewTarget.kind === 'pdf', which is the statebackToDocumentproduces.That state is where the context aside can end up empty on routes with no context target. I raised the behavior on
apps/desktop/src/components/workspace-content.tsxLines 82-93.Add a case that sets a PDF target, drives the provider to
'document', and asserts what the context aside renders.🤖 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 `@apps/desktop/src/components/workspace-content.test.tsx` around lines 144 - 176, Add a test alongside the existing PDF preview coverage that sets previewPanelState.target to a PDF, drives the workspace provider view to "document" (the state produced by backToDocument), and renders WorkspaceHost through renderWorkspace. Assert the Context aside’s actual expected content in this state, including the no-context-target behavior, and verify the PDF preview remains open as appropriate.
🤖 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 `@apps/desktop/src/components/context-sidebar/note-actions-section.tsx`:
- Line 54: Update the pdfSessionActive guard in note-actions-section.tsx to
require session.assetPath to match previewTarget.assetPath in addition to
confirming a PDF target and loaded document. Align this entry-point check with
PdfSidebarBlock’s existing guard so enterPdf is only available when the PDF
session is usable.
In `@apps/desktop/src/components/error-boundary.tsx`:
- Around line 20-45: Add a resetKey prop to ErrorBoundaryProps and update
ErrorBoundary to clear its error state when that key changes, while preserving
the existing fallback behavior. Pass a stable preview-content key at the
workspace-content ErrorBoundary call site so switching assets resets the
boundary without unmounting the surrounding aside.
In `@apps/desktop/src/components/graph-workspace.tsx`:
- Around line 72-74: Translate the Chinese comments to English without changing
behavior: update the sidebar-stack/PDF-session provider comment in
apps/desktop/src/components/graph-workspace.tsx lines 72-74; update the
PDF-session entry-point comment, including the row-layout and bg-accent/5
rationale, in
apps/desktop/src/components/context-sidebar/note-actions-section.tsx lines
48-50; and update the WorkspaceHost wrapper comment plus the assertion comment
at line 174 in apps/desktop/src/components/workspace-content.test.tsx.
In `@apps/desktop/src/components/preview/highlight-layer.tsx`:
- Around line 240-287: Update startDrag to create an AbortController before
registering the pointer handlers, pass its signal to the window pointermove,
pointerup, and pointercancel listeners, and use a shared cleanup path that
removes the preview and aborts the controller. Handle pointercancel without
adding a selection, and ensure the owning highlight layer aborts any active drag
controller during unmount.
In `@apps/desktop/src/components/preview/pdf-sidebar-block.test.tsx`:
- Around line 62-65: Update the test setup in beforeEach to reset the shared
sessionState.session.viewer page state, including currentPageNumber, before each
test. Preserve the existing sessionStorage clearing and backToDocument mock
reset so assertions depend only on the current test’s interactions.
- Around line 172-174: Update the Chapter 1 assertion in the PDF sidebar test to
use the locator’s direct disabled-state assertion instead of optional-chaining
through element(). This must fail when the button is missing while verifying
that the located button is disabled.
In `@apps/desktop/src/components/preview/pdf-sidebar-block.tsx`:
- Around line 110-121: Update the exported PdfSidebarBlock docblock to
accurately describe that only the PDF actions section uses SidebarSection with
sessionStorage persistence, while Outline and Pages use PdfSection with per-open
preset state. Replace the plain block-content claim with the actual flex-column
and overflow-y-auto layout behavior, while retaining the panel’s tint and
session-matching behavior.
In `@apps/desktop/src/components/preview/pdf-viewer-shell.tsx`:
- Around line 383-406: Replace the manual createPortal and fixed role="dialog"
fullscreen wrapper in PdfViewerShell with the shadcn Dialog primitive from
"`@/components/ui/dialog`", using DialogContent around the recursive
PdfViewerShell. Move the full-bleed styling to DialogContent’s className and
overlayClassName, and control the Dialog open state and close behavior so focus
trapping, aria-modal, and Escape handling come from the primitive.
- Around line 170-184: Update the error rendering in the PDF viewer shell so the
container and viewer element remain mounted when error is non-null, displaying
the error as an overlay instead of replacing them. Preserve the existing error
reset and loading flow in the useEffect, allowing assetPath changes on the
reused PdfViewerShell instance to clear the error and load the next PDF.
In `@apps/desktop/src/components/preview/preview-panel.tsx`:
- Around line 273-283: Update the MarkdownPreview usage in NotePreview to
provide an onLinkClick handler that routes valid PDF asset targets through the
preview-panel flow, while forwarding links not consumed by that routing to the
existing external-link handler. Preserve the current wiki-link behavior and use
the component’s existing routing symbols rather than adding a separate
navigation path.
In `@apps/desktop/src/components/workspace-content.test.tsx`:
- Around line 20-22: Update the previewPanelState declaration in vi.hoisted to
use the production PreviewPanelTarget union via a type-only import, replacing
the widened inline object type while preserving the null initial state.
- Around line 37-38: Remove the stale “until preview-panel lands” wording near
the workspace-content test mock, but retain the mock itself. Replace the comment
with a concise explanation that the mock intentionally isolates this suite from
the real PreviewPanel implementation.
In `@apps/desktop/src/components/workspace-content.tsx`:
- Around line 104-117: Align the comment with the preview pane behavior in
workspace-content: either add the appropriate responsive visibility class to the
preview-panel aside so it is hidden below the intended breakpoint, or remove the
claim that it never appears below a breakpoint. Keep the chosen markup/comment
behavior consistent with the existing responsive pattern used by the context
aside.
- Around line 82-93: Update the context selection around pdfPanelOpen and
context so an open PDF preview remains represented in the context slot when
previewTarget is a PDF and contextSidebarFor returns no routeContext. Preserve
the existing route-context behavior and PDF takeover for routes that provide a
context target, ensuring AppShell still renders a reachable PDF sidebar entry on
search, settings, All notes, Tasks, and Chat routes.
In `@apps/desktop/src/hooks/use-list-resize.ts`:
- Around line 175-195: Separate cancellation handling from the normal release
flow: add a cancel handler for onPointerCancel that clears the active drag
state, restores the viewport-effective persisted height, and never calls
commitHeight. Reuse this effective-height restoration during unmount, replacing
any raw setting-value write; keep release’s normal commit behavior unchanged.
In `@apps/desktop/src/lib/annotations/annotation-reference.ts`:
- Line 28: Update the annotation-reference link construction around the return
expression to encode each slash-separated assetPath segment with
encodeURIComponent, rejoin the segments with "/", and append the page fragment
afterward. Preserve directory separators and the existing title/page format, and
add focused tests covering asset paths containing "#", "?", and parentheses.
In `@apps/desktop/src/lib/annotations/annotations-store.ts`:
- Around line 194-207: Update PendingWrite and the write-scheduling logic to
store the serialized annotation sidecar snapshot when a write is scheduled. In
the timer flush and the useEffect cleanup, use that stored snapshot instead of
annotationsRef.current, while preserving the existing path, generation, and
timeout handling.
- Around line 116-120: Update persist to serialize writeAnnotations calls within
the hook, awaiting or chaining each new write after the previous in-flight write
so overlapping edits complete in order and the latest content remains persisted.
Preserve the existing error logging for failed writes, and add an
annotation-store test covering overlapping delayed writes.
In `@apps/desktop/src/lib/annotations/pdf-href.ts`:
- Around line 35-53: Update parsePdfHref to reject decoded paths containing
empty, "." or ".." segments, or backslashes, while preserving valid
graph-relative PDF asset paths. Validate that href contains at most one "#"
before destructuring so extra fragments are rejected, and add regression tests
covering traversal segments and multiple fragments.
In `@apps/desktop/src/lib/deep-links/handle.ts`:
- Around line 98-100: Validate the preview link path in the `preview` case
before calling `io.openPreview`, reusing the existing `resolveNoteTarget`
validation used for note-target links. Reject traversal or otherwise invalid
paths at this deep-link boundary and only open the preview after validation
succeeds.
---
Outside diff comments:
In `@apps/desktop/src/editor/use-asset-persistence.ts`:
- Around line 35-57: Update stripLocationSuffix and the asset-resolution flow to
normalize the raw source before decoding, then use that normalized path for
isSafeAssetSource validation and every returned asset path in resolveImageUrl
and resolveAssetOpenPath. Apply the same normalized key in resolveFileInfo so
fragment-bearing references resolve the existing directory-listing cache entry.
---
Nitpick comments:
In `@apps/desktop/src-tauri/src/fs/mod.rs`:
- Around line 573-602: Remove directory creation from annotation_sidecar so it
only derives and validates the sidecar path without filesystem writes;
stage_bytes already creates the target parent for writes. Update the related
test to assert the returned path is under .reflect/annotations rather than
asserting the directory exists.
In `@apps/desktop/src/components/context-sidebar/note-actions-section.tsx`:
- Line 67: Update the label in the note-actions section’s Deepdive PDF action to
use a verb-object pattern consistent with its sibling actions, choosing either
“Open PDF panel” or “Browse PDF”.
In `@apps/desktop/src/components/preview/annotation-list.tsx`:
- Around line 79-88: Add an aria-pressed attribute to the annotation row button
in the item rendering, using the existing selectedId === item.id condition so
assistive technology receives the current selection state.
- Around line 56-65: In the annotation grouping logic, change the per-page
accumulator to mutable arrays and append items with push instead of recreating
each group via spread. Replace the manual pages array and second iteration with
Array.from(byPage), and update the sort comparator in that flow to use
descriptive destructured parameter names instead of a and b.
In `@apps/desktop/src/components/preview/highlight-layer.tsx`:
- Around line 233-237: Update the wrapper creation flow around the pointerdown
handler to capture the created element in a const with a non-null type, then use
that variable in startDrag and append operations. Remove the unnecessary
non-null assertion on wrapper while preserving the existing event behavior.
In `@apps/desktop/src/components/preview/pdf-sidebar-block.tsx`:
- Around line 259-283: Update PdfThumbnails/PdfThumbnail so thumbnails render
lazily based on viewport visibility, using an IntersectionObserver or list
virtualization. Avoid calling doc.getPage or page.render for offscreen pages,
while preserving navigation and rendering thumbnails as they enter the scroll
container’s viewport.
In `@apps/desktop/src/components/preview/pdf-viewer-shell.test.tsx`:
- Around line 73-97: Add a PdfViewerShell test that mocks readAssetBinary to
reject, asserts the rendered role="alert" error message, then re-renders with a
different assetPath and verifies the replacement document loads successfully.
Keep the existing successful-load coverage unchanged and exercise the
component’s recovery behavior after the failure.
- Around line 60-66: Replace the broad console.warn mock in beforeEach with a
scoped solution: add the exact pdf.js fake-worker warning to
apps/desktop/src/test-utils/allowed-console.ts with a justification, or filter
only that message while forwarding all other warnings. Preserve visibility of
unexpected warnings from the shell, React, and pdf.js.
In `@apps/desktop/src/components/workspace-content.test.tsx`:
- Around line 144-176: Add a test alongside the existing PDF preview coverage
that sets previewPanelState.target to a PDF, drives the workspace provider view
to "document" (the state produced by backToDocument), and renders WorkspaceHost
through renderWorkspace. Assert the Context aside’s actual expected content in
this state, including the no-context-target behavior, and verify the PDF preview
remains open as appropriate.
In `@apps/desktop/src/editor/note-editor.tsx`:
- Around line 414-433: Extract the duplicated PDF preview target construction
into a module-level helper named pdfPreviewTarget that accepts a PdfLinkRef and
returns the corresponding PreviewPanelTarget, preserving the conditional page
field. Replace the inline object construction in both handlers, including the
anchor-click branch and the code around the existing construction at lines
360-364, with setPreviewPanelTarget(pdfPreviewTarget(pdfHref)).
In `@apps/desktop/src/lib/annotations/annotation-reference.ts`:
- Line 23: Rename the single-character callback parameter in the title escaping
expression to the descriptive name character, and update its interpolation
reference accordingly. Preserve the existing replaceAll behavior.
In `@apps/desktop/src/lib/annotations/annotations-store.ts`:
- Around line 70-76: Document the exported annotation API types with TSDoc: add
interface-level descriptions for AnnotationItem and AnnotationFile, and document
PdfAnnotationsStatus and UsePdfAnnotationsResult. Keep the existing type shapes
unchanged and ensure each public type clearly describes its purpose.
In `@apps/desktop/src/lib/annotations/pdf-region-text.test.ts`:
- Around line 27-32: Rename the convertToViewportRectangle parameter from r to
rect and update all references within that function accordingly, preserving the
existing coordinate conversion behavior.
In `@apps/desktop/src/lib/resolve-note-preview-body.ts`:
- Around line 5-16: Move the JSDoc directly adjacent to the NotePreviewBody
interface declaration, removing the separating blank line, and mark its source,
frontmatter, and body properties readonly while preserving their existing types
and documentation.
In `@apps/desktop/src/providers/pdf-session-provider.tsx`:
- Around line 65-67: Update the session-clearing API around clear to accept the
owning assetPath, and only reset the session when that path still owns the
active session. Update PdfViewerShell cleanup to call clearSession(assetPath),
preserving the existing reset behavior for the matching owner while preventing
stale shells from clearing a newer session.
- Around line 27-36: Change PdfSession to a discriminated union with an explicit
unloaded state and a loaded state containing non-null viewer, pdfDocument, and
assetPath. Update EMPTY_SESSION and the register/clear session flows to
construct the appropriate variants, then adjust consumers such as
PdfSidebarBlock to narrow on the session status before accessing loaded fields.
In `@apps/desktop/src/providers/preview-panel-provider.test.tsx`:
- Around line 28-40: Update the test title in the `opens and closes a target`
test to remove the untested “defaults to no-op without a provider” claim,
leaving the test body unchanged.
- Around line 66-72: Remove the duplicate routed helper and reuse the existing
wrapper helper in the renderHook calls at the affected tests. Replace each
wrapper: routed reference with wrapper while preserving the current
RouterProvider and PreviewPanelProvider setup defined by wrapper.
In `@apps/desktop/src/providers/preview-panel-provider.tsx`:
- Around line 70-77: Rename the exported PreviewPanel interface to
PreviewPanelControls (or UsePreviewPanelResult) and update all imports, type
references, and exports using the interface, while preserving the existing React
component name and control API.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 796c2254-f837-4e9e-a405-72935b9699b8
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (65)
.oxfmtrc.jsonapps/desktop/package.jsonapps/desktop/public/pdf.worker.min.mjsapps/desktop/src-tauri/src/fs/mod.rsapps/desktop/src-tauri/src/lib.rsapps/desktop/src/components/context-sidebar/note-actions-section.test.tsxapps/desktop/src/components/context-sidebar/note-actions-section.tsxapps/desktop/src/components/error-boundary.tsxapps/desktop/src/components/graph-workspace.tsxapps/desktop/src/components/preview/annotation-context-menu.test.tsxapps/desktop/src/components/preview/annotation-context-menu.tsxapps/desktop/src/components/preview/annotation-list.tsxapps/desktop/src/components/preview/annotation-section.test.tsxapps/desktop/src/components/preview/annotation-section.tsxapps/desktop/src/components/preview/annotation-toolbar.tsxapps/desktop/src/components/preview/highlight-layer.tsxapps/desktop/src/components/preview/pdf-sidebar-block.test.tsxapps/desktop/src/components/preview/pdf-sidebar-block.tsxapps/desktop/src/components/preview/pdf-viewer-overrides.cssapps/desktop/src/components/preview/pdf-viewer-shell.test.tsxapps/desktop/src/components/preview/pdf-viewer-shell.tsxapps/desktop/src/components/preview/preview-panel.test.tsxapps/desktop/src/components/preview/preview-panel.tsxapps/desktop/src/components/settings-screen.test.tsxapps/desktop/src/components/sidebar-resize-handle.test.tsxapps/desktop/src/components/sidebar-resize-handle.tsxapps/desktop/src/components/workspace-content.test.tsxapps/desktop/src/components/workspace-content.tsxapps/desktop/src/desktop-root.tsxapps/desktop/src/editor/markdown-preview.tsxapps/desktop/src/editor/note-editor.tsxapps/desktop/src/editor/use-asset-persistence.tsapps/desktop/src/editor/use-wiki-link-hover-preview.tsxapps/desktop/src/hooks/use-list-resize.tsapps/desktop/src/hooks/use-sidebar-resize.test.tsapps/desktop/src/hooks/use-sidebar-resize.tsapps/desktop/src/lib/annotations/annotation-reference.test.tsapps/desktop/src/lib/annotations/annotation-reference.tsapps/desktop/src/lib/annotations/annotations-store.test.tsapps/desktop/src/lib/annotations/annotations-store.tsapps/desktop/src/lib/annotations/pdf-href.test.tsapps/desktop/src/lib/annotations/pdf-href.tsapps/desktop/src/lib/annotations/pdf-region-text.test.tsapps/desktop/src/lib/annotations/pdf-region-text.tsapps/desktop/src/lib/deep-links/deep-link.tsapps/desktop/src/lib/deep-links/handle.test.tsapps/desktop/src/lib/deep-links/handle.tsapps/desktop/src/lib/deep-links/parse.test.tsapps/desktop/src/lib/deep-links/parse.tsapps/desktop/src/lib/resolve-note-preview-body.tsapps/desktop/src/providers/deep-link-provider.tsxapps/desktop/src/providers/pdf-session-provider.tsxapps/desktop/src/providers/pdf-sidebar-view-provider.test.tsxapps/desktop/src/providers/pdf-sidebar-view-provider.tsxapps/desktop/src/providers/preview-panel-provider.test.tsxapps/desktop/src/providers/preview-panel-provider.tsxapps/desktop/src/providers/settings-provider.test.tsxapps/desktop/src/providers/sidebar-width.test.tsxapps/desktop/src/providers/sidebar-width.tsxapps/desktop/src/styles/index.csspackages/core/src/exports/platform.tspackages/core/src/graph/commands.test.tspackages/core/src/graph/commands.tspackages/core/src/settings/schema.test.tspackages/core/src/settings/schema.ts
Selecting text in highlight mode creates a text annotation from the selection: per-line normalized rects plus the selected text, matching the migrated sidecar shape. Tool is keyboard-reachable (t, Esc back to browse) like the existing browse/draw tools.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/components/preview/preview-panel.test.tsx (1)
240-248: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the collapsed-selection branch.
The first case has no range. It exits through
selection.rangeCount === 0.Create and add a collapsed
Rangebefore dispatchingmouseup. This verifies the separateselection.isCollapsedguard inPdfPreview.Proposed test update
- // No selection at all. - window.getSelection()?.removeAllRanges() + // A collapsed caret selection. + const collapsedRange = document.createRange() + collapsedRange.setStart(document.body, 0) + collapsedRange.collapse(true) + const collapsedSelection = window.getSelection() + collapsedSelection?.removeAllRanges() + collapsedSelection?.addRange(collapsedRange) document.dispatchEvent(new MouseEvent('mouseup', { bubbles: true })) expect(annotationsState.addAnnotation).not.toHaveBeenCalled()🤖 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 `@apps/desktop/src/components/preview/preview-panel.test.tsx` around lines 240 - 248, Update the first selection case in the test “does not create an annotation from a collapsed or out-of-page selection” to create and add a collapsed Range to window.getSelection() before dispatching mouseup. Keep the existing no-annotation assertion, ensuring PdfPreview’s selection.isCollapsed guard is exercised separately from the rangeCount === 0 path.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@apps/desktop/src/components/preview/preview-panel.test.tsx`:
- Around line 240-248: Update the first selection case in the test “does not
create an annotation from a collapsed or out-of-page selection” to create and
add a collapsed Range to window.getSelection() before dispatching mouseup. Keep
the existing no-annotation assertion, ensuring PdfPreview’s
selection.isCollapsed guard is exercised separately from the rangeCount === 0
path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 40261994-f109-425b-9126-2ea2be19aa01
📒 Files selected for processing (5)
apps/desktop/src/components/preview/annotation-toolbar.tsxapps/desktop/src/components/preview/preview-panel.test.tsxapps/desktop/src/components/preview/preview-panel.tsxapps/desktop/src/lib/annotations/pdf-selection.test.tsapps/desktop/src/lib/annotations/pdf-selection.ts
- derive text-highlight rects from PDF text content so they align with the rendered glyphs (browser selection rects drifted narrower) - load-failure no longer blocks the next PDF in the same viewer instance - serialize annotation sidecar writes; snapshot pending writes so a graph switch flush cannot write empty state - validate preview deep-link paths and PDF href traversal at the boundary - use the shadcn Dialog for fullscreen and DropdownMenu elsewhere; reset error boundaries per target; route asset PDF links in note previews - pointercancel cleanup for drags and list resize; test hardening; comments unified to English
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)
apps/desktop/src/components/preview/preview-panel.tsx (1)
170-202: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd
sessionto the effect dependencies.
createTextHighlightreadssession.pdfDocumentfrom the closure at Line 175. The dependency array at Line 202 omitssession.PdfViewerShellpublishes the session after the document loads, which produces a newsessionvalue. If the user enters highlight mode before the document publishes, the effect keeps the oldsessionand every selection capture returns early at Line 177. The capture then recovers only whenmode,color, oraddAnnotationchanges.🐛 Proposed fix
- }, [mode, color, addAnnotation]) + }, [mode, color, addAnnotation, session])🤖 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 `@apps/desktop/src/components/preview/preview-panel.tsx` around lines 170 - 202, Update the effect dependency array containing mode, color, and addAnnotation to include session, ensuring the createTextHighlight closure uses the latest session.pdfDocument after the document loads.
🧹 Nitpick comments (3)
apps/desktop/src/lib/deep-links/handle.ts (1)
98-121: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the shared note-resolution block.
This case repeats the resolve, stale-check, and not-found sequence from the
openNotecase at Lines 50-70. Only the operation label and the terminal action differ. Extract a helper that takes the target, the label, and returns the resolved path ornull.♻️ Sketch
async function resolveTargetPath( target: string, label: string, isStale: (() => boolean) | undefined, ): Promise<string | null> { let path: string | null try { path = await resolveNoteTarget(target) } catch (cause) { if (isStale?.() !== true) { startOperation(label).fail(errorMessage(cause)) } return null } if (isStale?.() === true) { return null } if (path === null) { startOperation(label).fail(`Note not found: ${truncate(target)}`) } return path }🤖 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 `@apps/desktop/src/lib/deep-links/handle.ts` around lines 98 - 121, Extract the duplicated note-resolution flow from the openNote and preview cases into a shared async helper that accepts the target, operation label, and stale-check callback, then returns the resolved path or null. Move resolveNoteTarget error handling, stale checks, and not-found reporting into this helper, and update both cases to use it while preserving their distinct terminal actions.apps/desktop/src/components/preview/highlight-layer.tsx (1)
33-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider scoping
activeDragsto the layer instance.
activeDragsis module state, so it is shared by everyHighlightLayermounted at the same time.abortAllDragsat Line 125 then aborts drags that belong to another layer. The current composition mounts one layer at a time, so there is no defect today.PdfViewerShellalready mounts itself recursively for fullscreen, so a second layer is one prop change away.Hold the map in a
useRefand pass it tosyncOverlaysandstartDragalongsidelatest.🤖 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 `@apps/desktop/src/components/preview/highlight-layer.tsx` around lines 33 - 42, Scope activeDrags to each HighlightLayer instance by storing the map in a useRef rather than module state. Update abortAllDrags and the layer lifecycle to use that instance-specific map, and pass it alongside latest through syncOverlays and startDrag so drag creation and cleanup cannot affect another layer.apps/desktop/src/components/preview/pdf-viewer-shell.tsx (1)
301-315: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the manual Escape listener and let the Dialog primitive own dismissal.
DialogContentpasses all props toDialogPrimitive.Popup, with no escape/dismissal override, and the controlled Root callsonOpenChangewhen a dialog dismisses. The Escape path at lines 301-315 is redundant and can fire outside the dialog; delete the effect and keep the primitive’s standard behavior.🤖 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 `@apps/desktop/src/components/preview/pdf-viewer-shell.tsx` around lines 301 - 315, Remove the fullscreen Escape-key useEffect and its window keydown listener from the PDF viewer shell. Rely on the Dialog primitive’s default dismissal behavior and controlled Root onOpenChange flow to exit fullscreen, leaving the existing fullscreen state handling otherwise 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 `@apps/desktop/src/lib/annotations/pdf-region-text.ts`:
- Around line 35-52: Update textItemNormalizedRect to reorder the normalized y
coordinates before returning, ensuring the lower y value precedes the higher one
regardless of viewport axis orientation. Preserve the existing x normalization
and rectangle shape while using the sorted y pair in the returned
NormalizedRect.
---
Outside diff comments:
In `@apps/desktop/src/components/preview/preview-panel.tsx`:
- Around line 170-202: Update the effect dependency array containing mode,
color, and addAnnotation to include session, ensuring the createTextHighlight
closure uses the latest session.pdfDocument after the document loads.
---
Nitpick comments:
In `@apps/desktop/src/components/preview/highlight-layer.tsx`:
- Around line 33-42: Scope activeDrags to each HighlightLayer instance by
storing the map in a useRef rather than module state. Update abortAllDrags and
the layer lifecycle to use that instance-specific map, and pass it alongside
latest through syncOverlays and startDrag so drag creation and cleanup cannot
affect another layer.
In `@apps/desktop/src/components/preview/pdf-viewer-shell.tsx`:
- Around line 301-315: Remove the fullscreen Escape-key useEffect and its window
keydown listener from the PDF viewer shell. Rely on the Dialog primitive’s
default dismissal behavior and controlled Root onOpenChange flow to exit
fullscreen, leaving the existing fullscreen state handling otherwise unchanged.
In `@apps/desktop/src/lib/deep-links/handle.ts`:
- Around line 98-121: Extract the duplicated note-resolution flow from the
openNote and preview cases into a shared async helper that accepts the target,
operation label, and stale-check callback, then returns the resolved path or
null. Move resolveNoteTarget error handling, stale checks, and not-found
reporting into this helper, and update both cases to use it while preserving
their distinct terminal actions.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4fc2a47c-825c-426d-9193-86da401c601e
📒 Files selected for processing (22)
apps/desktop/src/components/context-sidebar/note-actions-section.tsxapps/desktop/src/components/graph-workspace.tsxapps/desktop/src/components/preview/highlight-layer.tsxapps/desktop/src/components/preview/pdf-sidebar-block.test.tsxapps/desktop/src/components/preview/pdf-sidebar-block.tsxapps/desktop/src/components/preview/pdf-viewer-shell.tsxapps/desktop/src/components/preview/preview-panel.test.tsxapps/desktop/src/components/preview/preview-panel.tsxapps/desktop/src/components/workspace-content.test.tsxapps/desktop/src/components/workspace-content.tsxapps/desktop/src/hooks/use-list-resize.tsapps/desktop/src/lib/annotations/annotation-reference.test.tsapps/desktop/src/lib/annotations/annotation-reference.tsapps/desktop/src/lib/annotations/annotations-store-persistence.test.tsxapps/desktop/src/lib/annotations/annotations-store.tsapps/desktop/src/lib/annotations/pdf-href.test.tsapps/desktop/src/lib/annotations/pdf-href.tsapps/desktop/src/lib/annotations/pdf-region-text.tsapps/desktop/src/lib/annotations/pdf-selection.test.tsapps/desktop/src/lib/annotations/pdf-selection.tsapps/desktop/src/lib/deep-links/handle.test.tsapps/desktop/src/lib/deep-links/handle.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- apps/desktop/src/lib/annotations/annotation-reference.ts
- apps/desktop/src/lib/annotations/annotation-reference.test.ts
- apps/desktop/src/lib/annotations/pdf-href.ts
- apps/desktop/src/components/graph-workspace.tsx
- apps/desktop/src/components/preview/preview-panel.test.tsx
- apps/desktop/src/components/workspace-content.tsx
- apps/desktop/src/lib/deep-links/handle.test.ts
- apps/desktop/src/components/context-sidebar/note-actions-section.tsx
- apps/desktop/src/components/workspace-content.test.tsx
- apps/desktop/src/lib/annotations/pdf-href.test.ts
- apps/desktop/src/hooks/use-list-resize.ts
- apps/desktop/src/components/preview/pdf-sidebar-block.tsx
PageViewport.convertToViewportRectangle returns the raw y-flipped corners ([left, bottom, right, top]), which the item-bbox math misread as [left, top, right, bottom] — inverting every text item rect vertically, so selection hit-testing matched nothing and no text annotation was created. Min/max the converted corners and add a regression test with unsorted viewport output.
…e captures Text items are typically one per line, so using their full bbox made a partial-line selection highlight the whole line. Clip each hit item horizontally to the selection rects (keeping canvas-aligned vertical bounds) and take the annotation text from the selection string. Multi-page selections now split into one annotation per page, and mixed image/text selections highlight only the text runs.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/src/components/preview/preview-panel.test.tsx (1)
310-310: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename
ntopageNumber.The coding guidelines prohibit single-character variable names.
🤖 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 `@apps/desktop/src/components/preview/preview-panel.test.tsx` at line 310, Rename the map callback parameter n to pageNumber in the pages construction, updating its usages within the callback while preserving the existing behavior.Source: Coding guidelines
🤖 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 `@apps/desktop/src/components/preview/preview-panel.test.tsx`:
- Around line 343-345: Remove the duplicate block-scoped declarations in
apps/desktop/src/components/preview/preview-panel.test.tsx:343-345, keeping
exactly one byPage declaration and one payload declaration in the affected test
scope. Also remove duplicate declarations in
apps/desktop/src/lib/annotations/pdf-selection.test.ts:129-131, keeping exactly
one each for page1, page2, and page3.
---
Nitpick comments:
In `@apps/desktop/src/components/preview/preview-panel.test.tsx`:
- Line 310: Rename the map callback parameter n to pageNumber in the pages
construction, updating its usages within the callback while preserving the
existing behavior.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b8ee2cc8-6bdb-4aae-9232-cec20afde8a7
📒 Files selected for processing (5)
apps/desktop/src/components/preview/preview-panel.test.tsxapps/desktop/src/components/preview/preview-panel.tsxapps/desktop/src/lib/annotations/pdf-region-text.test.tsapps/desktop/src/lib/annotations/pdf-selection.test.tsapps/desktop/src/lib/annotations/pdf-selection.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/lib/annotations/pdf-region-text.test.ts
- apps/desktop/src/components/preview/preview-panel.tsx
# Conflicts: # apps/desktop/src/editor/note-editor.tsx
ocavue
requested changes
Aug 9, 2026
ocavue
left a comment
ocavue
left a comment
Contributor
There was a problem hiding this comment.
Hi. We couldn’t accept a PR with minified code embedded (apps/desktop/public/pdf.worker.min.mjs)
… captions - Linked PDF images (`[](assets/….pdf#page=N)`) render as chips like text references: clicking jumps to the PDF page instead of opening a lightbox; the jump target shows as a caption under the chip's image via a ProseMirror widget decoration (the inline widget nests in the hidden `.md-mark` wrapper, so the stylesheet lifts its `opacity: 0`). - Text reference labels show the annotated text, not the PDF file name. - Load the pdf.js worker from pdfjs-dist via Vite instead of a vendored minified file. - Patch @meowdown/core (pnpm patchedDependencies) so a resized linked image's trailing size comment folds into the hidden image source instead of leaking as visible text; upstream fix to follow.
Author
this file is removed. now it's using pdfjs-dist from npm. btw, I found another bug in @meowdown/core, and I opened a PR as well. Currently it's pachted in this repo. |
…-panel # Conflicts: # apps/desktop/src/editor/note-editor.tsx # pnpm-lock.yaml
Author
|
Hi, @ocavue . What else should I do to make this PR accepted? |
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
2 participants
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.
Adds a PDF annotation reading experience. Clicking a PDF link opens an
in-app split-pane preview (pdf.js via
readAssetBinary), migrated SiYuanannotation sidecars render as highlights, and the context sidebar gains a
PDF panel (pages + outline + actions) stacked over the document panel.
Highlights
presets (fit width / fit page / actual size), fullscreen, and re-fit on
panel resize.
rectangles; right-click a highlight for Copy text (extracts the covered
region's text for empty rect annotations) / Copy reference / Delete.
Copy reference pastes a markdown link that jumps back to the PDF page.
outline + actions) over the document panel; a back link returns to it,
preserving each section's open/closed state.
v/rswitch browse/draw modes,Escexits draw.parsePdfHrefand the asset pipeline).following the adding-a-command guide.
Verification
pnpm typecheck✅pnpm lint✅cargo test✅ (360 passed)Summary by CodeRabbit