# feat(file-browser): add PDF support with viewer and file type checks#254
Open
gauravprasadgp wants to merge 8 commits intodaggerhashimoto:masterfrom
Open
# feat(file-browser): add PDF support with viewer and file type checks#254gauravprasadgp wants to merge 8 commits intodaggerhashimoto:masterfrom
gauravprasadgp wants to merge 8 commits intodaggerhashimoto:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end PDF support (type detection, viewer component, backend streaming with Range support and larger size cap) and relaxes framing protections to allow same-origin embedding (CSP/frame-ancestors and X-Frame-Options changed). Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client UI
participant Tab as TabbedContentArea / PdfViewer
participant Browser as Browser (iframe)
participant Server as Server /api/files/raw
participant FS as Filesystem (fs.createReadStream)
UI->>Tab: open PDF file
Tab->>Browser: render iframe src="/api/files/raw?path=...&agentId=..."
Browser->>Server: GET /api/files/raw (optional Range header)
Server->>FS: fs.createReadStream(filePath, {start, end})
FS-->>Server: stream chunks
Server-->>Browser: stream response (ReadableStream), 206 + Content-Range if partial
Browser-->>Tab: iframe loads PDF content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/file-browser/hooks/useOpenFiles.ts (1)
518-523:⚠️ Potential issue | 🟠 MajorPDF tabs still won’t restore from persisted state.
Line 518 correctly skips text-fetch for PDFs on open, but restore logic (Line 357+) still uses
/api/files/readfor all persisted tabs. PDF entries will be dropped on reload because binary reads are non-ok.🛠️ Suggested fix in restore loop
for (const path of persistedPaths) { + if (isImageFile(basename(path)) || isPdfFile(basename(path))) { + files.push({ + path, + name: basename(path), + content: '', + savedContent: '', + dirty: false, + locked: false, + mtime: 0, + loading: false, + }); + continue; + } + const { scopedPathKey, token } = nextReadRequestToken(targetAgentId, path);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/file-browser/hooks/useOpenFiles.ts` around lines 518 - 523, The restore path drops PDF/image tabs because the persisted-restore loop still calls the text-only endpoint (/api/files/read) for every persisted tab; update the restore logic that processes persistedOpenFiles/persistedTabs (the loop that currently calls the read endpoint) to detect isImageFile(basename(path)) || isPdfFile(basename(path)) and skip the /api/files/read call for those entries—just add them back into openFiles via setOpenFiles (or the existing accumulator) with loading:false and the same metadata (path, name, type) so binary tabs persist unchanged; leave text-file behavior unchanged and only call /api/files/read for non-image/non-pdf files.
🧹 Nitpick comments (1)
server/middleware/security-headers.ts (1)
59-60: Consider limiting framing relaxation to/api/files/rawinstead of all responses.This change is valid for PDF embedding, but applying it globally broadens clickjacking exposure for unrelated pages. Route-scoped overrides would preserve tighter defaults elsewhere.
Also applies to: 75-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/middleware/security-headers.ts` around lines 59 - 60, The CSP currently relaxes frame-src globally; change it so the default Content-Security-Policy keeps "frame-ancestors 'self'" and the restrictive frame-src without tradingview hosts, and add a route check in the middleware (inspect the request URL/path in the same middleware function that builds the headers) to only append the relaxed frame-src string ("frame-src 'self' https://s3.tradingview.com https://www.tradingview.com https://www.tradingview-widget.com https://s.tradingview.com") when the request path is "/api/files/raw". Update both occurrences of the relaxed directive (the one at the shown diff and the similar one at the other occurrence) so unrelated routes keep the tighter policy. Ensure the header key (Content-Security-Policy) is set once per response based on this conditional.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/middleware/security-headers.ts`:
- Line 60: Update the tests in server/middleware/security-headers.test.ts to
match the new framing policy: change any assertion expecting
Content-Security-Policy frame-ancestors 'none' to expect "frame-ancestors
'self'" and change any assertion expecting X-Frame-Options: DENY to expect
X-Frame-Options: SAMEORIGIN (or remove the X-Frame-Options check if the
middleware no longer sets it). Locate the assertions that reference
CSP/frame-ancestors and X-Frame-Options in the test and update the expected
header values to the new policy used by the security-headers middleware.
In `@server/routes/file-browser.ts`:
- Around line 793-797: The handler in file-browser.ts currently sets maxSize for
PDFs to ~50MB but still uses fs.readFile which buffers the whole file; change
the logic in the request handler that reads the file (replace the fs.readFile
usage) to stream the file with fs.createReadStream (or stream.pipeline) to the
response instead, set appropriate headers (Content-Type, Content-Length) and
handle stream errors and client aborts; keep the maxSize check using the
existing maxSize variable but stop using full-buffer reads when sending PDF (and
large images) to avoid memory spikes.
In `@src/features/file-browser/FileTreeNode.tsx`:
- Line 70: The precedence bug in FileTreeNode.tsx makes isPdfFile bypass the
directory check; update the canOpen expression so isPdfFile(entry.name) is only
considered when the entry is not a directory — i.e., ensure the entire file-type
checks are grouped under the !isDir condition used in the component (the const
canOpen variable), so directories (isDir === true) cannot be treated as openable
even if their name matches *.pdf.
---
Outside diff comments:
In `@src/features/file-browser/hooks/useOpenFiles.ts`:
- Around line 518-523: The restore path drops PDF/image tabs because the
persisted-restore loop still calls the text-only endpoint (/api/files/read) for
every persisted tab; update the restore logic that processes
persistedOpenFiles/persistedTabs (the loop that currently calls the read
endpoint) to detect isImageFile(basename(path)) || isPdfFile(basename(path)) and
skip the /api/files/read call for those entries—just add them back into
openFiles via setOpenFiles (or the existing accumulator) with loading:false and
the same metadata (path, name, type) so binary tabs persist unchanged; leave
text-file behavior unchanged and only call /api/files/read for non-image/non-pdf
files.
---
Nitpick comments:
In `@server/middleware/security-headers.ts`:
- Around line 59-60: The CSP currently relaxes frame-src globally; change it so
the default Content-Security-Policy keeps "frame-ancestors 'self'" and the
restrictive frame-src without tradingview hosts, and add a route check in the
middleware (inspect the request URL/path in the same middleware function that
builds the headers) to only append the relaxed frame-src string ("frame-src
'self' https://s3.tradingview.com https://www.tradingview.com
https://www.tradingview-widget.com https://s.tradingview.com") when the request
path is "/api/files/raw". Update both occurrences of the relaxed directive (the
one at the shown diff and the similar one at the other occurrence) so unrelated
routes keep the tighter policy. Ensure the header key (Content-Security-Policy)
is set once per response based on this conditional.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: de52e38a-2bc2-404c-860a-2aed322cca91
📒 Files selected for processing (7)
server/middleware/security-headers.tsserver/routes/file-browser.tssrc/features/file-browser/FileTreeNode.tsxsrc/features/file-browser/PdfViewer.tsxsrc/features/file-browser/TabbedContentArea.tsxsrc/features/file-browser/hooks/useOpenFiles.tssrc/features/file-browser/utils/fileTypes.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/routes/file-browser.ts (1)
790-813:⚠️ Potential issue | 🟠 MajorHandle
Rangerequests for PDF responses.The native browser PDF viewer typically requests byte ranges for incremental rendering and page seeks. This handler always returns
200with the full body, so large PDFs will be downloaded end-to-end before later pages can render, and seeks can re-transfer the whole file. Please addRangeparsing and return206withContent-Range/Accept-Rangesfor🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/file-browser.ts` around lines 790 - 813, Add proper HTTP Range support for PDF responses: check c.req/c.request.headers.get('range') when ext === '.pdf', parse "bytes=start-end", validate against stat.size, and if valid create a ranged fsSync.createReadStream(resolved, { start, end }), set status 206 and headers 'Content-Range', 'Accept-Ranges: bytes', and 'Content-Length' (use end-start+1) and stream that (convert with nodeStreamToWebStream if needed); if no Range header keep the existing full-response behavior, and if the Range is invalid return 416 with 'Content-Range: bytes */<totalSize>' — reuse variables resolved, stat, fileStream creation logic, and nodeStreamToWebStream/webStream conversion for the actual streaming.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/features/file-browser/hooks/useOpenFiles.ts`:
- Around line 367-386: The PDF/image branch in useOpenFiles (where
isImageFile/isPdfFile and createSnapshotBackedOpenFile are used) needs to opt
out of text reloads: when an external change is detected by handleFileChanged
(which currently calls reloadFile -> /api/files/read), do not call reloadFile
for raw assets; instead trigger a raw-asset refresh path (e.g., increment a
viewerVersion or query param stored on the open-file entry and signal the file
viewer to remount the iframe) so the viewer reloads the binary resource; apply
the same change for the other image/PDF handling location noted around the
second reference (lines ~539-543) so both places use the viewerVersion bump
instead of the text reload flow.
---
Outside diff comments:
In `@server/routes/file-browser.ts`:
- Around line 790-813: Add proper HTTP Range support for PDF responses: check
c.req/c.request.headers.get('range') when ext === '.pdf', parse
"bytes=start-end", validate against stat.size, and if valid create a ranged
fsSync.createReadStream(resolved, { start, end }), set status 206 and headers
'Content-Range', 'Accept-Ranges: bytes', and 'Content-Length' (use end-start+1)
and stream that (convert with nodeStreamToWebStream if needed); if no Range
header keep the existing full-response behavior, and if the Range is invalid
return 416 with 'Content-Range: bytes */<totalSize>' — reuse variables resolved,
stat, fileStream creation logic, and nodeStreamToWebStream/webStream conversion
for the actual streaming.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ccea93e5-c365-4d84-b77c-f709cb552145
📒 Files selected for processing (4)
server/middleware/security-headers.test.tsserver/routes/file-browser.tssrc/features/file-browser/FileTreeNode.tsxsrc/features/file-browser/hooks/useOpenFiles.ts
✅ Files skipped from review due to trivial changes (1)
- server/middleware/security-headers.test.ts
… and trigger remount on viewer version change
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/routes/file-browser.ts (1)
833-884: Collapse this to a single stream body path.Line 836 checks
fileStream.readable, but a freshly createdfs.ReadStreamis already readable, sonodeStreamToWebStream()never runs. Keeping both paths is misleading and leaves dead code to maintain. Prefer one explicit conversion strategy here.♻️ Proposed simplification
-import { pipeline } from 'node:stream/promises'; +import { Readable } from 'node:stream'; @@ - const fileStream = fsSync.createReadStream(resolved, { start, end }); - - // Convert Node.js stream to Web Stream for Response compatibility - const webStream = fileStream.readable ? fileStream as any : nodeStreamToWebStream(fileStream); + const fileStream = fsSync.createReadStream(resolved, { start, end }); + const webStream = Readable.toWeb(fileStream) as ReadableStream<Uint8Array>; @@ -/** - * Convert a Node.js ReadableStream to a Web Stream (ReadableStream). - * Handles error events and proper cleanup. - */ -function nodeStreamToWebStream(nodeStream: fsSync.ReadStream): ReadableStream<Uint8Array> { - return new ReadableStream({ - start(controller) { - nodeStream.on('data', (chunk: string | Buffer) => { - const bytes = typeof chunk === 'string' ? Buffer.from(chunk) : chunk; - controller.enqueue(new Uint8Array(bytes)); - }); - nodeStream.on('end', () => { - controller.close(); - }); - nodeStream.on('error', (err: Error) => { - console.error('Stream read error:', err); - controller.error(err); - }); - }, - cancel(reason) { - nodeStream.destroy(); - }, - }); -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/file-browser.ts` around lines 833 - 884, The code currently branches on fileStream.readable (fileStream) but fs.createReadStream is always readable, so remove the conditional and always convert the Node ReadStream to a web-compatible ReadableStream via nodeStreamToWebStream(fileStream); update the Response to use that single webStream variable and remove the unreachable branch and any dead checks (e.g., the fileStream.readable check), keeping the nodeStreamToWebStream function as the single conversion strategy and ensuring error handling/cleanup remains intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/features/file-browser/hooks/useOpenFiles.ts`:
- Around line 367-386: The PDF shortcut branch treats PDFs as viewable
everywhere but PdfViewer uses /api/files/raw which is only allowed for local
workspaces; update the isPdfFile check to require local-workspace support (e.g.,
add a condition like && isLocalWorkspace() or workspace.supportsLocalFiles)
before restoring a PDF tab so that non-local/gateway workspaces fall back to the
supported-state error path; apply the same gating change to the other PDF
shortcut occurrence around the 539-543 area and keep usage of getDirtySnapshot,
createSnapshotBackedOpenFile, and the existing image branch unchanged.
---
Nitpick comments:
In `@server/routes/file-browser.ts`:
- Around line 833-884: The code currently branches on fileStream.readable
(fileStream) but fs.createReadStream is always readable, so remove the
conditional and always convert the Node ReadStream to a web-compatible
ReadableStream via nodeStreamToWebStream(fileStream); update the Response to use
that single webStream variable and remove the unreachable branch and any dead
checks (e.g., the fileStream.readable check), keeping the nodeStreamToWebStream
function as the single conversion strategy and ensuring error handling/cleanup
remains intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c7082e05-dfb5-43c7-a0be-3ac07684f488
📒 Files selected for processing (5)
server/routes/file-browser.tssrc/features/file-browser/ImageViewer.tsxsrc/features/file-browser/PdfViewer.tsxsrc/features/file-browser/hooks/useOpenFiles.tssrc/features/file-browser/types.ts
✅ Files skipped from review due to trivial changes (1)
- src/features/file-browser/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/features/file-browser/PdfViewer.tsx
Comment on lines
+367
to
+386
| // Skip /api/files/read for image and PDF files; restore them directly | ||
| const fileName = basename(path); | ||
| if (isImageFile(fileName) || isPdfFile(fileName)) { | ||
| const dirtySnapshot = getDirtySnapshot(); | ||
| if (dirtySnapshot) { | ||
| files.push(createSnapshotBackedOpenFile(path, dirtySnapshot)); | ||
| } else { | ||
| files.push({ | ||
| path, | ||
| name: fileName, | ||
| content: '', | ||
| savedContent: '', | ||
| dirty: false, | ||
| locked: false, | ||
| mtime: 0, | ||
| loading: false, | ||
| }); | ||
| } | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Gate the PDF raw-asset path behind local-workspace support.
These branches now treat PDFs as immediately viewable everywhere, but PdfViewer loads them from /api/files/raw, and server/routes/file-browser.ts, Line 775 still rejects raw requests for non-local workspaces. In a gateway-backed workspace, restoring or opening a PDF tab will therefore land in a broken viewer instead of a supported-state error. Either keep the PDF shortcut local-only here or add remote raw-file support first.
Also applies to: 539-543
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/file-browser/hooks/useOpenFiles.ts` around lines 367 - 386, The
PDF shortcut branch treats PDFs as viewable everywhere but PdfViewer uses
/api/files/raw which is only allowed for local workspaces; update the isPdfFile
check to require local-workspace support (e.g., add a condition like &&
isLocalWorkspace() or workspace.supportsLocalFiles) before restoring a PDF tab
so that non-local/gateway workspaces fall back to the supported-state error
path; apply the same gating change to the other PDF shortcut occurrence around
the 539-543 area and keep usage of getDirtySnapshot,
createSnapshotBackedOpenFile, and the existing image branch unchanged.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/routes/file-browser.ts (2)
22-22: Unused import:pipelineis never used.The
pipelineimport fromnode:stream/promisesis not referenced anywhere in this file. Remove it to keep imports clean.🧹 Proposed fix
-import { pipeline } from 'node:stream/promises';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/file-browser.ts` at line 22, The import pipeline from 'node:stream/promises' in file-browser.ts is unused; remove the named import pipeline (and the corresponding import entry) so the file no longer imports an unused symbol and imports remain clean.
861-884: Consider usingReadable.toWeb()instead of a custom implementation, but note the error logging trade-off.Node.js provides
Readable.toWeb()(available since v17, compatible with your >=22.0.0 requirement) which handles stream conversion with proper cleanup. However, your custom implementation includesconsole.errorlogging for stream errors thatReadable.toWeb()does not provide. If this error visibility is important for debugging or monitoring, keep the current implementation; otherwise, you could simplify to:import { Readable } from 'node:stream'; const webStream = Readable.toWeb(fileStream) as ReadableStream<Uint8Array>;Then remove the
nodeStreamToWebStreamfunction entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/file-browser.ts` around lines 861 - 884, The nodeStreamToWebStream function currently reimplements stream conversion and logs errors via console.error; replace it with Node's built-in conversion to simplify the code unless the explicit error logging is required. Either remove nodeStreamToWebStream and use Readable.toWeb(fileStream) where file streams are converted, or if you need to keep error visibility, keep nodeStreamToWebStream but change console.error to use the project's logger and ensure nodeStream.destroy(reason) is called in cancel with the provided reason; update all call sites that used nodeStreamToWebStream to use Readable.toWeb(fileStream) if you remove the helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/routes/file-browser.ts`:
- Line 22: The import pipeline from 'node:stream/promises' in file-browser.ts is
unused; remove the named import pipeline (and the corresponding import entry) so
the file no longer imports an unused symbol and imports remain clean.
- Around line 861-884: The nodeStreamToWebStream function currently reimplements
stream conversion and logs errors via console.error; replace it with Node's
built-in conversion to simplify the code unless the explicit error logging is
required. Either remove nodeStreamToWebStream and use Readable.toWeb(fileStream)
where file streams are converted, or if you need to keep error visibility, keep
nodeStreamToWebStream but change console.error to use the project's logger and
ensure nodeStream.destroy(reason) is called in cancel with the provided reason;
update all call sites that used nodeStreamToWebStream to use
Readable.toWeb(fileStream) if you remove the helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a178830-c2b9-4637-9761-b5b83e3c1a32
📒 Files selected for processing (1)
server/routes/file-browser.ts
…-in Readable.toWeb
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
server/routes/file-browser.ts (2)
833-836: Consider handling stream errors for better observability.The Node.js stream created at line 833 can emit errors asynchronously (e.g., if the file is deleted mid-read). While
Readable.toWeb()propagates these errors and the connection will close, there's no logging for debugging failed transfers.🔧 Optional: Add error logging
// Stream file with optional range support const fileStream = fsSync.createReadStream(resolved, { start, end }); + fileStream.on('error', (err) => { + console.error(`[file-browser] Stream error for ${filePath}:`, err.message); + }); // Convert Node.js stream to Web Stream using Node's built-in conversion const webStream = Readable.toWeb(fileStream);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/file-browser.ts` around lines 833 - 836, The Node.js file stream created as fileStream and converted with Readable.toWeb(...) can emit asynchronous 'error' events; add an 'error' listener on fileStream that logs the error and context (e.g., the resolved path and byte range start/end) using the module's logger, and ensure you remove or ignore duplicate logging after the stream finishes/close to avoid leaks; this will surface failures during reads (like file deletion mid-read) for easier debugging while leaving Readable.toWeb(fileStream) behavior intact.
801-830: Range parsing handles the common case but omits suffix ranges.The regex
^bytes=(\d+)-(\d*)$doesn't match suffix-length ranges likebytes=-500(meaning "last 500 bytes") which is valid per RFC 7233. Most PDF viewers use explicit start positions, so this is unlikely to cause issues in practice.The validation and 416 response handling are correctly implemented.
🔧 Optional: Support suffix ranges
const rangeHeaderValue = c.req.header('range'); if (rangeHeaderValue && ext === '.pdf') { - const rangeMatch = rangeHeaderValue.match(/^bytes=(\d+)-(\d*)$/); + const rangeMatch = rangeHeaderValue.match(/^bytes=(\d*)-(\d*)$/); if (rangeMatch) { - const rangeStart = parseInt(rangeMatch[1], 10); - const rangeEnd = rangeMatch[2] ? parseInt(rangeMatch[2], 10) : stat.size - 1; + const hasSuffix = rangeMatch[1] === ''; + let rangeStart: number; + let rangeEnd: number; + + if (hasSuffix) { + // Suffix range: bytes=-500 means last 500 bytes + const suffixLen = parseInt(rangeMatch[2], 10); + rangeStart = Math.max(0, stat.size - suffixLen); + rangeEnd = stat.size - 1; + } else { + rangeStart = parseInt(rangeMatch[1], 10); + rangeEnd = rangeMatch[2] ? parseInt(rangeMatch[2], 10) : stat.size - 1; + } // Validate range🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/file-browser.ts` around lines 801 - 830, The Range parsing currently only matches `bytes=start-end` and misses suffix ranges like `bytes=-500`; update the regex and handling around rangeHeaderValue/rangeMatch so suffix-length ranges are supported: change the pattern to accept empty start (e.g. /^bytes=(\d*)-(\d*)$/), then if the captured start is empty but the end (suffix length) exists compute start = Math.max(0, stat.size - parseInt(suffixLength,10)), end = stat.size - 1, set statusCode = 206 and rangeHeader accordingly; keep the existing logic for explicit start/end parsing and the same validation that returns 416 with Content-Range `bytes */${stat.size}` when invalid. Ensure this logic runs only when ext === '.pdf' and continues to set start, end, statusCode, and rangeHeader variables used later.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/routes/file-browser.ts`:
- Around line 832-855: The /api/files/raw endpoint is being compressed which
breaks Range semantics; update the compression middleware (the compress() usage
in server/app.ts) to exclude this route by adding '/api/files/raw' to the
exclusion condition so responses from the file streaming handler
(file-browser.ts where Readable.toWeb, Content-Range and Content-Length are set)
are not compressed; ensure the compress() config or its conditional wrapper
checks the request.url/path and returns next() without applying compression for
'/api/files/raw' (and any equivalent route variants) so partial content (206),
Content-Range and Content-Length remain correct.
---
Nitpick comments:
In `@server/routes/file-browser.ts`:
- Around line 833-836: The Node.js file stream created as fileStream and
converted with Readable.toWeb(...) can emit asynchronous 'error' events; add an
'error' listener on fileStream that logs the error and context (e.g., the
resolved path and byte range start/end) using the module's logger, and ensure
you remove or ignore duplicate logging after the stream finishes/close to avoid
leaks; this will surface failures during reads (like file deletion mid-read) for
easier debugging while leaving Readable.toWeb(fileStream) behavior intact.
- Around line 801-830: The Range parsing currently only matches
`bytes=start-end` and misses suffix ranges like `bytes=-500`; update the regex
and handling around rangeHeaderValue/rangeMatch so suffix-length ranges are
supported: change the pattern to accept empty start (e.g.
/^bytes=(\d*)-(\d*)$/), then if the captured start is empty but the end (suffix
length) exists compute start = Math.max(0, stat.size -
parseInt(suffixLength,10)), end = stat.size - 1, set statusCode = 206 and
rangeHeader accordingly; keep the existing logic for explicit start/end parsing
and the same validation that returns 416 with Content-Range `bytes
*/${stat.size}` when invalid. Ensure this logic runs only when ext === '.pdf'
and continues to set start, end, statusCode, and rangeHeader variables used
later.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d1f0e0fa-316f-42da-ad80-dcc969d98856
📒 Files selected for processing (1)
server/routes/file-browser.ts
…xplicit byte ranges, improve error handling
…hance stream error handling
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
This MR extends the file-browser feature to support PDF rendering and viewing. PDFs can now be opened directly in the UI via the browser's native PDF viewer (iframe), with appropriate file size validation and security headers updated to allow same-origin framing.
Files changed
server/middleware/security-headers.ts— ChangedX-Frame-Optionsto'SAMEORIGIN', updatedframe-ancestorsto'self', and added'self'toframe-srcfor PDF viewer iframe support.server/routes/file-browser.ts— Added.pdfMIME type, increased file size limit to 50 MB for PDFs (vs. 10 MB for images), and replacedfs.readFile()withfs.createReadStream()to stream large files instead of buffering them in memory. Implemented HTTP Range request support for PDFs (status 206) with properContent-RangeandAccept-Rangesheaders. Uses Node's built-inReadable.toWeb()for efficient stream conversion to Web Streams.server/middleware/security-headers.test.ts— Updated tests to expectframe-ancestors 'self'andX-Frame-Options: SAMEORIGIN.src/features/file-browser/types.ts— Added optionalviewerVersionproperty toOpenFilefor triggering raw asset viewer remounts.src/features/file-browser/PdfViewer.tsx— New component that renders PDFs using browser's built-in viewer; usesviewerVersionkey to trigger remount on external file changes.src/features/file-browser/ImageViewer.tsx— Updated to useviewerVersionkey for triggering image reloads on external changes.src/features/file-browser/TabbedContentArea.tsx— IntegratedPdfViewerinto the tabbed content switcher.src/features/file-browser/FileTreeNode.tsx— Fixed precedence bug incanOpenlogic to ensure PDFs are only openable if not a directory; updated file open logic to allow PDFs.src/features/file-browser/hooks/useOpenFiles.ts— Updated hook to handle PDF file loading state, fixed restore logic to skip/api/files/readfor image and PDF files, and updatedhandleFileChangedto bumpviewerVersionfor raw assets instead of callingreloadFile.src/features/file-browser/utils/fileTypes.ts— AddedisPdfFile()utility andPDF_EXTENSIONSconstant.Why
Users can now view PDF files directly in the file-browser UI without leaving the application, improving workflows for document review and reference. This is a non-breaking enhancement that extends existing image-viewing functionality to a new file type.
How
Implementation approach:
File type detection: Added
isPdfFile()utility tofileTypes.ts, similar to existingisImageFile()pattern.Backend changes:
application/pdfMIME type mapping.fs.readFile()withfs.createReadStream()in the/api/files/rawendpoint to stream large files (PDFs, large images) instead of buffering them in memory, avoiding memory spikes for files up to 50 MB.Range: bytes=start-endheader, validates against file size, streams the requested byte range with properContent-RangeandAccept-Ranges: bytesheaders. Returns 416 (Range Not Satisfiable) for invalid ranges.Frontend components:
PdfViewer.tsxrenders PDFs via<iframe>pointing to/api/files/raw, mirroring theImageViewerpattern.TabbedContentArea.tsxconditionally rendersPdfViewerfor PDF files.FileTreeNode.tsxallows PDFs to be opened (fixed precedence bug incanOpenlogic to ensure directories cannot be opened as PDFs).useOpenFiles.tstreats PDFs like images for loading state management and skips text file reads during restore.PdfViewerandImageViewernow use aviewerVersionkey on their elements (iframe/img) to trigger remount when external file changes are detected, ensuring the viewer fetches updated content without attempting text-file reloads.File change detection: Updated
handleFileChanged()to detect raw assets (images, PDFs) and bump theirviewerVersioninstead of callingreloadFile(). This forces the viewer component to remount and refetch the binary resource from/api/files/raw.Security: Changed
X-Frame-Optionsfrom'DENY'to'SAMEORIGIN', updated CSPframe-ancestorsdirective to'self', and added'self'toframe-src(all required for the native PDF viewer iframe). Frame sources remain restricted to same-origin and trusted trading view origins.Trade-offs:
pdfjs-dist, avoiding additional dependencies and keeping the implementation lightweight.server/routes/file-browser.tsif needed.fs.createReadStream()to handle large files efficiently without buffering entire file contents in memory. Uses Node's built-inReadable.toWeb()for clean, standard stream conversion./api/files/rawendpoint, which validates and returns 501 (Not Supported) for remote workspaces. Both images and PDFs are handled consistently at the client level, with endpoint-level validation ensuring proper isolation.Type of Change
Checklist
npm run lintpassesnpm run build && npm run build:serversucceedsnpm test -- --runpassesScreenshots
Notes for reviewers:
frame-ancestorsandX-Frame-Options) are necessary for the iframe-based PDF viewer to work; they remain restrictive by allowing only same-origin requests.PdfViewercomponent follows the same pattern asImageViewerfor consistency.Summary by CodeRabbit
New Features
Improvements
Security