Split PdfViewer.svelte (469 lines) into renderer module + controls component
#196
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Context
PdfViewer.svelteis the largest component in the codebase at 469 lines. It mixes three concerns: PDF.js rendering logic, page/zoom controls UI, and the canvas+annotation layout.Proposed Split
1.
usePdfRenderer.svelte.ts(~160 lines)Extract all imperative PDF.js logic:
loadDocument()— PDF loading via dynamic importrenderPage()— canvas + text layer rendering with DPR handlingprerender()— neighbor page pre-cachingpdfDoc,currentPage,totalPages,scale,loading,errorprevPage,nextPage,zoomIn,zoomOut2.
PdfControls.svelte(~120 lines)The control bar markup (page nav, zoom buttons, annotation toggle):
currentPage,totalPages,scale,showAnnotations,annotationCount,onPrev,onNext,onZoomIn,onZoomOut,onToggleAnnotations3.
PdfViewer.sveltebecomes orchestrator (~100 lines)usePdfRendererandPdfControlsAcceptance Criteria
npm run checkpasses👨💻 Felix Brandt -- Senior Fullstack Developer
Good decomposition along visual boundaries. A few things I'd want nailed down before implementation:
usePdfRenderer.svelte.ts -- API surface
pdfDoc,currentPage,totalPages,scale,loading,erroras exposed state. These should all be$staterunes inside the module, returned as a single object. Will the module accept canvas/textLayer refs as parameters, or will it query them internally? Accepting refs as params keeps the module testable without a DOM.loadDocument()does a dynamicimport('pdfjs-dist')-- will the module own the CDN worker URL setup (pdfjsLib.GlobalWorkerOptions.workerSrc), or does the caller configure that? I'd keep it inside the module so the concern is fully encapsulated.prerender()for neighbor pages -- does it write to the same canvas, or to offscreen canvases? If offscreen, who owns those refs? This needs to be explicit in the module contract.PdfControls.svelte -- props count
currentPage,totalPages,scale,showAnnotations,annotationCount,onPrev,onNext,onZoomIn,onZoomOut,onToggleAnnotations). That exceeds the 3-param guideline from our style guide. Consider grouping:pdfStateobject for the read-only values (currentPage,totalPages,scale)controlsobject for the callbacks (onPrev,onNext,onZoomIn,onZoomOut)showAnnotations,annotationCount,onToggleAnnotationsas a separate annotation concern -- or a second small component (AnnotationToggle)Orchestrator line count
Testing strategy
usePdfRenderer.svelte.tsis the most valuable thing to unit test here. Since it's a pure module returning state and methods, it can be tested with Vitest without a browser -- mockpdfjs-distat the import boundary, assert state transitions onprevPage/nextPage/zoomIn/zoomOut.Scroll-sync effect
usePdfRenderer. Scroll-sync depends on annotation data, which is a concern of the parent, not the renderer. Should the module expose agoToPage(n)method instead, and let the orchestrator call it when the selected annotation changes? That keeps annotation knowledge out of the renderer module.🏗️ Markus Keller -- Senior Application Architect
Clean separation of concerns. The three-part split (imperative logic module, presentational control bar, orchestrator shell) follows the pattern we use elsewhere in the codebase. A few architectural observations:
Module boundary clarity
usePdfRenderer.svelte.tsis a Svelte 5 reactive module (runes in.svelte.tsfiles). This is the correct pattern for encapsulating imperative browser APIs behind reactive state. One thing to define explicitly: what are the inputs to this module? At minimum it needs:Dependency direction
PdfViewer (orchestrator) --> usePdfRenderer (logic) + PdfControls (UI). This is correct -- logic and presentation are siblings, orchestrated by the parent. Neither should know about the other.Annotation layer ownership
No new external dependencies
Acceptance criteria gap
🧪 Sara Holt -- Senior QA Engineer
A 469-line component splitting into three pieces is a testability win. Here's what I'd want covered:
Test strategy per artifact
usePdfRenderer.svelte.tsPdfControls.sveltePdfViewer.svelte(orchestrator)Regression risks to test explicitly
disabled={currentPage <= 1}without a guard in the handler), the extraction tousePdfRenderermust move the guard into the module, not just the UI.window.devicePixelRatio = 2and verifies the canvas dimensions are doubled would catch regressions.prerender()function for neighbor pages is a side effect. After extraction, verify it doesn't fire during initial load (causing double-render) or on unmount (causing errors on destroyed canvases).Testability improvement from this refactor
usePdfRenderercan be tested in isolation by mockingpdfjs-dist. This drops the unit test complexity significantly and removes the need for a browser environment for logic-only tests.usePdfRendereras part of this ticket, not as a follow-up. The module is the highest-risk piece since it owns all the imperative PDF.js interaction.Acceptance criteria suggestion
🔒 Nora "NullX" Steiner -- Application Security Engineer
This is a structural refactor, not a feature change, so the attack surface should remain identical. A few things to verify:
PDF.js dynamic import
loadDocument()uses a dynamicimport('pdfjs-dist'). After extraction tousePdfRenderer.svelte.ts, confirm the import path is still a static string literal, not constructed from user input. A dynamic import with a variable path (import(userInput)) would be a code injection vector. This is almost certainly fine, but worth a line-level check during review.Canvas and text layer -- XSS surface
document.createElement('span')internally (safe), but if any custom code in the current PdfViewer manipulatesinnerHTMLof the text layer (e.g., for highlighting search terms), that logic must remain sanitized after extraction. Verify noinnerHTMLassignments exist that take PDF-derived content.Worker URL configuration
pdfjsLib.GlobalWorkerOptions.workerSrc) is a sensitive configuration -- if it's set to a CDN URL, ensure it uses a versioned, integrity-checked URL (SRI hash via import map or bundler config). If it's a local file, confirm it's served from the app's own origin. After this refactor, the worker config moves intousePdfRenderer-- a good time to verify this is locked down.Blob URL handling
usePdfRendererwill own the URL handling. Confirm that:URL.revokeObjectURL) to prevent memory leaks that could be exploited for resource exhaustionNo new trust boundaries
pdfDocobject) to components that shouldn't have it. Keep the module's return type narrow -- expose methods and primitive state, not the raw PDF.js document object.🎨 Leonie Voss -- UI/UX Design Lead
The split is structural, but any time the control bar component is extracted, there's a risk of subtle visual regressions. Here's what I'd watch:
PdfControls as a standalone component -- visual contract
.sveltefile, its styling must be self-contained. Verify that no parent CSS in the current PdfViewer is styling the controls via descendant selectors (e.g.,.pdf-viewer .controls button). If so, those styles must move into PdfControls or be converted to Tailwind utility classes on the elements themselves.Annotation toggle -- visual grouping
Responsive behavior preservation
Loading and error states
loadingis true orerroris set, the control bar should reflect this (disabled buttons, a loading indicator, or hiding entirely). After the split, these states flow from the renderer through the orchestrator to PdfControls. Verify the visual treatment of these states survives the refactor -- a missingdisabledprop on a button during loading would be a regression.Acceptance criteria suggestion
⚙️ Tobias Wendt -- DevOps & Platform Engineer
Pure frontend refactor, no infra impact. A few things on the build/CI side:
Bundle size check
usePdfRenderer.svelte.tsas a separate module should not change the bundle size -- Vite's tree-shaking and code-splitting handle.svelte.tsmodules the same as inline script blocks. But worth confirming: runnpm run buildbefore and after and compare the output chunk sizes in.svelte-kit/output. Ifpdfjs-distsuddenly ends up in a different chunk or gets duplicated, that's a regression.import('pdfjs-dist')should still produce a separate async chunk. Verify this doesn't change to a static import during the refactor, which would pull the entire PDF.js library (~400KB) into the main bundle.CI pipeline impact
npm run check(svelte-check) is already in the acceptance criteria -- good. This will catch any type errors from the module extraction.Dev server behavior
.svelte.tsmodules well, but the first time a developer opens a document detail page after this change, the HMR boundary will be different (three files instead of one). If HMR doesn't pick up changes tousePdfRenderer.svelte.tscorrectly during development, a full page reload may be needed. Not a blocker, but worth noting in the PR description so developers know what to expect.Source map quality
usePdfRenderer.svelte.ts:42instead ofPdfViewer.svelte:387, making production error reports more actionable.No deployment changes needed
Comprehensive Response to All Persona Reviews
I read the full source of
PdfViewer.svelte(469 lines),AnnotationLayer.svelte(176 lines),DocumentViewer.svelte(107 lines), the existing test filePdfViewer.svelte.spec.ts(53 lines), and the blob URL creation sites. Here are concrete answers to every concern.Felix Brandt -- Senior Fullstack Developer
usePdfRenderer API surface -- refs as params vs internal query:
The module should accept
canvasElandtextLayerElas parameters. Currently these are$statebindings at lines 40-41. Passing them in keeps the module testable without a DOM -- correct call.Worker URL ownership:
Lines 62-71 show the dynamic import and
GlobalWorkerOptions.workerSrcsetup happening inonMount. This should move entirely intousePdfRenderer-- the module owns the full PDF.js lifecycle. The caller never needs to know about the worker.Prerender -- offscreen canvases:
prerender()at lines 161-170 does NOT render to any canvas. It only callsdoc.getPage(n)to warm PDF.js's internal page cache. No offscreen canvas refs needed. The module contract is simple:prerenderis a fire-and-forget side effect with no output.PdfControls -- 10 props:
Confirmed:
currentPage,totalPages,scale,showAnnotations,annotationCount,onPrev,onNext,onZoomIn,onZoomOut,onToggleAnnotations= 10 props. Grouping intopdfState+controlsobjects is cleaner but adds indirection for a pure presentational component. I'd go with KISS here -- 10 flat props for a leaf component is tolerable. The annotation toggle is visually separated already (line 392: it's in its own conditional block, rendered only whenannotations.length > 0). Extracting a separateAnnotationTogglecomponent for ~35 lines of SVG button feels like over-splitting.Scroll-sync placement:
Strong agree. The scroll-sync effect (lines 222-246) depends on
annotationsandactiveAnnotationId, which are annotation concerns, not renderer concerns. The module should expose agoToPage(n: number)method, and the orchestrator calls it whenactiveAnnotationIdchanges. This keeps annotation knowledge out of the renderer. TherequestAnimationFramescroll-into-view logic (lines 240-245) also stays in the orchestrator since it queriesdocument.querySelectorfor annotation DOM elements.Markus Keller -- Senior Application Architect
Module inputs:
The module needs: (1) PDF file URL, (2) canvas ref, (3) textLayer ref. Scale is internal state. Container width is not used -- scale is a fixed multiplier (line 35:
scale = 1.5), not computed from container dimensions. Input count is 3, well within the guideline.State pass-through risk:
The orchestrator passes ~5-6 read-only values from the renderer to PdfControls. This is acceptable. If it grows, we can spread the renderer's returned state object directly as Felix suggested.
AnnotationLayer ownership:
AnnotationLayer is ALREADY a separate component (
frontend/src/lib/components/AnnotationLayer.svelte, 176 lines). It is imported at PdfViewer line 4 and composed at lines 453-461. No extraction needed -- the orchestrator just keeps composing it as-is.Keyboard navigation:
There is NO keyboard navigation in the current PdfViewer. No
keydown/keypresshandlers exist anywhere in the component. The only keyboard handling is inAnnotationLayer.svelteline 115-117 (Enter/Space on annotation buttons for a11y). So there is nothing to preserve or accidentally break. This does NOT need to be added as part of this refactor -- it's a separate feature if desired.Sara Holt -- Senior QA Engineer
Existing tests:
PdfViewer.svelte.spec.tsexists with 3 tests:These are browser-rendered component tests using
vitest-browser-sveltewith a mockedpdfjs-dist. They cover button presence but NOT: disabled states, callback invocation, page bounds clamping, zoom bounds, or error states.Page bounds clamping:
Currently implemented in the handlers themselves:
prevPage()line 248-250:if (currentPage > 1)-- guard is in the handler, not just the templatenextPage()line 252-254:if (pdfDoc && currentPage < pdfDoc.numPages)-- guard is in the handlerzoomOut()line 260:if (scale > 0.5)-- guard is in the handlerThese guards will transfer cleanly into
usePdfRenderer. The template also hasdisabledattributes (lines 312, 334) which are redundant UI hints -- correct pattern.DPR handling:
Line 113:
const dpr = window.devicePixelRatio || 1. Canvas is sized atviewport.width(which includesscale * dpr) and CSS-sized atviewport.width / dpr. A unit test mockingwindow.devicePixelRatio = 2and asserting canvas dimensions would be valuable. Adding this to theusePdfRenderertest suite is feasible.Prerender side effects:
prerender()is called at line 207, only inside the.then()of a successfulrenderPage. It won't fire during initial load (beforepdfDocis set) or on unmount. After extraction, this call sequence stays the same.Recommended test additions for this ticket (not follow-up):
usePdfRenderer: prevPage clamps at 1, nextPage clamps at totalPages, zoomOut clamps at 0.5, loadDocument sets error state on rejection, loading transitionsPdfControls: prev disabled on page 1, next disabled on last page, callback invocationAcceptance criteria addition -- agreed:
"Existing E2E tests in
documents.spec.ts(59 PDF-related assertions) pass without modification" should be added.Nora "NullX" Steiner -- Application Security Engineer
Dynamic import path:
Lines 64-66 use static string literals only:
No user input anywhere near these import paths. Safe after extraction -- the strings move as-is into
usePdfRenderer.innerHTML / XSS surface:
One
innerHTMLassignment exists at line 144:textDiv.innerHTML = ''. This is a clearing operation (empty string), not injecting content. All text layer content is rendered by PDF.js'sTextLayerclass usingdocument.createElement('span')internally. No custom innerHTML with PDF-derived content anywhere. Safe.Worker URL:
Line 66-68: The worker URL comes from a Vite
?urlimport of a local file (pdfjs-dist/build/pdf.worker.min.mjs). This resolves to the app's own origin at build time -- no CDN, no external URL. Vite handles the asset hashing. After extraction intousePdfRenderer, this stays identical.Blob URL handling:
Blob URLs are created in the page routes, NOT in PdfViewer:
frontend/src/routes/documents/[id]/+page.svelteline 40:fileUrl = URL.createObjectURL(blob)frontend/src/routes/enrich/[id]/+page.svelteline 42: sameNeither page calls
URL.revokeObjectURL(). This is a pre-existing memory leak, unrelated to this refactor. Worth a separate bug ticket but out of scope here. PdfViewer receives the URL as a prop string -- it never creates or manages blob URLs.Return type narrowness:
Agreed.
usePdfRenderershould return primitive state (currentPage,totalPages,scale,loading,error) and methods (prevPage,nextPage,zoomIn,zoomOut,goToPage,loadDocument). The rawpdfDocobject should NOT be exposed. CurrentlypdfDocis only read externally for thedisabledcheck at line 334 (disabled={!pdfDoc || ...}), which can be replaced with a booleanisLoadedin the return type.Leonie Voss -- UI/UX Design Lead
Parent CSS / descendant selectors:
No CSS files contain
.pdf-viewer,.textLayer, or.pdf-pageselectors. All styling is inline Tailwind utilities on the elements themselves. The only class-based styling istextLayer(line 449) andpdf-page(line 442), and no global CSS targets these. PdfControls extraction carries zero risk of breaking inherited styles.Control bar positioning:
The control bar (lines 305-429) uses
shrink-0inside aflex flex-colparent. It's flow-positioned (not sticky/fixed) at the top of the flex column. The orchestrator must preserve theflex flex-colwrapper, and PdfControls is just a flex child. Simple.Annotation toggle visual separation:
The annotation toggle (lines 392-428) is already visually separated -- it's in its own flex group at the right side of the
justify-betweencontrol bar. Page nav is left, zoom is center, annotation toggle is right. The existing layout achieves the cognitive separation Leonie wants without a divider element.Responsive behavior:
There are NO breakpoint-specific styles in the PdfViewer control bar. No
sm:,md:,lg:classes anywhere in lines 305-429. The controls use flex-wrap-free layout with small buttons (p-1= 4px padding) and small text (text-xs). At 320px, the control bar is tight but doesn't overflow (3 groups in ajustify-betweenflex row). Touch targets: the nav/zoom buttons arep-1with a 16x16 SVG, giving ~24x24px hit area -- below the 44x44 recommendation. This is a pre-existing issue, not introduced by this refactor.Loading/error state flow:
loadinganderrorstates (lines 433-438, 266-279) are currently used to conditionally render the canvas area and show error messages. After extraction, these flow fromusePdfRendererthrough the orchestrator. The control bar does NOT currently react to loading/error -- it renders regardless. If we want disabled buttons during loading, that's a new feature, not a regression.Pixel-identical verification:
Agreed as a lightweight check. The proofshot skill can capture before/after at key viewports. Adding this to acceptance criteria is reasonable.
Tobias Wendt -- DevOps & Platform Engineer
Bundle size / dynamic import preservation:
The dynamic import at lines 64-66 uses static string literals. Moving them into
usePdfRenderer.svelte.tskeeps them in a separate file, but Vite's code-splitting handles.svelte.tsmodules identically to inline script. Theimport('pdfjs-dist')will still produce an async chunk. I'll verify withnpm run buildbefore/after and compare chunk sizes in the PR.Static import risk:
The type imports at line 3 (
import type { PDFDocumentProxy, ... }) are type-only and erased at build time. The runtime import at line 64 is dynamic. After extraction,usePdfRenderer.svelte.tswill useimport typeat the top and dynamicimport()at runtime -- same pattern. No risk of pulling 400KB into the main bundle.HMR boundary:
Correct that
.svelte.tsmodules have different HMR boundaries. Worth noting in the PR description. Not a blocker.Source maps:
Agreed -- three focused files produce better stack traces than one 469-line file. No action needed.
No deployment changes:
Confirmed. Pure frontend refactor, same Docker image, same build pipeline.
Summary of Decisions
goToPage(n)pdfDocexposureisLoaded: booleanin return type?url, stays in renderer moduleusePdfRenderer, 2-3 for PdfControls