feat(webdriver-utils): add scaleToFit for full-page Automate captures (PER-10530) - #2388
feat(webdriver-utils): add scaleToFit for full-page Automate captures (PER-10530)#2388rishigupta1599 wants to merge 9 commits into
Conversation
… (PER-10530) Full-page POA captures are capped at a 50,000px stitched-image budget, which mobile-common enforces by truncating the DOM walk at MAX_PIXEL_HEIGHT_LIMIT/dpr -- 16,666 CSS px at DPR 3. Pages taller than that lose their tail silently. scaleToFit opts a capture into downscaling every tile by a fixed 1/dpr instead, so a page of up to 50,000 CSS px fits the same budget. The factor is fixed at construction and deliberately NOT derived from the measured page height: Percy diffs pixel to pixel and a uniform resize moves both dimensions, so a height-derived factor would make output WIDTH a function of page height. A page drifting across a threshold would then change width, leaving snapshot and baseline with different dimensions -- unalignable, not merely different. Two pieces here: - Schema entry on /config/snapshot. The section sets additionalProperties: false, so without a declaration the key is rejected before it ever reaches the Automate session. Gated onlyAutomate, like fullPage. - PERCY_SCALE_TO_FIT in addDefaultOptions(), so a whole run can opt in without editing per-snapshot config -- how support enables this for a customer hitting the truncation. Coerced to a real boolean, because mobile-common compares with `== true` and a truthy string would silently no-op. No provider changes: automateProvider already forwards `options` wholesale, and SeleniumHub passes it through verbatim, so the camelCase spelling is the contract with mobile-common. Requires browserstack/mobile-common#1255. Verified: webdriver-utils 240/240, core unit/config 21/21, eslint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…metadata The previous commit plumbs the option INTO the Automate session. This carries the result back OUT: mobile-common reports `scale_to_fit` and `applied_scale_factor` in its response, but getTiles() built metadata from `screenshotType` alone, so neither ever reached percy-api. percy-api needs the factor to relax calculate_max_tiles_limit. A scaleToFit capture walks a taller page and therefore returns roughly 1/factor times the usual tile count -- so without this the extended walk is pointless: the page is rejected at the tile-count gate instead of being truncated upstream. Sending only a boolean would leave the API guessing a worst-case DPR, so send the magnitude that was actually applied. Both keys are added only when mobile-common reports it really scaled. They land in comparison_details.metadata -- a 1:1 child of comparisons and the largest table on the platform -- so the default path must not grow every row with constants. Verified: webdriver-utils 242/242, eslint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment-only change, no behaviour difference. Keeps the load-bearing "why" on each -- why the schema entry is required, why the boolean is coerced, why the metadata keys are conditional -- and drops the surrounding exposition. Verified unchanged: webdriver-utils 242/242, core unit/config 21/21, eslint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
One block was still 3 lines after the previous pass. 242/242, eslint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI failed with "Cannot read property 'map' of undefined": PercyConfig.validate returns undefined when there are no errors, and onlyAutomate only errors when PERCY_TOKEN is set to a non-automate token. My local run had one set, CI does not -- so the spec asserted an absolute outcome that only held locally. Assert scaleToFit is treated the SAME as fullPage instead. Both are onlyAutomate, so the comparison holds whether the token is absent, web, or automate, and an undeclared key would still draw 'unknown property' where fullPage drew none. Verified passing with PERCY_TOKEN unset, web_*, and auto_*; and verified BOTH schema specs fail in all three states when the scaleToFit entry is removed, so neither can pass vacuously. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…te options percyAutomateRequestHandler builds the provider options by ENUMERATING each global snapshot config key, then merging per-screenshot options on top. Adding scaleToFit to the schema made `.percy.yml` accept it, but because it was not in that list it was silently dropped on the way to the provider -- the page truncated exactly as before while validation and the build stayed green. So of the two levels only per-screenshot actually worked. Both work now. The failure mode is the reason for the test: it asserts global-only, a per-screenshot override of a global true, and per-screenshot opt-in with global unset. Verified it fails when the merge line is removed, so it cannot rot. Verified: core utils 140/140, core unit/config 21/21, eslint clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review findings on the scaleToFit option. BLOCKER: comparisonSchema.metadata sets additionalProperties:false and PercyConfig.validate DELETES unknown keys from the object it validates, so both new metadata keys were stripped before upload. percy-api therefore never received appliedScaleFactor and never relaxed its tile-count limit -- the same fail-open class as the global-config path, one layer further down, and equally invisible in a green build. Declares both keys (with a 0 < x <= 1 bound on the factor) and adds a round-trip test asserting the values survive validate, which a structural assertion would not catch. Also: - gate scaleToFit on fullPage. The host shrinks tiles only in its full-page loop, so outside it the flag reported a factor for unscaled tiles. - let an explicit `scaleToFit: false` beat PERCY_SCALE_TO_FIT, so the env var is a run-wide default rather than an unopt-outable override. - require a finite applied_scale_factor before reporting the pair, so a half-pair cannot ask the API to relax by nothing. - mirror the metadata block in playwrightProvider, which inherits the option from GenericProvider but never read the factor back. - add scaleToFit to percy doctor's automateOnlyKeys, the third enumeration of this option, so a web-token user gets the warning. - look the config schema up by absence of $id instead of index [0]. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CI enforces 100% global coverage and the new playwrightProvider metadata block was untested, so lines 140-141 dropped branches to 99.34% and failed all four retries. Local `test:coverage` reports zeros for this package, so the threshold is only observable in CI. Covers both arms: scale_to_fit with a finite factor reports the pair, and scale_to_fit without a factor omits it. Also fixes an object-property-newline lint error in the config round-trip test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Re-review findings. PlaywrightProvider.screenshot fully overrides GenericProvider.screenshot and never called super, so addDefaultOptions was skipped on that path entirely -- the fullPage gate, the boolean coercion and PERCY_SCALE_TO_FIT were all inert for playwright captures, and raw options reached the host. Calling it also restores the freezeAnimation default and the percyCSS newline strip, both of which were missing there for the same reason. Also tighten the reported factor to (0, 1]: Number.isFinite(0) and Number.isFinite(-1) are both true, so a host reporting 0 produced exactly the half-pair the guard exists to prevent -- the schema drops appliedScaleFactor while leaving scaleToFit true, plus a user-visible warning. The shared playwright test fixture passed the literal string 'options', which addDefaultOptions cannot write to; it now passes a real object. 246 specs pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude Code PR ReviewPR: #2388 • Head: c3286cf • Reviewers: stack-code-reviewer SummaryAdds an opt-in Review Table
Focus areas — both verified cleanSchema declaration survives The Findings
Low — naming
Test qualityNo vacuous tests found. Each new test was checked against the implementation and would fail if the guard it covers were deleted — including the Note for the author (not a finding)An untracked Verdict: PASS — no High or Critical findings. Finding 1 is worth taking before merge: it is the only regression guard missing on the line this PR turns on. |
1 participant
Why
Full-page POA captures are bounded by a 50,000px stitched-image budget. mobile-common enforces it by truncating the DOM walk at
MAX_PIXEL_HEIGHT_LIMIT / dpr— 16,666 CSS px at DPR 3 — so any page taller than that silently loses its tail. That is the PER-10298 / PPLT-5851 dealblocker (Adecco, Akkodis).scaleToFitopts a capture into downscaling every tile by a fixed1/dprinstead, so a page of up to 50,000 CSS px lands exactly on the same budget:page_height × dpr × (1/dpr) == page_height.Why the factor is fixed, not height-derived
Percy diffs pixel to pixel, and a uniform resize moves both dimensions. A height-derived factor would make output width a function of page height — so a page drifting from 16,600 to 18,000 px would change width, leaving snapshot and baseline with different dimensions: unalignable, not merely different.
Constant width forces a constant factor, and the ceiling forces
f ≤ 50000/(50000 × 3) = 1/3. The three constraints (constant width, ≤ 50,000px, coverage to 50,000 CSS px) are only jointly satisfiable atf = 1/dpr. It is also opt-in, so quality never changes for anyone who does not ask for it.Changes
packages/core/src/config.js— declarescaleToFiton the snapshot schema. That section setsadditionalProperties: false, so without a declaration the key is rejected before it ever reaches the Automate session. GatedonlyAutomate, exactly likefullPage.packages/webdriver-utils/src/providers/genericProvider.js—PERCY_SCALE_TO_FITinaddDefaultOptions(), so a whole run can opt in without editing per-snapshot config (how support would enable this for an affected customer). Coerced to a real boolean, because mobile-common compares with== trueand a truthy string would silently no-op.No provider changes needed.
automateProvider.getTiles()already forwardsoptionswholesale and SeleniumHub passes it through verbatim, so the camelCase spelling is the contract with mobile-common.Notes for review
Two things I got wrong first and corrected, worth knowing if you extend these tests:
/config/snapshot, not/snapshot— the latter$refs only a hand-picked subset and reports evenfullPageasunknown property. My first specs validated against/snapshotand passed vacuously.onlyAutomateis evaluated when AJV compiles the schema, not per-validation, so flippingPERCY_TOKENinside a spec cannot change the outcome. The specs now assert the declaration structurally plus the real validation error, and explicitly assert the message is notunknown propertyso they cannot pass vacuously again.Testing
packages/webdriver-utils: 240/240packages/coretest/unit/config.test.js: 21/21eslintclean on all four filesThe full
packages/coresuite does not pass in my environment — 155 failures, allFailed to launch browser / Timed out after 30000ms, i.e. Chrome cannot start here. Unrelated to this change, but I could not use it as a signal, so I ran the config specs in isolation.Depends on
browserstack/mobile-common#1255 — the capture-side implementation. This PR only plumbs the option; without it the option is accepted and ignored.
🤖 Generated with Claude Code