feat: Readiness gate in PercyDOM.serialize() — works for URL + SDK paths (PER-7348)

[Shivanshu-07]

Summary

Integrates the readiness gate into PercyDOM.serialize() itself — the only location that works for both URL-based and SDK-based snapshot paths.

Why this matters

The previous approach (PR #2172) put readiness in page.js before the PercyDOM.serialize() call. This only worked for the URL-capture path (CLI percy snapshot, Storybook). For the SDK path (Cypress/Selenium/Puppeteer), page.snapshot() is never called — SDKs call PercyDOM.serialize() directly in the test browser. Readiness never ran for SDK snapshots.

How it works now

PercyDOM.serialize({ readiness: { preset: 'strict', notPresentSelectors: ['.skeleton'] } })
  │
  ├── waitForReady(readiness)  ← runs readiness checks FIRST
  │     ├── DOM stability (MutationObserver)
  │     ├── Network idle (PerformanceObserver)
  │     ├── Font ready (document.fonts.ready)
  │     ├── Image ready (above-fold img.complete)
  │     ├── JS idle (Long Task API + requestIdleCallback)
  │     ├── Ready selectors (poll for elements)
  │     └── Not-present selectors (wait for loaders to disappear)
  │
  └── serializeDOMSync(options)  ← THEN serializes the stable DOM

This works identically for:

• CLI percy snapshot: CLI calls PercyDOM.serialize(options) via page.eval() — readiness runs in the CLI browser
• Cypress: cy.percySnapshot() → SDK calls PercyDOM.serialize() in test browser — readiness runs there
• Selenium/Puppeteer: Same — percySnapshot() → PercyDOM.serialize() in test browser
• Storybook: CLI navigates stories and calls serialize — readiness runs before capture

Key design

• serialize() returns a Promise when readiness is configured, stays sync when not (backward compatible)
• page.eval() uses awaitPromise: true which handles async automatically
• Readiness diagnostics attached to result as readiness_diagnostics
• Per-snapshot override: cy.percySnapshot('name', { readiness: { preset: 'disabled' } })
• Global config from .percy.yml flows via the options parameter
• No changes to discovery.js, percy.js, or api.js — serialize handles everything
• No _fromSDK flag, no domSnapshot dropping, no re-navigation

Files changed

File	Change
packages/dom/src/readiness.js	NEW — 7 readiness checks with abort mechanism
packages/dom/src/serialize-dom.js	Calls waitForReady() before serializing when readiness configured
packages/dom/src/index.js	Exports waitForReady
packages/core/src/page.js	Passes readiness config to serialize options
packages/core/src/config.js	Readiness schema for .percy.yml + per-snapshot
packages/dom/test/readiness.test.js	905 lines — unit tests for all 7 checks
packages/dom/test/serialize-readiness.test.js	163 lines — integration tests for readiness-in-serialize

Test coverage

serialize-readiness.test.js verifies:

• Sync behavior preserved when no readiness config
• Sync behavior when preset: 'disabled'
• Returns Promise when readiness configured
• Resolves with serialized DOM after readiness passes
• Attaches readiness_diagnostics to result
• Waits for skeleton removal before serializing (key E2E scenario)
• Waits for ready_selectors before serializing
• Graceful degradation on timeout (still serializes)
• Per-snapshot override (SDK flow simulation)
• Backward compatibility for non-readiness options

Related

• Previous approach: PR feat: Fully Loaded Snapshot Capture — Readiness Gate (PER-7348) #2172 (readiness in page.js — URL path only)
• API smart debugging: percy-api PR #5629
• Jira: PER-7348

[rishigupta1599]

why is this required?, if readiness flag is there call waitForReadiness with a try catch

[Shivanshu-07]

Fixed. serializeDOMWithReadiness now wraps the waitForReady call in a try/catch and falls back to synchronous serialization on error (graceful degradation). See packages/dom/src/serialize-dom.js:102-121 on commit c879f34.

[rishigupta1599]

should not be required why output of serialize is changing?

[Shivanshu-07]

The page.js change IS required — here's why:

PercyDOM.serialize(options) now returns a Promise when readiness is configured (it calls waitForReady() which is async). When no readiness is configured, it returns a plain object (sync, same as before).

Inside page.eval(), the injected function runs in the browser. If serialize() returns a Promise, that Promise ends up as { domSnapshot: Promise, url: ... }. CDP's awaitPromise: true only awaits the top-level return value — it won't resolve a Promise nested inside an object.

So we need to check: if serialize returned a Promise, .then() it to get the final { domSnapshot, url } shape as the top-level return. If it returned a plain object, wrap it directly. This ensures CDP always receives a resolvable value.

Without this handling, readiness-enabled snapshots would return domSnapshot: [object Promise] instead of the actual HTML.

[rishigupta1599]

function was sync till now now we are making it async for readiness flow, does all SDK handle that.
In current case if for any reason DOM serialization breaks user will not even get failure as there test suite will move forward

[rishigupta1599]

And SDK publish DOM Snapshot as is and will end up sending a Promise

[Shivanshu-07]

Valid concern — fixed. Split serializeDOM into two functions:

• serializeDOM() — remains synchronous and backward-compatible. All existing SDKs (@percy/cypress, @percy/puppeteer, @percy/selenium-webdriver) that do PercyDOM.serialize(options) without await continue to work unchanged.
• serializeDOMWithReadiness() — new async variant. Used only by the CLI URL-capture path (page.js uses CDP awaitPromise: true to await it) and by new SDK versions that opt into readiness-gated capture.

If readiness fails internally, the function falls back to sync serialization (graceful degradation) rather than breaking the capture. See packages/dom/src/serialize-dom.js:93-121.

[rishigupta1599]

Review Summary

The architecture is solid -- moving readiness into PercyDOM.serialize() is the right call for URL + SDK parity. Good test coverage (1000+ lines) and clean abort/cleanup logic.

However, there are several issues that need addressing before merge, ranging from a config naming mismatch bug that will silently break user overrides, to missing abort propagation, and a coupling concern in the JS idle check. See inline comments.

[rishigupta1599]

Bug: camelCase / snake_case mismatch breaks user config overrides

The config schema in config.js defines properties in camelCase (stabilityWindowMs, networkIdleWindowMs, timeoutMs), but readiness.js presets and this spread use snake_case (stability_window_ms, etc.).

When a user sets .percy.yml:

snapshot:
  readiness:
    preset: strict
    stabilityWindowMs: 500

The spread { ...preset, ...options } sets stabilityWindowMs: 500 but the code reads config.stability_window_ms (from the preset default), silently ignoring the override.

You need either:

1. A normalization step to convert camelCase options to snake_case before the spread, or
2. Align the config schema to use snake_case (matching the existing enable_javascript / dom_transformation pattern in serialize-dom.js), or
3. Support both in the destructuring like serialize-dom.js does: enableJavaScript = options?.enable_javascript

This is likely to bite every user who configures readiness via .percy.yml or per-snapshot SDK options.

[Shivanshu-07]

Fixed. Added normalizeOptions() helper in packages/dom/src/readiness.js that accepts either camelCase (what the schema defines) or snake_case, and maps both to the snake_case keys used internally.

The merge also now only overrides preset defaults for user keys whose value is not undefined, so a partial override no longer wipes other preset values:

let userOptions = normalizeOptions(options);
let config = { ...preset };
for (let key of Object.keys(userOptions)) {
  if (userOptions[key] !== undefined) config[key] = userOptions[key];
}

Added a direct unit test covering both naming conventions, precedence when both are passed, and falsy-value (0, false) preservation.

[rishigupta1599]

Coupling: checkJSIdle reuses stability_window_ms instead of its own config

checks.push(checkJSIdle(config.stability_window_ms, aborted)...)

JS idle window and DOM stability window are conceptually different. With the strict preset (stability_window_ms: 1000), the JS idle check requires 1 full second of no long tasks before confirming idle -- that's very aggressive and will cause unnecessary timeouts on pages with normal JS activity.

Consider adding a dedicated js_idle_window_ms config (defaulting to something like 200ms) or at minimum document why these are intentionally coupled.

[Shivanshu-07]

Fixed. Added a dedicated js_idle_window_ms config (camelCase jsIdleWindowMs in the schema) that is independent of stability_window_ms.

Preset defaults now decouple them sensibly:

Preset	stability_window_ms	js_idle_window_ms
fast	100	100
balanced	300	300
strict	1000	500

With strict, DOM-stability still waits 1000ms but the JS idle check only needs 500ms of no long tasks — no more 1-full-second timeouts on pages with normal JS activity.

If a user passes a custom config that predates this option, runAllChecks falls back to stability_window_ms for backward compat. Added integration tests proving the decoupling works and the fallback still works.

[rishigupta1599]

Missing abort propagation in checkFontReady

Unlike every other check function, checkFontReady doesn't accept or honor the aborted signal. It has a hardcoded 5s internal timeout that runs independently of the orchestrator's effectiveTimeout.

If a user sets timeoutMs: 2000 and fonts take 3s, the overall timeout fires at 2s and aborts all other checks -- but the font check's internal Promise.race keeps the 5s timer alive. While it won't block (the orchestrator moves on), it leaves a dangling timer.

Accept aborted and clear the internal timeout on abort, consistent with the other checks.

[Shivanshu-07]

Fixed. checkFontReady now accepts an aborted handle and clears the 5s font timer on abort:

function checkFontReady(aborted) {
  let start = performance.now();
  if (!document.fonts?.ready) return Promise.resolve({ passed: true, duration_ms: 0, skipped: true });
  let fontTimer;
  let result = Promise.race([
    document.fonts.ready.then(() => ({ passed: true, duration_ms: Math.round(performance.now() - start) })),
    new Promise(r => { fontTimer = setTimeout(() => r({ passed: false, duration_ms: 5000, timed_out: true }), 5000); })
  ]);
  if (aborted) aborted.onAbort(() => { if (fontTimer) clearTimeout(fontTimer); });
  return result;
}

Also updated runAllChecks to pass the abort handle through. Matches the abort pattern used by every other check.

[rishigupta1599]

Simplification: the manual thenable check is redundant

Since eval() already uses awaitPromise: true (line 143), CDP will automatically await any Promise returned from the function. You can simplify this to:

let capture = await this.eval(async (_, options) => {
  let result = await PercyDOM.serialize(options);
  return { domSnapshot: result, url: document.URL };
}, { ...opts, readiness });

await on a non-thenable (sync path) just returns the value directly, so backward compat is preserved. This avoids the manual duck-typing of .then.

[Shivanshu-07]

Fixed. Simplified to use native await — CDP awaitPromise: true auto-awaits the returned Promise, and await on the sync path is a no-op:

let capture = await this.eval(async (_, options) => {
  let fn = (PercyDOM.serializeDOMWithReadiness || PercyDOM.serialize);
  let domSnapshot = await fn(options);
  return { domSnapshot, url: document.URL };
}, { ...opts, readiness });

The PercyDOM.serializeDOMWithReadiness || PercyDOM.serialize fallback keeps page.js compatible with older injected dom bundles that don't yet export the new async variant.

[rishigupta1599]

Performance: performance.getEntriesByType('resource') called every 50ms

This allocates a new array of all resource entries on every tick. On resource-heavy pages (hundreds of images, scripts, stylesheets), this is expensive -- especially since you only need the count.

Consider using PerformanceObserver for 'resource' entries (like you do for longtask in checkJSIdle) to incrementally track count changes, instead of polling the full entry list every 50ms.

[Shivanshu-07]

Fixed. Replaced the 50ms polling of performance.getEntriesByType('resource') with a PerformanceObserver that subscribes incrementally to 'resource' entries:

try {
  observer = new PerformanceObserver(list => {
    if (aborted.value) return;
    if (list.getEntries().length > 0) resetIdleTimer();
  });
  observer.observe({ type: 'resource', buffered: false });
} catch (e) {
  // Older browser — fall back to polling
  ...
}

No more scanning the entire resource list every tick — the observer fires only when a new resource entry is added, matching the pattern already used in checkJSIdle for longtasks. The polling path is preserved as a fallback for very old browsers that lack PerformanceObserver.

[rishigupta1599]

Naming convention inconsistency with the rest of the config schema

The readiness properties use camelCase (stabilityWindowMs, networkIdleWindowMs, timeoutMs) but the internal readiness implementation uses snake_case, and other Percy config options like enable_javascript, dom_transformation, reshuffle_invalid_tags use snake_case in the schema.

Consider aligning to snake_case (stability_window_ms, network_idle_window_ms, timeout_ms) for consistency with the rest of the Percy config, or add explicit camelCase to snake_case mapping in readiness.js.

[Shivanshu-07]

I verified this and I think the premise here might be inverted. Grepping the existing schema in packages/core/src/config.js:

enableJavaScript, cliEnableJavaScript, disableShadowDOM,
forceShadowAsLightDOM, enableLayout, domTransformation, reshuffleInvalidTags,
deferUploads, useSystemProxy, skipBaseBuild, browserName, browserVersion,
osVersion, deviceName, percyBrowserCustomName, diffSensitivity,
imageIgnoreThreshold, carouselsEnabled, bannersEnabled, adsEnabled,
minHeight, percyCSS, scopeOptions, ignoreRegionSelectors, freezeAnimation,
freezeAnimatedImage, responsiveSnapshotCapture, testCase, thTestCaseExecutionId,
fullPage ...

Every existing snapshot option in the schema is already camelCase — there are no enable_javascript / dom_transformation / reshuffle_invalid_tags entries in config.js. So the readiness schema (stabilityWindowMs, timeoutMs, etc.) is actually consistent with the rest of the file.

The snake_case you're seeing in readiness.js is the internal naming used after normalization (matches the diagnostic output keys like stability_window_ms, network_idle_window_ms, which are the payload contract with the backend). The normalizeOptions() helper bridges the two, and it accepts both forms so direct snake_case overrides still work if anyone is using them.

Happy to rename if you'd still prefer snake_case for the user-facing schema, but the current naming was chosen specifically to match the surrounding camelCase conventions.

[rishigupta1599]

Questionable: href in LAYOUT_ATTRIBUTES

Changing href on an <a> tag doesn't affect layout. This is only layout-relevant for <link> stylesheet elements. As written, any JS that updates an anchor's href will be incorrectly classified as a layout mutation, resetting the stability timer.

Consider either removing href or scoping it: check mutation.target.tagName === 'LINK' before treating an href change as layout-affecting.

[Shivanshu-07]

Fixed. href removed from the generic LAYOUT_ATTRIBUTES set, replaced with a scoped check in isLayoutMutation:

// href is only layout-affecting on <link> elements (stylesheets).
// On <a> tags changing href is a no-op for layout.
if (attr === 'href') return mutation.target.tagName === 'LINK';

href is still in the MutationObserver attributeFilter so the observer fires — the classifier just returns false for <a> tags now, so it doesn't reset the stability timer.

Added direct unit tests for both cases (<a href> returns false, <link href> returns true) in the new readiness-helpers test file.

[rishigupta1599]

Excessive istanbul ignore coverage suppression

There are ~15 /* istanbul ignore next */ annotations across this 392-line file, covering core logic paths (abort branches, network idle, image ready, JS idle, selector polling, style parsing, etc.).

This undermines the coverage story in the PR description. Many of these are testable -- e.g., abort branches can be triggered by setting very short timeouts, the style parsing can be tested with direct unit tests of hasLayoutStyleChange / parseStyleProps if exported.

Consider:

1. Exporting the internal helpers for direct unit testing
2. Using short timeouts to exercise abort/timeout paths
3. Reserving istanbul ignore for truly untestable browser APIs only

[Shivanshu-07]

Fixed. Dropped from ~15 function-wide istanbul ignore annotations to narrow, justified ignores on genuinely untestable paths only. Concrete changes:

1. Exported internal helpers from readiness.js for direct unit testing:

   • isLayoutMutation, hasLayoutStyleChange, parseStyleProps, normalizeOptions, createAbortHandle

   (These are not added to the public index.js surface — tests import from ../src/readiness, matching the pattern used by serialize-frames, serialize-cssom, transform-dom etc.)

2. Added packages/dom/test/readiness-helpers.test.js — 36 new unit tests covering every branch of the exported helpers (empty inputs, multi-decl parsing, prefix-matched layout props like min-/max-/flex/z-index, data-/aria- filtering, <a> vs <link> href handling, camelCase precedence, falsy-value preservation, abort callback ordering, etc.).

3. Removed function-wide ignores from six check functions (checkDOMStability, checkNetworkIdle, checkImageReady, checkJSIdle, checkReadySelectors, checkNotPresentSelectors). These are now measured through integration tests.

4. Narrow ignores remain only on genuinely untestable paths, each with a one-line reason:

   • Browser-API availability (document.fonts?.ready, Long Task API try/catch, requestIdleCallback else-branch) — these are always available in Chrome/Firefox; fallbacks are for older browsers.
   • Font 5s timeout race — impractical to wait for in tests.
   • Defensive if (aborted.value) guards inside setInterval/MutationObserver callbacks — abort() clears the interval/disconnects the observer synchronously, so the check is dead code in practice.
   • Empty-selector early returns — orchestrator only calls those checks when selectors.length > 0.
   • Error-safety-net catch blocks.
   • /* istanbul ignore else */ for the requestIdleCallback availability check (only the fallback branch is ignored, not the whole statement).

Result: readiness.js now shows 100% lines / 100% branches / 100% functions / 100% statements instead of having 6 functions wholly excluded from the report.