Restore mobile PDF preview compatibility for browsers without Array.prototype.at#729
Restore mobile PDF preview compatibility for browsers without Array.prototype.at#729
Array.prototype.at#729Conversation
Agent-Logs-Url: https://github.com/kekingcn/kkFileView/sessions/287f0212-d368-4b59-ae70-0374fb3fe76f Co-authored-by: klboke <18591662+klboke@users.noreply.github.com>
Agent-Logs-Url: https://github.com/kekingcn/kkFileView/sessions/287f0212-d368-4b59-ae70-0374fb3fe76f Co-authored-by: klboke <18591662+klboke@users.noreply.github.com>
Agent-Logs-Url: https://github.com/kekingcn/kkFileView/sessions/287f0212-d368-4b59-ae70-0374fb3fe76f Co-authored-by: klboke <18591662+klboke@users.noreply.github.com>
Array.prototype.at
|
整体看下来没问题,这个修复方式比较克制,风险也相对可控。 我的 review 结论:
目前我没有看到 blocker,可以合。 一个后续可选增强建议(不阻塞这次 PR):
总体来说,这个 PR 我这边是 LGTM 的。 |
|
补充一下我这边的本地验证结果(不是只看 diff): 我把这个 PR 拉到本地分支后,额外做了几轮验证:
另外我还补做了一个更贴近问题本身的本地模块级验证:
本地结果是:
所以从我这边的本地验证看,这个 PR 不只是“把 compatibility 脚本接上了”,而是确实能补上 |
klboke
left a comment
There was a problem hiding this comment.
本地已补充验证:兼容层可实际修复 .at() 缺失场景,且未破坏现有 PDF 预览链路。Approve。
There was a problem hiding this comment.
Pull request overview
Restores PDF.js initialization on older mobile browsers/WebViews that lack Array.prototype.at, preventing blank PDF previews and subsequent worker/fake-worker startup failures.
Changes:
- Added a small
at()compatibility shim and ensured it loads beforepdf.mjsin the viewer. - Imported the same shim from
pdf.worker.mjsso both worker and fake-worker paths are covered. - Added regression tests to ensure the shim remains wired into both entrypoints.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| server/src/main/resources/static/pdfjs/web/viewer.html | Loads the compatibility shim before pdf.mjs. |
| server/src/main/resources/static/pdfjs/web/compatibility.mjs | Adds an at() polyfill for arrays/strings/typed arrays. |
| server/src/main/resources/static/pdfjs/build/pdf.worker.mjs | Imports the compatibility shim in the worker entrypoint. |
| server/src/test/java/cn/keking/PdfViewerCompatibilityTests.java | Verifies shim wiring in viewer and worker assets. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Float32Array, | ||
| Float64Array, | ||
| globalThis.BigInt64Array, | ||
| globalThis.BigUint64Array, | ||
| ]; |
There was a problem hiding this comment.
compatibility.mjs references globalThis directly when building atTargets. In environments without native globalThis (which can overlap with browsers missing Array.prototype.at), this will throw before the polyfill installs and prevent PDF.js from booting. Use a safe global-object resolution (e.g., prefer globalThis when defined, otherwise self/window) before accessing BigInt typed array constructors.
| if (Number.isNaN(relativeIndex)) { | ||
| relativeIndex = 0; | ||
| } | ||
| relativeIndex = Math.trunc(relativeIndex); |
There was a problem hiding this comment.
The polyfill implementation depends on Number.isNaN and Math.trunc, but the compatibility module runs before PDF.js (and its bundled polyfills) load. On older engines these methods may be missing, causing the shim itself to crash. Consider avoiding these dependencies (e.g., NaN check via x !== x and integer conversion via Math.floor/Math.ceil) so the shim only requires the minimum baseline needed to run module scripts.
| if (Number.isNaN(relativeIndex)) { | |
| relativeIndex = 0; | |
| } | |
| relativeIndex = Math.trunc(relativeIndex); | |
| if (relativeIndex !== relativeIndex) { | |
| relativeIndex = 0; | |
| } | |
| relativeIndex = | |
| relativeIndex < 0 ? Math.ceil(relativeIndex) : Math.floor(relativeIndex); |
| const atTargets = [ | ||
| Array, | ||
| String, | ||
| Int8Array, |
There was a problem hiding this comment.
The String polyfill uses this[resolvedIndex], which returns UTF-16 code units; native String.prototype.at is code-point aware (handles surrogate pairs). Since String.prototype.at is included in atTargets, either implement the code-point semantics or drop String from the targets to avoid introducing subtly incorrect behavior.
| void shouldLoadCompatibilityModuleBeforePdfJs() throws IOException { | ||
| String viewerHtml = readResource("/static/pdfjs/web/viewer.html"); | ||
|
|
||
| assertTrue(viewerHtml.contains("<script src=\"compatibility.mjs\" type=\"module\"></script>")); | ||
| assertTrue(viewerHtml.indexOf("compatibility.mjs") < viewerHtml.indexOf("../build/pdf.mjs")); | ||
| } |
There was a problem hiding this comment.
These assertions are very brittle because they depend on exact HTML/script formatting (attribute order, whitespace, self-closing style). A small upstream PDF.js update could break the test without changing behavior. Consider making the check more resilient (e.g., regex for a module script whose src ends with compatibility.mjs, and order based on match positions) while still ensuring the shim loads before pdf.mjs.
| void shouldLoadCompatibilityModuleInPdfWorker() throws IOException { | ||
| String workerScript = readResource("/static/pdfjs/build/pdf.worker.mjs"); | ||
|
|
||
| assertTrue(workerScript.contains("import \"../web/compatibility.mjs\";")); |
There was a problem hiding this comment.
This assertion requires the worker import line to match exactly (including quote style and semicolon). To reduce false negatives on future PDF.js asset updates, consider loosening it to a substring/regex that tolerates whitespace/quotes while still verifying the compatibility module is imported from the worker entrypoint.
| assertTrue(workerScript.contains("import \"../web/compatibility.mjs\";")); | |
| assertTrue(workerScript.matches("(?s).*import\\s+[\"']\\.\\./web/compatibility\\.mjs[\"']\\s*;?.*")); |
Mobile PDF preview could fail during PDF.js initialization with
pattern.at is not a function, leaving the viewer blank on older mobile browsers. The failure also broke worker startup, cascading into fake-worker initialization errors.Problem scope
kkFileViewrely on.at()in both the viewer/runtime path and the worker path..at(), so PDF rendering fails before the document can load.Compatibility shim
server/src/main/resources/static/pdfjs/web/compatibility.mjs.at()for:ArrayStringViewer/worker wiring
pdf.mjsinserver/src/main/resources/static/pdfjs/web/viewer.htmlserver/src/main/resources/static/pdfjs/build/pdf.worker.mjsRegression coverage
server/src/test/java/cn/keking/PdfViewerCompatibilityTests.javaRepresentative change
Screenshot
Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
repository.aspose.com/usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java --enable-native-access=ALL-UNNAMED -classpath /usr/share/apache-maven-3.9.14/boot/plexus-classworlds-2.9.0.jar -Dclassworlds.conf=/usr/share/apache-maven-3.9.14/bin/m2.conf -Dmaven.home=/usr/share/apache-maven-3.9.14 -Dlibrary.jansi.path=/usr/share/apache-maven-3.9.14/lib/jansi-native -Dmaven.multiModuleProjectDirectory=/home/REDACTED/work/kkFileView/kkFileView org.codehaus.plexus.classworlds.launcher.Launcher -B clean test(dns block)/usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java --enable-native-access=ALL-UNNAMED -classpath /usr/share/apache-maven-3.9.14/boot/plexus-classworlds-2.9.0.jar -Dclassworlds.conf=/usr/share/apache-maven-3.9.14/bin/m2.conf -Dmaven.home=/usr/share/apache-maven-3.9.14 -Dlibrary.jansi.path=/usr/share/apache-maven-3.9.14/lib/jansi-native -Dmaven.multiModuleProjectDirectory=/home/REDACTED/work/kkFileView/kkFileView org.codehaus.plexus.classworlds.launcher.Launcher -B clean package -Dmaven.test.skip=true(dns block)/usr/lib/jvm/temurin-17-jdk-amd64/bin/java /usr/lib/jvm/temurin-17-jdk-amd64/bin/java --enable-native-access=ALL-UNNAMED -classpath /usr/share/apache-maven-3.9.14/boot/plexus-classworlds-2.9.0.jar -Dclassworlds.conf=/usr/share/apache-maven-3.9.14/bin/m2.conf -Dmaven.home=/usr/share/apache-maven-3.9.14 -Dlibrary.jansi.path=/usr/share/apache-maven-3.9.14/lib/jansi-native -Dmaven.multiModuleProjectDirectory=/home/REDACTED/work/kkFileView/kkFileView org.codehaus.plexus.classworlds.launcher.Launcher -f pom.xml -B -V -e -Dfindbugs.skip -Dcheckstyle.skip -Dpmd.skip=true -Dspotbugs.skip -Denforcer.skip -Dmaven.javadoc.skip(dns block)If you need me to access, download, or install something from one of these locations, you can either: