SCHOL-857: New pdf.js reader - #207
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
alea12
left a comment
There was a problem hiding this comment.
Thank you @jackiequach for this work! I appreciate you working on refactoring along the way. I confirmed in-app experience like zoom & rotate has become way snappier. Left some comments for your review
There was a problem hiding this comment.
We have another constants configuration in src/constants.ts. Could you add a brief comment on what kind of constants should live where?
There was a problem hiding this comment.
Added comments in both files.
| if (!fileUrl) { | ||
| throw new Error('A PDF fileUrl is required'); | ||
| } |
There was a problem hiding this comment.
Not a blocker; prop design suggestion - I think the boundary here should be such that this child component does not allow undefined fileUrls. One way to tighten this would be to define PdfReaderProps to guarantee that either webpubManifestUrl or manifest is present, so usePdfReader will always be able to resolve a fileUrl. Related, resolveResourceUrl should throw an error if href is not accessible.
There was a problem hiding this comment.
Updated to resolve the fileUrl in usePdfReader instead and throw an error there.
|
|
||
| const rotateCounterClockwise = React.useCallback(async () => { | ||
| dispatch({ type: 'ROTATE_COUNTER_CLOCKWISE' }); | ||
| const [pdfLoadFailed, setPdfLoadFailed] = useState(false); |
There was a problem hiding this comment.
pdfLoadFailed is never set to true. I think this would result in an infinite skeleton state without an indication that PDF fetch/render failed
| | { type: 'GO_FORWARD' } | ||
| | { type: 'GO_BACKWARD' } | ||
| | { type: 'GO_TO_PAGE'; page: number } | ||
| | { type: 'GO_TO_HREF'; href: string } |
There was a problem hiding this comment.
These navigation actions hijack root browser's scroll position:
Screen.Recording.2026-08-19.at.5.31.06.PM.mov
Is this a desired behavior?
There was a problem hiding this comment.
Fixed, it scrolls only the internal container.
|
Also, direct page navigation seems to have degraded (http://127.0.0.1:3000/item/1e21d527-35cf-45e3-8c51-f1e60e92fe5d?previewItemId=24077967&previewPage=00000015). Would this be something that we want to handle in the enhanced-search FE? |
|
For future reference, here are the steps I took to test on a separate repo: Local dev build + testing with
|
ES needs to be updated to use the fragment identifier We do need a separate to ticket to handle passing a page number to the web reader from the relevant snippet links so that the reader doesn't need to fully reload. |

Confluence doc
SCHOL-857
This PR will not introduce any breaking changes to the existing web reader.
PdfReaderis replaced with an implementation using pdf.js directly instead of react-pdf.Initial load
Every page is always in the DOM as a fixed-size
divplaceholder whose dimensions are set from the page's intrinsic size (fetched viapdfDoc.getPage(i)at load time, before any canvas is painted). This gives the scroll container a correct, stable total height and a working scrollbar immediately, before any rendering has happened. The canvas, text layer, and annotation layer inside each page are only painted once the page scrolls into anIntersectionObserverwindow (300px above/below the viewport) and are torn down again when the page scrolls out.State management
Reader state (current page, total pages, scale, fit mode, rotation, navigation request counter) is managed by a
pdfReaderReducerwith auseReducer. Actions mirror the pattern inusePdfReader's reducer but are simpler because it is a single-file viewer.Note: there is no multi-resource state machine (INACTIVE → FETCHING_RESOURCE → RENDERING_IFRAME → READY) implemented in the new reader to simplify the implementation.
Actions that require DOM measurements before the state change can commit (ZOOM_IN, ZOOM_OUT, ROTATE_CCW) are queued as a
pendingActionstate rather than dispatched directly.PdfReaderprocesses them in auseEffect, callingcaptureViewportAnchor()synchronously against the current DOM before passing the action to the reducer. This preserves the pre-change scroll position across layout recalculations.Scroll position preservation (viewport anchor)
On every zoom or rotation, before the state change is committed,
captureViewportAnchor()records:SCROLLSPY_ANCHOR_RATIO)intraPageRatio)viewportOffset)After the state change renders, a
useLayoutEffectruns synchronously before the browser paints, recomputes the anchor page's newtopandheightfrom the updated scale/rotation, and setsscrollTopto restore the exact same content to the same screen position. The anchor is idempotent, a second capture within the same user action (e.g. rotate + refit scale) is a no-op, so both state writes resolve against the same pre-action snapshot.CSS over Chakra
Plain CSS is the better fit for the PDF reader since it is better for performance. Chakra's runtime style injection and prop-driven re-renders add overhead that compounds across all of the page components, whereas CSS rules are static and resolved entirely by the browser. The viewer also needs control over text/annotation layer positioning (position: absolute, transform-origin, z-index stacking) that maps naturally to CSS but would be awkward to express through Chakra's prop system.
Testing
Locally replace the urls in
single-resource-short.jsonwith a larger pdf and go to/pdf/single-resource-shortorpdf/fixed-height-embedded-collectionTODO in separate PR:
addTocToManifestsince the TOC is now retrieved directly inPdfReader. (breaking change, will need to be removed from consuming apps)react-pdfdependency