perf: draw the card grids from thumbnails - #406
nGervasyuk wants to merge 2 commits into
Conversation
Opening the variations dialog spent ~1.5s and ~7 MB on 68 requests pulling a full-size diff for each of 34 cards, only for CSS to shrink each one to about a hundred pixels wide. The card grid in the run list did the same. The API now stores a small copy of the checkpoint and of the diff beside each run, so the grids ask for those instead. Runs from before those existed, and runs with no saved diff, fall back to the full-size picture and look exactly as they do today. The lightbox behind a thumbnail keeps the full-size image: it is the picture the reviewer opened it to look at.
There was a problem hiding this comment.
🟡 Changes recommended
It introduces/extends a nullability mismatch around diffName (evidenced by null as never in tests) that should be resolved or consistently modeled to avoid masking real runtime contract issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR improves UI performance by rendering test-run card grids (run list and matching variations) from backend-provided thumbnail images instead of full-size screenshots/diffs, while preserving full-size images for the lightbox.
Changes:
- Extend
TestRunwith optionalimageThumbnailName/diffThumbnailNamefields. - Add
thumbnailFor()helper to prefer thumbnails for grids with a fallback toimageFor(). - Update both grid renderers to use
thumbnailFor()and add Jest coverage for thumbnail selection/fallback behavior.
File summaries
| File | Description |
|---|---|
| src/types/testRun.ts | Adds thumbnail name fields to the TestRun model. |
| src/components/TestRunList/TestRunGrid.tsx | Switches grid card images to use thumbnailFor() for thumbnail loading. |
| src/components/TestDetailsDialog/MatchingVariationsDialog.tsx | Switches variation thumbnails to use thumbnailFor() while keeping imageFor() for the lightbox. |
| src/_helpers/testRunImage.helper.ts | Introduces thumbnailFor() helper with fallback to full-size images. |
| src/_helpers/testRunImage.helper.test.ts | Adds tests validating thumbnail selection and fallback behavior. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| diffName: string; | ||
| // Small copies the API makes at ingest for the card grids to draw. Absent on | ||
| // runs ingested before they existed, and on runs with no saved diff. | ||
| imageThumbnailName?: string; | ||
| diffThumbnailName?: string; |
There was a problem hiding this comment.
Agreed, and done in 1c12cbd — this was the better half of the pair, so I took it rather than the empty-string workaround.
diffName has always arrived as null from the API: a run has no diff until it is compared, and none at all if it matched its baseline. Prisma has it as String? and the DTO narrowed it on the way out. The type was simply wrong, and being wrong is what pushed my test into a cast.
I measured the blast radius before committing, since a widened type usually is one: exactly three call sites objected, all of them places that already guarded — staticService.getImage opens with if (!name) return noImage, ImageDetails renders Loading... for a falsy name, DrawArea falls back to a placeholder. So it is four signature widenings and no behaviour change; tsc finishes on the same error count as before, the only difference being a pre-existing error whose line number moved by one.
I did not go further and tighten the call sites that assume diffName is present, as you suggested — those are pre-existing and unrelated to thumbnails, and this PR is already paired with a backend migration.
|
|
||
| it("falls back when the run has no diff at all", () => { | ||
| const testRun = run({ | ||
| diffName: null as never, |
There was a problem hiding this comment.
You are right that the cast was the problem, though I went the other way on the remedy.
null as never is a type-escape standing exactly where the interesting behaviour is, and an empty string would have swapped it for a case that never actually occurs — the API sends null here, not "". That would leave the real path untested while looking tidy.
Fixed in 1c12cbd by making the type honest instead: TestRun.diffName is now string | null, and the test passes a plain null. See the reply on the sibling thread for why that turned out to cost nothing.
`TestRun.diffName` was typed as a required string while the API has always sent null for it — a run has no diff until it has been compared, and none at all if it matched its baseline. The type simply disagreed with the payload, and my test for the no-diff case papered over that with `null as never`, which is exactly the kind of cast that hides the thing it is standing on. The type now admits null, and the cast is gone. Nothing needed a new guard: every place this touches already checked for a falsy name and rendered a placeholder or "Loading..." — `getImage`, `ImageDetails`, `DrawArea`. Only their signatures said otherwise, so this is four type widenings and no behaviour change. The two new thumbnail fields are typed the same way, for the same reason. Found by Copilot on Visual-Regression-Tracker#406.
|
Needs backend#377, which stores the thumbnails. Without it every run simply has none and the grids fall back to what they draw today, so the two can ship in either order — but this one does nothing until the backend is out.
Why
Opening the variations dialog spent ~1.5 s and ~7 MB across 68 requests pulling a full-size diff for each of 34 cards, only for CSS to shrink each one to roughly a hundred pixels wide. Two requests per card — the API redirect, then the S3 GET — at ~200 kB apiece. The card grid in the run list did the same, and with
Show diffoff it pulled 1.4 MB screenshots instead.What
A
thumbnailForhelper beside the existingimageFor: it asks for the small copy the API stored at ingest, and falls back to the full-size picture when there is none. Both grids use it.The lightbox behind a thumbnail deliberately keeps
imageFor— the full-size image is the thing the reviewer opened it to look at.Falling back
Runs ingested before thumbnails existed, and runs with no saved diff, have no small copy and render exactly as they do today. There is no probing and no broken image: the run either carries a name or it does not.
Tests
Four, watched failing first: the diff's small copy, the screenshot's small copy when
Show diffis off, the fallback when nothing is stored, and the fallback when the run has no diff at all — that last one matters because a run can carry a stalediffThumbnailNamewhilediffNameis null, and the grid must not ask for a picture of a diff it is not showing.jest 40/40, Playwright 93/93,
tscunchanged at its 30 pre-existing errors.