Site Screenshot False Positives

Fixing site-screenshots false positives

A false positive here is a screenshot diff, or a failed run, caused by the harness or by the difference between production and staging rather than by the branch. They are expensive out of proportion to their size: a reviewer opens a 94-pixel diff, finds nothing, and trusts the next one less.

Most of what follows is method. The catalogue is the part that moves.

Keeping this current

Update this file in the same PR as the fix. A class resolved without a row here is a class the next session re-derives — that has already happened twice, and it is what this file exists to stop. Land the row with the change and the two cannot drift.

What to write down, in rough order of how much it saves later:

• A resolved class gets a row in the catalogue: the symptom, the number you measured, and where the fix went. The number is the load-bearing part, because that is what identifies the class the next time it appears.
• A theory you disproved goes under Disproved, with the measurement that killed it. These are the expensive ones — the cost of a wrong theory is paid again in full by whoever reasons their way to it a second time, and two of the entries there had already been argued twice before they were written down.
• A class you decided not to fix stays under Open, with the reason. "Invisible and rare" is a legitimate reason and worth recording as a decision rather than leaving as an absence.
• A method that generalises goes in Method. Not every trick — only what changed how a question got answered.

And delete what stops being true. An entry that turns out to be wrong, or an open class that turns out not to be real, comes out rather than sitting there with a hedge attached: a catalogue that is short is far cheaper than one that is confidently wrong, because this file gets trusted before it gets checked. Move rows between sections as their status changes rather than restating them in two places.

How the comparison actually works

Get this wrong and you will chase diffs that aren't there.

• Screenshots are taken by shot-scraper against the URLs in config.yaml, driven by ops/templates/lxc-manager/site-screenshots on lxc-manager-1. Master runs against production; a branch runs against its staging server.
• The reported diff is git diff origin/master...HEAD — three dots, so against the merge base, not master's head. GitHub's /compare/<branch> link shows the same thing. Master's later "Update reference screenshots" commits therefore do not count as branch changes, and neither should they in your own analysis.
• A branch's screenshot branch keeps the config.yaml it was created with. A fix reaches a given owid-grapher branch only when that branch's screenshot branch is next created. This is why a normalisation can look like it did nothing.
• The reference images only change when master runs. Merging a normalisation does not update them; the next master run does, and that run rewrites whatever the normalisation touched — one-time churn to expect and to say out loud.
• owidbot reports ✅ / ⚠️ N pages changed / ❌ the run failed. A failed run leaves the compare link showing the previous run's diff.
• The step soft-fails on exit 1, so a failure is only visible in owidbot's comment and the Buildkite log.

Method

1. Diff against the merge base yourself. references/mergebase-diff.py does this: for each branch it resolves the merge base, downloads both sides of every changed file, and reports pixel counts, max delta, and the contiguous row bands. Row bands are what let you find the region without eyeballing a 7000-pixel-tall image.

2. Look for the same diff on unrelated branches. This is the single best discriminator and it has settled almost every case here. A byte-identical pixel count on two branches that touch different code is the harness, full stop — 66,596 px on four branches, 55,736 px on four, 22.1% on six. Check this before forming any theory.

3. Crop and look. A number tells you a region changed; only the picture tells you what. Crop both sides at the band with a little padding and read them side by side. Deltas of 1–3/255 mean resampling or decode; a max delta of 255 with one side at mean 255.0 means something is missing entirely; text that differs by a few glyphs is usually a date or a count.

4. Measure the mechanism before writing the fix. The theory that felt obvious has been wrong more than once. Instrument the shipped code and get a number: does the thing you blame actually happen? Two examples worth remembering — "images arrive after the decode pass" (measured: never, even at 250 kbps) and "the screenshot step races the preview deploy" (measured: passing runs overlap it exactly as much as failing ones).

5. A correlation needs a control group. Every screenshot run starts seconds after the bake, so "every failure happened during X" is worthless if every run happens during X. Find the runs that had the condition and passed.

6. Verify with a 2×2, not a before/after. Run the old code twice and the new code twice. Old vs old and new vs new give you the run-to-run noise floor; only then does old vs new mean anything. This caught a fix that changed 55,736 pixels for a reason unrelated to its purpose.

