fix(pdf): wait for the document to render before printing it#851
Closed
Kikobeats wants to merge 1 commit into
Closed
fix(pdf): wait for the document to render before printing it#851Kikobeats wants to merge 1 commit into
Kikobeats wants to merge 1 commit into
Conversation
An app shell that has not rendered anything yet satisfies every settle signal
the readiness gate has, so `prepare` returned and printed the placeholders.
On a report that renders nothing for ~1.5s the gate resolved at 458ms:
ready height=800 viewport=800 images=0 decoded=0 painted=0 text=0
covered=false fonts=true complete=true resets=0 timedOut=false
Each signal is individually correct and collectively useless here. The shell's
HTML arrived, so `readyState` is `complete`. It ships no images, so there is
nothing left to decode. And the app scrolls an inner pane rather than the
document, so the document height is pinned to the viewport and stays there
forever — height stability, the strongest settle signal, holds from the first
poll and never stops holding.
`painted` and `text` did report zero, but they cannot be used to catch this:
both are viewport-scoped by design, and a page whose content merely starts below
the fold reads zero for exactly the same reason while being perfectly ready.
Requiring them in the gate makes every such page burn its budget, which its own
tests assert against.
So ask a different question, document-wide rather than viewport-scoped, and ask
it before the gate: does the document carry anything at all? Poll until it does
or the budget runs out. A page that already has content answers on the first
evaluate — measured at 1ms — while the report waits 1.565s and then prints what
it actually renders.
This is one half of that report printing correctly; the other is walking it to
load what it defers until scrolled into view. With both, it goes from 120
skeleton placeholders to 0, and from 2,091 to 11,479 characters of real content.
The pdf package had no test setup, so it gains one here.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DdKhHYtYcxYyxaGnZnuJxU
📝 WalkthroughWalkthroughThe PDF package now detects meaningful DOM content during ChangesPDF content readiness
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant createPdf
participant waitUntilAuto
participant PageDOM
createPdf->>waitUntilAuto: automatic readiness check
waitUntilAuto->>PageDOM: evaluate document content
PageDOM-->>waitUntilAuto: content detected or timeout
waitUntilAuto-->>createPdf: continue PDF generation
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)packages/pdf/package.jsonTraceback (most recent call last): 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: 1
🧹 Nitpick comments (1)
packages/pdf/test/empty-shell.js (1)
57-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the timing assertions prove the polling contract.
The current bounds allow the prior premature-empty-shell behavior and a full content-budget delay for immediately populated pages to pass.
packages/pdf/test/empty-shell.js#L57-L64: assert that the empty shell waits for a meaningful portion of its configured content budget, as well as completing within a bounded upper limit.packages/pdf/test/empty-shell.js#L69-L79: use an upper bound below the full content-wait budget so a false-negative initial probe cannot pass after waiting unnecessarily.🤖 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 `@packages/pdf/test/empty-shell.js` around lines 57 - 64, Strengthen the timing assertions in the empty-shell tests: at packages/pdf/test/empty-shell.js lines 57-64, require elapsed time to exceed a meaningful portion of the configured content budget while retaining the bounded upper limit; at lines 69-79, set the upper-bound assertion below the full content-wait budget so an unnecessary false-negative initial probe cannot pass. Use the existing timing variables and test setup without changing the rendering behavior.
🤖 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 `@packages/pdf/src/index.js`:
- Around line 62-66: Update hasDocumentContent to ignore hidden or non-rendered
elements returned by the body querySelector check. Only treat a matched input,
button, svg, or other candidate element as content when it is renderable, while
preserving the existing innerText fallback for visible textual content.
---
Nitpick comments:
In `@packages/pdf/test/empty-shell.js`:
- Around line 57-64: Strengthen the timing assertions in the empty-shell tests:
at packages/pdf/test/empty-shell.js lines 57-64, require elapsed time to exceed
a meaningful portion of the configured content budget while retaining the
bounded upper limit; at lines 69-79, set the upper-bound assertion below the
full content-wait budget so an unnecessary false-negative initial probe cannot
pass. Use the existing timing variables and test setup without changing the
rendering behavior.
🪄 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 Plus
Run ID: 461fd973-4543-4427-9856-8847e085b42f
📒 Files selected for processing (3)
packages/pdf/package.jsonpackages/pdf/src/index.jspackages/pdf/test/empty-shell.js
Comment on lines
+62
to
+66
| const hasDocumentContent = () => { | ||
| const body = document.body | ||
| if (!body) return false | ||
| if (body.querySelector('img,svg,canvas,video,table,input,button,picture')) return true | ||
| return (body.innerText || '').trim().length > 0 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Ignore non-rendered elements in the content predicate.
A shell containing a hidden input, button, or decorative svg immediately returns true, so it can still bypass the wait and print before actual report content exists. Require matched elements to be renderable before treating them as document content.
🤖 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 `@packages/pdf/src/index.js` around lines 62 - 66, Update hasDocumentContent to
ignore hidden or non-rendered elements returned by the body querySelector check.
Only treat a matched input, button, svg, or other candidate element as content
when it is renderable, while preserving the existing innerText fallback for
visible textual content.
|
coverage: 80.564% (-0.06%) from 80.619% — fix/pdf-empty-shell into master |
5 tasks
Member
Author
|
Superseded by #852 (merged as v13.6.9). That PR covers the same empty-shell / SPA report case with the full readiness + overflow prepare path ( |
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
2 participants
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.
An app shell that has not rendered anything yet satisfies every settle signal the readiness gate has, so
preparereturns and prints the placeholders.On a report that renders nothing for ~1.5s, the gate resolved at 458ms:
Why every signal says "ready"
packages/screenshot/src/wait-for-ready.js:completetrueimagesDecodedtrueheight === lastHeighttrueThe third one is the trap. This app scrolls an inner
overflow:autopane rather than the document, so the document height never moves — not while loading, not after. Height stability, the strongest settle signal, holds from the first poll and never stops holding.The blank-page fallback does not catch it either: it asks only
isWhiteScreenshot, and grey skeletons on white are not white.Why
painted/textcannot be the fixThey did report zero, and my first attempt added them to the gate's quiet condition. That is wrong: both are viewport-scoped by design, so a page whose content merely starts below the fold reads zero for the same reason while being perfectly ready. The existing suite says so directly, and turned red in five places:
Requiring viewport-scoped content in the shared gate makes every such page burn its budget. Reverted.
What this does instead
Ask a different question — document-wide rather than viewport-scoped — and ask it before the gate: does the document carry anything at all, anywhere?
Poll until it does or the budget runs out. Cheap element probe first,
innerTextonly if that misses.Tests
The pdf package had
test: exit 0and no test directory, so it gains one here.packages/pdf/test/empty-shell.js:preparereturned only once real content existed. Disabling the wait turns this red.Screenshot suite unchanged: 44 passing. Lint clean.
Half of a fix
This is one of the two things that report needs. The other is #850, walking it to load what it defers until scrolled into view. Measured in isolation:
Verified together on the real report: 8 pages, 729 kB, no placeholders, in 4.3s.
🤖 Generated with Claude Code
https://claude.ai/code/session_01DdKhHYtYcxYyxaGnZnuJxU
Note
Medium Risk
Changes core PDF prepare timing for all
waitUntil: 'auto'renders; bounded budget limits hang risk but could still affect slow or minimal-DOM pages.Overview
Fixes PDFs printing empty app shells when
waitUntil: 'auto'treats the page as settled before real UI appears (e.g. complete HTML, no images, fixed document height with inner scroll).prepare/waitUntilAutonow runs a document-wide content poll (hasDocumentContent+waitForDocumentContent) beforewaitForReady, using ~25% of the load budget and 150ms polls. It looks for substantive elements or any trimmedinnerTextanywhere in the body—not viewport-scoped painted/text signals—so below-the-fold-ready pages are not broken.The
@browserless/pdfpackage switches from a no-op test script to AVA withempty-shell.js: wait for delayed shell content, bounded timeout when content never arrives, and no extra delay when content is already present.Reviewed by Cursor Bugbot for commit 9d07766. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Tests