Conversation
WalkthroughPDF export now stages temporary HTML and PDF files, signals document readiness, validates Chromium navigation and output, captures diagnostics, and guarantees cleanup. Tests cover URI handling, readiness HTML, successful export, classified failures, and filesystem interactions. ChangesPDF export pipeline
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PreviewController
participant FileSystem
participant Chromium
participant OutputChannel
PreviewController->>FileSystem: write temporary print HTML
PreviewController->>Chromium: navigate to local PDF HTML
Chromium-->>PreviewController: return DOM dump and staged PDF
PreviewController->>PreviewController: validate readiness and PDF size
PreviewController->>OutputChannel: write PDF diagnostics
PreviewController->>FileSystem: copy final PDF and remove temporary files
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/extension/preview/PreviewPanel.ts`:
- Around line 1959-1982: Update the PDF readiness flow around the Chromium
invocation and getPdfNavigationError so readiness is established reliably before
the dumped DOM is inspected. Either make the page’s data-omv-pdf-ready signal
synchronous or explicitly wait for the image.decode() and document.fonts.ready
promises to settle before evaluating the dump, while preserving navigation-error
reporting for genuinely incomplete exports.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ba506699-97ea-4500-89a7-e8e5a091c5a2
⛔ Files ignored due to path filters (1)
.e2e-workspace/sample.pdfis excluded by!**/*.pdf
📒 Files selected for processing (3)
src/extension/preview/PreviewPanel.tstest/unit/helpers/vscodeMock.tstest/unit/previewPanel.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/extension/preview/PreviewPanel.ts`:
- Around line 1949-1963: Split the Chromium export around the existing
runProcess call into separate invocations for DOM dumping and PDF printing,
preserving the required shared browser flags and arguments for each action.
Ensure the PDF invocation uses --no-pdf-header-footer instead of
--print-to-pdf-no-header and still targets targetPdfUri.fsPath.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 523c4e7f-ed68-4f41-8e2a-c8d620050214
⛔ Files ignored due to path filters (1)
.e2e-workspace/sample.pdfis excluded by!**/*.pdf
📒 Files selected for processing (3)
src/extension/preview/PreviewPanel.tstest/unit/helpers/vscodeMock.tstest/unit/previewPanel.test.ts
… in PreviewController
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/extension/preview/PreviewPanel.ts (1)
2342-2388: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a hard timeout for the spawned Chromium process.
runProcessresolves only onclose/error, with no upper bound.--virtual-time-budgetcaps render work, but a Chromium binary that stalls at startup (profile lock, GPU probe, crashpad handler, etc.) would leaveexportPdfawaiting indefinitely with no cancellation path. A watchdog that kills the child and resolves with a distinct failure code keeps the export command responsive.♻️ Sketch: add a watchdog timeout
return new Promise((resolve) => { let stdout = ''; let stderr = ''; let settled = false; const child = spawn(command, args, { stdio: ['ignore', 'pipe', 'pipe'] }); + const timer = setTimeout(() => { + if (settled) return; + settled = true; + child.kill('SIGKILL'); + resolve({ ok: false, code: 'ETIMEDOUT', stdout, stderr }); + }, PDF_EXPORT_PROCESS_TIMEOUT_MS); // e.g. virtual-time-budget + generous startup marginRemember to
clearTimeout(timer)in theerrorandclosehandlers before resolving.Please confirm the intended failure semantics (retry next candidate vs. abort) so the timeout
codeslots cleanly into the existingENOENT/status handling intryHeadlessPdfExport.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/extension/preview/PreviewPanel.ts` around lines 2342 - 2388, Update runProcess to add a hard watchdog timeout for the spawned child process, clearing the timer in both the error and close handlers before resolving. On timeout, kill the child, settle only once, and resolve with a distinct timeout failure code while preserving captured stdout/stderr; ensure tryHeadlessPdfExport treats that code consistently with existing process failures, retrying the next candidate rather than hanging or aborting unexpectedly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/extension/preview/PreviewPanel.ts`:
- Around line 2342-2388: Update runProcess to add a hard watchdog timeout for
the spawned child process, clearing the timer in both the error and close
handlers before resolving. On timeout, kill the child, settle only once, and
resolve with a distinct timeout failure code while preserving captured
stdout/stderr; ensure tryHeadlessPdfExport treats that code consistently with
existing process failures, retrying the next candidate rather than hanging or
aborting unexpectedly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 414d212c-5694-4bb4-8e62-3342f8361643
⛔ Files ignored due to path filters (1)
.e2e-workspace/sample.pdfis excluded by!**/*.pdf
📒 Files selected for processing (3)
src/extension/preview/PreviewPanel.tstest/unit/helpers/vscodeMock.tstest/unit/previewPanel.test.ts
1 participant
Summary
finallyblockRoot cause
PDF export called
sourceHtmlUri.toString(true). In VS Code's URI API,trueskips encoding, so spaces,#,%, and non-ASCII characters in temporary paths could be interpreted as URL syntax by Chromium. Chromium still exited successfully and wrote a PDF containing itsERR_FILE_NOT_FOUNDpage, and the extension treated that as a successful export.Preview rendering was unaffected because it uses VS Code webview URIs. HTML export was unaffected because it writes the standalone HTML directly and never asks Chromium to navigate to the temporary file.
User impact
PDF exports now contain the rendered preview for normal, nested, spaced, encoded, and Unicode paths. Navigation or rendering failures produce a useful extension error instead of overwriting the target with a Chromium error-page PDF. Existing preview and HTML export behavior is preserved.
Validation
npm test— 124 tests passednpm run lint— passednpm run build— passednpm run test:e2e— completed successfully using cached VS Code 1.110.1git diff --check— passed#,%, Unicode, a local SVG, styles, and fonts — passedpdfinfo,pdftotext, and rasterized visual outputNotes
Direct PDF export still requires a locally installed Chrome, Edge, or Chromium executable.
Summary by CodeRabbit