fix: photograph the same image every run - #28
Conversation
Waiting for images in #26 took five of six screenshots still, and left one moving by 227 pixels between two runs of one build. Two things were behind that, neither of them a wait. A srcSet is a set of permissions rather than an instruction. Asked for 108px with 192w and 384w on offer, Chromium took the 192 on some runs and the 384 on others, and the two downscale to slightly different pixels. Each image is now pinned to what the selection rule actually asks for — the smallest candidate covering the drawn size — before the shot. The compositor was free to vary too: which rasteriser drew an image, and whether it refined a decode after first paint. Those are pinned by flag. Sixteen runs of one build, ninety comparisons, no differing pixels. A two pixel shift injected into the layout is still seen, at thirty-five thousand pixels, so the pinning has not blinded the comparison.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe Storybook verification script now launches Chromium with GPU-related rasterization disabled and sRGB output forced. Before waiting for images to settle, Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![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: b43dab2c9a
ℹ️ 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".
The selector was written inline with no way to fail visibly: a parsing or threshold regression would have restored the flake while the run stayed green. It is a function now, with the boundary it turns on — a candidate that matches the drawn width exactly is wide enough — along with the fallback past the widest, descriptors the rule cannot read, and a width the layout has not resolved yet. The run also checks the pin took. Not by currentSrc alone: a URL that 404s still becomes the current source, so that test passes while the page shows nothing. It asserts the pinned file is the one showing and that it decoded, which is what fails when the rule starts producing nonsense.
Change
#26 made the browser check wait for images, which took the number of screenshots moving between two runs of one build from five to one. That last one,
buddy-map-390, kept differing by 227 pixels, and I closed that PR saying the flake was fixed. It was not.Two causes were left, neither of them a wait.
A
srcSetis a set of permissions, not an instruction. The Buddy portrait asks for108pxand offers192wand384w. Chromium took the 192 on some runs and the 384 on others — both drawn at 108 pixels, downscaled slightly differently, which is the whole of the difference. Nothing about the page changed between those runs; the choice is the browser's to make. Each responsive image is now pinned before the shot to what the selection rule actually asks for: the smallest candidate that covers the drawn size.The compositor was free to vary as well — which rasteriser drew an image, and whether it refined a decode after first paint. Those are pinned by flag: GPU rasterisation off, checker imaging and partial raster off, colour profile fixed to sRGB.
Validation
just checkpasses (exit 0).Stability, by running the check repeatedly against one unchanged build and comparing every screenshot to the first run:
The pinning has not blinded the comparison. A two-pixel shift injected into the layout is still caught, at 35,013 pixels on one screenshot and 70,736 on another. That was checked because pinning an image is the kind of normalisation that could quietly stop the check seeing things.
Each cause was identified by measurement rather than by trying flags. The candidate difference was found by logging
currentSrcat the moment of the screenshot and watching it alternate between the two files across runs; an earlier guess that the HTTP cache was choosing it was tested by disabling the cache, which changed nothing, and a second guess about the network-quality estimate was tested by pinning it, which left one run in five still on the wrong file.About the claim in #26
That PR reported eight runs and 42 comparisons at zero differing pixels. It was measured honestly and it did not generalise: the flake is intermittent, and eight runs in one worktree happened to miss it. This one is sixteen runs and ninety comparisons, which is better evidence but the same kind of evidence. If it moves again, the thing to do is log what the browser chose rather than reach for another flag.
Review follow-up
The selector was written inline in the page with nothing asserting its result, so a parsing or threshold regression could have restored the flake — or pinned an undersized file — while the run stayed green. Fair, and fixed two ways.
pickSourceis a function with its own tests: the narrowest candidate that covers the drawn width, the boundary it turns on (a candidate matching that width exactly is wide enough, one pixel more is not), the fallback past the widest, descriptors the rule cannot read (2x, a bare URL, a zero or negative width), an empty or non-string attribute, and a width the layout has not resolved yet — which falls back to the widest rather than pinning the smallest file on offer.The run then checks the pin took. The first version of that check asserted
currentSrcmatched and did not fail when I fed it a deliberately broken URL: a source that 404s still becomes the current source, so the assertion passed while the page showed nothing. It now requires the pinned file to be the one showing and to have decoded, which does fail on that fault — confirmed by injecting it.Output is unchanged by the refactor: the screenshots are byte-identical to the ones the inline version produced, and four further runs of the rebuilt harness agree across 18 comparisons.