Skip to content

feat(review): visual before/after screenshot capture (web-visible PRs, flag-gated OFF) - #1056

Merged
JSONbored merged 2 commits into
mainfrom
feat/visual-capture
Jun 22, 2026
Merged

feat(review): visual before/after screenshot capture (web-visible PRs, flag-gated OFF)#1056
JSONbored merged 2 commits into
mainfrom
feat/visual-capture

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

What (Task 7 / #1021 — the last feature port)

Ports reviewbot's before/after screenshot capture into gittensory. For PRs touching web-visible files only (frontend pages / public OG images), the unified comment gains a "Visual preview" table: production ("before") vs the PR's preview deploy ("after"), rendered via the BROWSER binding and served from a public route for GitHub's camo proxy.

Default OFF + inert. GITTENSORY_REVIEW_SCREENSHOTS="false"; even ON it requires the repo to be in GITTENSORY_REVIEW_REPOS. Capture is try/catch-wrapped (a failure never sinks a review). The public route 404s when the flag is off.

Files

  • NEW src/review/visual/{paths,shot,preview-url,capture}.ts + src/review/visual-wire.ts
  • src/api/routes.ts public GET /gittensory/shot (outside /v1/, flag-gated 404-when-off)
  • src/review/unified-comment-bridge.ts optional beforeAfter → "Visual preview" collapsible
  • src/queue/processors.ts gated capture at the comment-build site
  • src/env.d.ts + wrangler.jsonc GITTENSORY_REVIEW_SCREENSHOTS (OFF), package.json @cloudflare/puppeteer
  • tests: visual-paths, visual-wire, visual-collapsible

Security

  • SSRF: captureShot rejects any non-public host via isSafeHttpUrl before the browser navigates (loopback/link-local/private/169.254.169.254 blocked).
  • ?key=: must start with gittensory/shots/ + reject .. (no arbitrary R2 read).
  • ?url=: host-allowlisted (*.workers.dev/*.pages.dev/PUBLIC_SITE_ORIGIN) + SSRF-guarded + viewport clamped.
  • Public route: outside /v1/ (camo can fetch) but 404s when the flag is off — no on-demand render surface until enabled.
  • Preview-URL source: PR-comment fallback restricted to cloudflare-workers-and-pages[bot].
  • web-visible-only: backend .ts/.md/.json/.py PRs never trigger capture (isVisualPath ported verbatim).
  • Renderer escapes HTML, so the collapsible uses markdown ![](url) image syntax (no angle brackets) — preserves the anti-injection control.

Typecheck clean; full unit suite (3330) green. Merging is inert; I'll do a deeper review + provision/verify before flipping the flag on.

Part of epic #983.

…, flag-gated OFF)

Port reviewbot's visual capture into the gittensory worker, behind a new
GITTENSORY_REVIEW_SCREENSHOTS flag (default OFF) ANDed with the per-repo
cutover allowlist (GITTENSORY_REVIEW_REPOS). Screenshots fire ONLY for
web-visible changes (frontend pages / public OG images) via isVisualPath;
backend .ts/.md/.json/.py PRs never trigger capture.

- src/review/visual/paths.ts: isVisualPath ported verbatim (3 VISUAL_PATTERNS)
- src/review/visual/shot.ts: handleShot/captureShot/renderScreenshot + SVG
  placeholders; env.BROWSER + env.REVIEW_AUDIT; SSRF guard (isSafeHttpUrl) +
  host allowlist + r2-prefix/'..' key validation preserved
- src/review/visual/preview-url.ts: Deployments API -> checks -> cloudflare-bot
  PR comment fallback + getPreviewBuildState + deployment_status mapping
- src/review/visual/capture.ts: route mapping + before/after render orchestration
- src/review/visual-wire.ts: isScreenshotsEnabled + screenshotsAllowed (AND
  per-repo cutover gate)
- unified-comment-bridge: optional beforeAfter -> 'Visual preview' collapsible
  (markdown image table, public-safe)
- routes.ts: PUBLIC GET /gittensory/shot OUTSIDE /v1/ (camo-proxy fetchable)
- env.d.ts + wrangler.jsonc: GITTENSORY_REVIEW_SCREENSHOTS flag (default OFF)
- processors.ts: gated, try/catch-wrapped capture at the unified-comment site
- tests: isVisualPath (web-visible-only), screenshots flag/gate, collapsible

Requires installing @cloudflare/puppeteer@^1.1.0 (declared in package.json;
absent from the shared node_modules).
…inert when flag-OFF

Adds @cloudflare/puppeteer to the lockfile (CI npm ci now resolves the visual import). Hardens the public /gittensory/shot route: returns 404 when GITTENSORY_REVIEW_SCREENSHOTS is off, so the on-demand ?url= render surface only exists once the feature is deliberately enabled (flag-OFF = truly inert, no attack surface).
@dosubot dosubot Bot added the size:XL label Jun 22, 2026
@ghost

ghost commented Jun 22, 2026

Copy link
Copy Markdown

Note

🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦🟦

💡 Gittensory review — advisory only

0 files · no blockers · readiness 0/100

💡 Advisory only — no action taken

Signal Result Evidence
Code review ✅ No blockers synthesized
Gate result ⚠️ Skipped #1056 is no longer open.

💰 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.

@ghost ghost left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gittensory approves — the gate is satisfied.

@codecov

codecov Bot commented Jun 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 10.28369% with 253 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.39%. Comparing base (2288644) to head (4971c24).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/review/visual/preview-url.ts 1.72% 114 Missing ⚠️
src/review/visual/capture.ts 5.55% 68 Missing ⚠️
src/review/visual/shot.ts 9.23% 59 Missing ⚠️
src/queue/processors.ts 18.18% 7 Missing and 2 partials ⚠️
src/api/routes.ts 25.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1056      +/-   ##
==========================================
- Coverage   96.75%   95.39%   -1.37%     
==========================================
  Files         143      148       +5     
  Lines       17580    17861     +281     
  Branches     6364     6472     +108     
==========================================
+ Hits        17010    17038      +28     
- Misses        108      359     +251     
- Partials      462      464       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@JSONbored
JSONbored merged commit ac7429e into main Jun 22, 2026
24 of 28 checks passed
@JSONbored
JSONbored deleted the feat/visual-capture branch June 22, 2026 19:34
JSONbored added a commit that referenced this pull request Jun 22, 2026
…ke timers)

The test drove an async flow — real WebCrypto token-decrypt + async D1/fetch interleaved with one setTimeout(sleep 3000) — via a fake-timer pump loop. Under CI full-shard coverage load the real macrotask lagged the microtask flush the pump relied on, so the scheduled sleep was never fired and the test HUNG → 15s timeout (the CI-only flake that blocked #1050/#1051/#1052/#1056).

Fix: drop fake timers entirely. Mock setTimeout so the sleep(3000) backoff fires on a real 0ms tick, then await processSubmitDraft directly — the flow runs to completion on the real event loop (probe 404 → instant backoff → probe 200 → open PR) with no pump, no race, no real 3s wait, no weakened assertions. Verified stable across 6+ consecutive plain + coverage runs locally.
JSONbored added a commit that referenced this pull request Jun 22, 2026
…ke timers) (#1057)

The test drove an async flow — real WebCrypto token-decrypt + async D1/fetch interleaved with one setTimeout(sleep 3000) — via a fake-timer pump loop. Under CI full-shard coverage load the real macrotask lagged the microtask flush the pump relied on, so the scheduled sleep was never fired and the test HUNG → 15s timeout (the CI-only flake that blocked #1050/#1051/#1052/#1056).

Fix: drop fake timers entirely. Mock setTimeout so the sleep(3000) backoff fires on a real 0ms tick, then await processSubmitDraft directly — the flow runs to completion on the real event loop (probe 404 → instant backoff → probe 200 → open PR) with no pump, no race, no real 3s wait, no weakened assertions. Verified stable across 6+ consecutive plain + coverage runs locally.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant