From e165c856f84e6adb52b70f8b1b0c7dbf30b0e25e Mon Sep 17 00:00:00 2001 From: "mykola.gervasyuk" Date: Fri, 4 Sep 2026 09:36:44 +0300 Subject: [PATCH 1/7] perf: sign a run once at ingest instead of on every review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Measured on production: opening "Approve variations" spends ~4.5s in matchingSiblings before a single thumbnail is requested. For a screen with 34 locales that call fetches 70 full-size screenshots back out of S3 and decodes them, eight at a time. The concurrency bound added with the worker pool was chosen to protect memory and CPU; on S3 the work is network-bound, so eight is simply the number of round trips it waits on. None of that has to happen at review time. The diff already decodes both screenshots at ingest, so the signature is now produced by that same pass and stored on the run. Grouping a screen becomes a column read. - computePixelmatchDiff optionally returns the signature, computed from the images it has already decoded and ignore-masked. Both paths call one signatureOfDecoded, and a test pins the fused answer to be identical to the standalone job's — if they drifted, stored signatures would silently stop matching computed ones and variations would stop grouping. - It is asked for on every comparison rather than only when the project has bulk approve switched on, so turning the flag on later does not leave a build's runs unsigned. - saveDiffResult writes it, and always overwrites: recomputing a diff after the reviewer edits the ignore areas must not leave the old signature describing a change that no longer exists. - Runs with nothing stored — ingested before this, or compared by something other than pixelmatch — fall back to the existing worker-pool path, so no backfill is needed and old builds keep working. The column is nullable with no default, so the migration is catalogue-only: no table rewrite, no long lock on a TestRun table holding a hundred builds. --- .../migration.sql | 6 ++ prisma/schema.prisma | 9 +- src/_data_/index.ts | 1 + .../libs/pixelmatch/pixelmatch.core.spec.ts | 87 +++++++++++++++++++ .../libs/pixelmatch/pixelmatch.core.ts | 31 ++++++- .../pixelmatch/pixelmatch.service.spec.ts | 66 ++++++++++++++ .../libs/pixelmatch/pixelmatch.service.ts | 7 ++ src/compare/libs/pixelmatch/signature.core.ts | 24 +++-- src/test-runs/diffResult.ts | 8 ++ src/test-runs/test-runs.service.spec.ts | 80 +++++++++++++++++ src/test-runs/test-runs.service.ts | 37 +++++++- 11 files changed, 345 insertions(+), 11 deletions(-) create mode 100644 prisma/migrations/20260904120000_add_test_run_change_signature/migration.sql create mode 100644 src/compare/libs/pixelmatch/pixelmatch.core.spec.ts diff --git a/prisma/migrations/20260904120000_add_test_run_change_signature/migration.sql b/prisma/migrations/20260904120000_add_test_run_change_signature/migration.sql new file mode 100644 index 00000000..62cbaa8d --- /dev/null +++ b/prisma/migrations/20260904120000_add_test_run_change_signature/migration.sql @@ -0,0 +1,6 @@ +-- AlterTable +-- Nullable with no default, so this is a catalogue-only change on Postgres 11+: +-- no table rewrite and no long lock, which matters on a TestRun table holding +-- a hundred builds' worth of runs. Existing rows keep NULL and fall back to +-- computing the signature on demand. +ALTER TABLE "TestRun" ADD COLUMN "changeSignature" TEXT; diff --git a/prisma/schema.prisma b/prisma/schema.prisma index 4835aeae..104dd8aa 100644 --- a/prisma/schema.prisma +++ b/prisma/schema.prisma @@ -1,5 +1,5 @@ generator client { - provider = "prisma-client-js" + provider = "prisma-client-js" binaryTargets = ["native", "debian-openssl-3.0.x", "linux-arm64-openssl-3.0.x"] } @@ -72,6 +72,13 @@ model TestRun { ignoreAreas String @default("[]") tempIgnoreAreas String @default("[]") vlmDescription String? + // Position-independent colour signature of this run's change, as JSON, so the + // variations dialog can group a screen's locales without fetching and + // decoding every sibling's screenshots at review time. Written once, next to + // the diff that already decoded them. Null on runs that predate this, that + // have no diff, or that were compared by something other than pixelmatch — + // those fall back to computing it on demand. + changeSignature String? baseline Baseline? build Build @relation(fields: [buildId], references: [id]) project Project? @relation(fields: [projectId], references: [id]) diff --git a/src/_data_/index.ts b/src/_data_/index.ts index fcbb4df9..7fc3bc27 100644 --- a/src/_data_/index.ts +++ b/src/_data_/index.ts @@ -99,6 +99,7 @@ export const generateTestRun = (testRun?: Partial): TestRun => { branchName: 'develop', merge: false, vlmDescription: null, + changeSignature: null, ...testRun, }; }; diff --git a/src/compare/libs/pixelmatch/pixelmatch.core.spec.ts b/src/compare/libs/pixelmatch/pixelmatch.core.spec.ts new file mode 100644 index 00000000..5e9e47c9 --- /dev/null +++ b/src/compare/libs/pixelmatch/pixelmatch.core.spec.ts @@ -0,0 +1,87 @@ +import { PNG } from 'pngjs'; +import { computePixelmatchDiff } from './pixelmatch.core'; +import { computeChangeSignature } from './signature.core'; + +const WIDTH = 60; +const HEIGHT = 60; + +const png = (paint: (set: (x: number, y: number, rgb: [number, number, number]) => void) => void): Buffer => { + const image = new PNG({ width: WIDTH, height: HEIGHT }); + for (let i = 0; i < WIDTH * HEIGHT; i++) { + image.data[i * 4] = 255; + image.data[i * 4 + 1] = 255; + image.data[i * 4 + 2] = 255; + image.data[i * 4 + 3] = 255; + } + paint((x, y, [r, g, b]) => { + const index = (y * WIDTH + x) * 4; + image.data[index] = r; + image.data[index + 1] = g; + image.data[index + 2] = b; + }); + return PNG.sync.write(image); +}; + +const block = (left: number, top: number, rgb: [number, number, number]): Buffer => + png((set) => { + for (let y = top; y < top + 12; y++) { + for (let x = left; x < left + 12; x++) { + set(x, y, rgb); + } + } + }); + +const blank = png(() => undefined); +const RED: [number, number, number] = [255, 0, 0]; + +const diffJob = (baseline: Buffer, image: Buffer, withSignature: boolean) => + computePixelmatchDiff({ + kind: 'diff', + baseline, + image, + ignoreAreas: [], + threshold: 0.1, + includeAA: false, + allowDiffDimensions: false, + diffTolerancePercent: 0, + saveDiff: false, + withSignature, + }); + +describe('computePixelmatchDiff with a signature', () => { + // The whole point of computing it here is to reuse the decode the diff + // already paid for. If the fused version answered differently from the + // standalone one, stored signatures would stop matching computed ones and + // variations would silently stop grouping. + it('gives the same signature the standalone job would', () => { + const image = block(10, 10, RED); + + const fused = diffJob(blank, image, true).signature; + const standalone = computeChangeSignature({ + kind: 'signature', + baseline: blank, + image, + ignoreAreas: [], + threshold: 0.1, + includeAA: false, + }).signature; + + expect(fused).toEqual(standalone); + expect(fused).not.toBeNull(); + }); + + it('leaves the signature out when it was not asked for', () => { + expect(diffJob(blank, block(10, 10, RED), false).signature).toBeUndefined(); + }); + + it('has no signature when the screenshots are identical', () => { + expect(diffJob(blank, blank, true).signature).toBeUndefined(); + }); + + it('still reports the diff it was asked for', () => { + const result = diffJob(blank, block(10, 10, RED), true); + + expect(result.equal).toBe(false); + expect(result.pixelMisMatchCount).toBe(144); + }); +}); diff --git a/src/compare/libs/pixelmatch/pixelmatch.core.ts b/src/compare/libs/pixelmatch/pixelmatch.core.ts index 47bbc71e..9dc12968 100644 --- a/src/compare/libs/pixelmatch/pixelmatch.core.ts +++ b/src/compare/libs/pixelmatch/pixelmatch.core.ts @@ -2,6 +2,7 @@ import { PNG } from 'pngjs'; import Pixelmatch from 'pixelmatch'; import { IgnoreAreaDto } from '../../../test-runs/dto/ignore-area.dto'; import { applyIgnoreAreas, scaleImageToSize } from '../../utils'; +import { signatureOfDecoded } from './signature.core'; /** * CPU-bound part of the pixelmatch comparison, extracted so it can run inside @@ -20,6 +21,13 @@ export interface PixelmatchJobInput { allowDiffDimensions: boolean; diffTolerancePercent: number; saveDiff: boolean; + /** + * Also produce the change signature the variations dialog matches on. Free + * here next to the diff — the images are decoded already — and computed once + * at ingest so reviewing never has to fetch and decode a build's screenshots + * again just to group them. + */ + withSignature?: boolean; } export interface PixelmatchJobOutput { @@ -28,6 +36,9 @@ export interface PixelmatchJobOutput { pixelMisMatchCount?: number; diffPercent?: number; diffBuffer?: Buffer | Uint8Array; + // absent when not asked for, when the screenshots are identical, or when + // their dimensions differ and there is nothing meaningful to sign + signature?: number[]; } // postMessage turns Buffers into Uint8Array views over their own ArrayBuffer. @@ -74,5 +85,23 @@ export function computePixelmatchDiff(input: PixelmatchJobInput): PixelmatchJobO diffBuffer = PNG.sync.write(diff); } - return { equal: false, isSameDimension, pixelMisMatchCount, diffPercent, diffBuffer }; + // Same source images, same ignore areas, same threshold as the standalone + // signature job — sharing signatureOfDecoded is what keeps the two answers + // identical, so a stored signature still matches one computed on the fly. + const signature = + input.withSignature && isSameDimension + ? signatureOfDecoded(baselineIgnored, imageIgnored, { + threshold: input.threshold, + includeAA: input.includeAA, + }) + : null; + + return { + equal: false, + isSameDimension, + pixelMisMatchCount, + diffPercent, + diffBuffer, + ...(signature ? { signature } : {}), + }; } diff --git a/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts b/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts index 1696b97d..2ad99a9f 100644 --- a/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts +++ b/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts @@ -257,3 +257,69 @@ describe('getDiff', () => { }); }); }); + +describe('change signature', () => { + // Correctness of the signature itself is pinned in pixelmatch.core.spec + // against the real pixelmatch; Pixelmatch is mocked in this file, so what is + // worth testing here is only the plumbing — that the service asks for one and + // carries what comes back into the result. + const initWithPool = async (output: Record) => { + const run = jest.fn().mockResolvedValue(output); + const module: TestingModule = await Test.createTestingModule({ + providers: [ + PixelmatchService, + { provide: DiffWorkerPool, useValue: { run } }, + { + provide: StaticService, + useValue: { + getImageBuffer: jest.fn().mockResolvedValue(Buffer.from('png')), + saveImage: jest.fn(), + deleteImage: jest.fn(), + }, + }, + ], + }).compile(); + return { service: module.get(PixelmatchService), run }; + }; + + const compare = (service: PixelmatchService) => + service.getDiff( + { + baseline: 'baseline.png', + image: 'image.png', + ignoreAreas: [], + diffTollerancePercent: 0, + saveDiffAsFile: false, + }, + DEFAULT_CONFIG + ); + + // Asked for on every comparison, not only when the project has bulk approve + // switched on: computing it lazily would leave every run ingested before the + // flag was turned on without one. + it('asks for a signature and carries it into the result', async () => { + const { service, run } = await initWithPool({ + equal: false, + isSameDimension: true, + pixelMisMatchCount: 10, + diffPercent: 5, + signature: [0.25, 0.75], + }); + + const result = await compare(service); + + expect(run).toHaveBeenCalledWith(expect.objectContaining({ kind: 'diff', withSignature: true })); + expect(result.changeSignature).toEqual([0.25, 0.75]); + }); + + it('leaves it out when the comparison produced none', async () => { + const { service } = await initWithPool({ + equal: false, + isSameDimension: true, + pixelMisMatchCount: 10, + diffPercent: 5, + }); + + expect((await compare(service)).changeSignature).toBeUndefined(); + }); +}); diff --git a/src/compare/libs/pixelmatch/pixelmatch.service.ts b/src/compare/libs/pixelmatch/pixelmatch.service.ts index 0cb9c800..a5092f79 100644 --- a/src/compare/libs/pixelmatch/pixelmatch.service.ts +++ b/src/compare/libs/pixelmatch/pixelmatch.service.ts @@ -45,6 +45,12 @@ export class PixelmatchService implements ImageComparator { allowDiffDimensions: config.allowDiffDimensions, diffTolerancePercent: data.diffTollerancePercent, saveDiff: data.saveDiffAsFile, + // Asked for on every comparison, not only when the project has bulk + // approve of variations switched on: the pass costs little beside the + // full-size diff that has already been paid for, and computing it lazily + // would leave every run ingested before the flag was turned on without + // one — which is exactly the build someone then tries to review. + withSignature: true, }); if (output.equal) { @@ -59,6 +65,7 @@ export class PixelmatchService implements ImageComparator { isSameDimension: output.isSameDimension, pixelMisMatchCount: output.pixelMisMatchCount, diffPercent: output.diffPercent, + ...(output.signature ? { changeSignature: output.signature } : {}), }; if (result.diffPercent > data.diffTollerancePercent) { diff --git a/src/compare/libs/pixelmatch/signature.core.ts b/src/compare/libs/pixelmatch/signature.core.ts index 9909354c..a2701fdb 100644 --- a/src/compare/libs/pixelmatch/signature.core.ts +++ b/src/compare/libs/pixelmatch/signature.core.ts @@ -106,17 +106,31 @@ export function computeChangeSignature(input: SignatureJobInput): SignatureJobOu applyIgnoreAreas(baselineImage, input.ignoreAreas); applyIgnoreAreas(checkpointImage, input.ignoreAreas); + return { signature: signatureOfDecoded(baselineImage, checkpointImage, input) }; +} + +/** + * The signature of a pair that is already decoded, of equal size, and with its + * ignore areas blanked. Split out so the diff can produce a signature from the + * images it has just decoded rather than paying for a second decode — the two + * must not drift apart, so there is only ever one implementation. + */ +export function signatureOfDecoded( + baselineImage: RawImage, + checkpointImage: RawImage, + options: { threshold: number; includeAA: boolean } +): number[] | null { const baseline = downscale(baselineImage, SIGNATURE_MAX_DIMENSION); const image = downscale(checkpointImage, SIGNATURE_MAX_DIMENSION); const { width, height } = baseline; const mask = new PNG({ width, height }); const changedPixels = Pixelmatch(baseline.data, image.data, mask.data, width, height, { - threshold: input.threshold, - includeAA: input.includeAA, + threshold: options.threshold, + includeAA: options.includeAA, diffMask: true, }); if (changedPixels === 0) { - return { signature: null }; + return null; } const bucketSize = 256 / COLOR_BUCKETS_PER_CHANNEL; @@ -133,9 +147,9 @@ export function computeChangeSignature(input: SignatureJobInput): SignatureJobOu const total = histogram.reduce((sum, value) => sum + value, 0); if (total === 0) { - return { signature: null }; + return null; } - return { signature: histogram.map((value) => value / total) }; + return histogram.map((value) => value / total); } function cosineSimilarity(a: number[], b: number[]): number { diff --git a/src/test-runs/diffResult.ts b/src/test-runs/diffResult.ts index b4219595..239a092a 100644 --- a/src/test-runs/diffResult.ts +++ b/src/test-runs/diffResult.ts @@ -12,4 +12,12 @@ export interface DiffResult { * Can be displayed as bullet points in UI */ vlmDescription?: string; + /** + * Position-independent colour signature of the change, produced by the same + * pass that produced the diff. Stored on the run so the variations dialog can + * group a screen's locales without decoding their screenshots all over again. + * Absent when nothing changed, when the dimensions differ, or when the + * comparison was not pixelmatch. + */ + changeSignature?: number[]; } diff --git a/src/test-runs/test-runs.service.spec.ts b/src/test-runs/test-runs.service.spec.ts index 57d2da60..14571c3d 100644 --- a/src/test-runs/test-runs.service.spec.ts +++ b/src/test-runs/test-runs.service.spec.ts @@ -350,11 +350,47 @@ describe('TestRunsService', () => { pixelMisMatchCount: null, diffPercent: null, vlmDescription: null, + changeSignature: null, }, }); expect(eventTestRunUpdatedMock).toHaveBeenCalledWith(testRun); }); + // Written here, next to the diff that decoded the screenshots anyway, so + // the variations dialog never has to decode them again. + it('stores the change signature the comparison produced', async () => { + const testRunUpdateMock = jest.fn().mockResolvedValueOnce(testRun); + service = await initService({ testRunUpdateMock }); + + await service.saveDiffResult('some id', { + status: TestStatus.unresolved, + diffName: 'diff image name', + pixelMisMatchCount: 11, + diffPercent: 22, + isSameDimension: true, + changeSignature: [0.25, 0.75], + }); + + expect(testRunUpdateMock.mock.calls[0][0].data).toMatchObject({ + changeSignature: JSON.stringify([0.25, 0.75]), + }); + }); + + it('stores no signature when the comparison produced none', async () => { + const testRunUpdateMock = jest.fn().mockResolvedValueOnce(testRun); + service = await initService({ testRunUpdateMock }); + + await service.saveDiffResult('some id', { + status: TestStatus.ok, + diffName: null, + pixelMisMatchCount: 0, + diffPercent: 0, + isSameDimension: true, + }); + + expect(testRunUpdateMock.mock.calls[0][0].data).toMatchObject({ changeSignature: null }); + }); + it('with results', async () => { const diff: DiffResult = { status: TestStatus.unresolved, @@ -382,6 +418,7 @@ describe('TestRunsService', () => { pixelMisMatchCount: diff.pixelMisMatchCount, diffPercent: diff.diffPercent, vlmDescription: diff.vlmDescription, + changeSignature: null, }, }); expect(eventTestRunUpdatedMock).toHaveBeenCalledWith(testRun); @@ -774,6 +811,49 @@ describe('TestRunsService', () => { ]); }); + // The whole point of storing it: a build's screenshots are fetched and + // decoded once at ingest, never again to group a screen at review time. + it('groups from the signatures stored at ingest, without decoding anything', async () => { + const signed = (id: string, signature: number[], overrides = {}) => + sibling(id, { changeSignature: JSON.stringify(signature), ...overrides }); + const testRun = signed('target', SAME_PALETTE, { customTags: '' }); + const matching = signed('locale-a', SAME_PALETTE); + const different = signed('locale-c', OTHER_PALETTE); + + const built = await initMatchingService({ + testRun, + siblings: [matching, different], + signatureOf: () => { + throw new Error('should not be computing a signature'); + }, + }); + + const result = await built.service.getMatchingVariations(testRun.id); + + expect(built.compareGetChangeSignatureMock).not.toHaveBeenCalled(); + expect(result.variations.map((variation) => variation.id)).toEqual([testRun.id, matching.id]); + expect(result.skipped.map((item) => item.reason)).toEqual(['different change pattern']); + }); + + // builds ingested before the column existed, and runs compared by something + // other than pixelmatch, still have to group + it('falls back to computing for a run with nothing stored', async () => { + const testRun = sibling('target', { customTags: '', changeSignature: JSON.stringify(SAME_PALETTE) }); + const unsigned = sibling('locale-a'); + + const built = await initMatchingService({ + testRun, + siblings: [unsigned], + signatureOf: () => SAME_PALETTE, + }); + + const result = await built.service.getMatchingVariations(testRun.id); + + const asked = built.compareGetChangeSignatureMock.mock.calls.map(([input]) => input.image.toString()); + expect(asked).toEqual([unsigned.imageName]); + expect(result.variations.map((variation) => variation.id)).toEqual([testRun.id, unsigned.id]); + }); + it('skips a far larger change without paying for its signature', async () => { const testRun = sibling('target', { customTags: '' }); const bigger = sibling('locale-b', { diffPercent: 30 }); diff --git a/src/test-runs/test-runs.service.ts b/src/test-runs/test-runs.service.ts index 2104e37f..d97166a7 100644 --- a/src/test-runs/test-runs.service.ts +++ b/src/test-runs/test-runs.service.ts @@ -292,13 +292,21 @@ export class TestRunsService { /** * Position-independent color signature of the change between a test run's * baseline and image. Null when there is no baseline, dimensions differ, or - * nothing changed. The bytes go to the worker pool undecoded — decoding is - * the expensive part and must not happen on the event loop. + * nothing changed. * - * Memoized: a reviewer who reopens the variations dialog, or steps back to a - * screen already looked at, would otherwise pay for the same decodes again. + * Normally this was written at ingest, beside the diff that had already + * decoded both screenshots, and grouping a screen costs nothing but the row + * it is read from. The rest of this is the fallback for runs that carry no + * stored signature — ingested before the column existed, or compared by + * something other than pixelmatch: their bytes go to the worker pool + * undecoded, and the result is memoized so reopening the dialog on an old + * build does not pay for the same decodes twice. */ private async getChangeSignature(testRun: TestRun, config: PixelmatchConfig): Promise { + const stored = parseStoredSignature(testRun.changeSignature, this.logger); + if (stored) { + return stored; + } if (!testRun.baselineName) { return null; } @@ -368,6 +376,10 @@ export class TestRunsService { diffPercent: diffResult && diffResult.diffPercent, status: diffResult ? diffResult.status : TestStatus.new, vlmDescription: diffResult && diffResult?.vlmDescription, + // Always written, never merged: a recomputed diff — after the + // reviewer edits the ignore areas, say — must not leave the previous + // signature behind describing a change that no longer exists. + changeSignature: diffResult?.changeSignature ? JSON.stringify(diffResult.changeSignature) : null, }, }) .then((testRun) => { @@ -676,6 +688,23 @@ async function mapWithConcurrency(items: T[], limit: number, task: (item: return results; } +/** + * The signature stored on a run, or null when there is none to read. Bad JSON + * is not worth failing a review over: the caller falls back to computing it. + */ +function parseStoredSignature(stored: string | null | undefined, logger: Logger): number[] | null { + if (!stored) { + return null; + } + try { + const parsed = JSON.parse(stored); + return Array.isArray(parsed) && parsed.length > 0 ? parsed : null; + } catch (error) { + logger.warn(`Ignoring unreadable stored change signature: ${error}`); + return null; + } +} + // Same palette but a much larger/smaller change area signals a different or // additional change (an extra element changed too), not just per-locale text // reflow — which stays well under this ratio. Such variations go to manual review. From 1b637cb2bf171d583f188abf48b4a5e72f6fe20c Mon Sep 17 00:00:00 2001 From: "mykola.gervasyuk" Date: Fri, 4 Sep 2026 11:21:02 +0300 Subject: [PATCH 2/7] fix: discard a stored signature when the comparison settings move on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A signature only means anything under the threshold it was computed with. The in-memory memo has always keyed on threshold and includeAA; when the same value started being persisted, that guard was not carried over. So a project whose imageComparisonConfig is edited after a build was ingested could compare a stored signature, taken under the old settings, against a sibling's computed fresh under the new ones. Nothing breaks loudly: variations simply stop grouping as well as they did, and the reviewer has no way to tell why. The settings now travel with the signature, and a stored one whose settings no longer match the project's is discarded — which falls back to computing it, exactly as for a run that never had one. The optimisation now knows what it was computed under. Found by CodeRabbit on #376. --- .../pixelmatch/pixelmatch.service.spec.ts | 8 ++++- .../libs/pixelmatch/pixelmatch.service.ts | 10 +++++- src/test-runs/diffResult.ts | 23 +++++++++--- src/test-runs/test-runs.service.spec.ts | 36 ++++++++++++++++--- src/test-runs/test-runs.service.ts | 28 +++++++++++---- 5 files changed, 87 insertions(+), 18 deletions(-) diff --git a/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts b/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts index 2ad99a9f..2a659f40 100644 --- a/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts +++ b/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts @@ -309,7 +309,13 @@ describe('change signature', () => { const result = await compare(service); expect(run).toHaveBeenCalledWith(expect.objectContaining({ kind: 'diff', withSignature: true })); - expect(result.changeSignature).toEqual([0.25, 0.75]); + // stamped with the settings it was computed under, so a stored one can be + // discarded when the project's config moves on + expect(result.changeSignature).toEqual({ + threshold: DEFAULT_CONFIG.threshold, + includeAA: DEFAULT_CONFIG.ignoreAntialiasing, + signature: [0.25, 0.75], + }); }); it('leaves it out when the comparison produced none', async () => { diff --git a/src/compare/libs/pixelmatch/pixelmatch.service.ts b/src/compare/libs/pixelmatch/pixelmatch.service.ts index a5092f79..55b184a8 100644 --- a/src/compare/libs/pixelmatch/pixelmatch.service.ts +++ b/src/compare/libs/pixelmatch/pixelmatch.service.ts @@ -65,7 +65,15 @@ export class PixelmatchService implements ImageComparator { isSameDimension: output.isSameDimension, pixelMisMatchCount: output.pixelMisMatchCount, diffPercent: output.diffPercent, - ...(output.signature ? { changeSignature: output.signature } : {}), + ...(output.signature + ? { + changeSignature: { + threshold: config.threshold, + includeAA: config.ignoreAntialiasing, + signature: output.signature, + }, + } + : {}), }; if (result.diffPercent > data.diffTollerancePercent) { diff --git a/src/test-runs/diffResult.ts b/src/test-runs/diffResult.ts index 239a092a..c9bfce15 100644 --- a/src/test-runs/diffResult.ts +++ b/src/test-runs/diffResult.ts @@ -1,5 +1,17 @@ import { TestStatus } from '@prisma/client'; +/** + * A signature and the comparison settings it was computed under. A signature + * only means anything under its own threshold, so comparing one taken at a + * different setting quietly makes grouping worse — the settings travel with it + * so a stored one can be discarded when the project's config moves on. + */ +export interface StampedSignature { + threshold: number; + includeAA: boolean; + signature: number[]; +} + export interface DiffResult { status: TestStatus; diffName: string; @@ -14,10 +26,11 @@ export interface DiffResult { vlmDescription?: string; /** * Position-independent colour signature of the change, produced by the same - * pass that produced the diff. Stored on the run so the variations dialog can - * group a screen's locales without decoding their screenshots all over again. - * Absent when nothing changed, when the dimensions differ, or when the - * comparison was not pixelmatch. + * pass that produced the diff, carrying the settings it was produced under. + * Stored on the run so the variations dialog can group a screen's locales + * without decoding their screenshots all over again. Absent when nothing + * changed, when the dimensions differ, or when the comparison was not + * pixelmatch. */ - changeSignature?: number[]; + changeSignature?: StampedSignature; } diff --git a/src/test-runs/test-runs.service.spec.ts b/src/test-runs/test-runs.service.spec.ts index 14571c3d..4ed710e3 100644 --- a/src/test-runs/test-runs.service.spec.ts +++ b/src/test-runs/test-runs.service.spec.ts @@ -368,11 +368,11 @@ describe('TestRunsService', () => { pixelMisMatchCount: 11, diffPercent: 22, isSameDimension: true, - changeSignature: [0.25, 0.75], + changeSignature: { threshold: 0.1, includeAA: true, signature: [0.25, 0.75] }, }); expect(testRunUpdateMock.mock.calls[0][0].data).toMatchObject({ - changeSignature: JSON.stringify([0.25, 0.75]), + changeSignature: JSON.stringify({ threshold: 0.1, includeAA: true, signature: [0.25, 0.75] }), }); }); @@ -815,7 +815,10 @@ describe('TestRunsService', () => { // decoded once at ingest, never again to group a screen at review time. it('groups from the signatures stored at ingest, without decoding anything', async () => { const signed = (id: string, signature: number[], overrides = {}) => - sibling(id, { changeSignature: JSON.stringify(signature), ...overrides }); + sibling(id, { + changeSignature: JSON.stringify({ threshold: 0.1, includeAA: true, signature }), + ...overrides, + }); const testRun = signed('target', SAME_PALETTE, { customTags: '' }); const matching = signed('locale-a', SAME_PALETTE); const different = signed('locale-c', OTHER_PALETTE); @@ -835,10 +838,35 @@ describe('TestRunsService', () => { expect(result.skipped.map((item) => item.reason)).toEqual(['different change pattern']); }); + // A signature only means anything under the threshold it was computed with. + // The in-memory memo has always keyed on that; the stored one has to too, + // or changing a project's comparison config silently mixes signatures from + // two different settings and grouping quietly gets worse. + it('recomputes when the project config has moved on since ingest', async () => { + const stale = JSON.stringify({ threshold: 0.9, includeAA: false, signature: SAME_PALETTE }); + const testRun = sibling('target', { customTags: '', changeSignature: stale }); + const matching = sibling('locale-a', { changeSignature: stale }); + + const built = await initMatchingService({ + testRun, + siblings: [matching], + signatureOf: () => SAME_PALETTE, + }); + + const result = await built.service.getMatchingVariations(testRun.id); + + const asked = built.compareGetChangeSignatureMock.mock.calls.map(([input]) => input.image.toString()); + expect(asked.sort()).toEqual([testRun.imageName, matching.imageName].sort()); + expect(result.variations.map((variation) => variation.id)).toEqual([testRun.id, matching.id]); + }); + // builds ingested before the column existed, and runs compared by something // other than pixelmatch, still have to group it('falls back to computing for a run with nothing stored', async () => { - const testRun = sibling('target', { customTags: '', changeSignature: JSON.stringify(SAME_PALETTE) }); + const testRun = sibling('target', { + customTags: '', + changeSignature: JSON.stringify({ threshold: 0.1, includeAA: true, signature: SAME_PALETTE }), + }); const unsigned = sibling('locale-a'); const built = await initMatchingService({ diff --git a/src/test-runs/test-runs.service.ts b/src/test-runs/test-runs.service.ts index d97166a7..d1cb8c8a 100644 --- a/src/test-runs/test-runs.service.ts +++ b/src/test-runs/test-runs.service.ts @@ -4,7 +4,7 @@ import { IgnoreAreaDto } from './dto/ignore-area.dto'; import { StaticService } from '../static/static.service'; import { PrismaService } from '../prisma/prisma.service'; import { Baseline, Prisma, TestRun, TestStatus, TestVariation } from '@prisma/client'; -import { DiffResult } from './diffResult'; +import { DiffResult, StampedSignature } from './diffResult'; import { EventsGateway } from '../shared/events/events.gateway'; import { TestRunResultDto } from '../test-runs/dto/testRunResult.dto'; import { TestVariationsService } from '../test-variations/test-variations.service'; @@ -303,7 +303,7 @@ export class TestRunsService { * build does not pay for the same decodes twice. */ private async getChangeSignature(testRun: TestRun, config: PixelmatchConfig): Promise { - const stored = parseStoredSignature(testRun.changeSignature, this.logger); + const stored = parseStoredSignature(testRun.changeSignature, config, this.logger); if (stored) { return stored; } @@ -689,16 +689,30 @@ async function mapWithConcurrency(items: T[], limit: number, task: (item: } /** - * The signature stored on a run, or null when there is none to read. Bad JSON - * is not worth failing a review over: the caller falls back to computing it. + * The signature stored on a run, if it is still usable. Null — meaning + * "recompute" — when there is none, when it cannot be read, or when the + * project's comparison settings have moved on since it was written. + * + * That last case is the point. Comparing a signature taken at one threshold + * against a sibling's taken at another quietly makes grouping worse, and the + * reviewer has no way to tell why. The in-memory memo has always keyed on the + * same settings; this is the stored equivalent. */ -function parseStoredSignature(stored: string | null | undefined, logger: Logger): number[] | null { +function parseStoredSignature( + stored: string | null | undefined, + config: PixelmatchConfig, + logger: Logger +): number[] | null { if (!stored) { return null; } try { - const parsed = JSON.parse(stored); - return Array.isArray(parsed) && parsed.length > 0 ? parsed : null; + const parsed: StampedSignature = JSON.parse(stored); + if (!Array.isArray(parsed?.signature) || parsed.signature.length === 0) { + return null; + } + const sameConfig = parsed.threshold === config.threshold && parsed.includeAA === config.ignoreAntialiasing; + return sameConfig ? parsed.signature : null; } catch (error) { logger.warn(`Ignoring unreadable stored change signature: ${error}`); return null; From b9b7c423db7a38a1fbab545d99302974ae5c19b7 Mon Sep 17 00:00:00 2001 From: "mykola.gervasyuk" Date: Mon, 7 Sep 2026 12:14:56 +0300 Subject: [PATCH 3/7] fix: only trust a stored signature that still has the right shape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A signature is a fixed-length histogram, and parseStoredSignature accepted any non-empty array as one. Comparing a stored vector of one length against a current one of another walks off the end of the shorter and scores the similarity over undefined entries — it returns a number, and the number means nothing. Nothing throws; grouping just quietly gets worse. That is the same failure the config stamp was added for, one level down: if COLOR_BUCKETS_PER_CHANNEL is ever changed, every signature already in the database becomes the wrong shape. The length and element types are now checked against what this build produces, so a stored signature that no longer fits is recomputed instead of half-used. Also types saveDiffResult's parameter as nullable. It genuinely is — a comparison that produced nothing clears the row back to "new", the body has always handled that, and a test passes null — but the signature claimed otherwise, which is the sort of small lie that invites a real one later. Both found by Copilot on #376. --- src/compare/libs/pixelmatch/signature.core.ts | 6 ++++ src/test-runs/test-runs.service.spec.ts | 31 +++++++++++++++++-- src/test-runs/test-runs.service.ts | 16 ++++++++-- 3 files changed, 48 insertions(+), 5 deletions(-) diff --git a/src/compare/libs/pixelmatch/signature.core.ts b/src/compare/libs/pixelmatch/signature.core.ts index a2701fdb..822208d2 100644 --- a/src/compare/libs/pixelmatch/signature.core.ts +++ b/src/compare/libs/pixelmatch/signature.core.ts @@ -30,6 +30,12 @@ export interface SignatureJobOutput { // COLOR_BUCKETS_PER_CHANNEL^3 histogram buckets. const COLOR_BUCKETS_PER_CHANNEL = 4; +// How many buckets a signature has. Exported so a stored one can be checked +// against the shape this build produces: comparing signatures of two different +// lengths yields a similarity score computed over undefined entries — a number, +// and a meaningless one. +export const SIGNATURE_LENGTH = COLOR_BUCKETS_PER_CHANNEL ** 3; + // Longest side (px) images are downscaled to before computing the color // signature — keeps the histogram representative while cutting pixelmatch cost. const SIGNATURE_MAX_DIMENSION = 500; diff --git a/src/test-runs/test-runs.service.spec.ts b/src/test-runs/test-runs.service.spec.ts index 4ed710e3..10a4f62e 100644 --- a/src/test-runs/test-runs.service.spec.ts +++ b/src/test-runs/test-runs.service.spec.ts @@ -1,6 +1,7 @@ import { mocked } from 'jest-mock'; import { Test, TestingModule } from '@nestjs/testing'; import { SIGNATURE_CONCURRENCY, TestRunsService } from './test-runs.service'; +import { SIGNATURE_LENGTH } from '../compare/libs/pixelmatch/signature.core'; import { PrismaService } from '../prisma/prisma.service'; import { StaticService } from '../static/static.service'; import { TestStatus, TestRun, TestVariation } from '@prisma/client'; @@ -747,8 +748,12 @@ describe('TestRunsService', () => { }); describe('getMatchingVariations', () => { - const SAME_PALETTE = [1, 0]; - const OTHER_PALETTE = [0, 1]; + // Full-length histograms, because a stored one is only reused when its shape + // matches what this build produces. Orthogonal, so they never match. + const histogram = (hot: number): number[] => + Array.from({ length: SIGNATURE_LENGTH }, (_unused, index) => (index === hot ? 1 : 0)); + const SAME_PALETTE = histogram(0); + const OTHER_PALETTE = histogram(SIGNATURE_LENGTH - 1); // A sibling of the reviewed screen. imageName doubles as the run's identity // in the signature mocks below, which see buffers rather than runs. @@ -860,6 +865,28 @@ describe('TestRunsService', () => { expect(result.variations.map((variation) => variation.id)).toEqual([testRun.id, matching.id]); }); + // A signature is a fixed-length histogram. One of the wrong length compared + // against one of the right length gives a similarity score computed over + // undefined entries — a number, silently meaningless. The same self- + // invalidating guard as the config stamp: if the shape moves on, recompute. + it.each([ + ['the wrong length', [1, 0]], + ['values that are not numbers', new Array(SIGNATURE_LENGTH).fill('nope')], + ])('recomputes a stored signature with %s', async (_case, signature) => { + const stored = JSON.stringify({ threshold: 0.1, includeAA: true, signature }); + const testRun = sibling('target', { customTags: '', changeSignature: stored }); + + const built = await initMatchingService({ + testRun, + siblings: [], + signatureOf: () => SAME_PALETTE, + }); + + await built.service.getMatchingVariations(testRun.id); + + expect(built.compareGetChangeSignatureMock).toHaveBeenCalled(); + }); + // builds ingested before the column existed, and runs compared by something // other than pixelmatch, still have to group it('falls back to computing for a run with nothing stored', async () => { diff --git a/src/test-runs/test-runs.service.ts b/src/test-runs/test-runs.service.ts index d1cb8c8a..423a6705 100644 --- a/src/test-runs/test-runs.service.ts +++ b/src/test-runs/test-runs.service.ts @@ -15,7 +15,7 @@ import { UpdateTestRunDto } from './dto/update-test.dto'; import { parseConfig } from '../compare/utils'; import { DEFAULT_CONFIG } from '../compare/libs/pixelmatch/pixelmatch.service'; import { PixelmatchConfig } from '../compare/libs/pixelmatch/pixelmatch.types'; -import { signaturesMatch } from '../compare/libs/pixelmatch/signature.core'; +import { signaturesMatch, SIGNATURE_LENGTH } from '../compare/libs/pixelmatch/signature.core'; @Injectable() export class TestRunsService { @@ -366,7 +366,10 @@ export class TestRunsService { return this.findOne(id); } - async saveDiffResult(id: string, diffResult: DiffResult): Promise { + // Nullable because it genuinely is: a comparison that produced nothing at all + // clears the row back to "new". The body has always handled that; the + // signature just did not say so. + async saveDiffResult(id: string, diffResult: DiffResult | null): Promise { return this.prismaService.testRun .update({ where: { id }, @@ -708,7 +711,14 @@ function parseStoredSignature( } try { const parsed: StampedSignature = JSON.parse(stored); - if (!Array.isArray(parsed?.signature) || parsed.signature.length === 0) { + // Shape first: a histogram of the wrong length, or one carrying anything + // that is not a number, compares against a current signature to produce a + // score that looks fine and means nothing. Recomputing is always safe. + const wellFormed = + Array.isArray(parsed?.signature) && + parsed.signature.length === SIGNATURE_LENGTH && + parsed.signature.every((value) => typeof value === 'number' && Number.isFinite(value)); + if (!wellFormed) { return null; } const sameConfig = parsed.threshold === config.threshold && parsed.includeAA === config.ignoreAntialiasing; From 86f79ecf8a8bdbef5ab541f0096e0348a4f9aad6 Mon Sep 17 00:00:00 2001 From: "mykola.gervasyuk" Date: Fri, 4 Sep 2026 11:09:20 +0300 Subject: [PATCH 4/7] perf: draw the grids from thumbnails made at ingest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The variations dialog spends ~1.5s and ~7 MB on 68 requests after the matching call returns, pulling a full-size diff for each of 34 cards and letting CSS shrink it to about a hundred pixels wide. The card grid in the list does the same. The comparison already has both pictures decoded, and already draws the diff, so the small copies are made there and stored beside the run: - computePixelmatchDiff optionally returns a small PNG of the checkpoint and of the diff, resized with the same downscale the signature uses — moved to compare/utils so there is one implementation rather than two. - They are saved on the same condition as the diff itself. A thumbnail of a diff that was never saved would point at a file that does not exist. - Stored as ordinary images, so they follow the same storage, the same URLs and the same deletion as everything else, with no naming convention for the UI to guess at and nothing to probe for when one is missing. - delete() removes them with the run. Two extra objects per run left behind would outlive every build that referenced them, and nothing collects them. Runs from before this have no thumbnail and fall back to the full-size file, so old builds look exactly as they do now and no backfill is needed. The migration adds two nullable columns with no default: catalogue-only on Postgres 11+, no table rewrite, no long lock. --- .../migration.sql | 6 +++ prisma/schema.prisma | 7 +++ src/_data_/index.ts | 2 + .../libs/pixelmatch/pixelmatch.core.spec.ts | 29 ++++++++++- .../libs/pixelmatch/pixelmatch.core.ts | 16 ++++++ .../pixelmatch/pixelmatch.service.spec.ts | 43 +++++++++++++++- .../libs/pixelmatch/pixelmatch.service.ts | 18 +++++++ src/compare/libs/pixelmatch/signature.core.ts | 34 +------------ .../libs/pixelmatch/thumbnail.core.spec.ts | 46 +++++++++++++++++ src/compare/libs/pixelmatch/thumbnail.core.ts | 25 ++++++++++ src/compare/utils/index.ts | 31 ++++++++++++ src/test-runs/diffResult.ts | 7 +++ src/test-runs/dto/testRun.dto.ts | 5 ++ src/test-runs/test-runs.service.spec.ts | 49 +++++++++++++++++++ src/test-runs/test-runs.service.ts | 8 +++ 15 files changed, 291 insertions(+), 35 deletions(-) create mode 100644 prisma/migrations/20260904140000_add_test_run_thumbnails/migration.sql create mode 100644 src/compare/libs/pixelmatch/thumbnail.core.spec.ts create mode 100644 src/compare/libs/pixelmatch/thumbnail.core.ts diff --git a/prisma/migrations/20260904140000_add_test_run_thumbnails/migration.sql b/prisma/migrations/20260904140000_add_test_run_thumbnails/migration.sql new file mode 100644 index 00000000..f6e4cb67 --- /dev/null +++ b/prisma/migrations/20260904140000_add_test_run_thumbnails/migration.sql @@ -0,0 +1,6 @@ +-- AlterTable +-- Nullable with no default, so this is a catalogue-only change on Postgres 11+: +-- no table rewrite and no long lock. Existing rows keep NULL and the UI falls +-- back to the full-size image for them. +ALTER TABLE "TestRun" ADD COLUMN "imageThumbnailName" TEXT, + ADD COLUMN "diffThumbnailName" TEXT; diff --git a/prisma/schema.prisma b/prisma/schema.prisma index 104dd8aa..51bb7e87 100644 --- a/prisma/schema.prisma +++ b/prisma/schema.prisma @@ -79,6 +79,13 @@ model TestRun { // have no diff, or that were compared by something other than pixelmatch — // those fall back to computing it on demand. changeSignature String? + // Small copies of the checkpoint and of the diff, made at ingest from the + // pixels the comparison had already decoded. The card grids draw pictures a + // hundred-odd pixels wide, and pulling the full-size files for that cost + // megabytes per screen. Null on runs ingested before this, which fall back to + // the full-size file. + imageThumbnailName String? + diffThumbnailName String? baseline Baseline? build Build @relation(fields: [buildId], references: [id]) project Project? @relation(fields: [projectId], references: [id]) diff --git a/src/_data_/index.ts b/src/_data_/index.ts index 7fc3bc27..9b0c46ed 100644 --- a/src/_data_/index.ts +++ b/src/_data_/index.ts @@ -100,6 +100,8 @@ export const generateTestRun = (testRun?: Partial): TestRun => { merge: false, vlmDescription: null, changeSignature: null, + imageThumbnailName: null, + diffThumbnailName: null, ...testRun, }; }; diff --git a/src/compare/libs/pixelmatch/pixelmatch.core.spec.ts b/src/compare/libs/pixelmatch/pixelmatch.core.spec.ts index 5e9e47c9..d3f51ad6 100644 --- a/src/compare/libs/pixelmatch/pixelmatch.core.spec.ts +++ b/src/compare/libs/pixelmatch/pixelmatch.core.spec.ts @@ -1,6 +1,7 @@ import { PNG } from 'pngjs'; import { computePixelmatchDiff } from './pixelmatch.core'; import { computeChangeSignature } from './signature.core'; +import { THUMBNAIL_MAX_DIMENSION } from './thumbnail.core'; const WIDTH = 60; const HEIGHT = 60; @@ -34,7 +35,7 @@ const block = (left: number, top: number, rgb: [number, number, number]): Buffer const blank = png(() => undefined); const RED: [number, number, number] = [255, 0, 0]; -const diffJob = (baseline: Buffer, image: Buffer, withSignature: boolean) => +const diffJob = (baseline: Buffer, image: Buffer, withSignature: boolean, withThumbnails = false) => computePixelmatchDiff({ kind: 'diff', baseline, @@ -46,6 +47,7 @@ const diffJob = (baseline: Buffer, image: Buffer, withSignature: boolean) => diffTolerancePercent: 0, saveDiff: false, withSignature, + withThumbnails, }); describe('computePixelmatchDiff with a signature', () => { @@ -85,3 +87,28 @@ describe('computePixelmatchDiff with a signature', () => { expect(result.pixelMisMatchCount).toBe(144); }); }); + +describe('computePixelmatchDiff with thumbnails', () => { + // Made here from the pixels the diff has already decoded and already drawn, + // so a thumbnail costs a resize rather than a second read of the screenshot. + it('returns a small picture of both the checkpoint and the diff', () => { + const result = diffJob(blank, block(10, 10, RED), false, true); + + const image = PNG.sync.read(Buffer.from(result.imageThumbnail)); + const diff = PNG.sync.read(Buffer.from(result.diffThumbnail)); + expect(Math.max(image.width, image.height)).toBeLessThanOrEqual(THUMBNAIL_MAX_DIMENSION); + expect(Math.max(diff.width, diff.height)).toBeLessThanOrEqual(THUMBNAIL_MAX_DIMENSION); + }); + + it('makes none when they were not asked for', () => { + const result = diffJob(blank, block(10, 10, RED), false, false); + + expect(result.imageThumbnail).toBeUndefined(); + expect(result.diffThumbnail).toBeUndefined(); + }); + + // nothing to show and nothing to shrink + it('makes none when the screenshots are identical', () => { + expect(diffJob(blank, blank, false, true).imageThumbnail).toBeUndefined(); + }); +}); diff --git a/src/compare/libs/pixelmatch/pixelmatch.core.ts b/src/compare/libs/pixelmatch/pixelmatch.core.ts index 9dc12968..f0a77814 100644 --- a/src/compare/libs/pixelmatch/pixelmatch.core.ts +++ b/src/compare/libs/pixelmatch/pixelmatch.core.ts @@ -3,6 +3,7 @@ import Pixelmatch from 'pixelmatch'; import { IgnoreAreaDto } from '../../../test-runs/dto/ignore-area.dto'; import { applyIgnoreAreas, scaleImageToSize } from '../../utils'; import { signatureOfDecoded } from './signature.core'; +import { encodeThumbnail } from './thumbnail.core'; /** * CPU-bound part of the pixelmatch comparison, extracted so it can run inside @@ -28,6 +29,12 @@ export interface PixelmatchJobInput { * again just to group them. */ withSignature?: boolean; + /** + * Also return small PNGs of the checkpoint and of the diff. The grids draw + * these a hundred-odd pixels wide; sending the full-size files and letting + * CSS shrink them is what made opening the variations dialog cost megabytes. + */ + withThumbnails?: boolean; } export interface PixelmatchJobOutput { @@ -39,6 +46,10 @@ export interface PixelmatchJobOutput { // absent when not asked for, when the screenshots are identical, or when // their dimensions differ and there is nothing meaningful to sign signature?: number[]; + // absent when not asked for, or when the screenshots matched and there is + // nothing to show + imageThumbnail?: Buffer | Uint8Array; + diffThumbnail?: Buffer | Uint8Array; } // postMessage turns Buffers into Uint8Array views over their own ArrayBuffer. @@ -96,6 +107,10 @@ export function computePixelmatchDiff(input: PixelmatchJobInput): PixelmatchJobO }) : null; + const thumbnails = input.withThumbnails + ? { imageThumbnail: encodeThumbnail(imageIgnored), diffThumbnail: encodeThumbnail(diff) } + : {}; + return { equal: false, isSameDimension, @@ -103,5 +118,6 @@ export function computePixelmatchDiff(input: PixelmatchJobInput): PixelmatchJobO diffPercent, diffBuffer, ...(signature ? { signature } : {}), + ...thumbnails, }; } diff --git a/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts b/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts index 2a659f40..c4996d34 100644 --- a/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts +++ b/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts @@ -169,7 +169,9 @@ describe('getDiff', () => { threshold: 0.1, } ); - expect(saveImageMock).toHaveBeenCalledTimes(1); + // the full-size diff first, then the two thumbnails made beside it + expect(saveImageMock.mock.calls[0][0]).toBe('diff'); + expect(saveImageMock).toHaveBeenCalledTimes(3); expect(result).toStrictEqual({ status: TestStatus.unresolved, diffName, @@ -247,7 +249,9 @@ describe('getDiff', () => { DEFAULT_CONFIG ); - expect(saveImageMock).toHaveBeenCalledTimes(1); + // the full-size diff first, then the two thumbnails made beside it + expect(saveImageMock.mock.calls[0][0]).toBe('diff'); + expect(saveImageMock).toHaveBeenCalledTimes(3); expect(result).toStrictEqual({ status: TestStatus.unresolved, diffName, @@ -318,6 +322,41 @@ describe('change signature', () => { }); }); + it('saves the thumbnails and reports the names they went under', async () => { + const saveImage = jest.fn().mockResolvedValueOnce('image.thumb.png').mockResolvedValueOnce('diff.thumb.png'); + const module: TestingModule = await Test.createTestingModule({ + providers: [ + PixelmatchService, + { + provide: DiffWorkerPool, + useValue: { + run: jest.fn().mockResolvedValue({ + equal: false, + isSameDimension: true, + pixelMisMatchCount: 10, + diffPercent: 5, + imageThumbnail: Buffer.from('small image'), + diffThumbnail: Buffer.from('small diff'), + }), + }, + }, + { + provide: StaticService, + useValue: { + getImageBuffer: jest.fn().mockResolvedValue(Buffer.from('png')), + saveImage, + deleteImage: jest.fn(), + }, + }, + ], + }).compile(); + + const result = await compare(module.get(PixelmatchService)); + + expect(result.imageThumbnailName).toBe('image.thumb.png'); + expect(result.diffThumbnailName).toBe('diff.thumb.png'); + }); + it('leaves it out when the comparison produced none', async () => { const { service } = await initWithPool({ equal: false, diff --git a/src/compare/libs/pixelmatch/pixelmatch.service.ts b/src/compare/libs/pixelmatch/pixelmatch.service.ts index 55b184a8..5f4441c7 100644 --- a/src/compare/libs/pixelmatch/pixelmatch.service.ts +++ b/src/compare/libs/pixelmatch/pixelmatch.service.ts @@ -51,6 +51,7 @@ export class PixelmatchService implements ImageComparator { // would leave every run ingested before the flag was turned on without // one — which is exactly the build someone then tries to review. withSignature: true, + withThumbnails: true, }); if (output.equal) { @@ -80,6 +81,23 @@ export class PixelmatchService implements ImageComparator { if (output.diffBuffer) { result.diffName = await this.staticService.saveImage('diff', Buffer.from(output.diffBuffer)); } + // Kept to the same condition as the diff itself: a thumbnail of a diff + // that was never saved would point at a file that does not exist. Stored + // as ordinary images so they follow the same storage, the same deletion + // and the same URLs as everything else, with no naming convention for + // the UI to guess at. + if (output.imageThumbnail && output.diffThumbnail) { + const [imageThumbnailName, diffThumbnailName] = await Promise.all([ + this.staticService.saveImage('screenshot', Buffer.from(output.imageThumbnail)), + this.staticService.saveImage('diff', Buffer.from(output.diffThumbnail)), + ]); + // only when both came back: a result carrying one half of a pair, or a + // key set to undefined, is worse than none at all + if (imageThumbnailName && diffThumbnailName) { + result.imageThumbnailName = imageThumbnailName; + result.diffThumbnailName = diffThumbnailName; + } + } result.status = TestStatus.unresolved; } else { result.status = TestStatus.ok; diff --git a/src/compare/libs/pixelmatch/signature.core.ts b/src/compare/libs/pixelmatch/signature.core.ts index 822208d2..020d37a7 100644 --- a/src/compare/libs/pixelmatch/signature.core.ts +++ b/src/compare/libs/pixelmatch/signature.core.ts @@ -1,7 +1,8 @@ import { PNG } from 'pngjs'; import Pixelmatch from 'pixelmatch'; import { IgnoreAreaDto } from '../../../test-runs/dto/ignore-area.dto'; -import { applyIgnoreAreas } from '../../utils'; +import { RawImage } from '../../utils'; +import { applyIgnoreAreas, downscale } from '../../utils'; /** * CPU-bound part of "do these two screens carry the same change?": PNG decode, @@ -44,42 +45,11 @@ const SIGNATURE_MAX_DIMENSION = 500; // cosine similarity is at least this value. export const SIGNATURE_SIMILARITY_THRESHOLD = 0.9; -interface RawImage { - data: Buffer; - width: number; - height: number; -} - // postMessage turns Buffers into Uint8Array views over their own ArrayBuffer. function toBuffer(data: Buffer | Uint8Array): Buffer { return Buffer.isBuffer(data) ? data : Buffer.from(data.buffer, data.byteOffset, data.byteLength); } -// Nearest-neighbour downscale so the longest side is at most maxDimension. -// Returns the original when already small enough. -function downscale(source: RawImage, maxDimension: number): RawImage { - const scale = maxDimension / Math.max(source.width, source.height); - if (scale >= 1) { - return source; - } - const width = Math.max(1, Math.round(source.width * scale)); - const height = Math.max(1, Math.round(source.height * scale)); - const data: Buffer = Buffer.alloc(width * height * 4); - for (let y = 0; y < height; y++) { - const sourceY = Math.min(source.height - 1, Math.floor(y / scale)); - for (let x = 0; x < width; x++) { - const sourceX = Math.min(source.width - 1, Math.floor(x / scale)); - const sourceIndex = (sourceY * source.width + sourceX) * 4; - const targetIndex = (y * width + x) * 4; - data[targetIndex] = source.data[sourceIndex]; - data[targetIndex + 1] = source.data[sourceIndex + 1]; - data[targetIndex + 2] = source.data[sourceIndex + 2]; - data[targetIndex + 3] = source.data[sourceIndex + 3]; - } - } - return { data, width, height }; -} - /** * Position-independent signature of a change: a normalized histogram of the * colors that the changed pixels take in the new image. Because it ignores diff --git a/src/compare/libs/pixelmatch/thumbnail.core.spec.ts b/src/compare/libs/pixelmatch/thumbnail.core.spec.ts new file mode 100644 index 00000000..4dcfa7a8 --- /dev/null +++ b/src/compare/libs/pixelmatch/thumbnail.core.spec.ts @@ -0,0 +1,46 @@ +import { PNG } from 'pngjs'; +import { encodeThumbnail, THUMBNAIL_MAX_DIMENSION } from './thumbnail.core'; + +const solid = (width: number, height: number): PNG => { + const png = new PNG({ width, height }); + for (let i = 0; i < width * height; i++) { + png.data[i * 4] = 200; + png.data[i * 4 + 1] = 100; + png.data[i * 4 + 2] = 50; + png.data[i * 4 + 3] = 255; + } + return png; +}; + +describe('encodeThumbnail', () => { + // The grids draw these a hundred-odd pixels wide. Sending the full + // screenshot and letting CSS shrink it cost ~7 MB and 68 requests every time + // the variations dialog opened. + it('brings a screenshot down to the thumbnail size', () => { + const thumbnail = PNG.sync.read(encodeThumbnail(solid(1284, 2778))); + + expect(Math.max(thumbnail.width, thumbnail.height)).toBe(THUMBNAIL_MAX_DIMENSION); + }); + + it('keeps the proportions of the screenshot it came from', () => { + const source = solid(1284, 2778); + + const thumbnail = PNG.sync.read(encodeThumbnail(source)); + + expect(thumbnail.width / thumbnail.height).toBeCloseTo(source.width / source.height, 2); + }); + + it('is a fraction of the bytes of the original', () => { + const source = solid(1284, 2778); + + expect(encodeThumbnail(source).length).toBeLessThan(PNG.sync.write(source).length / 10); + }); + + // a small screenshot re-encoded larger would be worse than leaving it alone + it('never enlarges an image that is already small', () => { + const thumbnail = PNG.sync.read(encodeThumbnail(solid(100, 80))); + + expect(thumbnail.width).toBe(100); + expect(thumbnail.height).toBe(80); + }); +}); diff --git a/src/compare/libs/pixelmatch/thumbnail.core.ts b/src/compare/libs/pixelmatch/thumbnail.core.ts new file mode 100644 index 00000000..1fef0cd6 --- /dev/null +++ b/src/compare/libs/pixelmatch/thumbnail.core.ts @@ -0,0 +1,25 @@ +import { PNG } from 'pngjs'; +import { downscale, RawImage } from '../../utils'; + +/** + * Longest side, in pixels, of the picture the grids draw. They lay these out a + * hundred-odd pixels wide, so this leaves room for a retina screen and nothing + * more. Sending the full screenshot and letting CSS shrink it cost roughly 7 MB + * and 68 requests every time the variations dialog opened. + */ +export const THUMBNAIL_MAX_DIMENSION = 400; + +/** + * A small PNG of an already-decoded image, for the card grids. Produced beside + * the diff, from the pixels it has already decoded, so a thumbnail costs a + * resize rather than a second read of the screenshot. + * + * An image already within the bound is re-encoded as it is: enlarging it would + * make it bigger on the wire for no gain. + */ +export function encodeThumbnail(source: RawImage): Buffer { + const small = downscale(source, THUMBNAIL_MAX_DIMENSION); + const png = new PNG({ width: small.width, height: small.height }); + small.data.copy(png.data); + return PNG.sync.write(png); +} diff --git a/src/compare/utils/index.ts b/src/compare/utils/index.ts index 79e593b1..430389d9 100644 --- a/src/compare/utils/index.ts +++ b/src/compare/utils/index.ts @@ -34,3 +34,34 @@ export const parseConfig = (configJson: string, defaultConfig: T, logger: Log } return defaultConfig; }; + +export interface RawImage { + data: Buffer; + width: number; + height: number; +} + +// Nearest-neighbour downscale so the longest side is at most maxDimension. +// Returns the original when already small enough. +export function downscale(source: RawImage, maxDimension: number): RawImage { + const scale = maxDimension / Math.max(source.width, source.height); + if (scale >= 1) { + return source; + } + const width = Math.max(1, Math.round(source.width * scale)); + const height = Math.max(1, Math.round(source.height * scale)); + const data: Buffer = Buffer.alloc(width * height * 4); + for (let y = 0; y < height; y++) { + const sourceY = Math.min(source.height - 1, Math.floor(y / scale)); + for (let x = 0; x < width; x++) { + const sourceX = Math.min(source.width - 1, Math.floor(x / scale)); + const sourceIndex = (sourceY * source.width + sourceX) * 4; + const targetIndex = (y * width + x) * 4; + data[targetIndex] = source.data[sourceIndex]; + data[targetIndex + 1] = source.data[sourceIndex + 1]; + data[targetIndex + 2] = source.data[sourceIndex + 2]; + data[targetIndex + 3] = source.data[sourceIndex + 3]; + } + } + return { data, width, height }; +} diff --git a/src/test-runs/diffResult.ts b/src/test-runs/diffResult.ts index c9bfce15..366c7bc2 100644 --- a/src/test-runs/diffResult.ts +++ b/src/test-runs/diffResult.ts @@ -33,4 +33,11 @@ export interface DiffResult { * pixelmatch. */ changeSignature?: StampedSignature; + /** + * Names the small copies of the checkpoint and the diff were saved under, for + * the card grids to draw instead of the full-size files. Absent when the + * comparison produced no diff, or was not pixelmatch. + */ + imageThumbnailName?: string; + diffThumbnailName?: string; } diff --git a/src/test-runs/dto/testRun.dto.ts b/src/test-runs/dto/testRun.dto.ts index 4aa53168..2c37a1ff 100644 --- a/src/test-runs/dto/testRun.dto.ts +++ b/src/test-runs/dto/testRun.dto.ts @@ -8,6 +8,9 @@ export class TestRunDto { buildId: string; @ApiProperty() imageName: string; + // small copies for the card grids; null on runs ingested before these existed + imageThumbnailName?: string; + diffThumbnailName?: string; @ApiProperty() diffName: string; @ApiProperty() @@ -51,6 +54,8 @@ export class TestRunDto { this.id = testRun.id; this.buildId = testRun.buildId; this.imageName = testRun.imageName; + this.imageThumbnailName = testRun.imageThumbnailName; + this.diffThumbnailName = testRun.diffThumbnailName; this.diffName = testRun.diffName; this.diffPercent = testRun.diffPercent; this.diffTollerancePercent = testRun.diffTollerancePercent; diff --git a/src/test-runs/test-runs.service.spec.ts b/src/test-runs/test-runs.service.spec.ts index 10a4f62e..7a1bf992 100644 --- a/src/test-runs/test-runs.service.spec.ts +++ b/src/test-runs/test-runs.service.spec.ts @@ -352,6 +352,8 @@ describe('TestRunsService', () => { diffPercent: null, vlmDescription: null, changeSignature: null, + imageThumbnailName: null, + diffThumbnailName: null, }, }); expect(eventTestRunUpdatedMock).toHaveBeenCalledWith(testRun); @@ -420,6 +422,8 @@ describe('TestRunsService', () => { diffPercent: diff.diffPercent, vlmDescription: diff.vlmDescription, changeSignature: null, + imageThumbnailName: null, + diffThumbnailName: null, }, }); expect(eventTestRunUpdatedMock).toHaveBeenCalledWith(testRun); @@ -747,6 +751,51 @@ describe('TestRunsService', () => { }); }); + describe('thumbnails', () => { + it('stores the names the comparison saved them under', async () => { + const testRunUpdateMock = jest.fn().mockResolvedValueOnce(generateTestRun()); + service = await initService({ testRunUpdateMock }); + + await service.saveDiffResult('some id', { + status: TestStatus.unresolved, + diffName: 'diff.png', + pixelMisMatchCount: 11, + diffPercent: 22, + isSameDimension: true, + imageThumbnailName: 'image.thumb.png', + diffThumbnailName: 'diff.thumb.png', + }); + + expect(testRunUpdateMock.mock.calls[0][0].data).toMatchObject({ + imageThumbnailName: 'image.thumb.png', + diffThumbnailName: 'diff.thumb.png', + }); + }); + + // two extra objects per run: left behind on delete they would outlive every + // build that ever referenced them, and nothing would ever collect them + it('removes them with the run they belong to', async () => { + const testRun = generateTestRun({ + imageName: 'image.png', + diffName: 'diff.png', + imageThumbnailName: 'image.thumb.png', + diffThumbnailName: 'diff.thumb.png', + }); + const deleteImageMock = jest.fn(); + service = await initService({ + testRunFindUniqueMock: jest.fn().mockResolvedValueOnce(testRun), + testRunDeleteMock: jest.fn().mockResolvedValueOnce(testRun), + deleteImageMock, + }); + + await service.delete(testRun.id); + + expect(deleteImageMock.mock.calls.map(([name]) => name).sort()).toEqual( + ['diff.png', 'diff.thumb.png', 'image.png', 'image.thumb.png'].sort() + ); + }); + }); + describe('getMatchingVariations', () => { // Full-length histograms, because a stored one is only reused when its shape // matches what this build produces. Orthogonal, so they never match. diff --git a/src/test-runs/test-runs.service.ts b/src/test-runs/test-runs.service.ts index 423a6705..e9345421 100644 --- a/src/test-runs/test-runs.service.ts +++ b/src/test-runs/test-runs.service.ts @@ -383,6 +383,10 @@ export class TestRunsService { // reviewer edits the ignore areas, say — must not leave the previous // signature behind describing a change that no longer exists. changeSignature: diffResult?.changeSignature ? JSON.stringify(diffResult.changeSignature) : null, + // Overwritten together with the diff they were made from, so a + // recomputed comparison never leaves a picture of the old change. + imageThumbnailName: diffResult?.imageThumbnailName ?? null, + diffThumbnailName: diffResult?.diffThumbnailName ?? null, }, }) .then((testRun) => { @@ -465,6 +469,10 @@ export class TestRunsService { await Promise.all([ this.staticService.deleteImage(testRun.diffName), this.staticService.deleteImage(testRun.imageName), + // left behind, these would outlive every build that referenced them and + // nothing would ever collect them + this.staticService.deleteImage(testRun.imageThumbnailName), + this.staticService.deleteImage(testRun.diffThumbnailName), ]); try { From c920dfcbb520d37d9f87225fa072b48803998f1b Mon Sep 17 00:00:00 2001 From: "mykola.gervasyuk" Date: Fri, 4 Sep 2026 12:13:39 +0300 Subject: [PATCH 5/7] fix: do not strand thumbnails nothing will ever reference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two ways the thumbnails could be written and then orphaned, both found by CodeRabbit on #377. shouldAutoApprove compares a run against each past baseline with saveDiffAsFile off and throws the result away. The thumbnails were tied to the diff being over tolerance rather than to the diff actually being kept, so every one of those attempts stored two objects that nothing referenced and nothing deletes. They are no longer even asked for when the caller is not keeping the diff, and are stored only once the diff itself has landed. Recalculating a diff — which happens whenever a reviewer edits the ignore areas — deleted the old diff file but not the old thumbnails, while saveDiffResult replaced all three names on the row. That stranded two more objects per edit. All three now go together. The first of these was hiding behind a test that compared with saveDiffAsFile off and still expected thumbnail names, which is exactly the combination that should produce none. It now runs with the flag on, and a negative case covers the flag off. --- .../pixelmatch/pixelmatch.service.spec.ts | 55 +++++++++++++------ .../libs/pixelmatch/pixelmatch.service.ts | 9 ++- src/test-runs/test-runs.service.spec.ts | 23 ++++++++ src/test-runs/test-runs.service.ts | 5 ++ 4 files changed, 72 insertions(+), 20 deletions(-) diff --git a/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts b/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts index c4996d34..6c5de4c5 100644 --- a/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts +++ b/src/compare/libs/pixelmatch/pixelmatch.service.spec.ts @@ -286,14 +286,14 @@ describe('change signature', () => { return { service: module.get(PixelmatchService), run }; }; - const compare = (service: PixelmatchService) => + const compare = (service: PixelmatchService, saveDiffAsFile = false) => service.getDiff( { baseline: 'baseline.png', image: 'image.png', ignoreAreas: [], diffTollerancePercent: 0, - saveDiffAsFile: false, + saveDiffAsFile, }, DEFAULT_CONFIG ); @@ -322,24 +322,16 @@ describe('change signature', () => { }); }); - it('saves the thumbnails and reports the names they went under', async () => { - const saveImage = jest.fn().mockResolvedValueOnce('image.thumb.png').mockResolvedValueOnce('diff.thumb.png'); + const initSaving = async (output: Record) => { + const saveImage = jest + .fn() + .mockResolvedValueOnce('diff.png') + .mockResolvedValueOnce('image.thumb.png') + .mockResolvedValueOnce('diff.thumb.png'); const module: TestingModule = await Test.createTestingModule({ providers: [ PixelmatchService, - { - provide: DiffWorkerPool, - useValue: { - run: jest.fn().mockResolvedValue({ - equal: false, - isSameDimension: true, - pixelMisMatchCount: 10, - diffPercent: 5, - imageThumbnail: Buffer.from('small image'), - diffThumbnail: Buffer.from('small diff'), - }), - }, - }, + { provide: DiffWorkerPool, useValue: { run: jest.fn().mockResolvedValue(output) } }, { provide: StaticService, useValue: { @@ -350,13 +342,40 @@ describe('change signature', () => { }, ], }).compile(); + return { service: module.get(PixelmatchService), saveImage }; + }; - const result = await compare(module.get(PixelmatchService)); + const overTolerance = { + equal: false, + isSameDimension: true, + pixelMisMatchCount: 10, + diffPercent: 5, + imageThumbnail: Buffer.from('small image'), + diffThumbnail: Buffer.from('small diff'), + }; + + it('saves the thumbnails and reports the names they went under', async () => { + const { service } = await initSaving({ ...overTolerance, diffBuffer: Buffer.from('the diff') }); + + const result = await compare(service, true); expect(result.imageThumbnailName).toBe('image.thumb.png'); expect(result.diffThumbnailName).toBe('diff.thumb.png'); }); + // shouldAutoApprove compares against past baselines with saveDiffAsFile off + // and throws the result away. Storing thumbnails for those comparisons would + // leave two objects per attempt that nothing ever references or deletes. + it('stores nothing when the caller is not keeping the diff', async () => { + const { service, saveImage } = await initSaving(overTolerance); + + const result = await compare(service, false); + + expect(saveImage).not.toHaveBeenCalled(); + expect(result.imageThumbnailName).toBeUndefined(); + expect(result.diffThumbnailName).toBeUndefined(); + }); + it('leaves it out when the comparison produced none', async () => { const { service } = await initWithPool({ equal: false, diff --git a/src/compare/libs/pixelmatch/pixelmatch.service.ts b/src/compare/libs/pixelmatch/pixelmatch.service.ts index 5f4441c7..1e22b3e7 100644 --- a/src/compare/libs/pixelmatch/pixelmatch.service.ts +++ b/src/compare/libs/pixelmatch/pixelmatch.service.ts @@ -51,7 +51,11 @@ export class PixelmatchService implements ImageComparator { // would leave every run ingested before the flag was turned on without // one — which is exactly the build someone then tries to review. withSignature: true, - withThumbnails: true, + // Only when the caller is keeping the diff. shouldAutoApprove compares + // against past baselines with this off and throws the result away — + // making thumbnails for those would burn the CPU and, worse, store two + // objects per attempt that nothing ever references or deletes. + withThumbnails: data.saveDiffAsFile, }); if (output.equal) { @@ -86,7 +90,8 @@ export class PixelmatchService implements ImageComparator { // as ordinary images so they follow the same storage, the same deletion // and the same URLs as everything else, with no naming convention for // the UI to guess at. - if (output.imageThumbnail && output.diffThumbnail) { + // tied to the diff actually landing, not merely to being over tolerance + if (result.diffName && output.imageThumbnail && output.diffThumbnail) { const [imageThumbnailName, diffThumbnailName] = await Promise.all([ this.staticService.saveImage('screenshot', Buffer.from(output.imageThumbnail)), this.staticService.saveImage('diff', Buffer.from(output.diffThumbnail)), diff --git a/src/test-runs/test-runs.service.spec.ts b/src/test-runs/test-runs.service.spec.ts index 7a1bf992..66e1a4c8 100644 --- a/src/test-runs/test-runs.service.spec.ts +++ b/src/test-runs/test-runs.service.spec.ts @@ -772,6 +772,29 @@ describe('TestRunsService', () => { }); }); + // Recalculating replaces the names on the row, so whatever they pointed at + // before has to go with the old diff — otherwise every ignore-area edit + // leaves two more objects nothing references. + it('removes the previous ones when the diff is recalculated', async () => { + const testRun = generateTestRun({ + diffName: 'old-diff.png', + imageThumbnailName: 'old-image.thumb.png', + diffThumbnailName: 'old-diff.thumb.png', + }); + const deleteImageMock = jest.fn(); + service = await initService({ + deleteImageMock, + compareGetDiffMock: jest.fn().mockResolvedValueOnce({ status: TestStatus.ok }), + testRunUpdateMock: jest.fn().mockResolvedValueOnce(testRun), + }); + + await service.calculateDiff('projectId', testRun); + + expect(deleteImageMock.mock.calls.map(([name]) => name).sort()).toEqual( + ['old-diff.png', 'old-diff.thumb.png', 'old-image.thumb.png'].sort() + ); + }); + // two extra objects per run: left behind on delete they would outlive every // build that ever referenced them, and nothing would ever collect them it('removes them with the run they belong to', async () => { diff --git a/src/test-runs/test-runs.service.ts b/src/test-runs/test-runs.service.ts index e9345421..36fc6e5c 100644 --- a/src/test-runs/test-runs.service.ts +++ b/src/test-runs/test-runs.service.ts @@ -396,7 +396,12 @@ export class TestRunsService { } async calculateDiff(projectId: string, testRun: TestRun): Promise { + // The recomputed result replaces all three names on the row, so the old + // pictures go together: leaving the thumbnails behind would strand two + // more objects on every ignore-area edit. this.staticService.deleteImage(testRun.diffName); + this.staticService.deleteImage(testRun.imageThumbnailName); + this.staticService.deleteImage(testRun.diffThumbnailName); const diffResult = await this.compareService.getDiff({ projectId, data: { From 7f31c497b5180677e645f0b9ed5a9aeef8565a10 Mon Sep 17 00:00:00 2001 From: "mykola.gervasyuk" Date: Fri, 4 Sep 2026 12:37:57 +0300 Subject: [PATCH 6/7] fix: replace a run's pictures before dropping the old ones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit calculateDiff deleted the old diff before running the comparison — from before this branch; I had only widened it to the two thumbnails as well. A comparison that then failed left the row pointing at files already gone, so the reviewer opened the run and found broken pictures with nothing to fall back on. The old names are now dropped only once the replacement has been persisted. Deleting late can leak bytes if the process dies in between; deleting early breaks what the reviewer sees. The first is much the cheaper failure. Found by CodeRabbit on #377. --- src/test-runs/test-runs.service.spec.ts | 20 ++++++++++++++++++++ src/test-runs/test-runs.service.ts | 20 ++++++++++++++------ 2 files changed, 34 insertions(+), 6 deletions(-) diff --git a/src/test-runs/test-runs.service.spec.ts b/src/test-runs/test-runs.service.spec.ts index 66e1a4c8..aa636494 100644 --- a/src/test-runs/test-runs.service.spec.ts +++ b/src/test-runs/test-runs.service.spec.ts @@ -795,6 +795,26 @@ describe('TestRunsService', () => { ); }); + // Deleting before the replacement exists means a failed comparison leaves + // the row pointing at files that are gone — the reviewer opens the run and + // sees broken pictures, with nothing to recover from. + it('keeps the previous ones when the comparison fails', async () => { + const testRun = generateTestRun({ + diffName: 'old-diff.png', + imageThumbnailName: 'old-image.thumb.png', + diffThumbnailName: 'old-diff.thumb.png', + }); + const deleteImageMock = jest.fn(); + service = await initService({ + deleteImageMock, + compareGetDiffMock: jest.fn().mockRejectedValueOnce(new Error('storage is down')), + }); + + await expect(service.calculateDiff('projectId', testRun)).rejects.toThrow('storage is down'); + + expect(deleteImageMock).not.toHaveBeenCalled(); + }); + // two extra objects per run: left behind on delete they would outlive every // build that ever referenced them, and nothing would ever collect them it('removes them with the run they belong to', async () => { diff --git a/src/test-runs/test-runs.service.ts b/src/test-runs/test-runs.service.ts index 36fc6e5c..3840717d 100644 --- a/src/test-runs/test-runs.service.ts +++ b/src/test-runs/test-runs.service.ts @@ -397,11 +397,16 @@ export class TestRunsService { async calculateDiff(projectId: string, testRun: TestRun): Promise { // The recomputed result replaces all three names on the row, so the old - // pictures go together: leaving the thumbnails behind would strand two - // more objects on every ignore-area edit. - this.staticService.deleteImage(testRun.diffName); - this.staticService.deleteImage(testRun.imageThumbnailName); - this.staticService.deleteImage(testRun.diffThumbnailName); + // pictures go with it — leaving the thumbnails behind would strand two more + // objects on every ignore-area edit. + // + // They go *after* the new result is persisted, not before. Deleting first + // meant a comparison that failed left the row pointing at files that were + // already gone: the reviewer opens the run and finds broken pictures, with + // nothing to fall back on. Deleting late can only ever leak bytes, which is + // the cheaper of the two failures by a distance. + const previous = [testRun.diffName, testRun.imageThumbnailName, testRun.diffThumbnailName]; + const diffResult = await this.compareService.getDiff({ projectId, data: { @@ -412,7 +417,10 @@ export class TestRunsService { saveDiffAsFile: true, }, }); - return this.saveDiffResult(testRun.id, diffResult); + const saved = await this.saveDiffResult(testRun.id, diffResult); + + previous.forEach((name) => this.staticService.deleteImage(name)); + return saved; } async create({ From 12ae9259e6870abf8e9dc575d034b60add125baa Mon Sep 17 00:00:00 2001 From: "mykola.gervasyuk" Date: Fri, 4 Sep 2026 12:41:04 +0300 Subject: [PATCH 7/7] perf: only make thumbnails for a diff that is being kept MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A build is mostly runs that pass. Their comparison still produced two resized PNGs, seconds before the diff they belonged to was thrown away — work nobody ever saw the result of, on the majority of every ingest. Thumbnails now share the exact condition the diff buffer already had: over tolerance, and the caller keeping it. The service-level guard added earlier stopped them being *stored*; this stops them being *made*. Also annotates the two new DTO fields with @ApiPropertyOptional. Every other field in TestRunDto is annotated and the file already imported it, so without this the generated OpenAPI schema — and the client SDKs built from it — would simply not know the fields exist. Both found by Copilot on #377. --- .../libs/pixelmatch/pixelmatch.core.spec.ts | 28 +++++++++++++++++-- .../libs/pixelmatch/pixelmatch.core.ts | 13 ++++++--- src/test-runs/dto/testRun.dto.ts | 2 ++ 3 files changed, 36 insertions(+), 7 deletions(-) diff --git a/src/compare/libs/pixelmatch/pixelmatch.core.spec.ts b/src/compare/libs/pixelmatch/pixelmatch.core.spec.ts index d3f51ad6..dfc82e3a 100644 --- a/src/compare/libs/pixelmatch/pixelmatch.core.spec.ts +++ b/src/compare/libs/pixelmatch/pixelmatch.core.spec.ts @@ -35,7 +35,13 @@ const block = (left: number, top: number, rgb: [number, number, number]): Buffer const blank = png(() => undefined); const RED: [number, number, number] = [255, 0, 0]; -const diffJob = (baseline: Buffer, image: Buffer, withSignature: boolean, withThumbnails = false) => +const diffJob = ( + baseline: Buffer, + image: Buffer, + withSignature: boolean, + withThumbnails = false, + { saveDiff = true, diffTolerancePercent = 0 } = {} +) => computePixelmatchDiff({ kind: 'diff', baseline, @@ -44,8 +50,8 @@ const diffJob = (baseline: Buffer, image: Buffer, withSignature: boolean, withTh threshold: 0.1, includeAA: false, allowDiffDimensions: false, - diffTolerancePercent: 0, - saveDiff: false, + diffTolerancePercent, + saveDiff, withSignature, withThumbnails, }); @@ -107,6 +113,22 @@ describe('computePixelmatchDiff with thumbnails', () => { expect(result.diffThumbnail).toBeUndefined(); }); + // A build is mostly runs that pass. Resizing two images for each of those, + // when the diff they belong to is thrown away seconds later, is work nobody + // ever sees the result of. + it('makes none when the change is under tolerance', () => { + const result = diffJob(blank, block(10, 10, RED), false, true, { diffTolerancePercent: 90 }); + + expect(result.imageThumbnail).toBeUndefined(); + expect(result.diffThumbnail).toBeUndefined(); + }); + + it('makes none when the caller is not keeping the diff', () => { + const result = diffJob(blank, block(10, 10, RED), false, true, { saveDiff: false }); + + expect(result.imageThumbnail).toBeUndefined(); + }); + // nothing to show and nothing to shrink it('makes none when the screenshots are identical', () => { expect(diffJob(blank, blank, false, true).imageThumbnail).toBeUndefined(); diff --git a/src/compare/libs/pixelmatch/pixelmatch.core.ts b/src/compare/libs/pixelmatch/pixelmatch.core.ts index f0a77814..a8e248cf 100644 --- a/src/compare/libs/pixelmatch/pixelmatch.core.ts +++ b/src/compare/libs/pixelmatch/pixelmatch.core.ts @@ -91,8 +91,12 @@ export function computePixelmatchDiff(input: PixelmatchJobInput): PixelmatchJobO }); const diffPercent = (pixelMisMatchCount * 100) / (scaledImage.width * scaledImage.height); + // A build is mostly runs that pass, so anything produced for a diff that is + // about to be thrown away is work nobody ever sees the result of. + const keepingTheDiff = diffPercent > input.diffTolerancePercent && input.saveDiff; + let diffBuffer: Buffer; - if (diffPercent > input.diffTolerancePercent && input.saveDiff) { + if (keepingTheDiff) { diffBuffer = PNG.sync.write(diff); } @@ -107,9 +111,10 @@ export function computePixelmatchDiff(input: PixelmatchJobInput): PixelmatchJobO }) : null; - const thumbnails = input.withThumbnails - ? { imageThumbnail: encodeThumbnail(imageIgnored), diffThumbnail: encodeThumbnail(diff) } - : {}; + const thumbnails = + input.withThumbnails && keepingTheDiff + ? { imageThumbnail: encodeThumbnail(imageIgnored), diffThumbnail: encodeThumbnail(diff) } + : {}; return { equal: false, diff --git a/src/test-runs/dto/testRun.dto.ts b/src/test-runs/dto/testRun.dto.ts index 2c37a1ff..f0d01d7e 100644 --- a/src/test-runs/dto/testRun.dto.ts +++ b/src/test-runs/dto/testRun.dto.ts @@ -9,7 +9,9 @@ export class TestRunDto { @ApiProperty() imageName: string; // small copies for the card grids; null on runs ingested before these existed + @ApiPropertyOptional() imageThumbnailName?: string; + @ApiPropertyOptional() diffThumbnailName?: string; @ApiProperty() diffName: string;