feat(review): Before/After preview for changed images in code review (#1598) (#1601)
* feat(review): serve Before/After bytes of changed images via /api/review-image (#1598) Split "which object is each side" from "read it" (resolveDiffSideSources and its jj/GitButler siblings), so hunk expansion (text) and the image preview (bytes) share one ref table. Byte readers on both runtimes (git cat-file, jj file show, capped worktree reads with symlink/realpath containment), GitHub/GitLab base64 byte fetchers, and one shared request handler in packages/shared/review-image.ts (eligibility, caps, magic-byte sniffing, header dimensions, error mapping, nosniff + sandbox CSP + CORP headers). The Bun and Pi servers only supply where the current mode reads a side, and advertise imagePreviewSupported on every diff payload. * feat(review): Before/After preview for changed images in code review (#1598) ImageDiffPreview replaces the binary notice for a hunkless image chunk in the single-file and all-files views when the server advertises imagePreviewSupported: Before | After panes (one pane for added/deleted, "Contents unchanged" for identical bytes), dimensions and size deltas, a theme-token checkerboard, stacking for narrow/compact layouts and tall images. Lazy (IntersectionObserver), abortable, object-URL LRU per snapshot; SVG only via <img src=blob:>. The all-files view takes it through a render prop only the review app sets, so guide hosts keep the notice. Regenerated the guides.show viewer manifest; docs in AGENTS.md. * fix(review): address #1601 review: abortable image reads, bounded client cache, dotted file names - Server read limiter takes the request abort signal (Bun req.signal, Pi response close): queued reads for cards scrolled past are dropped before they run, and no gh/glab call starts for a request that is gone. - Client cache caps total bytes (128 MB) besides entries, never revokes an object URL a mounted <img> still uses, and ignores a response that lands after the page moved to another snapshot. - validateFilePath rejects ".." path segments (and absolute paths), not any ".." substring, so a..b.png previews; traversal stays refused, tested on /api/review-image and /api/file-content in both runtimes. - Only an HTTP 404 means a PR file is missing; a missing CLI is a 502. - All-files panes size to the image (same max); misplaced comments moved; AGENTS.md documents freshness, aborts and the cache bounds. * fix(review): refuse dot-only path segments; keep the first URL for overlapping image fetches - validateFilePath rejects any segment made only of dots, not just "..": "..." is Perforce's recursive wildcard and /api/file-content passes the path to p4 print in P4 sessions (a whole-subtree read). a..b.png stays valid. - fetchReviewImage: when two fetches for the same side overlap, the later response revokes its own URL and returns the cached entry instead of revoking a URL a card may already be showing.
M
Michael Ramos committed
359303e7009f5f8a1685b7df46a2f343e0819ca0
Parent: 33e12ad
Committed by GitHub <noreply@github.com>
on 9/23/2026, 8:26:35 PM