Skip to content

refactor(screenshot): return the fitted full-page options directly - #990

Merged
Kikobeats merged 1 commit into
masterfrom
Kikobeats/simplify-full-page-opts
Oct 13, 2026
Merged

Kikobeats merged 1 commit into
masterfrom
Kikobeats/simplify-full-page-opts

Conversation

@Kikobeats

@Kikobeats Kikobeats commented Oct 13, 2026 •

Copy link
Copy Markdown
Member

Summary

  • Follow-up to fix(screenshot): scale full-page captures past the png budget #986. When a full page exceeds the 40 megapixel PNG budget, fullPageScreenshotOpts copied the options into fitted, added the clip, then ran delete fitted.fullPage.
  • It now returns the object literal with fullPage: false, the same way packages/screenshot/src/index.js already turns fullPage off elsewhere. The temporary and the delete are gone.
  • No behavior change: Puppeteer throws only when options.clip && options.fullPage, so false passes like a missing key, and the returned options go straight into page.screenshot.

Outcome

Same captures, 3 fewer lines in the helper, no delete on the options object.

Test plan

  • standard src/index.js
  • ava test/full-page-budget.js test/white-retry.js in packages/screenshot: 9 passed, no test edits
  • Full @browserless/screenshot suite: left to CI

🤖 Generated with Claude Code


Note

Low Risk
Pure refactor of screenshot option shaping with no logic change; existing full-page budget tests pass unchanged.

Overview
Refactors fullPageScreenshotOpts when oversized full-page PNGs need a scaled clip: instead of building fitted, spreading options, then delete fitted.fullPage, it returns one object with fullPage: false set explicitly (matching how full-page is turned off elsewhere in the same file).

No intended behavior change—options still include captureBeyondViewport, the clip scale, and fullPage off so Puppeteer accepts clip without conflicting with fullPage.

Reviewed by Cursor Bugbot for commit edb4c5e. Bugbot is set up for automated code reviews on this repo. Configure here.

Summary by CodeRabbit

  • Bug Fixes
    • Large full-page screenshots now use a document-sized clip, helping preserve the full capture when the image exceeds the pixel limit.

The over-budget branch copied the options, set the clip, then deleted
`fullPage` from the copy. Return the object literal with `fullPage: false`
instead, the same way the rest of the file turns it off. Puppeteer only
rejects a clip when `fullPage` is truthy, so the capture is unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 0c29b9b3-0cf3-49fb-b5d5-8339db56fe51
📥 Commits

Reviewing files that changed from the base of the PR and between 7f76450 and edb4c5e.

📒 Files selected for processing (1)
  • packages/screenshot/src/index.js

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

📝 Walkthrough

For oversized full-page screenshots, the options now set fullPage to false and retain the document-sized clip and captureBeyondViewport: true.

Changes: Screenshot capture options

Layer / File(s) Summary
Oversized capture options
packages/screenshot/src/index.js
When a full-page screenshot exceeds FULL_PAGE_PNG_PIXELS, the function returns options with fullPage: false. The document-sized clip and captureBeyondViewport: true remain. The function no longer deletes fullPage before returning.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~5 minutes

Change: Refactor

Merge Risk: ⚪ Minimal · up to edb4c

The oversized capture retains its fitted clip, and the inspected Puppeteer implementation handles fullPage: false the same as omission. No actionable merge risk is established.

Pre-merge checks · Passed 5
Passed Passed checks (5 passed)
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly describes the main change: returning fitted full-page screenshot options directly after the refactor.
Docstring Coverage Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coveralls

Copy link
Copy Markdown

Coverage Status

coverage: 77.184%. first build — Kikobeats/simplify-full-page-opts into master

@Kikobeats
Kikobeats merged commit 93e7734 into master Oct 13, 2026
29 checks passed
@Kikobeats
Kikobeats deleted the Kikobeats/simplify-full-page-opts branch October 13, 2026 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants