feat(viewer): add image preview via chafa#55
Conversation
Renders image files as ANSI character art in the internal viewer (F3). Auto-detects image/* MIME types and switches to Image mode. Toggle between image and hex mode with h. - Spawn chafa --size WxH in event loop, cache parsed ANSI Text - Remove RefCell from cache fields -- render path is pure read-only - Add is_image_mime() helper, -- safety flag for chafa invocation - toggle_wrap / toggle_line_numbers no-op in Image mode - Show placeholder while chafa runs, error if chafa missing - Add 6 tests for Image mode, 957 total passing
Reviewer's GuideAdds an Image view mode to the internal viewer that renders image/* files as ANSI art using chafa, wires it into the main event loop and renderer, and documents the feature and dependency. Sequence diagram for image preview generation in viewersequenceDiagram
participant App as run_app
participant VS as ViewerState
participant Chafa
participant AnsiToTui
App->>VS: prepare_image_preview(area_width, area_height)
alt cached_image_size matches
VS-->>App: Ok(())
else needs_new_preview
VS->>Chafa: Command::new(chafa).arg(--size WxH).arg(file_path).output()
Chafa-->>VS: Output { status, stdout, stderr }
alt status.success
VS->>AnsiToTui: stdout.into_text()
AnsiToTui-->>VS: Text
VS->>VS: cached_image_text = Some(Text)
else failure_or_parse_error
VS->>VS: cached_image_text = Some(Text::raw(error_message))
end
VS->>VS: cached_image_size = Some((area_width, content_height))
VS-->>App: Ok(())
end
App->>App: terminal.draw(render_ui)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
toggle_line_numbersandtoggle_wrap, theis_hex_modecheck was effectively inverted compared to the previous behavior (now forcing a Hex view into Text on toggle instead of leaving Hex unchanged), which is likely unintended and will change existing viewer behavior. prepare_image_previewrunschafasynchronously from the main event loop on each size change; consider moving this into the existing background job infrastructure (similar toViewerLoader) so that large images or slowchafacalls don't block UI rendering.- If spawning
chafafails (e.g., not installed),prepare_image_previewreturns an error andcached_image_textstaysNone, so the UI shows "Generating preview…" indefinitely; you may want to catch this error, set a user-visible error message incached_image_text, and/or special-caseNotFoundto match the README message.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `toggle_line_numbers` and `toggle_wrap`, the `is_hex_mode` check was effectively inverted compared to the previous behavior (now forcing a Hex view into Text on toggle instead of leaving Hex unchanged), which is likely unintended and will change existing viewer behavior.
- `prepare_image_preview` runs `chafa` synchronously from the main event loop on each size change; consider moving this into the existing background job infrastructure (similar to `ViewerLoader`) so that large images or slow `chafa` calls don't block UI rendering.
- If spawning `chafa` fails (e.g., not installed), `prepare_image_preview` returns an error and `cached_image_text` stays `None`, so the UI shows "Generating preview…" indefinitely; you may want to catch this error, set a user-visible error message in `cached_image_text`, and/or special-case `NotFound` to match the README message.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Greptile SummaryThis PR adds image preview to the internal viewer (F3) by auto-detecting
Confidence Score: 3/5Not ready to merge as-is: the chafa call blocks the event loop on every image open and resize, and a missing chafa binary causes a silent per-frame retry loop with no user-visible error. The blocking subprocess on the main thread degrades the TUI on every image open, directly violating the project's event-loop contract. When chafa is absent, the silent error discard produces an infinite retry loop each dirty frame and contradicts the documented error message. Both issues affect the changed code path on every user interaction with the feature. src/ui/viewer.rs (prepare_image_preview error handling and size calculation) and src/main.rs (blocking call placement in event loop) Important Files Changed
Sequence DiagramsequenceDiagram
participant EL as Event Loop (main.rs)
participant VS as ViewerState
participant CH as chafa (subprocess)
participant ATT as ansi-to-tui
participant UI as render_image_view
EL->>VS: "dirty=true → prepare_image_preview(w, h)"
VS->>VS: check cached_image_size (cache miss)
VS->>CH: Command::new("chafa").output() [BLOCKING]
CH-->>VS: stdout (ANSI bytes) / io::Error if missing
Note over VS,CH: Error path: io::Error propagated via ?<br/>call site uses let _ = ..., error silently dropped<br/>cached_image_size stays None → retries every dirty frame
VS->>ATT: stdout.into_text()
ATT-->>VS: "Text<'static>"
VS->>VS: "cached_image_size = Some((w, h-2))<br/>cached_image_text = Some(text)"
EL->>UI: terminal.draw → render_image_view_with_colors
UI->>UI: "content_area.height = h-3 (renders h-3 lines)<br/>chafa produced h-2 lines → last line clipped"
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if let Some(vs) = viewer_state.as_mut() | ||
| && vs.view_mode == ViewMode::Image | ||
| && let Ok(size) = terminal.size() | ||
| { | ||
| let _ = vs.prepare_image_preview(size.width, size.height); | ||
| } |
There was a problem hiding this comment.
prepare_image_preview calls std::process::Command::output(), which blocks the calling thread until the chafa process exits. Because this is called directly inside the if dirty block of the main event loop, every first load and every terminal resize stalls keyboard/mouse event processing for the full duration of chafa execution. On large images or slow terminals this is visibly noticeable, and on a busy system it can take seconds. The project's AGENTS.md explicitly states: "offload heavy work via rayon or app::job_runner" — the same pattern used for directory reads and file operations. The chafa invocation should be dispatched as a background job and its result sent back through a channel, leaving the event loop free to process input and redraw.
|
|
||
| pub fn prepare_image_preview(&mut self, area_width: u16, area_height: u16) -> io::Result<()> { | ||
| let content_height = area_height.saturating_sub(2); | ||
| if area_width == 0 || content_height == 0 { | ||
| return Ok(()); | ||
| } | ||
| if self.cached_image_size == Some((area_width, content_height)) { |
There was a problem hiding this comment.
prepare_image_preview computes content_height = area_height.saturating_sub(2), so it tells chafa to produce output H-2 lines tall. However, render_image_view_with_colors subtracts 3 from the same height: Margin { vertical: 1 } removes 2 lines (top+bottom borders), and then inner_area.height.saturating_sub(1) removes a further line for the status bar. The actual content area rendered is H-3, one line shorter than the chafa canvas. The last row of chafa output is clipped or pushed into the status bar area.
| fn is_image_mime(mime: &Option<String>) -> bool { | ||
| mime.as_ref().is_some_and(|m| m.starts_with("image/")) | ||
| } |
There was a problem hiding this comment.
is_image_mime takes &Option<String> where Option<&str> would be more idiomatic
Passing &Option<T> and calling .as_ref() internally is less ergonomic than accepting Option<&str> directly, which is the canonical Rust pattern for borrowed optional strings.
| fn is_image_mime(mime: &Option<String>) -> bool { | |
| mime.as_ref().is_some_and(|m| m.starts_with("image/")) | |
| } | |
| fn is_image_mime(mime: Option<&str>) -> bool { | |
| mime.is_some_and(|m| m.starts_with("image/")) | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an image preview feature to the file viewer by utilizing chafa to render images as ANSI character art. Key changes include adding the ansi-to-tui dependency, updating ViewerState to support image detection and caching, and implementing the corresponding UI rendering logic. Review feedback identifies a significant responsiveness issue caused by executing the chafa process synchronously within the main event loop, which can freeze the UI. Furthermore, improvements were suggested to correct the image height calculation to avoid UI overlap and to provide better error messaging if the chafa executable is not found.
| if let Some(vs) = viewer_state.as_mut() | ||
| && vs.view_mode == ViewMode::Image | ||
| && let Ok(size) = terminal.size() | ||
| { | ||
| let _ = vs.prepare_image_preview(size.width, size.height); | ||
| } |
There was a problem hiding this comment.
Spawning a process and waiting for its output (Command::output()) inside the main event loop is a blocking operation. This will cause the UI to freeze while chafa is processing the image, which can be significant for large files or slow systems. Consider moving this to a background thread or integrating it into the existing ViewerLoader mechanism to maintain responsiveness.
| pub fn prepare_image_preview(&mut self, area_width: u16, area_height: u16) -> io::Result<()> { | ||
| let content_height = area_height.saturating_sub(2); | ||
| if area_width == 0 || content_height == 0 { | ||
| return Ok(()); | ||
| } | ||
| if self.cached_image_size == Some((area_width, content_height)) { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let size_str = format!("{}x{}", area_width, content_height); | ||
| let output = std::process::Command::new("chafa") | ||
| .arg("-f") | ||
| .arg("symbols") | ||
| .arg("--size") | ||
| .arg(&size_str) | ||
| .arg("--") | ||
| .arg(&self.file_path) | ||
| .output()?; | ||
|
|
||
| let parsed_text = if output.status.success() { | ||
| match output.stdout.into_text() { | ||
| Ok(text) => text, | ||
| Err(e) => Text::raw(format!("Failed to parse ANSI: {}", e)), | ||
| } | ||
| } else { | ||
| let err_msg = String::from_utf8_lossy(&output.stderr); | ||
| Text::raw(format!("Chafa error: {}", err_msg)) | ||
| }; | ||
|
|
||
| self.cached_image_size = Some((area_width, content_height)); | ||
| self.cached_image_text = Some(parsed_text); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The image preview logic has two issues:
- Height calculation mismatch: The height calculation for
chafasubtracts 2, butrender_image_view_with_colorsuses 3 lines for overhead (2 for borders, 1 for status bar). This will cause the image to be one line too tall, potentially overlapping the status bar or being truncated. - Error handling: If the
chafaexecutable is missing,Command::output()returns anio::Error. The current use of?propagates this error, but it is ignored in the main loop, leaving the user with a "Generating preview..." message indefinitely. It's better to catch the error here and return aTextobject containing a helpful error message.
pub fn prepare_image_preview(&mut self, area_width: u16, area_height: u16) -> io::Result<()> {
let content_height = area_height.saturating_sub(3);
if area_width == 0 || content_height == 0 {
return Ok(());
}
if self.cached_image_size == Some((area_width, content_height)) {
return Ok(());
}
let size_str = format!("{}x{}", area_width, content_height);
let output = std::process::Command::new("chafa")
.arg("-f")
.arg("symbols")
.arg("--size")
.arg(&size_str)
.arg("--")
.arg(&self.file_path)
.output();
let parsed_text = match output {
Ok(out) if out.status.success() => match out.stdout.into_text() {
Ok(text) => text,
Err(e) => Text::raw(format!("Failed to parse ANSI: {e}")),
},
Ok(out) => {
let err_msg = String::from_utf8_lossy(&out.stderr);
Text::raw(format!("Chafa error: {err_msg}"))
}
Err(e) => Text::raw(format!("Failed to execute chafa (is it installed?): {e}")),
};
self.cached_image_size = Some((area_width, content_height));
self.cached_image_text = Some(parsed_text);
Ok(())
}…semantics - Fix content_height: saturating_sub(2) → saturating_sub(3) to match actual render area (2 border + 1 status bar = 3) - Catch chafa spawn failure in prepare_image_preview, store error message in cache so user sees it instead of indefinite placeholder - Restore old !is_hex_mode() condition in toggle_wrap/toggle_line_numbers so Hex mode is not forcibly switched to Text on toggle - Change is_image_mime signature: &Option<String> → Option<&str>
1 participant
Renders image files as ANSI character art in the internal viewer (F3). Auto-detects image/* MIME types and switches to Image mode. Toggle between image and hex mode with h.
Summary by Sourcery
Add image preview support to the built-in viewer using chafa and integrate it into the viewing workflow.
New Features:
Enhancements:
Build:
Documentation:
Tests: