Skip to content

feat(review): downscale desktop Visual-preview thumbnails in self-host mode - #6347

Merged
JSONbored merged 1 commit into
mainfrom
feat/visual-preview-display-downscale-v2
Jul 16, 2026
Merged

feat(review): downscale desktop Visual-preview thumbnails in self-host mode#6347
JSONbored merged 1 commit into
mainfrom
feat/visual-preview-display-downscale-v2

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • shot.ts captures full-page, native-resolution PNGs (up to 1440px wide for the desktop viewport, taller still for a long page's full-page height), stored and embedded verbatim at <img width="360"> — every viewer's browser previously downloaded the full-resolution capture just to render a 360px-wide thumbnail.
  • New downscaleForDisplay/isDisplayDownscaleAvailable in image-downscale.ts (Worker no-op) + src/selfhost/stubs/image-downscale.ts (real sharp-based implementation, 720px width cap — 2x the display width for a sharp retina render, still a real reduction from 1440px) mirror the existing downscaleForVision seam's own hosted-no-op/self-host-real architecture exactly. Desktop-only: shot.ts's mobile viewport (390px) is already close enough to the table's 360px display width that a third resized copy wouldn't meaningfully help.
  • capturePage stores the downscaled copy at a sibling R2 key on a fresh render (only when it genuinely shrinks the bytes, and only once the write is confirmed to succeed — caught a real bug in my own first pass here, where thumbUrl was set even when the write silently failed via a bare .catch(() => undefined)), and re-verifies the sibling's existence on a cache hit rather than assuming it from the original's own presence.
  • The original full-resolution object at the existing key is completely unchanged: pixel-diff (compareCapturedScreenshots) always compares the true originals on both a fresh render and a cache hit, and "click to open full-size" keeps resolving to it.
  • unified-comment-bridge.ts's cell() embeds the thumbnail for <img src> when present, falling back to the full-resolution URL otherwise (hosted mode, mobile rows, or a resize that didn't actually shrink anything) — the href always points at the full-resolution original either way.

Course-correction from #6324's original text, which proposed working in both hosted and self-host modes: hosted mode has no image-resizing primitive available in the Workers runtime (confirmed by downscaleForVision's own pre-existing header comment: "a native image-resizing binding that can't run on the Cloudflare Workers runtime"), so this mirrors that seam's own honest hosted-no-op / self-host-real split rather than inventing new Worker-side infrastructure to force parity.

Closes #6324 (together with #6341, already merged, which added the visible caption line)

Test plan

  • test/unit/image-downscale.test.ts + test/unit/selfhost-image-downscale-stub.test.ts extended: the Worker no-op, and the real sharp resize against actual PNG fixtures (including the exact 1440×2397 dimensions observed live on fix(ui): apply the pre-2000 placeholder rule in normalizeFreshnessSources metagraphed#6036) — 100% coverage on both files
  • test/unit/visual-capture.test.ts extended (11 new tests): thumbnail generation/storage, the desktop-only gate, the skip-when-nothing-shrunk optimization, decode-failure degradation, cache-hit re-verification (including the bug I found and fixed), S3-direct-bucket-URL linking, and — the most important correctness check — that pixel-diff always receives the ORIGINAL bytes, never the downscaled copy, even in the same capture pass that generated a thumbnail
  • test/unit/visual-collapsible.test.ts extended: <img src> prefers the thumb URL while <a href> always uses the full-resolution URL; falls back correctly when no thumb is present
  • All existing tests across these files (159 total) pass unchanged
  • npm run typecheck clean
  • Full local npm run test:ci gate green, twice (before and after resolving a rebase conflict with feat(review): add a visible caption under each Visual/Scroll preview thumbnail #6341's merged caption change)

…t mode

Closes #6324

shot.ts captures full-page, native-resolution PNGs (up to 1440px wide for
the desktop viewport, taller still for a long page's full-page height),
stored and embedded verbatim at <img width="360"> -- every viewer's
browser previously downloaded the full-resolution capture just to render
a 360px-wide thumbnail.

New downscaleForDisplay/isDisplayDownscaleAvailable in image-downscale.ts
(Worker no-op) + src/selfhost/stubs/image-downscale.ts (real sharp-based
implementation, 720px width cap -- 2x the display width for a sharp
retina render, still a real reduction from 1440px) mirror the existing
downscaleForVision seam's own hosted-no-op/self-host-real architecture
exactly. Desktop-only: shot.ts's mobile viewport (390px) is already close
enough to the table's 360px display width that a third resized copy
wouldn't meaningfully help.

capturePage stores the downscaled copy at a SIBLING R2 key on a fresh
render (only when it genuinely shrinks the bytes, and only once the write
is CONFIRMED to succeed -- caught a real bug in my own first pass here,
where thumbUrl was set even when the write silently failed), and
re-verifies the sibling's existence on a cache hit rather than assuming it
from the original's own presence. The ORIGINAL full-resolution object at
the existing key is completely unchanged: pixel-diff (compareCapturedScreenshots)
always compares the true originals on both a fresh render and a cache hit,
and the "click to open full-size" link keeps resolving to it.
unified-comment-bridge.ts's cell() embeds the thumbnail for <img src> when
present, falling back to the full-resolution URL otherwise (hosted mode,
mobile rows, or a resize that didn't actually shrink anything) -- the
href always points at the full-resolution original either way.

Course-correction from #6324's original text, which proposed working in
both hosted and self-host modes: hosted mode has no image-resizing
primitive available in the Workers runtime (confirmed by
downscaleForVision's own pre-existing header comment), so this mirrors
that seam's own honest hosted-no-op / self-host-real split rather than
inventing new Worker-side infrastructure to force parity.
@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 16, 2026
@loopover-orb

loopover-orb Bot commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Warning

⏸️ LoopOver review result - manual review recommended

Review updated: 2026-07-16 04:27:38 UTC

8 files · 1 AI reviewer · 1 blocker · CI green · clean

⏸️ Suggested Action - Manual Review

Review summary
This PR adds a self-host-only downscaled DISPLAY thumbnail for desktop before/after screenshots, mirroring the existing downscaleForVision hosted-no-op/self-host-real seam, storing the thumb at a sibling R2 key and threading it through to the comment table's <img src> while keeping <a href> and the pixel-diff path pinned to the true original bytes. The implementation correctly gates thumb storage on a confirmed write (fixing the described bug where a bare `.catch(() => undefined)` would have silently set a broken URL), re-verifies the sibling's existence on cache hits rather than assuming it, and only stores the thumb when it genuinely shrinks the bytes. Test coverage is thorough, exercising both isDisplayDownscaleAvailable branches, write/read failure paths, and explicitly asserting the diff provider always receives original bytes even when a thumb was generated in the same call.

Nits — 5 non-blocking
  • src/review/visual/capture.ts: `thumbUrl` computation duplicates `resolveShotUrl(env, thumbKey) || undefined` in three places (cache-hit, fresh-render); consider a small local helper to DRY this up.
  • src/review/unified-comment-bridge.ts:466 `cell`'s default parameter `imgUrl: string = url ?? ""` is a bit indirect — an explicit `imgUrl ?? url` at call sites would read more clearly than relying on default-param fallback plus the `??` in the caller.
  • Consider extracting the repeated `resolveShotUrl(env, thumbKey) || undefined` pattern in capture.ts into a tiny local function shared by the cache-hit and fresh-render branches.
  • The doc comments are very long (multi-paragraph JSDoc blocks); a future pass could trim these once the seam is well-established, since the code itself is now self-explanatory.
  • 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.

Concerns raised — review before merging

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
📋 Copy for AI agents — paste into your coding agent
Fix the following blocker(s) from this PR review:

1. No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.

Decision drivers

  • ❌ Code review — 1 blocker (1 reviewer)
  • ❌ Gate result — Blocking (Repo-configured hard blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #6324
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: 42 registered-repo PR(s), 34 merged, 408 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 42 PR(s), 408 issue(s).
Improvement ✅ Minor risk: clean · value: minor
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 42 PR(s), 408 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Treat this as maintainer-lane context rather than normal contributor-lane activity.
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.
🧪 Chat with LoopOver

Ask LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

Full command reference: https://gittensory.aethereal.dev/docs/loopover-commands

🧪 Experimental — new and may change.

🟩 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 LoopOver, a quiet PR intelligence layer for OSS maintainers.

  • Re-run LoopOver review

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 16, 2026
@JSONbored
JSONbored merged commit 188b08f into main Jul 16, 2026
16 checks passed
@JSONbored
JSONbored deleted the feat/visual-preview-display-downscale-v2 branch July 16, 2026 04:42
@codecov

codecov Bot commented Jul 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 83.33333% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.59%. Comparing base (88953e8) to head (ed166ab).
⚠️ Report is 27 commits behind head on main.

Files with missing lines Patch % Lines
src/review/visual/capture.ts 76.92% 0 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6347      +/-   ##
==========================================
- Coverage   95.60%   95.59%   -0.01%     
==========================================
  Files         597      597              
  Lines       47191    47203      +12     
  Branches    15019    15026       +7     
==========================================
+ Hits        45117    45126       +9     
  Misses       1290     1290              
- Partials      784      787       +3     
Flag Coverage Δ
shard-1 43.96% <16.66%> (-0.19%) ⬇️
shard-2 36.79% <11.11%> (+0.39%) ⬆️
shard-3 32.37% <16.66%> (-0.17%) ⬇️
shard-4 34.06% <77.77%> (-0.57%) ⬇️
shard-5 31.54% <16.66%> (-0.04%) ⬇️
shard-6 45.30% <16.66%> (+0.43%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/review/unified-comment-bridge.ts 99.60% <100.00%> (ø)
src/review/visual/image-downscale.ts 100.00% <100.00%> (ø)
src/review/visual/capture.ts 94.90% <76.92%> (-1.21%) ⬇️

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. manual-review Gittensor contributor context

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Downscale display images and add visible captions to the Visual preview table

1 participant