7. Where possible, compare against ground truth, not just against stability. A deterministic screenshot can still be deterministically wrong. For the capture-time classes, the ground truth is a capture taken with the viewport never resized (scroll to the region, then a clipped viewport screenshot) — that is how the image-source fix was shown to be more faithful and not merely more stable.

8. Test the shipped source, not a copy. Extract the function out of config.yaml with yaml.safe_load(...)[0]["javascript"] and evaluate it under Playwright. A copied snippet drifts from what runs in CI, and every one of these fixes lives in that one string.

9. Instrument the shot's own timeline, not just its result. Several of these bugs live between two steps of the shot rather than in either, and a measurement taken after resolve() cannot see them. Assign to window.__something at the points you care about and read them afterwards. That is how the image pin was caught re-creating the race it removes: 0 painted-and-unloaded images at the wait, 4 at resolve(), 0 again a second later.

10. Say when you could not reproduce it. A warm laptop makes most of these deterministic. If the symptom only appears in CI, the honest claim is "the mechanism is gone", not "the symptom is fixed".

Constraints on config.yaml

• run.sh pipes the file through envsubst, so no ${...} in the JavaScript — not even in a template literal with a plain identifier. Use string concatenation.
• All seven shots share one YAML anchor (&default_javascript). Editing it changes every page.
• A throw inside the new Promise(async ...) executor does not reject the promise; it leaves it unsettled until the 300-second page timeout, so every page fails. Anything that can throw needs to be either guarded or avoided. This is why the code uses getClientRects() and computed visibility rather than Element.checkVisibility(), whose option names were renamed between the pinned Chromium 113 and current ones.
• Prefer failing the shot over photographing a state you know is wrong; the diagnostics on the failure path are cheaper to read than a misleading image.
• Scope normalisations by selector, not by matching text anywhere in the page. "Today" and "Yesterday" occur in article prose, and rewriting those hides real changes.
• Avoid tunable thresholds. Every fix here keys off something the page itself asserts.
• Visible is not the same as in the picture. A topic page lays out ~100 chart thumbnails inside the key-charts carousel's scroller and shows a handful, so of /energy's 111 unloaded images 108 are visible and clipped away by a scrolling ancestor. isVisible is the right test for charts, whose absence is a diff wherever they sit; anything that waits for something needs isPainted, or it waits for a hundred pictures nobody will see. That distinction is why waiting for images was believed to hang the shot and why, scoped properly, it costs nothing.
• Anything that mutates the DOM before the capture can create the race it removes. Assigning img.src restarts the load algorithm even for the URL already displayed. A step like that belongs before the wait that absorbs it, not in the last block before the shot — and the order is not visible from either block on its own, so say so where it matters.

Catalogue

Fixed

Open

• A chart whose config fetch fails spins forever. Build 33174: eight HTTP 500/502 responses for /grapher/<slug>.config.json from the branch's own staging server, and no page error(s) — grapher swallowed all eight and left all eight graphers loading. This is an owid-grapher bug and should be filed: a failed config fetch should not leave a grapher in a permanent loading state.

• A chart that requests nothing at all. Build 33178: the Key Charts carousel on /energy (708x575 at y=6559) drawn nothing, with no failed request, nothing in flight and no page error. Not a network problem — the client decided not to load. GrapherWithFallback logs a console.error when handed neither a slug nor a configUrl, which the diagnostics do not yet capture; adding console.error to them is the cheapest next step.

Disproved, so don't re-derive it

• Machine contention during the build window. The screenshot step, the Cloudflare preview deploy and the BDD tests all hang off the bake and start within two seconds of each other, so every failure is inside that window — and so is every success. Build 33112 ran entirely inside the preview deploy and passed in 2 minutes; 33121 and 33124 are the same branch 50 minutes apart, same overlap, opposite outcomes. lxc-manager-1 has 100 cores. ops#645, closed.

• Waiting longer for images. Measured at the decode pass and at the capture, unthrottled and at ~250 kbps with 300 ms RTT: 26 of 66 complete, both times, every time. The chart settle wait already outlasts every image fetch.

• Retrying a page. On build 33124 the charts were still empty 108 seconds after every other step on the box had finished. Stuck, not queued — another attempt has nothing to gain.

Where the code lives