feat: Fully Loaded Snapshot Capture — Readiness Gate (PER-7348)

[Shivanshu-07]

Summary

Adds a pre-snapshot readiness gate that waits for the page to be fully loaded and stable before capturing DOM. This reduces false visual diffs caused by skeleton loaders, partially rendered SPAs, and async content.

Approach: V2 Capture-Level Readiness — for SDK-provided snapshots, the CLI discards the SDK's domSnapshot and re-captures from the live URL with readiness checks. Zero SDK changes required.

Changes

@percy/dom (browser-side)

• New waitForReady(options) async function with 6 composable checks:
  • DOM stability (MutationObserver with layout-affecting filter)
  • Network idle (PerformanceObserver-based)
  • Font readiness (document.fonts.ready)
  • Image readiness (above-the-fold viewport check)
  • readySelectors — wait for elements to appear
  • notPresentSelectors — wait for skeleton loaders to disappear
• 3 named presets: balanced (default), strict, fast, disabled

@percy/core (CLI-side)

• readiness.js — CLI orchestrator that calls PercyDOM.waitForReady() via page.eval()
• page.js — Readiness gate inserted between insertPercyDom() and serialize(). Config priority: per-snapshot > per-page > global (Page._globalReadinessConfig)
• discovery.js — V2 re-capture: when SDK sends domSnapshot + readiness enabled, CLI deletes domSnapshot, clears cached root resource, forces URL-based capture path
• config.js — readiness schema with presets and per-snapshot overrides
• api.js — Tags SDK snapshots with _fromSDK flag
• percy.js — Preserves _fromSDK through validation, sets Page._globalReadinessConfig

Configuration

# .percy.yml
version: 2
snapshot:
  readiness:
    preset: strict
    notPresentSelectors:
      - .skeleton
      - .loading-spinner

POC Results

Build	Type	Readiness	Result
#648	Storybook	disabled	Skeletons
#649	Storybook	strict	Fully loaded (3.6s wait)
#652	SDK sim	disabled	Skeletons
#653	SDK sim	V2 re-capture	Fully loaded (1.9s wait)

Test Plan

• @percy/dom browser tests — 18 specs covering all 6 checks, presets, kill switch, timeout, mutation filtering
• CLI integration test — URL-based + SDK simulation with readiness
• Storybook POC — percy storybook with .percy.yml readiness config
• Existing CLI test suite (CI)

Known Limitations

• Post-interaction state: V2 re-captures from fresh page load — can't reproduce test interactions (button clicks, form fills)
• Auth-protected pages: CLI's browser has no test session — configure discovery.authorization or discovery.cookies
• Performance: V2 adds ~2-5s per snapshot for re-navigation
• Shadow DOM: MutationObserver doesn't observe inside shadow roots

Jira

PER-7348

[amandeepsingh333]

Code Review: Fully Loaded Snapshot Capture — Readiness Gate

Overall this is a well-structured feature with a solid approach (composable checks, named presets, V2 re-capture). The PR description and POC results are excellent. However, there are several issues that should be addressed before merge — most critically around resource cleanup and the V2 re-capture semantics.

Key Concerns

Severity	Count	Summary
🔴 High	3	Timer/observer leaks on timeout, race condition in check results
🟡 Medium	8	V2 JS semantics change, static mutable state, config resolution inconsistency, test gaps
🔵 Low	4	Duplicated presets, hardcoded font timeout, missing positive test

Top 3 Actionable Items

1. Add cleanup/abort mechanism for all readiness checks so timers and observers are cleared when the overall timeout fires
2. Preserve original enableJavaScript value when V2 re-capture deletes domSnapshot
3. Add a positive integration test for the V2 re-capture path

[amandeepsingh333]

🔴 HIGH — Memory/timer leak: This setInterval (and the ones in checkReadySelectors at line 196, checkNotPresentSelectors at line 207, and checkNetworkIdle at line 132) is never cleared when the overall waitForReady timeout fires.

When Promise.race in waitForReady resolves via the timeout branch, these promises are simply abandoned — but their intervals keep firing indefinitely, wasting CPU and potentially throwing errors if the page navigates away.

Fix: Pass an AbortSignal (or shared cancelled flag) into each check function, and clear intervals on abort:

function checkImageReady(config, signal) {
  return new Promise(resolve => {
    const interval = setInterval(() => {
      if (signal?.aborted) { clearInterval(interval); resolve({ passed: false, aborted: true }); return; }
      // existing logic...
    }, 100);
  });
}

Then in waitForReady, create an AbortController, pass controller.signal to all checks, and call controller.abort() in the timeout branch.

[Shivanshu-07]

Fixed in 80d28e1. Added an abort mechanism: waitForReady creates an abort handle, passes it to all checks, and calls abort() when the timeout fires. Each check registers cleanup callbacks via aborted.onAbort() that clear intervals, disconnect observers, and cancel timers. No more leaked intervals/observers on timeout.

[amandeepsingh333]

🔴 HIGH — Timeout race: partial result.checks can silently pass: When Promise.race resolves via timeout, result.checks only contains checks that completed before the timeout fired. Checks still running are simply missing from the object.

The problem: Object.values(result.checks).every(c => c.passed) returns true on an empty object (vacuous truth). So if ALL checks are still running at timeout, result.checks is {} and every() returns true.

result.passed is saved by timed_out being true on line 332, but this is fragile. After timeout, explicitly mark any check that hasn't reported as { passed: false, timed_out: true } so the checks object is always complete.

[Shivanshu-07]

Fixed in 80d28e1. runAllChecks now tracks result._expectedChecks (list of check names that were started). After timeout, waitForReady iterates expected checks and marks any missing ones as { passed: false, timed_out: true }. The checks object is always complete before every() runs.

[amandeepsingh333]

🟡 MEDIUM — V2 re-capture silently enables JavaScript: Deleting domSnapshot here changes the semantics of downstream code. On line 457, enableJavaScript is computed as !snapshot.domSnapshot — which now becomes true even if the SDK originally sent enableJavaScript: false.

This means re-capture always runs with JS enabled, which could change screenshot behavior (animations, hover states, JS-injected content).

Fix: Preserve the original enableJavaScript value before deleting domSnapshot:

const originalEnableJS = snapshot.enableJavaScript;
delete snapshot.domSnapshot;
snapshot.enableJavaScript = originalEnableJS ?? snapshot.enableJavaScript;

[Shivanshu-07]

Fixed in 80d28e1. Now explicitly preserving the original enableJavaScript value before deleting domSnapshot:

snapshot.enableJavaScript = snapshot.enableJavaScript ?? false;
delete snapshot.domSnapshot;

This ensures the downstream !snapshot.domSnapshot computation doesn't silently flip JS to enabled.

[amandeepsingh333]

🟡 MEDIUM — Static mutable state not safe for concurrent instances: Page._globalReadinessConfig is a class-level static. If multiple Percy instances are created (tests, programmatic usage), the last constructor wins. When any instance calls stop() (line 377: Page._globalReadinessConfig = null), it nullifies the config for all instances.

Fix: Attach readiness config to the Percy instance and pass it through the call chain rather than using a class-level static.

[Shivanshu-07]

Valid concern. In practice, Percy CLI creates only one instance per process (the server binds to a single port). Multiple instances are only used in tests, where afterEach resets the static. That said, the reviewer is right that this is a code smell. I'll track this as a follow-up refactor to pass readiness config through the instance chain instead of using a class static. For now, the test-level cleanup is sufficient and the risk is low in production.

[amandeepsingh333]

🟡 MEDIUM — Double insertPercyDom call: This calls await page.insertPercyDom(), but the caller in page.js (line 216) already called insertPercyDom() before invoking waitForReadiness. If insertPercyDom is idempotent this is just wasteful; if it's not, it could cause issues. Consider removing this redundant call since the caller handles it.

[Shivanshu-07]

Fixed in 80d28e1. Removed the redundant insertPercyDom() call from core/readiness.js. The caller (page.snapshot() line 218) already calls it before waitForReadiness(), and insertPercyDom() is idempotent (checks !window.PercyDOM before injecting), but the redundant call was unnecessary.

[amandeepsingh333]

🔵 LOW — js_idle_window_ms is undocumented dead code: This reads config.js_idle_window_ms which is never defined in any preset, the config schema, or resolveReadinessConfig. It always falls through to stability_window_ms. Either add it to the schema/presets or remove the fallback.

[Shivanshu-07]

Fixed in 80d28e1. Removed the config.js_idle_window_ms || fallback — now uses config.stability_window_ms directly since js_idle_window_ms was never in the config schema or presets.

[amandeepsingh333]

🔵 LOW — PRESETS duplicated: Both packages/core/src/readiness.js and packages/dom/src/readiness.js define identical PRESETS objects. If a preset value is updated in one but not the other, behavior will diverge. Consider a single source of truth — either export from one and import in the other, or have the CLI always pass fully-resolved config so the DOM module doesn't need presets.

[Shivanshu-07]

Acknowledged. The duplication exists because @percy/dom runs in the browser context (injected via eval) and can't import from @percy/core (Node.js). The CLI always passes fully-resolved config via resolveReadinessConfig(), so the DOM presets are only used as fallbacks when waitForReady() is called directly (e.g., in browser tests or if PercyDOM is used standalone). Will consider having the CLI always pass complete config to eliminate the need for DOM-side presets.

[amandeepsingh333]

🔵 LOW — Missing positive test for V2 re-capture: Tests cover "does not re-capture when readiness is disabled" and "does not re-capture for non-SDK snapshots", but there's no test verifying the happy path — _fromSDK: true + readiness enabled asserts that domSnapshot was deleted and the page was fetched from URL. The positive path should have test coverage.

[Shivanshu-07]

Fair point. A positive test for V2 re-capture (asserting domSnapshot was deleted and page navigated to URL) would need a full browser + server + readiness config integration test, which tends to be slow and flaky in CI. The negative tests cover the guard conditions. Will add a targeted positive test in a follow-up that verifies the domSnapshot deletion without requiring full browser capture.

[amandeepsingh333]

🔵 LOW — Test provides false confidence: This test spies on percy.snapshot with callFake(async () => {}), then asserts the spy was called with _fromSDK: true. But the spy replaces the real implementation — the flag preservation logic (save before validation, restore after) never actually runs. This test only verifies the test's own input, not that _fromSDK survives through validateSnapshotOptions.

Fix: Don't spy on snapshot — instead spy on a downstream method (e.g., #discover) to verify the flag reaches the actual consumer.

[Shivanshu-07]

Valid — the spy replaces the real implementation so the save/restore logic never executes. The test only verifies that the API passes _fromSDK. The actual flag preservation through validateSnapshotOptions is covered by the api.test.js tests which use the real endpoint. Will improve this test in a follow-up to spy on a downstream consumer instead.