Skip to content

Add PDF and CSV previews and fix Codex Windows spawn#372

Open
QuanCheng-QC wants to merge 2 commits into
developfrom
feature/workspace-file-preview-pdf-csv
Open

Add PDF and CSV previews and fix Codex Windows spawn#372
QuanCheng-QC wants to merge 2 commits into
developfrom
feature/workspace-file-preview-pdf-csv

Conversation

@QuanCheng-QC

Copy link
Copy Markdown
Collaborator
  • Add PDF detection and blob-based in-app PDF preview
  • Add CSV detection, parsing, table rendering, and 1000-row truncation
  • Make chat PDF/CSV attachments open the file preview instead of downloading
  • Fix Codex adapter spawning on Windows by avoiding cmd/stdin pipe issues

@vercel

vercel Bot commented May 7, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
openagents-workspace Ready Ready Preview, Comment May 27, 2026 3:37am

Request Review

@zomux zomux left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two items to address before merge:

1. Security: PDF iframe missing sandbox attribute
The CSV preview iframe correctly uses sandbox="allow-scripts allow-same-origin", but the PDF iframe (around line 330) has no sandbox at all. A malicious PDF could potentially trigger navigation or run embedded JS. Please add at minimum sandbox="" or sandbox="allow-same-origin".

2. Windows CI failures
The test (windows-latest, 20) and test (windows-latest, 22) checks are failing — these are directly relevant to the Codex spawn changes in this PR and should be investigated.

Minor notes (non-blocking):

  • Codex prompt passed as trailing CLI arg could hit OS argument length limits (~32KB on Windows). Consider a length check or fallback.
  • Empty catch {} blocks in _resolveToNodeCmd could use a debug log.
  • CSV parser and React text rendering are fine — no XSS risk there.

@zomux

zomux commented May 22, 2026

Copy link
Copy Markdown
Contributor

Hi @QuanCheng-QC — friendly follow-up on the review from May 17. The two blocking items are still open:

  1. PDF iframe missing sandbox attribute — the HTML preview iframe has sandbox="allow-scripts allow-same-origin" but the PDF iframe (~line 272 in file-preview.tsx) has none. Adding sandbox="" or sandbox="allow-same-origin" would close this security gap.

  2. Windows CI failurestest (windows-latest, 20) and test (windows-latest, 22) are failing, likely related to the Codex spawn changes. windows-latest, 18 passes, suggesting a Node 20/22-specific issue.

Let me know if you need help resolving either of these!

@zomux zomux added Request Data Chats Requested Changes PR has review feedback requiring author action and removed Request Data Chats labels May 24, 2026
@zomux

zomux commented May 24, 2026

Copy link
Copy Markdown
Contributor

Hi @QuanCheng-QC — following up again on the two open items from the review:

  1. PDF iframe sandbox — still missing. This is a one-line fix: add sandbox="" to the PDF iframe in file-preview.tsx (the CSV iframe already has it).

  2. Windows CI failurestest (windows-latest, 20) and test (windows-latest, 22) are still failing. These are likely related to the Codex spawn refactor in this PR and need investigation.

We can fix the sandbox issue ourselves, but the Windows test failures need your attention since they're tied to the Codex spawn changes. Let us know if you need any help!

@QuanCheng-QC

Copy link
Copy Markdown
Collaborator Author

Hi @zomux , thanks for the review. I’ve addressed the requested changes and pushed the updates. Could you please take another look when you have time?

@QuanCheng-QC QuanCheng-QC requested a review from zomux May 28, 2026 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Requested Changes PR has review feedback requiring author action

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants