feat(files): extract PDF viewer behind SSR boundary and polish file preview#4316
feat(files): extract PDF viewer behind SSR boundary and polish file preview#4316waleedlatif1 wants to merge 11 commits intostagingfrom
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 Previews are expanded and polished: adds Mermaid ( Editing and persistence are extended: XLSX sheets become editable and can be saved back to storage (base64-encoded) via the existing content API; the Reviewed by Cursor Bugbot for commit 9f34a27. Configure here. |
Greptile SummaryThis PR ships a batch of improvements to the Files module: it correctly fixes the
Confidence Score: 4/5Safe to merge after adding useCallback to setInputRef in data-table.tsx; all other changes are architecturally sound. One P1 defect: the cursor-reset bug in the CSV editor is not actually fixed despite being listed as a fix in the PR description. All other changes (SSR boundary, Monaco migration, cache-busting, route handler) are correct and well-structured. apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/data-table.tsx — setInputRef must be wrapped in useCallback Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[FileViewer - file-viewer.tsx] -->|next/dynamic ssr:false| B[MonacoEditor]
A -->|next/dynamic ssr:false| C[PdfViewerCore - pdf-viewer.tsx]
A --> D[PreviewPanel - preview-panel.tsx]
A --> E[DataTable - data-table.tsx]
D -->|previewType=markdown| F[MarkdownPreview + Callouts]
D -->|previewType=mermaid| G[MermaidFilePreview]
D -->|previewType=csv| H[CsvPreview → DataTable]
D -->|previewType=svg| I[SvgPreview]
D -->|previewType=html| J[HtmlPreview]
C -->|zoom wheel| K[applyZoomAt - cursor-anchored scroll]
C -->|IntersectionObserver| L[currentPage tracking]
M[useWorkspaceFileContent] -->|queryKey includes key| N[Cache busted on re-upload]
M2[useWorkspaceFileBinary] -->|queryKey includes key| N
style E fill:#f87171,color:#fff
style C fill:#86efac,color:#000
style B fill:#86efac,color:#000
Reviews (3): Last reviewed commit: "refactor(files): cleanup anti-patterns a..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…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_TYPES
|
@greptile |
|
@cursor review |
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.
|
@greptile |
|
@cursor review |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9f34a27. Configure here.
| node.focus() | ||
| node.select() | ||
| } | ||
| } |
There was a problem hiding this comment.
Ref callback missing useCallback causes cursor reset on keystroke
High Severity
setInputRef is declared as a plain inline function instead of being wrapped in useCallback. The PR description explicitly states this was "fixed by wrapping setInputRef in useCallback([], []) for a stable identity," but the actual code omits the wrapper. Since editValue state changes on every keystroke, DataTableBase re-renders, setInputRef gets a new identity, React tears down the old ref and mounts the new one, calling node.focus() and node.select() on every character typed — resetting the cursor selection to "select all" on each keystroke.
Reviewed by Cursor Bugbot for commit 9f34a27. Configure here.
| return () => { | ||
| resizeObserver.disconnect() | ||
| } | ||
| }, [shouldUseCodeRenderer]) |
There was a problem hiding this comment.
Stale saveImmediately captured in Monaco Ctrl+S command
Low Severity
handleEditorMount is a plain function that captures saveImmediately via closure when calling editor.addCommand. While saveImmediately from useAutosave has a stable identity (it uses useCallback with stable deps and reads from refs internally), the onSave passed to useAutosave is also a plain function that changes identity every render. This works only because useAutosave stores onSave in a ref (onSaveRef). The coupling is fragile — handleEditorMount relies on an implementation detail of useAutosave for correctness.
Reviewed by Cursor Bugbot for commit 9f34a27. Configure here.
| const handleSave = useCallback(async () => { | ||
| dataTableRef.current?.commitEdit() | ||
| const wb = workbookRef.current | ||
| if (!wb || isSavingRef.current) return |
There was a problem hiding this comment.
XLSX handleSave calls commitEdit then reads workbook synchronously but state update is async
Medium Severity
handleSave calls dataTableRef.current?.commitEdit() and immediately reads workbookRef.current on the next line. While commitEdit synchronously calls editConfig.onCellChange/onHeaderChange (which update the workbook ref synchronously), it also calls setEditingCell(null). This triggers an async React state update. If handleSave is called via Ctrl+S while a cell is being edited and onBlur fires simultaneously, there's a potential race where commitEdit runs twice — once from the imperative handle and once from the blur handler — potentially double-applying the edit.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 9f34a27. Configure here.
| const setInputRef = (node: HTMLInputElement | null) => { | ||
| if (node) { | ||
| node.focus() | ||
| node.select() | ||
| } |
There was a problem hiding this comment.
setInputRef still unstable — cursor resets on every keystroke
setInputRef is an inline arrow function. Every time editValue state changes (each character typed), DataTableBase re-renders and React sees a new function reference for the ref prop. React tears down the old ref (calls it with null) and mounts the new one (calls it with the node), triggering node.focus(); node.select() on every keystroke — exactly the bug this PR claims to fix.
The PR description explicitly states the fix is useCallback([], []), but useCallback is not even imported and the wrapper was never applied.
Add useCallback to the import on line 3 and wrap the callback:
import { forwardRef, memo, useCallback, useImperativeHandle, useRef, useState } from 'react'
| const setInputRef = (node: HTMLInputElement | null) => { | |
| if (node) { | |
| node.focus() | |
| node.select() | |
| } | |
| const setInputRef = useCallback((node: HTMLInputElement | null) => { | |
| if (node) { | |
| node.focus() | |
| node.select() | |
| } | |
| }, []) |
… 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)
…ommit in DataTable Wrap setInputRef in useCallback([], []) so React doesn't tear down and re-mount the input ref on every keystroke. Without stable identity, every editValue state change caused node.focus()/node.select() to fire, resetting the cursor selection to "select all" on each character typed. Add isCommittedRef to guard both the imperative commitEdit handle and the inline commitEdit (called by onBlur) against double-application. The ref is cleared in startEdit and set to true on the first commit, so a concurrent onBlur cannot re-apply the same edit.
1 participant
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