Skip to content

SCHOL-857: New pdf.js reader - #207

Open
jackiequach wants to merge 12 commits into
mainfrom
SCHOL-857/pdfjs-reader
Open

SCHOL-857: New pdf.js reader#207
jackiequach wants to merge 12 commits into
mainfrom
SCHOL-857/pdfjs-reader

Conversation

@jackiequach

@jackiequach jackiequach commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Confluence doc
SCHOL-857

This PR will not introduce any breaking changes to the existing web reader.

PdfReader is 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 div placeholder whose dimensions are set from the page's intrinsic size (fetched via pdfDoc.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 an IntersectionObserver window (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 pdfReaderReducer with a useReducer. Actions mirror the pattern in usePdfReader'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 pendingAction state rather than dispatched directly. PdfReader processes them in a useEffect, calling captureViewportAnchor() 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:

  • Which page sits at 30% down the viewport (SCROLLSPY_ANCHOR_RATIO)
  • What fraction of that page's height the anchor point falls at (intraPageRatio)
  • The pixel distance from the top of the viewport to the anchor (viewportOffset)

After the state change renders, a useLayoutEffect runs synchronously before the browser paints, recomputes the anchor page's new top and height from the updated scale/rotation, and sets scrollTop to 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.json with a larger pdf and go to /pdf/single-resource-short or pdf/fixed-height-embedded-collection

TODO in separate PR:

  • Remove addTocToManifest since the TOC is now retrieved directly in PdfReader. (breaking change, will need to be removed from consuming apps)
    • Remove react-pdf dependency

@vercel

vercel Bot commented Aug 11, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
web-reader Ready Ready Preview Aug 20, 2026 7:55pm

Request Review

@jackiequach jackiequach changed the title [WIP] SCHOL-857: New pdf.js reader SCHOL-857: New pdf.js reader Aug 19, 2026
@jackiequach
jackiequach marked this pull request as ready for review August 19, 2026 17:23
@jackiequach
jackiequach requested a review from alea12 as a code owner August 19, 2026 17:23

@alea12 alea12 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have another constants configuration in src/constants.ts. Could you add a brief comment on what kind of constants should live where?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comments in both files.

Comment thread src/PdfReader/types.ts Outdated
Comment thread src/PdfReader/PdfReaderContent.tsx Outdated
Comment on lines +99 to +101
if (!fileUrl) {
throw new Error('A PDF fileUrl is required');
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to resolve the fileUrl in usePdfReader instead and throw an error there.

Comment thread src/PdfReader/index.tsx Outdated

const rotateCounterClockwise = React.useCallback(async () => {
dispatch({ type: 'ROTATE_COUNTER_CLOCKWISE' });
const [pdfLoadFailed, setPdfLoadFailed] = useState(false);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pdfLoadFailed is never set to true. I think this would result in an infinite skeleton state without an indication that PDF fetch/render failed

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to properly set an error state when fetch/render fails. Added a per page render error using the existing error ui:
image

Comment thread src/PdfReader/reducer.ts
Comment on lines +14 to +17
| { type: 'GO_FORWARD' }
| { type: 'GO_BACKWARD' }
| { type: 'GO_TO_PAGE'; page: number }
| { type: 'GO_TO_HREF'; href: string }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, it scrolls only the internal container.

@alea12

alea12 commented Aug 19, 2026

Copy link
Copy Markdown
Member

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?

@alea12

alea12 commented Aug 19, 2026

Copy link
Copy Markdown
Member

For future reference, here are the steps I took to test on a separate repo:

Local dev build + testing with enhanced-search

1. Build & pack this branch

cd web-reader
npm run build
rm -f nypl-web-reader-*.tgz  # clean up old builds if exists
npm pack                     # → nypl-web-reader-5.0.2.tgz

2. Install the tarball into the enhanced-search web app

cd ../enhanced-search/web
npm install ../../web-reader/nypl-web-reader-5.0.2.tgz
rm -rf .next
npm run dev

After testing, restore with: git checkout package.json package-lock.json && npm install.

@jackiequach

jackiequach commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator Author

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?

ES needs to be updated to use the fragment identifier #page=X since the PDF reader is now following these PDF standards.

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.

@jackiequach
jackiequach requested a review from alea12 August 20, 2026 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants