fix(printer.service.ts): fix pdf rendering, issue #2374 solved #2375
Conversation
📝 WalkthroughWalkthroughThe printer service changes page initialization from pre-document evaluateOnNewDocument injection to a post-navigation flow: navigate to /artboard/preview, inject resume data via page.evaluate, then reload; it also waits for the first page selector and conditionally waits for profile image load. Exported/public interfaces unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant PrinterService
participant BrowserPage as Browser Page
Client->>PrinterService: requestPrint(resumeData)
PrinterService->>BrowserPage: goto('/artboard/preview') (domcontentloaded)
Note over BrowserPage: initial DOM parsed
PrinterService->>BrowserPage: evaluate(() => localStorage.setItem(...resumeData))
Note over BrowserPage: resume data written to localStorage
PrinterService->>BrowserPage: reload() (load)
par wait for page and selector
BrowserPage-->>PrinterService: load event
BrowserPage-->>PrinterService: '[data-page="1"]' selector present
end
alt profile picture present
PrinterService->>BrowserPage: wait for profile image selector
PrinterService->>BrowserPage: evaluate(() => await all document.images to load/error)
end
PrinterService->>BrowserPage: proceed with CSS, buffering, PDF generation
BrowserPage-->>PrinterService: PDF blob
PrinterService->>Client: return result / store PDF
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/printer/printer.service.ts (1)
136-146: Wait for the app to render after reload; load is too early and can yield zero-sized pages.After reload, querying [data-page] immediately may race the React render, producing width/height = 0 and flaky PDFs. Wait for DOM readiness and for page elements to exist.
- await page.reload({ waitUntil: "load" }); + await Promise.all([ + page.reload({ waitUntil: "domcontentloaded" }), + // Wait until first page is present before proceeding + page.waitForSelector('[data-page="1"]', { timeout: 15000 }), + ]);
🧹 Nitpick comments (1)
apps/server/src/printer/printer.service.ts (1)
127-129: Comment is inaccurate: using localStorage, not session storage.Update for clarity.
- // Set the data of the resume to be printed in the browser's session storage + // Set the data of the resume to be printed in the browser's localStorage
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/server/src/printer/printer.service.ts(1 hunks)
🔇 Additional comments (1)
apps/server/src/printer/printer.service.ts (1)
132-135: LGTM: set localStorage before applying via reload.This fixes the timing issue by caching data pre‑reload.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/server/src/printer/printer.service.ts (1)
130-135: Good fix: earlier-load via evaluate + reload; domcontentloaded avoids SPA hangs.This addresses the late localStorage injection and the prior networkidle0 concern. 👍
🧹 Nitpick comments (2)
apps/server/src/printer/printer.service.ts (2)
136-141: Avoid race between reload and waitForSelector; add explicit readiness (fonts, pages).Waiting for the selector while navigating can sporadically fail on nav context swaps. Sequential waits + a fonts/page-count check are more robust.
- await Promise.all([ - page.reload({ waitUntil: "load" }), - // Wait until first page is present before proceeding - page.waitForSelector('[data-page="1"]', { timeout: 15_000 }), - ]); + await page.reload({ waitUntil: "domcontentloaded" }); + // App ready: ensure first page + expected page count are present + await page.waitForSelector('[data-page="1"]', { timeout: 15_000 }); + await page.waitForFunction( + (expected) => document.querySelectorAll('[data-page]').length === expected, + {}, + numberPages, + ); + // Ensure web fonts are loaded before PDF rendering + await page.evaluate(async () => { + if (document.fonts?.ready) await document.fonts.ready; + });
127-129: Comment mismatch: using localStorage, not session storage.Update the comment for clarity.
- // Set the data of the resume to be printed in the browser's session storage + // Cache resume data in window.localStorage for the reload to pick up
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/server/src/printer/printer.service.ts(1 hunks)
2 participants
fix(printer-service): ensure PDF renders correctly by caching resume data before reload
Previously used evaluateOnNewDocument, which caused localStorage to be set too late.
Now the resume data is cached via evaluate() before reload, ensuring the page
loads with correct data and improving reliability and efficiency.
Summary by CodeRabbit
Bug Fixes
Performance & Stability