Skip to content

feat: ✨ add support for page lazy-loading - #42

Merged
sebastianMindee merged 17 commits into
build-chainfrom
add-lazier-page-loading
Oct 8, 2026
Merged

sebastianMindee merged 17 commits into
build-chainfrom
add-lazier-page-loading

Conversation

@sebastianMindee

@sebastianMindee sebastianMindee commented Oct 2, 2026 •

Copy link
Copy Markdown
Collaborator

PR Details

Description

Make pages load lazily so that huge documents can be loaded
Screencast-quick.webm
Also fixes zoom to be dependent on image size

Related Issue

Motivation and Context

How Has This Been Tested

Types of changes

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have added type hints (flow) to cover my changes.
  • All new and existing tests passed.

@sebastianMindee sebastianMindee changed the title gcmsg "feat: ✨ add support for lazier-loading" feat: ✨ add support for lazier-loading Oct 2, 2026
@sebastianMindee
sebastianMindee changed the base branch from main to build-chain October 2, 2026 16:05
@sebastianMindee
sebastianMindee requested review from despprie and ianardee and a balanced review from Copilot October 2, 2026 16:06
@sebastianMindee sebastianMindee changed the title feat: ✨ add support for lazier-loading feat: ✨ add support for page lazy-loading Oct 2, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Cancellation, document lifecycle, priority handling, and legacy error semantics contain unresolved functional regressions.

Review effort: Balanced
Findings: 4 Medium severity · 1 Low severity

Open (5)
What changed in this PR

Adds lazy PDF page rendering through a framework-agnostic handle and React hook.

Changes:

  • Adds queued, prioritized, cancellable page rendering.
  • Adds usePDFDocument with progressive prefetching and batching.
  • Updates legacy PDF utilities, exports, tests, and documentation.
File Description
src/​utils/​pdf.ts Implements lazy PDF rendering and caching.
src/​utils/​pdf.spec.ts Tests the new PDF handle.
src/​utils/​getPDFPageCount.ts Uses the new PDF handle.
src/​utils/​getImagesFromPDF.ts Migrates and deprecates eager rendering.
src/​index.ts Exports new public APIs.
src/​hooks/​usePDFDocument.ts Adds the progressive React hook.
src/​hooks/​usePDFDocument.spec.tsx Tests hook behavior.
README.md Documents lazy PDF APIs.
eslint.config.ts Adjusts deprecation linting for tests.
cypress/​integration/​pdf.spec.tsx Tests built PDF functionality.
cypress/​integration/​api.spec.ts Verifies new runtime exports.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/hooks/usePDFDocument.ts Outdated
Comment thread src/hooks/usePDFDocument.ts Outdated
Comment thread src/utils/getImagesFromPDF.ts Outdated
Comment thread src/utils/pdf.ts Outdated
Comment thread README.md

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Render cleanup races, stale hook updates, and unsafe range iteration can produce leaked URLs, incorrect state, or unhandled rejections.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Resolved since last review (5)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Resolution changes leave pages exposing old-resolution URLs

src/​hooks/​usePDFDocument.ts:125

resolution is not part of the state reset identity. After this option changes, previously requested indexes remain in requested, so prefetch skips them and loadPage renders at the new resolution only through the promotion branch, which discards the returned image; pages therefore continues exposing the old-resolution URLs. Reset/re-render page state when the resolution changes.

This issue also appears on line 186 of the same file.

Medium severity Destroy during render leaks the late-created object URL

src/​utils/​pdf.ts:320

destroy() can run while render() is awaiting canvas.toBlob(). In that case the new object URL is created after the revocation pass, and this block still resolves callers with it even though the handle is destroyed, leaking the URL and violating the destroy contract. Revoke a late URL and reject the request when destruction occurred before render completion.

This issue also appears on line 399 of the same file.

Comment thread src/utils/pdf.ts Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Render cancellation, destruction, and hook lifecycle races can expose stale results or reject valid replacement requests.

Review effort: Balanced
Findings: 2 Low severity

Open (2)
Resolved since last review (1)
Previously missed (5)

In code that hasn't changed since last review

Medium severity Ignore stale document generation callbacks

src/​hooks/​usePDFDocument.ts:183

These render callbacks update the latest hook state unconditionally. When source or maxPages changes, cleanup destroys the old handle, but its pending promises can still resolve/reject afterward and write a revoked old URL or destruction error into the new document's state; the old catch can also mutate the new requested set. Scope requests to a document generation and ignore all then/catch/finally work once that generation is stale.

This issue also appears on line 222 of the same file.

Medium severity Remove canceled requests from inFlight immediately

src/​utils/​pdf.ts:302

When the last caller aborts an active render, cancel() is invoked but this request remains in inFlight until the task rejection is processed. A new getPage call made immediately after abort() therefore attaches to the canceled request and is rejected too. Remove the canceled request from inFlight immediately, while ensuring its later settlement cannot delete a replacement request with the same key.

Medium severity Derive page dimensions from a viewport

src/​utils/​pdf.ts:346

