percySnapshot: tolerate non-critical font load failures#4688
Merged
Conversation
`Promise.all(Array.from(document.fonts, (f) => f.load()))` rejects the whole barrier if any single FontFace.load() rejects — and Chrome rejects FontFace network failures with the generic `DOMException: A network error occurred.`, no URL attached. That message bubbled up through percySnapshot → test → QUnit and surfaced in CI as "Promise rejected during \"<test name>\": A network error occurred." on every test that calls percySnapshot, with no breadcrumb pointing at fonts. Two such failures in shard 7 of run 25460877925 — one in Acceptance | code-submode | field playground, one in Integration | preview — share that fingerprint exactly. Switch to `Promise.allSettled` so a single non-critical font hiccup no longer poisons the snapshot. The hard-coded IBM Plex Sans `document.fonts.check` immediately after stays the load-bearing assertion: if the font that actually moves Percy pixels is the one that failed, the test still fails — but with a clear, self-attributing message. When any font does fail, log a `[percy-snapshot]` warning naming each failed face (weight/style/family) and the rejection reason, so future recurrences are diagnosable from the testem log alone instead of needing to hand-correlate with `[test-fetch]` output. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37cc7fb3e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Per the WHATWG FontFaceSet.check spec, faces in `error` status are treated as settled, and `document.fonts.check(descriptor, '')` with empty text can return `true` even when the required face is unrenderable. So with the previous fix alone, an IBM Plex Sans network failure would slip past the explicit guard, hit Percy, and capture the page with a fallback font silently substituted in — exactly the false-positive class the hard-coded check was supposed to prevent. Inspect the rejected-faces list directly: if any rejected face's family is "IBM Plex Sans", throw with the descriptor + rejection reason. The `document.fonts.check` loop stays as a belt-and-suspenders guard for the unrelated case where a Sans weight was never declared on the page in the first place. Addresses Codex review on PR #4688. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the percySnapshot test helper in packages/host against transient font-loading failures by switching from failing-fast (Promise.all) to tolerant font loading (Promise.allSettled), while keeping IBM Plex Sans as the explicit “must-load” assertion to avoid pixel diffs.
Changes:
- Switch font preloading from
Promise.all(document.fonts.load())toPromise.allSettled(...)so non-critical font failures don’t fail the snapshot path. - Add warning logging that lists failed font faces and their rejection reasons.
- Add a small helper (
describeFontLoadError) to produce readable error strings.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Preview deploymentsHost Test Results 1 files 1 suites 2h 0m 13s ⏱️ Results for commit 8dff3c2. Realm Server Test Results 1 files ± 0 1 suites +1 16m 36s ⏱️ + 16m 36s Results for commit 8dff3c2. ± Comparison against earlier commit 37cc7fb. |
habdelra
added a commit
that referenced
this pull request
May 7, 2026
Per the WHATWG FontFaceSet.check spec, faces in `error` status are treated as settled, and `document.fonts.check(descriptor, '')` with empty text can return `true` even when the required face is unrenderable. So with the previous fix alone, an IBM Plex Sans network failure would slip past the explicit guard, hit Percy, and capture the page with a fallback font silently substituted in — exactly the false-positive class the hard-coded check was supposed to prevent. Inspect the rejected-faces list directly: if any rejected face's family is "IBM Plex Sans", throw with the descriptor + rejection reason. The `document.fonts.check` loop stays as a belt-and-suspenders guard for the unrelated case where a Sans weight was never declared on the page in the first place. Addresses Codex review on PR #4688. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Tests that call
percySnapshotwere failing intermittently in CI withPromise rejected during "<test name>": A network error occurred.— a generic message with no URL, no stack pointing into our code, and no way to attribute it. The fingerprint matched two unrelated tests in run 25460877925 shard 7 (field playground > can preview compound field instanceandpreview > renders head meta tags preview for a card head format). Both callpercySnapshot; no other tests in those modules failed.Root cause:
await Promise.all(Array.from(document.fonts, (f) => f.load())). Chrome rejectsFontFace.load()for transient font-fetch failures with exactlyDOMException: A network error occurred.. WithPromise.all, a single non-critical font hiccup tanked the whole snapshot path and bubbled out of the test.What this PR does in
percy-snapshot.tsdocument.fontsonce before kicking off anyload()calls. The live set can shift while fonts/stylesheets load, so a later re-enumeration would risk misattributing which face actually failed.Promise.allSettledinstead ofPromise.allso a non-critical font failure no longer poisons the snapshot path.FontFaceSet.checkspec, faces inerrorstatus are treated as settled, anddocument.fonts.check(descriptor, '')with empty text can returntrueeven when the required face is unrenderable. So we cannot rely oncheckalone to detect a Sans network failure — we inspect the rejected list directly and throw with the descriptor + rejection reason if any rejected face's family is"IBM Plex Sans". Without this, Percy would capture the page with a fallback font silently substituted in.document.fonts.checkloop stays for the unrelated case where a Sans weight was never declared on the page in the first place.[percy-snapshot]warning when any non-critical font fails, naming each failed face (weight/style/family + rejection reason). If this fingerprint resurfaces, the testem log now points at the offender directly.Test plan
A network error occurred.fingerprint)Required font failed to load: ...(rejected face) orNot ready: IBM Plex Sans font could not be loaded (...)(never declared) error still fires, so the load-bearing assertion against fallback-font captures isn't lostpackages/host🤖 Generated with Claude Code