fix(core): stop compressing discovered resources in place

[rishigupta1599]

Problem

With PERCY_GZIP enabled, webfonts declared via @font-face were captured for the first snapshot of a build and missing from every snapshot after it, so the renderer 404'd fonts the page still needed (icon fonts render as blank boxes).

Root cause

processSnapshotResources compressed resources in place:

resource.content = Pako.gzip(resource.content);
resource.sha = sha256hash(resource.content);

Those objects are the same ones held by the build-wide resource cache — network.intercept.saveResource stores the object itself, not a copy. So the first snapshot's upload pass replaced every cached body with its gzip bytes.

Every later snapshot then had those entries replayed to the browser via Fetch.fulfillRequest using the resource's original headers, e.g. content-type: text/css with no content-encoding. Compressed bytes are not parseable as the text type they are labelled with, so the stylesheet yields no rules — and anything it references is therefore never requested and never captured.

A stylesheet carrying @font-face consequently produces no webfont for any snapshot after the first. Only stylesheet-referenced resources are affected: DOM <img> URLs are found by the preload scanner directly in markup, whereas @font-face and background-image URLs require the CSS to be parsed first.

Confirmed in the logs: the stylesheet is gzipped exactly once, then served from cache for the rest of the build, with isGzipped() true on it from then on — i.e. the cached entry itself had been mutated.

Fix

Compress a copy and leave the cached entry holding the original bytes. Cache hit rates are unchanged; only the mutation is gone.

Verification

Real builds replaying a reporter's own serialized DOMs and config (PERCY_GZIP=true, 11 snapshots), identical except for this change:

	font requests across the build	stylesheets captured
before	5 (first snapshot only)	11/11
after	11 (every snapshot)	11/11

Cache hits stay at 41–44 per snapshot in both, so caching still works. A control run with PERCY_GZIP unset never exhibited the bug.

New regression test keeps cached resources uncompressed when PERCY_GZIP is enabled, verified in both directions: it fails on the unfixed code — snapshot two's manifest contains the root, img.gif and style.css but no font.woff — and passes with the fix.

[rishigupta1599]

Claude Code PR Review

PR: #2381 • Head: d3e1410 • Reviewers: stack-code-reviewer

Summary

Stops processSnapshotResources from gzipping discovered resources in place. The resource objects are shared with the build-wide discovery cache (cache.set(r.url, r, …) stores the object itself, and Fetch.fulfillRequest later replays resource.content under resource.headers), so the first snapshot's upload pass replaced cached bodies with gzip bytes still labelled text/css. Later snapshots then got unparseable CSS, so @font-face URLs were never discovered. The fix compresses into a copy (uploaded = { ...resource, content, sha }) and pushes the copy to the upload set, leaving the cached entry holding the original bytes.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	No secrets in the diff.
High	Security	Authentication/authorization checks present	N/A	No auth surface touched.
High	Security	Input validation and sanitization	N/A	No new external input.
High	Security	No IDOR — resource ownership validated	N/A	Not applicable.
High	Security	No SQL injection (parameterized queries)	N/A	No SQL.
High	Correctness	Logic is correct, handles edge cases	Pass	Shallow copy preserves root, log, headers, status, url, mimetype, widths; only content/sha are overridden. Already-gzipped resources take the no-copy path (uploaded = resource), matching prior behaviour. The size check and kept.push both switch to uploaded consistently, so the post-gzip size gate still measures the bytes actually uploaded.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	The try/catch around gzip is unchanged: a failing resource is instrumented, warned and skipped, and uploaded stays the pre-assignment value so no half-written state escapes.
High	Correctness	No race conditions or concurrency issues	Pass	Removing the in-place mutation eliminates a cross-snapshot shared-state hazard; nothing new is shared.
Medium	Testing	New code has corresponding tests	Pass	keeps cached resources uncompressed when PERCY_GZIP is enabled covers the regression. Verified locally in both directions (see Verification).
Medium	Testing	Error paths and edge cases tested	Pass	Existing specs still cover gzip-throws, post-gzip oversize drop, raw-size ceiling, and the root/log exemption.
Medium	Testing	Existing tests still pass (no regressions)	Pass	Confirmed green on CI (per author); lint clean locally on both changed files.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	No new I/O.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable.
Medium	Quality	Follows existing codebase patterns	Pass	Spread-copy is the idiom used elsewhere in this file (e.g. { ...snapshot, resources }, { ...discovery, … }).
Medium	Quality	Changes are focused (single concern)	Pass	Six lines of source plus one regression test; nothing unrelated.
Low	Quality	Meaningful names, no dead code	Pass	uploaded reads clearly against resource; the now-redundant alreadyZipped local was correctly dropped.
Low	Quality	Comments explain why, not what	Pass	The added comment states the invariant (objects are shared with the cache) rather than restating the code.
Low	Quality	No unnecessary dependencies added	Pass	No dependency changes.

Findings

No confirmed findings.

Investigated and not confirmed

stack-code-reviewer reported one Critical finding — that the new spec keeps cached resources uncompressed when PERCY_GZIP is enabled fails against this very commit, with captured[1] missing font.woff, implying the fix is incomplete. That finding does not hold, and is excluded from the verdict.

The reviewer ran the spec with a bare npx jasmine packages/core/test/discovery.test.js. This repo's suite does not run that way: scripts/test.js loads jasmine with requires: [scripts/babel-register.cjs], helpers: [scripts/test-helpers.js] and forks the runner under --loader scripts/loader.js. Bare npx jasmine skips all of that, which is why the reviewer saw 197 specs instead of the real 1205 — the suite was running without its harness.

Re-run through the repo's own runner at this exact commit:

• With the fix — Discovery keeps cached resources uncompressed when PERCY_GZIP is enabled passes (Executed 1 of 1205 specs).
• Without the fix — reverting only the copy (back to resource.content = Pako.gzip(...)) makes the same spec fail on the captured[1] assertion, with the manifest containing the root, img.gif and style.css but no font.woff — exactly the failure signature the PR description claims.

So the test is a valid regression test that fails on the unfixed code and passes on the fixed code, and the reviewer's observation was an artifact of the invocation, not of the diff. The rest of that reviewer's analysis was independently spot-checked and holds: cache.set(r.url, r, …) (discovery.js:683) does store the same object that Fetch.fulfillRequest later replays (network.js:779-785), the only other Pako.gzip call site (percy.js:890) builds a local log payload and is unaffected, and resource.sha has no consumer that requires identity with the cached entry — the upload manifest is built from the returned resources in packages/client/src/client.js.

One note on the coverage annotation, non-blocking: swapping /* istanbul ignore next */ (which suppressed the whole if) for /* istanbul ignore else */ (which suppresses only the untested already-gzipped branch) is a tightening, not a loosening — the commonly-exercised true branch is now counted.

---

Verdict: PASS