Skip to content

fix(platform): restore pdf image detection on scanned files - #3103

Closed
Israeltheminer wants to merge 1 commit into
mainfrom
fix/pdfjs-getorinsertcomputed
Closed

fix(platform): restore pdf image detection on scanned files#3103
Israeltheminer wants to merge 1 commit into
mainfrom
fix/pdfjs-getorinsertcomputed

Conversation

@Israeltheminer

Copy link
Copy Markdown
Collaborator

Scanned PDFs index thin or empty because image detection never runs. This restores it.

Closes #3018.

Why

pdfjs 5.x calls Map.prototype.getOrInsertComputed, which Node 22 does not ship, so the operator-list read throws once per page:

Failed to read PDF operator list: this._intentStates.getOrInsertComputed is not a function

The damage is narrower than the log suggests, and worse where it lands. Text-layer extraction (getTextContent) still runs, so an ordinary digital PDF indexes fine and only produces noise. But the operator list is what finds embedded images and decides "scanned page → OCR / describe". Without it a scanned or image-heavy PDF never reaches OCR, and indexes as thin or empty text with nothing but a warning to say why.

What changed

One shim beside the existing ES2025 fills in pdfjs_dom_polyfill.ts, guarded like the rest so a real implementation wins once the runtime moves to Node ≥24.

Scope is Map only, from the library rather than from the proposal: every call site in both the main and worker builds — _intentStates, methodPromises, objs, _cachedBitmapsMap, popupToElements — is a plain Map. No WeakMap call exists, so none is shimmed.

Deliberately NOT the legacy pdfjs build. That would resolve the same call, and the loader already avoids it for Convex bundle size.

Risk

Silencing the warning would have been the wrong fix. Suppressing it without restoring the operator list leaves scanned PDFs indexing badly with nothing to explain it — a quality loss that no longer announces itself.

Presence, not truthiness. A callback returning undefined still inserts, so a second call does not recompute. Getting this wrong would be invisible until a cache entry legitimately held undefined.

The callable check runs before the lookup, as the proposal specifies. Without that ordering a present key returns happily and a bad callback goes unnoticed until some later call happens to miss.

Tests

Seven cases: the method is installed, compute-and-insert, an existing value returned without calling the callback, undefined stored as a real value, the computed value winning when the callback inserted the key itself, a non-callable callback rejected for both a present and an absent key, and a real implementation left alone.

Four deliberate breakages, all caught. One survived the first round: dropping the callable check still threw, because calling a string throws TypeError anyway — the case that distinguishes them is a present key, which is now tested.

The last case installs a stand-in for a native implementation and removes it in a finally, because the guard correctly refuses to replace it and every later suite in that worker would otherwise inherit it.

Scope

Does not change extraction, OCR or chunking. Scanned PDFs indexed before this reach the corpus with whatever thin text they produced; re-indexing them is a separate step.

Gate: repo-wide typecheck, oxlint --type-aware, oxfmt, knip, SAST 0 findings, platform suite 75,819 passing.

@Israeltheminer

Copy link
Copy Markdown
Collaborator Author

Superseded by #3170, which carries the same fix against the current tree.

Both files this branch touches were deleted by #3125. The polyfill module itself was re-homed to backend/core/lib/knowledge/extraction/pdfjs_dom_polyfill.ts, so the fix is now a small addition to ensureEs2025Shims rather than a new block — it sits beside the Promise.try and base64 fills that already exist for the same reason.

#3018 stays open until #3170 lands.

One thing worth recording from redoing it: Bun ships Map.prototype.getOrInsertComputed natively and Node does not. The suite runs under Node, so the assertions exercise the real gap — but a test run under Bun would have passed against the native implementation and proven nothing.

@Israeltheminer

Copy link
Copy Markdown
Collaborator Author

Closing: #3170 carried this fix and is merged. #3018 is closed by it.

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.

Bug: pdf.js operator-list read fails on every page it indexes

1 participant