page.view is [xMin, yMin, xMax, yMax], so these values are coordinates rather than width and height. PDFs with a non-zero MediaBox/CropBox origin will be rendered at the wrong target resolution; derive the dimensions from a unit-scale viewport instead (which also accounts for rotation).

This issue also appears on line 364 of the same file.

Medium severity Render iterator pages on demand

src/​utils/​pdf.ts:418

The first iterator step allocates promises and queues renders for the entire range. If a consumer reads one page and breaks, all remaining pages still render, defeating the on-demand behavior for large ranges (and later rejected promises can become unhandled). Request each page as the generator advances, or use a bounded prefetch window.

Low severity Test all HEIF MIME type mappings

src/​utils/​image.spec.ts:19

The implementation newly recognizes image/heic-sequence, but this parameterized test omits that MIME type, so a typo or regression in that specific mapping would go undetected. Include all four entries from HEIF_MIME_TYPES.

Comment thread README.md Outdated
Comment thread docs/docs/Utils/get-pdf-page-count.mdx Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

Rendering lifecycle races, orphaned range promises, and stale hook updates can leak resources or corrupt state.

Review effort: Balanced
Findings: None

Resolved since last review (2)
Previously missed (5)

In code that hasn't changed since last review

Medium severity Resolution changes leave pages rendered at the old resolution

src/​hooks/​usePDFDocument.ts:179

When the resolution option changes, already requested indexes remain in requested and their existing URLs remain in pages, while only future pages use the new value. The result can therefore mix old- and new-resolution pages and the initial pages are never rerendered. Invalidate/reload page state when resolution changes, or explicitly make this option immutable for the hook instance.

This issue also appears on line 186 of the same file.

Medium severity Pending renders resolve successfully after destruction

src/​utils/​pdf.ts:324

A render can finish just as destroy() runs. In that ordering this block skips caching but still resolves every pending caller with an image, contradicting the handle contract that pending calls reject after destruction (and potentially returning an already-revoked URL). Recheck destroyed after the awaited render and reject instead of settling successfully.

This issue also appears on line 364 of the same file.

Medium severity Eager page renders can leak and trigger unhandled rejections

src/​utils/​pdf.ts:418

getPages eagerly creates a promise for every page but only awaits them one at a time. If one render rejects, the signal aborts, or the consumer stops iterating early, the remaining promises keep rendering and can reject without handlers, causing unhandledrejection events. Request each page inside the yield loop so iterator cancellation/error leaves no orphaned work.

Low severity PDFSource label shows an inconsistent function type

docs/​docs/​Utils/​get-pdf-page-count.mdx:7

This label currently reads as though PDFSource itself has the displayed function type. Keep the API signature label consistent with the other utility pages.

Low severity Parameterized MIME test omits image/heic-sequence

src/​utils/​image.spec.ts:19

The production MIME set also adds image/heic-sequence, but the parameterized test omits that new branch. Include it so all newly supported MIME variants are covered.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Source or resolution changes can race worker destruction and allow obsolete render callbacks to corrupt current hook state.

Review effort: Balanced
Findings: 1 High severity

Open (1)

Comment thread src/hooks/usePDFDocument.ts

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

Render lifecycle races, cancellation deduplication, source-buffer ownership, and page scaling need correction.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (4)

In code that hasn't changed since last review

Medium severity Ignore render callbacks from stale document generations

src/​hooks/​usePDFDocument.ts:213

This only guards against a resolution change; callbacks from a render on the previous document can still update the new document when the source changes at the same resolution. Destroying the old handle rejects pending renders with a non-AbortError, so the old catch can leave a stale error on the new ready state, and its finally can remove a new render's loading marker. Tag renders with a document/source generation and ignore all then/catch/finally work once that generation is stale.

Medium severity Copy PDF data before transferring it to the worker

src/​utils/​pdf.ts:165

pdf.js transfers DocumentInitParameters.data to its worker and takes ownership of the backing buffer. Passing these objects through directly therefore detaches caller-owned Uint8Array/ArrayBuffer values, so reusing the source (for example when this hook reopens after maxPages changes) can fail. Copy the bytes before passing them to pdf.js.

Medium severity Remove cancelled requests from in-flight deduplication immediately

src/​utils/​pdf.ts:302

When the final caller aborts an actively rendering request, cancellation starts but the request remains in inFlight until the render task rejects. A new getPage for the same key during that interval attaches to the already-cancelled request and is rejected too. Remove the cancelled request from deduplication immediately, and make settle delete the map entry only when it still points to that request so it cannot remove a replacement request.

Medium severity Calculate page dimensions from a unit-scale viewport

src/​utils/​pdf.ts:366

page.view contains coordinates [x1, y1, x2, y2], not [width, height]. PDFs whose page box does not start at (0, 0) therefore get the wrong scale and miss the requested pixel resolution. Derive the dimensions from a unit-scale viewport, which also accounts for page rotation and user units.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

Stale PDF work is not cancelled, numeric options need validation, and the documented zoom break conflicts with the PR classification.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Resolved since last review (1)
Previously missed (3)

In code that hasn't changed since last review

Medium severity Correct breaking-change classification for zoom.max semantics

