Uh oh!
There was an error while loading. Please reload this page.
feat(files): extract PDF viewer behind SSR boundary and polish file preview - #4316
Conversation
…review
## Core architectural fix
Move all react-pdf / pdfjs-dist code into a new pdf-viewer.tsx module and
import it exclusively via next/dynamic({ ssr: false }). pdfjs-dist v5
references DOMMatrix at module evaluation time, which crashed SSR. The
previous workaround (a DOMMatrix polyfill in instrumentation.ts) is removed
in favour of this proper hard module boundary.
## PDF viewer improvements
- Cursor-anchored zoom: Ctrl/⌘+wheel and trackpad-pinch now zoom toward the
cursor instead of the top-left corner. Toolbar ± buttons anchor to the
viewport centre. Uses the canonical scroll-adjust formula used by map and
canvas viewers.
- Horizontal scroll: dropping flex-col from the scroll container lets the
zoomed pages wrapper overflow naturally and produces a horizontal scrollbar
at zoom > 1×.
- Loading skeleton: replaced the conditional inline skeleton with an
absolute inset-0 overlay so it fills the scroll container correctly in all
layout contexts.
- Shadow tokens: fixed shadow-[var(--shadow-medium)] and
shadow-[var(--shadow-card)] to use the Tailwind utility classes
shadow-medium and shadow-card directly.
## File viewer cleanup
- data-table.tsx: wrap setInputRef in useCallback([]) so the ref callback
has a stable identity across renders. Previously the inline function got a
new identity on every keystroke (because editValue state changed), causing
React to teardown/remount the ref and re-run node.select() on every
character typed.
- preview-panel.tsx: keep useMemo on ctxValue passed to Context.Provider —
Context uses Object.is, so a new object every render causes unnecessary
consumer re-renders.
- resource-content.tsx: remove unnecessary useCallback/useMemo wrappers on
handlers and derived values that have no memoization observers.
## API route
- Wrap content route with withRouteHandler for automatic request-ID tracking
via AsyncLocalStorage; remove manual generateRequestId() calls.
- Add resourceName to audit record; add encoding param support (base64 /
utf-8).
## Query hooks
- Include key (storage object key) in both useWorkspaceFileContent and
useWorkspaceFileBinary query key tuples so the cache is correctly busted
when a file is re-uploaded with a new storage key.
## Other
- Add Suspense boundaries to files/page.tsx and files/[fileId]/page.tsx
(required for useSearchParams inside the Files component).
- Add mmd to SUPPORTED_CODE_EXTENSIONS (Mermaid diagrams).
- Add https: to CSP img-src.
- Remove ==== separator comments from lib/copilot/constants.ts.
- New dependencies: pdfjs-dist 5.4.296, mermaid 11.14.0,
monaco-editor 0.55.1, @monaco-editor/react 4.7.0.The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Fixes SSR/runtime issues for PDFs by loading the new Upgrades text editing and rich previews: replaces the previous text editor implementation with a Monaco-based Adds spreadsheet editing: API and caching updates: the workspace file content Reviewed by Cursor Bugbot for commit 68aeb69. Configure here. |
Greptile SummaryThis PR delivers a well-structured batch of improvements to the Files module: a correct architectural fix for the Confidence Score: 5/5Safe to merge — no P0/P1 issues found; only minor P2 style suggestions. All findings are P2 (style/maintenance): mermaid.initialize called per render, duplicate skeleton JSX, and a useMemo deps-proxy pattern. Core architectural changes (SSR boundary, zoom math, cache-key fix, DataTable ref stability) are correct. Previous P0/P1 issues from earlier review rounds have all been addressed. No files require special attention. pdf-viewer.tsx and preview-panel.tsx have minor P2 style notes worth addressing before the module grows further. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
FV["file-viewer.tsx\n(FileViewer)"]
TE["text-editor.tsx\n(TextEditor)"]
PP["preview-panel.tsx\n(PreviewPanel)"]
DT["data-table.tsx\n(DataTable)"]
PS["preview-shared.tsx\n(shared utils + skeleton)"]
FC["file-category.ts\n(resolveFileCategory)"]
TES["text-editor-state.ts\n(reducer + state logic)"]
PV["pdf-viewer.tsx\n(PdfViewerCore)\n⚡ dynamic ssr:false"]
ME["mermaid\n(dynamic import)"]
Monaco["monaco-editor\n(dynamic ssr:false)"]
FV -->|"resolveFileCategory"| FC
FV -->|"dynamic import"| PV
FV --> TE
FV --> PS
TE --> TES
TE -->|"dynamic import"| Monaco
TE --> PP
PP --> DT
PP -->|"dynamic import"| ME
PP --> PS
PV --> PS
Reviews (7): Last reviewed commit: "refactor(files): cleanup pass — effect, ..." | Re-trigger Greptile |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…eleton tokens - Use toError() from @sim/utils/errors across all catch blocks in file-viewer.tsx, preview-panel.tsx, and route.ts instead of the prohibited `err instanceof Error ? err.message : fallback` pattern - Fix loading skeleton in files.tsx: bg-white → bg-[var(--surface-2)] and shadow-[var(--shadow-medium)] → shadow-medium
- csp.ts: revert bare https: from img-src — it defeats the existing
domain allowlist and opens info-leakage vectors
- files/page.tsx + files/[fileId]/page.tsx: add explicit fallback={null}
to <Suspense> to make intent clear (React defaults to null, but
omitting it looks like an oversight)
- preview-panel.tsx: restore pre passthrough in STATIC_MARKDOWN_COMPONENTS
so Streamdown's wrapping <pre> doesn't nest inside the custom code
block <div>, which produced invalid HTML and broken styling
- file-viewer.tsx: add 'webm' to VIDEO_PREVIEWABLE_EXTENSIONS to match
'video/webm' in VIDEO_PREVIEWABLE_MIME_TYPESwaleedlatif1
commented
Apr 28, 2026
waleedlatif1
commented
Apr 28, 2026
@cursor review |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
The bundle was regenerated non-deterministically during development (same pptxgenjs 4.0.1, different variable names in minifier output). No functional change — restore the prior version to keep the diff clean.
…c workbook mutation
Three bugs from Cursor Bugbot follow-up review:
1. Stale closure in handleEditorMount (Medium): useCallback([], []) captured
content='' at first render. When Monaco mounts after content loads (e.g.
switching from preview to editor mode), lastSyncedContentRef was never
initialized and external content changes stopped syncing. Fixed by keeping
a contentRef updated on every render and reading it inside handleEditorMount.
2. XLSX Ctrl+S discards active cell edit (Medium): handleSave read from
workbookRef.current before DataTable's in-progress editValue was committed.
Fixed by exposing commitEdit() from DataTable via useImperativeHandle
(using an always-current editStateRef so the handle stays stable) and
calling it at the top of handleSave.
3. Async workbook mutation fragility (Low): handleCellChange / handleHeaderChange
updated the workbook inside import('xlsx').then(), creating microtask-order
coupling with handleSave. Fixed by caching the xlsx module in xlsxModuleRef
on first parse and using it synchronously in both handlers.Six-pass cleanup over the file-viewer directory:
Effects (you-might-not-need-an-effect):
- AudioPreview, VideoPreview: replace reset useEffect with key={file.id} so
the component remounts on file change — React's canonical solution
- DocxPreview: same key-prop fix; removes a 5-setState reset effect that was
also clearing containerRef.current.innerHTML unnecessarily
Callbacks (you-might-not-need-a-callback):
- handleEditorMount, handleEditorChange: remove useCallback — MonacoEditor is
dynamic(), not React.memo, so reference stability has no observer
- markSavedContent: remove useCallback — called only through an onSaveRef,
never directly observed
- DataTable.setInputRef: remove useCallback — callback refs on native elements
are called regardless of reference identity
Design tokens (emcn-design-review):
- VideoPreview: bg-black → bg-[var(--surface-inverted)]
- HtmlPreview iframe: bg-white → bg-[var(--surface-2)]
useMemo, useState, and react-query passes found no issues.waleedlatif1
commented
Apr 28, 2026
waleedlatif1
commented
Apr 28, 2026
@cursor review |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
… theme Define sim-dark and sim-light Monaco themes using Sim's exact design tokens instead of the default vs/vs-dark which looked identical to stock VSCode. Chrome changes (both themes): - Background, gutter use --bg (not VSCode's near-black / pure-white defaults) - Line numbers use --text-muted instead of VSCode gray - Cursor switches to --brand-secondary (#33b4ff) - Selection highlight is brand blue at 15% opacity - Scrollbar shadow removed, track uses surface tokens - Bracket match, word highlight, find match all keyed to brand blue - Suggestion/hover widgets use --surface-2 / --border tokens - All hardcoded shadows removed (scrollbar.shadow = transparent) Syntax token changes (inherit: true — base handles unlisted tokens): - Comments: muted gray + italic (vs VSCode's bright green) - Strings: #3ab872 dark / #16825d light (vs VSCode orange-red) - Numbers: warm amber / warm orange (both readable on their backgrounds) - Keywords: #33b4ff dark / #0078d4 light (brand blue family) - Types: complementary blue-gray / purple
…ace-1 - Monaco cursor: #33b4ff (brand blue) → #e6e6e6 dark / #1a1a1a light (text cursor should be neutral, not loud) - VideoPreview background: var(--surface-inverted) → var(--surface-1) (consistent with PDF viewer, fits workspace context over cinema-black)
waleedlatif1
commented
Apr 28, 2026
waleedlatif1
commented
Apr 28, 2026
@cursor review |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
TextEditor, DocxPreview, PptxPreview, XlsxPreview, ImagePreview each moved to their own files. Shared utilities (PreviewError, resolvePreviewError, shouldSuppressStreamingDocumentError, PDF_PAGE_SKELETON) extracted to preview-shared.tsx. file-viewer.tsx is now the orchestrator + MIME constants + small stateless previews (~495 lines).
- Extract useBlobUrl hook shared by AudioPreview and VideoPreview, eliminating ~30 lines of duplicated state/effect logic - Stabilize markSavedContent with useCallback (matches setDraftContent) - Stabilize handleEditorChange with useCallback([setDraftContent]) - Fix pptx static render effect deps: drop redundant dataUpdatedAt (already encoded in cacheKey) and unused workspaceId
…wer logic Extract TextEditorContentState machine and file category resolution into plain .ts modules (text-editor-state.ts, file-category.ts) so they can be unit-tested without React or Next.js overhead. Update component files to import from the extracted modules, eliminating code duplication. Add two test files: - text-editor-state.test.ts: 32 tests covering resolveStreamingEditorContent, the reducer (edit / save-success), and syncTextEditorContentState across all phases (uninitialized, ready, streaming, reconciling) including reference-equality short-circuit checks for zero-allocation paths - file-category.test.ts: 90 tests covering MIME-type routing for all 8 categories, extension fallback, MIME-priority-over-extension, and case-insensitive extension handling
waleedlatif1
commented
Apr 28, 2026
waleedlatif1
commented
Apr 28, 2026
@cursor review |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…ing PDF key
- Add key={file.id} to IframePreview so React remounts on file switch,
preventing stale renderError from persisting across different files
- Replace key={streamingBuffer.byteLength} with a monotonic sequence
counter so same-size successive PDF compilations still trigger a remountwaleedlatif1
commented
Apr 28, 2026
waleedlatif1
commented
Apr 28, 2026
@cursor review |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- pdf-viewer: add setLoadError(null) in onLoadSuccess so the toolbar is not permanently hidden after a failed-then-successful PDF load - file-viewer: consolidate streaming-mode rendering so the debounce period (before rendering=true) shows a skeleton instead of null
…ixes
- text-editor: replace sync-external useEffect with "adjust during render"
pattern so the state machine advances immediately instead of after a paint
- text-editor: remove unnecessary useCallback from markSavedContent (no observer)
- files: narrow deleteTargetFile state to {id, name} — only those fields are used
- files: remove uploadFile (mutation object) from useCallback deps — .mutateAsync is stable
- files: remove unnecessary useCallback from handleNavigateToFiles (no observer)
- files: replace raw <button> with emcn Button for "Clear all filters" actionwaleedlatif1
commented
Apr 28, 2026
waleedlatif1
commented
Apr 28, 2026
@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 68aeb69. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
Summary
This PR delivers a batch of improvements to the Files module: a proper architectural fix for the SSR crash introduced by
pdfjs-distv5, several PDF viewer UX upgrades, and a set of correctness/cleanup fixes across the file preview stack.Core architectural fix — PDF SSR boundary
pdfjs-distv5 referencesDOMMatrixat module evaluation time. When any file that importsreact-pdfis server-evaluated (even with'use client'), Next.js crashes withDOMMatrix is not defined.The previous workaround was a
DOMMatrixpolyfill injected ininstrumentation.ts. This is removed. The correct fix:react-pdf/pdfjs-distcode into a newpdf-viewer.tsxmodulenext/dynamic(() => import('./pdf-viewer'), { ssr: false })next/dynamic({ ssr: false })creates a hard bundle boundary — the imported module is never evaluated during SSR, regardless of'use client'.'use client'alone does not prevent SSR evaluation in the Next.js App Router.PdfDocumentSourcetype is re-exported frompdf-viewer.tsxand imported withimport typeinfile-viewer.tsx— zero runtime edge, type-only.PDF viewer UX improvements
Cursor-anchored zoom
Previously,
style.zoommagnified from the top-left origin, causing the content under the cursor to drift away. Now zoom anchors at the cursor (or viewport centre for toolbar buttons) using the canonical scroll-adjust formula:This is the same algorithm used by Google Maps, Figma, and pdfjs-viewer.
Horizontal scroll at zoom > 1×
Dropping
flex-colfrom the scroll container lets the zoomed pages wrapper overflow naturally — a horizontal scrollbar appears when zoom causes pages to exceed container width. The pages wrapper becomesinline-blockso its width is content-driven, not stretched by flex.Loading skeleton
Replaced the conditional inline skeleton with an
absolute inset-0overlay. It fills the scroll container correctly in all layout contexts and disappears the moment the document loads.Bug fixes
data-table.tsx— ref callback re-runs on every keystrokesetInputRefwas an inline function. SinceeditValuestate changes on every keystroke,DataTablere-renders,setInputRefgets a new function identity, React tears down the old ref (calls it withnull) and mounts the new ref (calls it with the node), firingnode.select()on every character typed — resetting the cursor selection.Fixed by wrapping
setInputRefinuseCallback([], [])for a stable identity.Shadow token syntax
shadow-[var(--shadow-medium)]andshadow-[var(--shadow-card)]bypass the Tailwind utility classes defined intailwind.config.ts. Fixed toshadow-mediumandshadow-card(5+ occurrences acrossfile-viewer.tsxandpdf-viewer.tsx).API route —
withRouteHandlerThe file content route was missing
withRouteHandler, so logger calls did not include the automatic request ID fromAsyncLocalStorage. Wrapped withwithRouteHandler; removed manualgenerateRequestId()calls and[${requestId}]log prefixes. Also:resourceNameto audit recordencodingparam support (base64/utf-8)React Query — cache-busting fix
useWorkspaceFileContentanduseWorkspaceFileBinaryquery keys did not include the storage objectkey. When a file was re-uploaded (new storage key, same file ID), the old cached content was served. Fixed by includingkeyin both query key tuples.Other changes
useSearchParamsfiles/page.tsx,files/[fileId]/page.tsxuseCallback/useMemoin resource-contentresource-content.tsx====separator commentslib/copilot/constants.tsmmdto supported code extensions (Mermaid)lib/uploads/utils/validation.tshttps:to CSPimg-srclib/core/security/csp.tspdfjs-dist 5.4.296,mermaid 11.14.0,monaco-editor 0.55.1,@monaco-editor/react 4.7.0package.jsonTest plan
+/−buttons zoom around the visible viewport centre</>) scrolls to correct page at any zoom level