feat(review): wire up pixel-diff into a Diff column (self-host) - #3677
Conversation
Wires the existing, previously-unused screenshot-comparison module under src/visual-agent/ into the before/after capture pipeline via a module-swap seam (src/review/visual/pixel-diff.ts, a Worker-safe no-op by default) that scripts/build-selfhost.mjs's esbuild plugin swaps for a real implementation when bundling the self-host entry -- the same substitution pattern already used for @cloudflare/puppeteer in that build. The Worker's own bundle never applies that swap, so hosted mode is byte-identical (isVisualDiffAvailable() stays false, zero added cost). capturePage now optionally returns the raw screenshot bytes alongside the URL -- including on a cache hit, the common case for "before" (the same production shot is reused across many PR reviews) -- only when a caller opts in, so every existing call site is unaffected. buildCapture compares before/after per viewport when diffing is available and uploads the result through the same store screenshots already use. unified-comment-bridge.ts's "Visual preview" table gains a 5th Diff column: a clickable diff-overlay thumbnail when the comparison clears the module's own noise threshold, a dash otherwise -- making a visual change self-evident instead of requiring a reviewer to eyeball two full-page images side by side. Verified with a real `node scripts/build-selfhost.mjs` build: grepped the resulting bundle to confirm the real comparison module (not the no-op) is what actually gets included for self-host. Part of #3607. Closes #3674.
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
|
Warning 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏸️ Gittensory review result - manual review recommendedReview updated: 2026-07-06 01:50:30 UTC
⏸️ Suggested Action - Manual Review
Review summary Nits — 6 non-blocking
Review context
Contributor next steps
Signal definitions
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3677 +/- ##
==========================================
+ Coverage 93.29% 93.33% +0.04%
==========================================
Files 314 315 +1
Lines 32058 32080 +22
Branches 11749 11762 +13
==========================================
+ Hits 29907 29942 +35
+ Misses 1517 1507 -10
+ Partials 634 631 -3
🚀 New features to boost your workflow:
|
codecov/patch flagged 89.29% on the prior commit -- precise per-branch
analysis (not the human-readable summary, which turned out unreliable
for pinpointing exact gaps) found 4 real branches new to this PR that
no existing test reached:
- a cache hit with includeBytes=false (diffing unavailable, the
common/default case) returning just {url}, no bytes
- a cache hit whose body fails to read back, degrading to no bytes
rather than throwing
- a fresh successful render's two return shapes: with bytes (diffing
available) and without (the default), not just the cache-hit sides
of that same branch
- uploadDiffImage's put() failing, degrading to still returning the
(deterministic, key-derived) URL rather than losing it -- mirrors
capturePage's own identical fire-and-forget persistence pattern
Verified via raw v8 coverage-final.json branch counts cross-referenced
against Codecov's own added-lines list, not the summary text alone.
Summary
src/visual-agent/) into the before/after capture pipeline. It was ported during an earlier convergence effort with full test coverage but zero production callers.Bufferand a native-leaning PNG-decode step, which the Cloudflare Workers runtime doesn't guarantee —test/unit/worker-entry-boundary.test.tsforbids importing (or even naming) it from the Worker entry. This PR introduces a module-swap seam (src/review/visual/pixel-diff.ts, a Worker-safe no-op by default) thatscripts/build-selfhost.mjs's existing esbuild plugin swaps for a real implementation only when bundling the self-host entry — the exact same substitution pattern already used for@cloudflare/puppeteerin that same build. The Worker's own (wrangler) bundle never applies that swap, so hosted mode is byte-identical:isVisualDiffAvailable()staysfalse, zero added cost.capturePage(incapture.ts) now optionally returns the raw screenshot bytes alongside the URL — including on a cache hit, which is the common case for "before" (the same production shot is reused across many PR reviews) — only when a caller opts in, so every existing call site is unaffected.buildCapturecompares before/after per viewport when diffing is available and uploads the resulting diff image through the same store screenshots already use.unified-comment-bridge.ts's "Visual preview" table gains a 5th Diff column: a clickable diff-overlay thumbnail when the comparison clears the noise threshold, a dash otherwise — making a visual change self-evident instead of requiring a reviewer to eyeball two full-page images side by side.Part of #3607. Closes #3674.
Scope
type(scope): short summaryConventional Commit format, for examplefix(api): restore profile access checks.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Validation
git diff --checknpm run actionlint(not run — no workflow files changed)npm run typechecknpm run test:coveragelocally;codecov/patchrequires ≥99% coverage of the lines AND branches you changed (aim for 100% on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.npm run test:workers(not run — no Worker-runtime-specific code beyond the existing capture.ts/unified-comment-bridge.ts modules, both already covered by the targeted suite below)npm run build:mcp(not run — no MCP package changes)npm run test:mcp-pack(not run — no MCP package changes)npm run ui:openapi:check(not run — no API/OpenAPI changes)npm run ui:lint(not run — noapps/gittensory-ui/**changes)npm run ui:typecheck(not run — noapps/gittensory-ui/**changes)npm run ui:build(not run — noapps/gittensory-ui/**changes)npm audit --audit-level=moderateIf any required check was skipped, explain why:
src/review/visual/capture.ts,src/review/unified-comment-bridge.ts,src/review/visual/pixel-diff.ts(new),src/selfhost/stubs/pixel-diff.ts(new), andscripts/build-selfhost.mjschanged — no UI, MCP, wrangler binding, OpenAPI, or workflow files touched. Rannpx tsc --noEmit -p .(full project, clean) and the full affected test suite (95 tests across 10 files: the newpixel-diff.test.tsandselfhost-pixel-diff-stub.test.ts, extendedvisual-capture.test.tsandvisual-collapsible.test.ts, plus every neighboring visual-* test andworker-entry-boundary.test.tsto prove the boundary wasn't violated) — all passing.node scripts/build-selfhost.mjsbuild and grepped the resultingdist/server.mjsbundle, confirming the real comparison module (compareRouteScreenshots, plus its own dependencies) is genuinely present in the self-host bundle — not just that the esbuild resolver rule looked correct on paper.Safety
UI Evidencesection below with JPG/JPEG or PNG screenshots. (N/A — no visible UI surface of gittensory itself changed; this changes what the bot posts in a GitHub comment, not a UI page.)Notes