docs/​docs/​migration.md:16

This section documents that zoom.max has changed meaning for existing consumers, which is a behavioral breaking change, while the PR metadata explicitly marks the change as non-breaking. Either preserve the v1 semantics or classify/release this as a major change so consumers are not unexpectedly affected.

Medium severity Validate prefetch and batch as non-negative integers

src/​hooks/​usePDFDocument.ts:136

prefetch and batch are unrestricted numbers, but the batching arithmetic requires non-negative/positive integers. For example, batch: 0 makes batchBounds return NaN bounds, while fractional values can enqueue fractional page indexes and produce render errors. Validate or normalize these public options before using them.

This issue also appears in the following locations of the same file:

  • line 153
  • line 184
Medium severity Validate resolution before viewport calculation

src/​utils/​pdf.ts:211

Validate the public resolution value before computing a viewport. A value such as 0, a negative number, NaN, or Infinity currently reaches the scale calculation and creates a zero/invalid canvas instead of producing a defined API error.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

Obsolete document openings and old-resolution renders cannot be cancelled, causing substantial wasted work for large PDFs.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Cancel in-progress PDF loading on source change or unmount

src/​hooks/​usePDFDocument.ts:227

handle is assigned only after openPDF resolves, so this cleanup cannot stop an in-progress URL download or parse. When the source changes or the component unmounts, the obsolete document keeps consuming network and shared-worker resources until opening finishes—the exact case that is costly for very large PDFs. Preserve and cancel pdf.js's loading task (for example via an AbortSignal accepted by openPDF) and trigger that cancellation here.

Medium severity Abort stale renders when resolution changes

src/​hooks/​usePDFDocument.ts:255

Changing resolution only makes callbacks stale; these requests have no signal, so every old-resolution render remains queued. After a large loadRange, the obsolete queue is fully rendered before the new low-priority reloads, wasting CPU/memory and delaying updated pages. Associate an AbortController with each document/resolution generation, pass its signal here, and abort the previous generation before requeuing.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Render edge cases and hook lifecycle/cancellation races can produce failures, stale work, or leaked document handles.

Review effort: Balanced
Findings: 1 High severity · 2 Medium severity

Open (3)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Reject non-integer range bounds

src/​hooks/​usePDFDocument.ts:315

Non-integer range bounds are not normalized or rejected, so loadRange(0.5, 2) queues page indexes 0.5 and 1.5 and records render errors. Since this hook's invalid indexes are no-ops, reject non-integer bounds before entering the loop.

This issue also appears on line 324 of the same file.

Low severity Remove the unnecessary preposition in “accommodate for”

README.md:97

Remove the unnecessary preposition in “accommodate for.”

Comment thread src/utils/pdf.ts Outdated
Comment thread src/hooks/usePDFDocument.ts Outdated
Comment thread src/hooks/usePDFDocument.ts

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

Pending render generations can update stale hook state, and repeated page promotion accumulates redundant callers.

Review effort: Balanced
Findings: None

Resolved since last review (3)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Invalidate render generation before destroying the handle

src/​hooks/​usePDFDocument.ts:246

The cleanup destroys the old handle without advancing requested.current. If the source is cleared or the component unmounts while a page is rendering, that render's rejection callback still sees stale() as false and can write a destruction error into the reset state (or update after unmount). Invalidate and abort the render generation before destroying the handle.

This issue also appears on line 349 of the same file.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The new render path retains avoidable pdf.js page resources, and one hook test violates the configured lint rules.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Release pdf.js page resources after rendering

src/​utils/​pdf.ts:380

Each page proxy is retained by pdf.js for the document lifetime, and normal display renders do not automatically release its operator-list/image resources. Since this queue already caches the encoded image, navigating through a large document otherwise retains both representations until destroy(), undermining the lazy-loading memory benefit. Call page.cleanup() in a finally path after rendering/encoding has finished (including cancellation/error paths).

Comment thread src/hooks/usePDFDocument.spec.tsx Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The new documentation claims defaultZoom controls the initial zoom, but the runtime never consumes that option.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (2)

In code that hasn't changed since last review

Low severity Documented defaultZoom behavior is not implemented

docs/​docs/​customization.md:27

defaultZoom is never read by the viewer—the only runtime occurrences are its type/default declarations—so changing it does not set an initial zoom level. This newly documented behavior is therefore inaccurate; either initialize the fitted stage from options.zoom.defaultZoom (with coverage) or remove this claim.

Low severity Remove misleading comparison to unused defaultZoom

docs/​docs/​migration.md:16

The comparison to defaultZoom is misleading because defaultZoom is not consumed anywhere by the viewer at runtime; setting it currently has no effect. Either implement that option as a fitted-image-relative initial level or remove this comparison from the migration guidance.

Comment thread README.md Outdated
Co-authored-by: ianaré <97107275+ianardee@users.noreply.github.com>
@sebastianMindee
sebastianMindee merged commit b7c5e7d into build-chain Oct 8, 2026
4 checks passed
@sebastianMindee sebastianMindee mentioned this pull request Oct 8, 2026
11 tasks done
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.

4 participants