Skip to content

feat(review): capture full-page screenshots for before/after evidence#3669

Merged
loopover-orb[bot] merged 1 commit into
mainfrom
feat/selfhost-visual-fullpage-capture
Jul 6, 2026
Merged

feat(review): capture full-page screenshots for before/after evidence#3669
loopover-orb[bot] merged 1 commit into
mainfrom
feat/selfhost-visual-fullpage-capture

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

Closes #3667. Part of #3607.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked an issue, or this is small enough that the summary explains why an issue is not needed.

Validation

  • git diff --check
  • npm run actionlint (not run — no workflow files changed)
  • npm run typecheck
  • npm run test:coverage locally; codecov/patch requires ≥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 touched beyond the existing shot.ts module)
  • 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 — no apps/gittensory-ui/** changes)
  • npm run ui:typecheck (not run — no apps/gittensory-ui/** changes)
  • npm run ui:build (not run — no apps/gittensory-ui/** changes)
  • npm audit --audit-level=moderate
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries

If any required check was skipped, explain why:

  • Only src/review/visual/shot.ts and its direct test changed — no UI, MCP, wrangler binding, OpenAPI, or workflow files touched, so those checks have no surface to exercise. Ran npx tsc --noEmit -p . (full project, clean) and the full visual-* test suite (75 tests across 7 files, all passing) locally.
  • Real end-to-end verification beyond unit tests: built a standalone script using puppeteer-core connecting to a real browserless/chromium container (the same self-host infra shipped in Deploy self-host browser-rendering infrastructure (browserless/chrome + BROWSER_WS_ENDPOINT) #3608/feat(selfhost): add optional browserless/chromium service for visual review #3633) and navigated a real headless Chrome to a deliberately long (7300px) local test page with a marker div placed roughly 3800px down. Confirmed page.screenshot({ fullPage: false }) produces a 1440×900 PNG that never shows the marker, while page.screenshot({ fullPage: true }) — the exact call this PR makes — produces a 1440×7300 PNG (file command confirmed exact pixel dimensions) with the marker visible at its correct position (screenshot inspected visually, not just checked by byte size). This is the real mechanism the code change relies on, not a mock.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests. (N/A — no auth/session/CORS changes.)
  • API/OpenAPI/MCP behavior is updated and tested where needed. (N/A — no API/OpenAPI/MCP surface touched.)
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. (N/A — no UI changes.)
  • Visible UI changes include a UI Evidence section below with JPG/JPEG or PNG screenshots. (N/A — this changes the review-engine's OWN screenshot-capture mechanism, not a UI surface of gittensory itself; the real-render verification screenshot is described above rather than committed, per house rules against committing review-only evidence.)
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs. (No docs changes needed — this is an internal capture-quality fix with no new configurable surface.)

Notes

Switches captureShot from a viewport-only render (fullPage: false) to a
full-page render (fullPage: true), so before/after screenshots always
show the same position on the page for any change, however far down it
is -- a change midway down a long page previously fell outside the
fixed 1440x900/390x844 viewport and was silently absent from both
cells. Verified against a real headless-Chromium render (browserless,
per #3608's self-host infra) that a 7300px test page renders as a full
1440x7300 PNG with a deliberately-placed marker visible at its correct
position, not just the 900px viewport slice.

Part of #3607. Closes #3667.
@superagent-security

Copy link
Copy Markdown
Contributor

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

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 6, 2026
@loopover-orb

loopover-orb Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Tip

🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩

✅ Gittensory review result - approve/merge recommended

Review updated: 2026-07-06 00:58:52 UTC

2 files · 1 AI reviewer · no blockers · readiness 100/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This is a minimal, well-scoped one-line correctness fix: captureShot in src/review/visual/shot.ts now takes `fullPage: true` screenshots instead of viewport-only, so before/after captures show the same page position regardless of how far down a change lives on a long page. The change is exactly what the description claims, the added test directly asserts the new screenshot call arguments, and the surrounding SSRF/auth-wall/redirect logic is untouched. The change is correct and low-risk.

Nits — 5 non-blocking
  • The LOADING_SVG placeholder in src/review/visual/shot.ts is still a fixed 1440×900 viewBox with a comment claiming it 'reserves space and never resizes when the image swaps in' — now that real captures are full-page (often taller than 900px), that claim is stale and the cell will resize once the real screenshot loads; worth updating the comment or placeholder aspect ratio.
  • No explicit test verifies captureShot's own return value (png/authWalled) with fullPage true — coverage relies on the handleShot integration test asserting the screenshot() call args, which is sufficient for the branch but doesn't independently exercise captureShot directly like the other captureShot-specific test does.
  • Consider capping page height or noting a follow-up for pathologically long pages, since full-page capture on an infinite-scroll or very tall preview could produce a very large PNG — likely fine for phase 1 but worth a note in the linked epic (Epic: extend automated visual-capture convergence (#577) to self-host + metagraphed #3607) roadmap.
  • Update the LOADING_SVG/FAILED_SVG placeholder comments in src/review/visual/shot.ts to reflect that real captures may now differ in aspect ratio from the fixed 1440×900 placeholder.
  • PR author also opened the linked issue — Link an issue that was opened by a different contributor, or provide a rationale for why this self-authored issue represents genuine discovery work.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ Linked #3667
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 55 registered-repo PR(s), 46 merged, 464 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 55 PR(s), 464 issue(s).
Gate result ✅ Passing No configured blocker found.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: Python, TypeScript, JavaScript, Ruby, Go, Kotlin, MDX, Shell
  • Official Gittensor activity: 55 PR(s), 464 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • No action.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

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

  • Re-run Gittensory review

@codecov

codecov Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.29%. Comparing base (2156f42) to head (8e1a709).
⚠️ Report is 1 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3669   +/-   ##
=======================================
  Coverage   93.29%   93.29%           
=======================================
  Files         314      314           
  Lines       32057    32057           
  Branches    11748    11748           
=======================================
  Hits        29906    29906           
  Misses       1517     1517           
  Partials      634      634           
Files with missing lines Coverage Δ
src/review/visual/shot.ts 81.92% <100.00%> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown
Contributor

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 and CI is green.

@loopover-orb
loopover-orb Bot merged commit 2783a25 into main Jul 6, 2026
10 checks passed
@loopover-orb
loopover-orb Bot deleted the feat/selfhost-visual-fullpage-capture branch July 6, 2026 01:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(review): capture full-page screenshots so before/after show the same page position

1 participant