Skip to content

fix(reader): render the PDF reader with the pdfjs legacy build - #129

Merged
mrsibe merged 1 commit into
mainfrom
fix/pdf-reader-legacy-build
Sep 25, 2026
Merged

mrsibe merged 1 commit into
mainfrom
fix/pdf-reader-legacy-build

Conversation

@mrsibe

@mrsibe mrsibe commented Sep 25, 2026

Copy link
Copy Markdown
Owner

What does this PR do?

Makes the PDF reader actually render pages. The renderer now loads pdfjs-dist from its legacy build (pdfjs-dist/legacy/build/*) instead of the modern one, matching what the main-process loader has always done.

Adds a CI guard so the modern build cannot be imported again while the app runs on Chromium < 145.

Why?

The v1.4 release QA (#125) found the reader canvas blank. The first Console error is decisive:

TypeError: this[#methodPromises].getOrInsertComputed is not a function
    at #cacheSimpleMethod (pdfjs-dist.js)
    at WorkerTransport.getOptionalContentConfig
    at _PDFPageProxy.render
    at render (PdfSourceReader.tsx:88)

pdfjs-dist 6.3.289 calls Map.prototype.getOrInsertComputed / getOrInsert throughout the API build and the worker. That proposal only shipped in Chromium 145; Electron 39 is Chromium 142, so the call throws on the first page.render() and every page stays blank.

The legacy build bundles the core-js polyfills for Map/WeakMap getOrInsert/getOrInsertComputed. PdfLoader (main process) already imports pdfjs-dist/legacy/build/pdf.mjs, which is why PDF text extraction passed the packaged smoke test while rendering was broken.

This slipped through #124 (the pdfjs-dist upgrade for #121): that commit's own message records the reader rendering path as unverified manual QA, and the reader was the only call site left on the modern build.

Related issue

Related to #125. Related to #121 / #124 (the upgrade that introduced the regression).

What changed?

  • src/renderer/src/components/notebook/source/reader/PdfSourceReader.tsx: import the runtime and the worker from pdfjs-dist/legacy/build/*, with a comment on why and when to switch back (Chromium >= 145).
  • test/pdfjsScriptingBoundary.test.ts: new test that fails if any source file imports the modern build (pdfjs-dist or pdfjs-dist/build/*), while allowing import type ... from 'pdfjs-dist'. Its self-check now accepts the legacy subpath.

No dependency or package.json change; this only selects a different entry point of the same installed package.

How was this tested?

  • npm run typecheck (node, web, test) - pass
  • npm test - 271 pass, 0 fail (5/5 in the boundary suite)
  • npm run build - pass
  • The new guard was falsified: temporarily restoring import * as pdfjs from 'pdfjs-dist' makes it fail with the intended message, then it was reverted.
  • Inspected the built output: out/renderer/assets/pdf.worker.min-*.mjs carries the polyfill (getOrInsertComputed and target:"Map",proto:!0), and the renderer bundle references that legacy worker asset by hash.

Not verified here: canvas rendering, text-layer selection, bbox highlighting and zoom under a real window need a display. That is manual QA and belongs to #125. The reporter confirmed the reader now works locally.

Screenshots / recordings

Before After
Page canvas blank, getOrInsertComputed is not a function for every page Pages render (confirmed locally by the reporter)

Checklist

  • I have reviewed my own changes.
  • npm run typecheck passes.
  • npm run build passes.
  • I have tested the affected user workflow.
  • I have not included unrelated changes.
  • I have updated documentation when necessary.

Desktop / build changes

  • Not applicable

`pdfjs-dist` 6.3.289 calls `Map.prototype.getOrInsertComputed` (and
`getOrInsert`) throughout the API build and the worker, but those shipped in
Chromium 145 and Electron 39 is Chromium 142. The renderer imported the modern
build, so the first `page.render()` threw

    TypeError: this[#methodPromises].getOrInsertComputed is not a function

from `WorkerTransport.getOptionalContentConfig`, and every page painted blank.
The main-process loader already imports `pdfjs-dist/legacy/build/pdf.mjs`, whose
core-js polyfills install those methods, so text extraction was unaffected while
the reader was broken.

Point the reader and its worker at the same legacy build. Switch back to
`pdfjs-dist/build/*` once the app runs on Chromium >= 145.

The reader path slipped through #124 (the upgrade for #121), whose own message
records the rendering path as unverified manual QA. `test/pdfjsScriptingBoundary.test.ts`
now fails if any source file imports the modern build, and its self-check accepts
the legacy subpath.

Verified: `npm test` (271 pass), `npm run typecheck`, `npm run build`; the built
`out/renderer/assets/pdf.worker.min-*.mjs` carries the polyfill
(`target:"Map",proto:!0`) and the renderer bundle references that legacy worker.
The new guard was falsified by temporarily restoring the modern import, which
fails it as intended.
Not verified: canvas render, text-layer selection, bbox highlighting and zoom
under a real window - that needs manual QA, and is #125's job.
@github-actions github-actions Bot added the bug Something isn't working label Sep 25, 2026
@mrsibe
mrsibe merged commit cd6aac6 into main Sep 25, 2026
4 checks passed
@mrsibe
mrsibe deleted the fix/pdf-reader-legacy-build branch September 25, 2026 16:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant