feat: print the exported PDF instead of HTML layout#318
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
User does not have a PR Review subscription. Go to Team management and add this email to the PR Review subscription. |
Reviewer's GuideRefactors ExportService to centralize PDF generation in a new _generatePdf helper and updates export/print flows so both use the same jsPDF output, with printing now handled by loading an auto-printing PDF blob into an iframe instead of rendering an HTML layout. Sequence diagram for updated PDF-based print flowsequenceDiagram
actor User
participant UI as PrintButton
participant ExportService
participant jsPDFDoc as jsPDF_Document
participant Browser as Iframe
User->>UI: click
UI->>ExportService: handlePrint()
ExportService->>ExportService: _generatePdf()
ExportService->>jsPDFDoc: build pages
jsPDFDoc-->>ExportService: doc
ExportService->>jsPDFDoc: autoPrint()
ExportService->>jsPDFDoc: output(bloburl)
jsPDFDoc-->>ExportService: blobUrl
ExportService->>Browser: openPrintWindow(blobUrl)
Browser-->>Browser: create iframe with src=blobUrl
Browser-->>User: PDF opens and triggers print dialog
Browser-->>Browser: optionally call iframe.contentWindow.print()
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![pullpal-ai[bot]](https://avatars.githubusercontent.com/in/1237299?s=60&v=4){kind=small}
There was a problem hiding this comment.
The refactor to generate and print the PDF directly is a good improvement for output fidelity. However, the method _generatePdf is now publically used by both export and print, but its name and leading underscore suggest it is private. Consider renaming it to generatePdf (without underscore) if it is meant for internal reuse, or at least clarify its intended usage in a comment.
| } | ||
|
|
||
| async openPrintWindow(blobUrl) { | ||
| const printFrame = document.createElement('iframe'); |
There was a problem hiding this comment.
In openPrintWindow, you set the iframe's src to the blob URL before appending it to the DOM. For some browsers, setting src before appending can cause loading issues. Consider appending the iframe first, then setting src to ensure reliable loading across browsers.
| printFrame.onload = resolve; | ||
| setTimeout(resolve, 300); | ||
| // Also resolve after a timeout just in case | ||
| setTimeout(resolve, 1000); |
There was a problem hiding this comment.
The timeout for resolving the iframe's load event is increased to 1000ms, and the iframe is removed after 5000ms. These are arbitrary values and may not always be sufficient, especially for large PDFs or slow devices. Consider making these timeouts configurable or at least documenting why these values were chosen.
| setTimeout(() => { | ||
| printFrame.remove(); | ||
| }, 1000); | ||
| URL.revokeObjectURL(blobUrl); |
There was a problem hiding this comment.
In openPrintWindow, you revoke the blob URL after removing the iframe, which is good. However, if the print dialog is still open or the PDF is not fully loaded, this could cause issues. Consider revoking the blob URL only after confirming the print dialog has closed, if possible, or after a longer timeout.
There was a problem hiding this comment.
The new implementation removes a large amount of HTML/CSS-based print logic. If there are any accessibility or ARIA considerations in the old approach (such as alt text for images), make sure these are not lost in the PDF output. PDFs can be less accessible than HTML for screen readers.
| // which injects JS into the PDF to trigger print automatically when opened. | ||
| // However, for iframes sometimes it's necessary, but we'll try without, | ||
| // or just let the PDF handle it if supported. Some browsers require manual trigger. | ||
| try { |
There was a problem hiding this comment.
The try/catch block for calling printFrame.contentWindow?.print() is good for robustness, but the comment could be clearer about which browsers require this and under what circumstances. Consider linking to browser compatibility documentation or providing more context.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
openPrintWindow, you can drop theeparameter in thecatchblock instead of using// eslint-disable-line no-unused-vars; this keeps linting clean without needing a suppression. - The 5-second timeout before removing the iframe and revoking the
blobUrlmay be brittle for slower devices or large PDFs; consider tying cleanup to theafterprintevent or making the delay less hardcoded to avoid cutting off the PDF while the print dialog is still in use.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `openPrintWindow`, you can drop the `e` parameter in the `catch` block instead of using `// eslint-disable-line no-unused-vars`; this keeps linting clean without needing a suppression.
- The 5-second timeout before removing the iframe and revoking the `blobUrl` may be brittle for slower devices or large PDFs; consider tying cleanup to the `afterprint` event or making the delay less hardcoded to avoid cutting off the PDF while the print dialog is still in use.
## Individual Comments
### Comment 1
<location path="src/services/ExportService.js" line_range="191-200" />
<code_context>
- printFrame.contentWindow?.focus();
- printFrame.contentWindow?.print();
+ // We don't need to manually call .print() because doc.autoPrint() was used,
+ // which injects JS into the PDF to trigger print automatically when opened.
+ // However, for iframes sometimes it's necessary, but we'll try without,
+ // or just let the PDF handle it if supported. Some browsers require manual trigger.
+ try {
+ printFrame.contentWindow?.focus();
+ printFrame.contentWindow?.print();
+ } catch (e) { // eslint-disable-line no-unused-vars
+ // Ignored: cross-origin frame access might throw depending on blob handling
</code_context>
<issue_to_address>
**issue (bug_risk):** Clarify the interaction between `autoPrint()` and the explicit `print()` call to avoid double dialogs.
The current comment says we "don't need" to call `.print()` because `autoPrint()` handles it, but we still always call `printFrame.contentWindow?.print()`. This could cause two print dialogs in some browsers (one from the injected script and one from the manual call). Consider either removing the manual call, or gating it to cases where `autoPrint()` isn't supported. Also update the comment so it accurately describes the behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| // We don't need to manually call .print() because doc.autoPrint() was used, | ||
| // which injects JS into the PDF to trigger print automatically when opened. | ||
| // However, for iframes sometimes it's necessary, but we'll try without, | ||
| // or just let the PDF handle it if supported. Some browsers require manual trigger. | ||
| try { | ||
| printFrame.contentWindow?.focus(); | ||
| printFrame.contentWindow?.print(); | ||
| } catch (e) { // eslint-disable-line no-unused-vars | ||
| // Ignored: cross-origin frame access might throw depending on blob handling | ||
| } |
There was a problem hiding this comment.
issue (bug_risk): Clarify the interaction between autoPrint() and the explicit print() call to avoid double dialogs.
The current comment says we "don't need" to call .print() because autoPrint() handles it, but we still always call printFrame.contentWindow?.print(). This could cause two print dialogs in some browsers (one from the injected script and one from the manual call). Consider either removing the manual call, or gating it to cases where autoPrint() isn't supported. Also update the comment so it accurately describes the behavior.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c2b10de5c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| printFrame.onload = resolve; | ||
| setTimeout(resolve, 300); | ||
| // Also resolve after a timeout just in case | ||
| setTimeout(resolve, 1000); |
There was a problem hiding this comment.
The print flow can fire too early because the load wait unconditionally resolves after 1 second even if the PDF viewer has not finished loading the blob URL. On slower devices or multi-page exports, contentWindow.print() may run against a not-yet-ready frame, which can produce blank/partial prints or no dialog at all. This should wait for a real ready signal (or retry) instead of using a fixed timeout as a success path.
Useful? React with 👍 / 👎.
| printFrame.contentWindow?.print(); | ||
| } catch (e) { // eslint-disable-line no-unused-vars | ||
| // Ignored: cross-origin frame access might throw depending on blob handling | ||
| } |
There was a problem hiding this comment.
If printFrame.contentWindow.print() throws (which can happen when browsers expose PDF blobs through a cross-origin/internal viewer), the catch block suppresses the failure and the method still resolves. In that scenario the caller cannot show the existing "Print Failed" toast and users get a no-op Print action. Re-throwing or reporting this path is needed so failed prints are visible and recoverable.
Useful? React with 👍 / 👎.
What
Refactored the export and print functionality in
ExportService.jsto ensure that clicking "Print" natively prints the generated PDF rather than relying on an HTML equivalent.Why
The previous implementation for printing rebuilt an HTML DOM representation of the pages for the browser to print, which could be slightly inconsistent with the exact generated jsPDF output. By using the new
_generatePdfhelper, the exported PDF output is fed directly into an iframe withdoc.autoPrint()enabled.Verification
Tested exporting and printing flows successfully. No regressions found in standard layout handling.
PR created automatically by Jules for task 13362181141554358321 started by @guitarbeat
Summary by Sourcery
Refactor export and print flows to generate a single jsPDF document and use it for both downloading and printing.
New Features:
Enhancements: