Repository navigation
feat(pager): add inline image diff previews - #39
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50c971ec4d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if key.kind == KeyEventKind::Press | ||
| && key.modifiers == KeyModifiers::ALT | ||
| && matches!(key.code, KeyCode::Char(']' | 'P' | '^' | '_')) | ||
| { | ||
| consume_control_string(source, Instant::now() + SEQUENCE_DEADLINE)?; |
There was a problem hiding this comment.
When a user presses Alt+], Alt+Shift+P, Alt+^, or Alt+_—including via a custom binding such as <a-P>—this branch always treats the key as a terminal control string, discards it, and consumes subsequent input until a terminator or timeout. Since the capability probe runs only during startup, control-string filtering should be limited to an actual pending reply or validate the following payload rather than permanently making these legitimate Alt combinations unusable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 17af522. The broad Alt+], P, ^, _ discard is gone. Only Alt+_ uses private, bounded lossless lookahead; a complete validated Kitty reply is suppressed, while malformed/incomplete candidates and unrelated events are returned in order. Split APC uses the same recognizer. The input module's caller interface is unchanged (codebase-design: keep protocol/replay knowledge local at the existing input seam).
Added eight regression tests; four were confirmed failing before the fix. All 267 tests pass, cargo fmt --check and git diff --check pass, and Clippy completes with existing warnings. Live-terminal smoke testing remains outstanding and is documented in the updated merge-confidence section.
- Render Before/After previews for PNG, JPEG, GIF, WebP, BMP, ICO, and TIFF in Kitty and Ghostty, with Added/Deleted full-width states backed by worktree, index, and revision blobs - Add background decode with memory/dimension limits, 20 MiB cap, and striped binary fallback for unsupported terminals, oversized files, and corrupt images - Support capped inline image sections in folder diffs across split/unified and wrapped views with viewport-driven loading and cache-aware unloading - Add diff path parsing for quoted/renamed headers, terminal graphics protocol handling, `LAZYGITRS_IMAGE_PREVIEW` opt-out/experimental flags, and tmux/screen/Zellij fallback
50c971e to
09fc1bb
Compare
Summary
Documentation / limitations
README documents supported formats (PNG, JPEG, GIF first frame, WebP, BMP, ICO, TIFF), terminal support, and
LAZYGITRS_IMAGE_PREVIEW=off.Files over 20 MiB, SVG/video, and tmux/screen/Zellij retain placeholders. iTerm2/WezTerm and Sixel support is experimental; whole-commit overviews retain placeholders.
Validation
cargo test— 267 passed (after input fix17af522)cargo fmt --checkgit diff --checkcargo clippy --all-targets— completed successfully, with existing warnings (not warning-clean)Live terminal rendering was not manually verified in this pass.
Merge confidence — Codex review
8.5/10 (high, still conditional on a live-terminal smoke test). Updated after
17af522; this is a qualitative engineering assessment, not a measured failure probability.Reviewed only this branch's changes against
origin/main. Refactorf327a09isolated capability probing, bounded source loading, and inline diff eligibility/source selection behind private module seams, with 11 regression tests. Follow-up17af522addresses the automated review's P2: genuine Alt+], Alt+P, Alt+^, and Alt+_ are no longer unconditionally discarded along with subsequent input.Design: the input module retains its existing caller interface. A private recognizer buffers bounded lookahead and suppresses only a complete validated Kitty reply; malformed, incomplete, unrelated, and non-key events are returned in order. The split-APC and already-parsed Alt+_ paths share this implementation; unrelated parser behavior was left alone. No reply state or replay management was added to GUI callers.
Evidence: all 267 local tests pass; formatting and diff checks are clean; Clippy completes with existing warnings. Eight new input tests cover genuine Alt shortcuts, following input, split APC, malformed/truncated candidates, non-key events, success/error replies and both terminator encodings, buffer limits, and the short initial probe. Four regression tests were confirmed failing before the fix. Final read-only review found no blockers in the fix.
Why confidence increased: the known input-loss regression is now addressed and covered, rather than left as a pre-merge issue.
Remaining risk: automated/test-backend coverage does not verify actual terminal graphics, scoped deletion, or overlay cleanup. No live Kitty/Ghostty smoke test was performed; experimental iTerm2/WezTerm/Sixel cleanup remains explicitly unverified. Alt+_ remains inherently ambiguous with APC and uses a short 15 ms initial probe; a genuine keystroke sequence identical to a complete protocol reply cannot be distinguished at this event seam. The reported GitHub checks are release-planning checks with artifact builds skipped, not a substitute for cross-platform build/runtime validation.
Before merging: smoke-test Kitty or Ghostty with a changed image, added/deleted images, mixed text/image folder diffs while scrolling, opening/closing an overlay, the affected Alt bindings followed by ordinary input, and
LAZYGITRS_IMAGE_PREVIEW=offfallback. If those pass, I would recommend merging. GitHub repository review/check requirements must also be satisfied before merging.