fix(files): support Safari < 17.4 in PDF preview#4992
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@greptile |
|
@cursor review |
PR SummaryLow Risk Overview Wraps the PDF viewer in In Reviewed by Cursor Bugbot for commit 0b409f9. Configure here. |
pdf.js 5.x calls Promise.withResolvers (Safari >= 17.4) and URL.parse (Safari >= 18) at module-evaluation time, so on older engines importing react-pdf threw an uncaught TypeError that unwound to the workspace error boundary — every PDF preview (chat and Files tab) rendered as "Something went wrong" for those users. - Polyfill both APIs in a side-effect module imported before react-pdf - Serve the legacy pdf.js worker build, which self-polyfills (the worker context is unreachable from main-thread polyfills) - Wrap the PDF preview in an error boundary so a viewer crash degrades to the standard preview fallback instead of replacing the workspace
edfdb0d to
1d06dd9
Compare
|
@greptile |
|
@cursor review |
Greptile SummaryThis PR fixes a production crash in PDF preview for Safari < 17.4 caused by
Confidence Score: 5/5Safe to merge — the changes are narrowly scoped to PDF preview compatibility and graceful error handling, with no impact on unrelated features. The polyfills are correctly guarded with typeof checks so they never clobber native implementations; the legacy worker switch is isolated to the pdf.js worker context; PreviewErrorBoundary is keyed on the boundary (not the child) so it resets correctly on data updates; and the files.tsx mode-init refactor correctly handles all state transitions including the hard-load race. Previous review concerns were already addressed in e913d69. No correctness issues remain. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Browser as "Browser (Safari < 17.4)"
participant Polyfills as "browser-polyfills.ts"
participant ReactPdf as "react-pdf / pdf.js (main)"
participant Worker as "pdf.js legacy worker"
participant Boundary as "PreviewErrorBoundary"
Browser->>Polyfills: import (side-effects)
Polyfills->>Browser: "patch Promise.withResolvers if missing"
Polyfills->>Browser: "patch URL.parse if missing"
Browser->>ReactPdf: import react-pdf
Note over ReactPdf: "module evaluates with patched APIs"
Browser->>Worker: "workerSrc = legacy/build/pdf.worker.min.mjs"
Note over Worker: "self-polyfilling build"
Browser->>Boundary: render PreviewErrorBoundary (keyed)
Boundary->>ReactPdf: render PdfViewerCore
alt "Render succeeds"
Worker-->>ReactPdf: pages decoded
ReactPdf-->>Browser: PDF rendered
else "Render throws"
ReactPdf-->>Boundary: error caught
Boundary-->>Browser: PreviewError fallback shown
end
Reviews (6): Last reviewed commit: "fix(files): settle preview mode when a d..." | Re-trigger Greptile |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
2 issues from previous reviews remain unresolved.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 1d06dd9. Configure here.
…nent stack Key the boundary (not the child) by file id + data version so a tripped boundary remounts and retries when the preview content updates, and include React's componentStack in crash logs.
|
@greptile |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit e913d69. Configure here.
On a hard load of /files/<id> the preview-mode initializer ran before the files list arrived, fell back to the code editor, and the route-change effect never corrected it (the route id never changed). Previewable files (html, markdown, csv, svg, mermaid) opened as source instead of the rendered preview. Defer recording the applied route target until the file record exists so the mode is applied as soon as the list loads, without clobbering manual mode toggles afterwards.
|
@greptile |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit df46f60. Configure here.
…dFile memo Local review follow-ups: reuse the memoized selectedFile/selectedFileRef instead of a second O(n) scan per render, and collapse the applied-mode ref to string | null — initializing at null (the list view) preserves the pre-existing fresh-mount behavior while still deferring deep-link mode application until the file record loads.
|
@greptile |
|
@cursor review |
Gate the mode effect on the files query having resolved (record found or initial load finished) rather than on the record existing, so an invalid or deleted deep-link id decides 'editor' once instead of deferring indefinitely.
|
@greptile |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 0b409f9. Configure here.

Summary
Promise.withResolvers(Safari ≥ 17.4) andURL.parse(Safari ≥ 18) at module-evaluation time, so importingreact-pdfthrew an uncaught TypeError that unwound to the workspace error boundarylib/core/utils/browser-polyfills.ts, imported for side effects before thereact-pdfimport inpdf-viewer.tsx(the crash happens while pdf.js evaluates, so import order matters)workerSrcto the legacy pdf.js worker build — the worker runs in its own context that main-thread polyfills can't reach, and the legacy build bundles its own polyfills (same pdfjs-dist version, so the API/worker version handshake is unaffected)PreviewErrorBoundary(styled after the existing workflowErrorBoundary) so a viewer crash degrades to the standardPreviewErrorfallback instead of replacing the whole workspace viewType of Change
Testing
Reproduced the production incident headlessly: bundled the real
PdfViewerCoreagainst the exact PDF bytes prod served, removed the two APIs to simulate Safari 17.3 → crashed withPromise.withResolvers is not a function(blank render). With this fix the same simulation renders both pages with zero console errors; modern-browser profile verified unaffected.tsc --noEmitandbiome checkclean on all changed files.Checklist