src/ui/graphics.rs (2)

64-89: Consider adding timeout or size limit for subprocess and image loading.

The load_cclip_image method spawns a subprocess and reads all output into memory without any size limit or timeout. For large images or a hung cclip process, this could cause issues.

💡 Suggested improvements

1. Consider adding a timeout for the subprocess
2. Consider limiting the maximum image size to prevent OOM on very large images

     pub fn load_cclip_image(&mut self, rowid: &str) -> Result<()> {
+        const MAX_IMAGE_SIZE: usize = 50 * 1024 * 1024; // 50MB limit
+        
         // Run cclip get to fetch image bytes
         let mut child = Command::new("cclip")
             .args(["get", rowid])
             .stdout(Stdio::piped())
             .stderr(Stdio::null())
             .spawn()?;
 
         let mut bytes = Vec::new();
         if let Some(mut stdout) = child.stdout.take() {
-            stdout.read_to_end(&mut bytes)?;
+            stdout.take(MAX_IMAGE_SIZE as u64).read_to_end(&mut bytes)?;
         }
         child.wait()?;

---

42-49: LGTM - Consider simplifying the match expression.

The logic correctly identifies high-resolution graphics support. Minor style suggestion:

✨ Simpler pattern

     pub fn supports_graphics(&self) -> bool {
         use ratatui_image::picker::ProtocolType;
-        match self.picker.protocol_type() {
-            ProtocolType::Halfblocks => false,
-            _ => true,
-        }
+        !matches!(self.picker.protocol_type(), ProtocolType::Halfblocks)
     }

src/modes/cclip/run.rs (3)

216-218: Consider preserving error context from ImageManager initialization.

Using .ok() silently discards the error. While the warning at lines 254-257 helps, the actual error details are lost. Consider logging the error for debugging.

💡 Preserve error details

     // Initialize ImageManager for ratatui-image
-    let mut image_manager = crate::ui::ImageManager::new().ok();
+    let mut image_manager = match crate::ui::ImageManager::new() {
+        Ok(manager) => Some(manager),
+        Err(e) => {
+            // Debug logging could help diagnose terminal detection issues
+            if cli.verbose.unwrap_or(0) >= 2 {
+                eprintln!("debug: ImageManager init failed: {}", e);
+            }
+            None
+        }
+    };

---

421-430: Silent failure on image load could leave UI in inconsistent state.

If load_cclip_image fails (line 425), the error is silently ignored but the DISPLAY_STATE is still updated to Image. This could show stale content or nothing.

💡 Handle load failure explicitly

             if current_is_image {
                 if let (Some(rowid), Some(manager)) = (&current_rowid_opt, &mut image_manager) {
                     if displayed_rowid_opt.as_deref() != Some(rowid) {
                         // Load the new image
-                        if manager.load_cclip_image(rowid).is_ok() {
-                            if let Ok(mut state) = crate::ui::DISPLAY_STATE.lock() {
-                                *state = crate::ui::DisplayState::Image(ratatui::layout::Rect::default(), rowid.clone());
-                            }
+                        match manager.load_cclip_image(rowid) {
+                            Ok(()) => {
+                                if let Ok(mut state) = crate::ui::DISPLAY_STATE.lock() {
+                                    *state = crate::ui::DisplayState::Image(ratatui::layout::Rect::default(), rowid.clone());
+                                }
+                            }
+                            Err(_) => {
+                                // Clear state on load failure to avoid showing stale image
+                                manager.clear();
+                                if let Ok(mut state) = crate::ui::DISPLAY_STATE.lock() {
+                                    *state = crate::ui::DisplayState::Empty;
+                                }
+                            }
                         }
                     }
                 }

---

316-318: Minor formatting artifact.

There appears to be trailing whitespace after the constraint array closing bracket. This is a minor formatting issue.

src/modes/cclip/run.rs (2)

216-223: eprintln! during alternate screen may flash briefly.

At this point the terminal backend is on stderr, so eprintln! writes directly to the alternate screen buffer. The next terminal.draw() will overwrite it, but it could flash for one frame. Consider using log::warn! or deferring the warning.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/modes/cclip/run.rs` around lines 216 - 223, Replace the direct stderr
write (eprintln!) in the ImageManager initialization with a logging call so the
warning doesn't flash on the alternate screen; specifically update the
ImageManager::new() match in run.rs (the initialization that sets image_manager)
to call log::warn! (or another configured logger) with the error details instead
of eprintln!, ensuring you keep the same message context and include the error
variable `e` so the warning is captured by the configured logger rather than
written to stderr.

---

1386-1403: Unify mouse-click copy operation with CclipItem::copy_to_clipboard.

The Enter/Ctrl+Y handler cleanly delegates to CclipItem::copy_to_clipboard, but the mouse-click handler still manually constructs the cclip get → wl-copy/xclip piping. Refactor the mouse-click path to use CclipItem::from_line() and copy_to_clipboard() to eliminate duplication and improve maintainability.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/modes/cclip/run.rs` around lines 1386 - 1403, The mouse-click handler
currently duplicates the manual `cclip get` → `wl-copy`/`xclip` piping; refactor
it to parse the clicked line with
super::CclipItem::from_line(original_line.clone()) and call the existing
CclipItem::copy_to_clipboard() (same as the Enter/Ctrl+Y path), handling Err by
calling ui.set_temp_message(format!("Copy failed: {}", e)) and continuing; on
success perform the same terminal cleanup sequence
(terminal.show_cursor().wrap_err(...)?; drop(terminal); execute
DisableMouseCapture and LeaveAlternateScreen on stderr; disable_raw_mode();
return Ok(())). Use the same error handling and flow as in the Enter/Ctrl+Y
branch to eliminate duplication.

src/ui/dmenu_ui.rs (1)

678-690: Consider using splitn for consistency with other parsing methods.

get_image_info uses split('\t') while sibling methods (is_cclip_image_item, get_cclip_content_for_display, get_cclip_rowid) consistently use splitn. This works correctly but is a minor inconsistency.

♻️ Optional: align with existing parsing style

     pub fn get_image_info(&self, item: &crate::common::Item) -> String {
         if !self.is_cclip_image_item(item) {
             return String::new();
         }
 
-        let parts: Vec<&str> = item.original_line.split('\t').collect();
+        let parts: Vec<&str> = item.original_line.splitn(4, '\t').collect();
         if parts.len() >= 3 {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/dmenu_ui.rs` around lines 678 - 690, get_image_info currently uses
item.original_line.split('\t') which is inconsistent with sibling parsers;
change it to use splitn(3, '\t') so it behaves like is_cclip_image_item,
get_cclip_content_for_display, and get_cclip_rowid, then collect/check the
resulting parts as before (ensure you still read the third part when parts.len()
>= 3 and return "Unknown Image" otherwise).

src/ui/graphics.rs (1)

105-127: Dual graphics detection: GraphicsAdapter::detect() vs ImageManager picker.

GraphicsAdapter::detect() uses env-var heuristics while ImageManager uses actual terminal protocol negotiation (Picker::from_query_stdio). They can disagree — e.g., a Sixel-capable terminal not matching the env-var check would get GraphicsAdapter::None, skipping needed Clear widget calls.

Currently GraphicsAdapter is only used in dmenu/run.rs (which has no image rendering), so this is low-risk, but worth noting for future maintenance.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/graphics.rs` around lines 105 - 127, GraphicsAdapter::detect()
currently uses environment-variable heuristics and can disagree with the runtime
protocol negotiation used by ImageManager (Picker::from_query_stdio), causing
mismatches (e.g., Sixel capable terminals being classified as None and skipping
Clear widget calls); update GraphicsAdapter::detect() to prefer actual protocol
negotiation by calling the same detection logic as ImageManager (use
Picker::from_query_stdio or a shared helper) and map its result to
GraphicsAdapter variants, ensuring codepaths that rely on GraphicsAdapter (e.g.,
dmenu/run.rs) get the same capability decision as ImageManager so Clear and
other image-related widgets are executed consistently.

src/ui/graphics.rs (1)

42-58: ProtocolType import is repeated in every method — move to module level.

The use ratatui_image::picker::ProtocolType; statement is duplicated across supports_graphics, is_kitty, and is_sixel. A single top-level use would be cleaner.

♻️ Proposed refactor

Add at the top of the file alongside other imports:

+use ratatui_image::picker::ProtocolType;

Then remove the three inner use statements.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/graphics.rs` around lines 42 - 58, Move the repeated import into the
module scope: add a single top-level `use ratatui_image::picker::ProtocolType;`
alongside other imports, then remove the three inner `use` lines inside the
methods `supports_graphics`, `is_kitty`, and `is_sixel`; ensure the methods
continue to call `self.picker.protocol_type()` and pattern-match against
`ProtocolType::Halfblocks`, `ProtocolType::Kitty`, and `ProtocolType::Sixel`
respectively.

src/modes/cclip/run.rs (3)

313-343: Pre-loop layout calculation is stale after a terminal resize.

Lines 352–362 call ui.info_with_image_support using content_panel_width/height derived from the terminal size at startup. The draw closure (Lines 582–596) recalculates this correctly every frame, so the mismatch only affects the very first draw. If that initial sizing diverges (e.g., the user starts in a small window), the first-rendered text wrap width will be wrong until the next key/mouse event triggers a redraw.

Consider moving the initial ui.info_with_image_support call inside the loop (or removing it entirely, since the draw loop calls it unconditionally), rather than duplicating the layout math outside.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/modes/cclip/run.rs` around lines 313 - 343, The initial pre-loop layout
uses content_panel_width/content_panel_height (calculated before entering the
main draw loop) and calls ui.info_with_image_support with those stale sizes,
causing the first frame to render with incorrect wrapping after a terminal
resize; remove that pre-loop call or move the ui.info_with_image_support
invocation into the same per-frame draw closure (the closure around draw that
already recalculates layout every frame) so that ui.info_with_image_support
always receives the up-to-date content_panel_width/content_panel_height used by
the draw closure; locate references to ui.info_with_image_support and the
pre-loop content_panel_* calculations and eliminate the duplicate/out-of-date
call so only the per-frame draw path performs the layout and rendering.

---

404-424: DisplayState::Image(Rect, String) — the Rect field is unused.

The Rect is always set to Rect::default() (Line 422) and always destructured as _ when read (Line 406). It carries no runtime information. Simplify to DisplayState::Image(String) to reduce confusion.

♻️ Proposed refactor

In src/ui/graphics.rs:

 pub enum DisplayState {
     Empty,
-    Image(Rect, String),
+    Image(String),
 }

In src/modes/cclip/run.rs:

-    crate::ui::DisplayState::Image(_, displayed_rowid) => Some(displayed_rowid.clone()),
+    crate::ui::DisplayState::Image(displayed_rowid) => Some(displayed_rowid.clone()),
...
-    *state = crate::ui::DisplayState::Image(ratatui::layout::Rect::default(), rowid.to_string());
+    *state = crate::ui::DisplayState::Image(rowid.to_string());

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/modes/cclip/run.rs` around lines 404 - 424, The DisplayState::Image
variant currently carries an unused Rect and should be simplified to hold only a
String: update the enum definition (in src/ui/graphics.rs) to change
DisplayState::Image(Rect, String) -> DisplayState::Image(String), then update
all places that construct or pattern-match this variant—e.g. in this file adjust
the lock/match on crate::ui::DISPLAY_STATE (replace destructuring that ignores
Rect with just the String), and update the site that sets the state after
loading an image (replace DisplayState::Image(ratatui::layout::Rect::default(),
rowid.to_string()) with DisplayState::Image(rowid.to_string())). Also search for
other uses of DisplayState::Image and adapt them to the new single-String
variant to keep types consistent (e.g., any code that previously expected a Rect
should be removed or refactored).

---

459-466: term_is_foot re-evaluated on every iteration — move outside the loop.

std::env::var("TERM") never changes at runtime. Computing it inside the loop is harmless but wasteful. The check at Lines 837-841 (synchronized-updates teardown) uses the same variable and also benefits from hoisting it.

♻️ Proposed refactor

+    let term_is_foot = std::env::var("TERM")
+        .unwrap_or_default()
+        .starts_with("foot");
+
     loop {
         ...
-        let term_is_foot = std::env::var("TERM")
-            .unwrap_or_default()
-            .starts_with("foot");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/modes/cclip/run.rs` around lines 459 - 466, Compute TERM once before
entering the loop and store it in the existing term_is_foot boolean, then use
that same term_is_foot value inside the loop where the escape sequence is
written (the std::io::stderr() / write_all/flush block) and also reuse it in the
synchronized-updates teardown codepath instead of re-calling
std::env::var("TERM") again; move the
std::env::var("TERM").unwrap_or_default().starts_with("foot") expression out of
the loop so the TERM check is hoisted and both the escape-write block and the
teardown reference the single term_is_foot variable.

src/modes/cclip/run.rs (1)

908-962: Significant code duplication in clipboard history reload logic.

The "reload items → recreate with formatter → preserve selection by rowid → restore scroll" pattern appears three times: delete handler (~Line 908), tag handler (~Line 1149), and untag handler (~Line 1238). Each copy is ~50 lines of near-identical code.

Consider extracting a helper method, e.g.:

fn reload_and_restore(
    ui: &mut DmenuUI,
    tag_filter: Option<&str>,
    formatter: &TagMetadataFormatter,
    show_line_numbers: bool,
    show_tag_color_names: bool,
) { /* shared reload logic */ }

This would reduce ~150 lines to ~50 + 3 call sites.

Also applies to: 1149-1208, 1238-1298

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/modes/cclip/run.rs` around lines 908 - 962, Duplicate clipboard-reload
logic (recreating Item list, preserving/restoring ui.selected and
ui.scroll_offset, filtering) appears in three places; extract it into a helper
like reload_and_restore(ui: &mut DmenuUI, updated_items: Vec<CclipItem>,
formatter: &TagMetadataFormatter, show_line_numbers: bool, show_tag_color_names:
bool) that builds Items (using
cclip_item.get_display_name_with_number_formatter_options /
get_display_name_with_formatter_options and Item::new_simple), sets ui.hidden,
clears ui.shown and calls ui.filter(), then restores selection/scroll with the
same bounds-check logic (ui.selected, ui.scroll_offset) currently duplicated;
replace the three inline blocks with calls to this helper to eliminate ~50-line
duplicates.

src/ui/graphics.rs (3)

45-58: supports_graphics() excludes Halfblocks — intentional?

Halfblocks is a valid rendering backend in ratatui-image (uses Unicode half-block characters). By returning false for it, image previews will be disabled on terminals without Kitty/Sixel support, even though Halfblocks provides a usable (if lower-fidelity) fallback. If this is intentional, a brief doc comment explaining the rationale would help future readers.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/graphics.rs` around lines 45 - 58, supports_graphics currently
excludes ProtocolType::Halfblocks which prevents using the half-block Unicode
fallback; update supports_graphics to treat Halfblocks as supported (e.g.,
return true for ProtocolType::Halfblocks or invert to check for a
non-none/unsupported variant instead of excluding Halfblocks) and add a short
doc comment explaining that Halfblocks is a lower-fidelity but valid fallback;
refer to the supports_graphics function, picker.protocol_type(), and
ProtocolType::Halfblocks when making the change.

---

80-91: child.wait().await on Line 84 runs outside the timeout boundary.

After read_future completes (within the 1.5 s timeout), the child.wait().await at Line 84 is unbounded. If the child process doesn't exit after stdout is consumed (e.g., it's stuck on stderr or a signal handler), this blocks the async executor indefinitely. This is unlikely in practice since stdout EOF typically triggers process exit, but a second timeout or try_wait loop would make it robust.

♻️ Suggested improvement

                 Ok(res) => {
-                    let _ = child.wait().await;
+                    // Best-effort wait with a short timeout; kill if stuck
+                    if tokio::time::timeout(
+                        std::time::Duration::from_millis(500),
+                        child.wait(),
+                    )
+                    .await
+                    .is_err()
+                    {
+                        let _ = child.kill().await;
+                    }
                     res?
                 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/graphics.rs` around lines 80 - 91, The current code calls
child.wait().await after read_future completes which runs outside the
tokio::time::timeout and can block the executor; wrap the process wait in a
bounded operation — e.g., replace the direct child.wait().await with a
tokio::time::timeout(...) around child.wait(), or implement a non-blocking loop
using child.try_wait() with small sleeps — so that after Ok(res) you await the
child exit under a second timeout, and on timeout kill the child
(child.kill().await) and return an Err; update the match arm handling for
Ok(res) to use this additional timeout and propagate errors accordingly.

---

33-43: Picker::from_query_stdio is the correct ratatui-image 10.0 API and is properly used in the code.

Both ImageManager::new() and GraphicsAdapter::detect() call Picker::from_query_stdio() to query terminal capabilities. However, they are in separate execution paths: ImageManager is used only in cclip mode (line 214, src/modes/cclip/run.rs), while GraphicsAdapter::detect() is only invoked in dmenu mode (during rendering). In practice, the terminal is not queried twice in a single execution—the concern is mitigated by the architecture.

If the codebase ever evolves to use both in the same flow, consider refactoring GraphicsAdapter to accept a Picker instance rather than re-querying, or storing the picker result for reuse to avoid redundant terminal I/O.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/graphics.rs` around lines 33 - 43, Both ImageManager::new() and
GraphicsAdapter::detect() call Picker::from_query_stdio(), which duplicates
terminal queries; to prevent redundant I/O if both paths are used together,
refactor GraphicsAdapter to accept a Picker (or a shared picker result) instead
of creating its own. Update GraphicsAdapter::detect to take a &Picker or change
the GraphicsAdapter constructor to accept a Picker instance; modify call sites
to pass the existing Picker from ImageManager::new (or a central place that
creates the Picker) and remove the direct Picker::from_query_stdio() call inside
GraphicsAdapter::detect so the picker is created once and reused.

src/modes/cclip/run.rs (4)

797-841: Fullscreen preview: image is not re-loaded after returning from modal.

After the fullscreen loop exits (line 833), DISPLAY_STATE is reset to Empty and previous_was_image is set to false. On the next main-loop iteration, the image-loading block (line 356-378) will detect that displayed_rowid_opt doesn't match the current rowid and will re-trigger load_cclip_image. This means a redundant subprocess call + image decode just to restore the inline preview. The image data is already in manager.protocol — resetting DISPLAY_STATE without clearing manager.protocol would avoid the re-load. Alternatively, skip clearing self.protocol on the clear call and only reset DISPLAY_STATE to Image(rowid) after the modal.

This is a minor performance concern on the happy path, not a correctness bug.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/modes/cclip/run.rs` around lines 797 - 841, The fullscreen modal resets
DISPLAY_STATE to Empty and previous_was_image to false which forces
load_cclip_image to re-run on next loop; instead preserve the loaded image data
in image_manager by NOT clearing manager.protocol (or explicitly repopulating
DISPLAY_STATE to Image(current_rowid) after the modal) so the inline preview can
be restored from manager.protocol without spawning the subprocess again—locate
the modal loop handling around image_manager / manager.render and replace the
DISPLAY_STATE reset with either leaving manager.protocol intact or setting
crate::ui::DISPLAY_STATE to crate::ui::DisplayState::Image(current_rowid) (and
avoid clearing manager.protocol in any terminal.clear/clear-call path).

---

1170-1171: Duplicate comment on consecutive lines.

Line 1170 and 1171 both read // Reload clipboard history to get updated tags.

Proposed fix

                                         } else {
-                                            // Reload clipboard history to get updated tags
                                             // Reload clipboard history to get updated tags
                                             let updated_items_res = if let Some(ref tag_name) =

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/modes/cclip/run.rs` around lines 1170 - 1171, Remove the duplicated
comment line so the comment "// Reload clipboard history to get updated tags"
only appears once immediately above the let updated_items_res = if let Some(ref
tag_name) = ... expression; keep the single descriptive comment and delete the
consecutive duplicate to avoid redundancy around the updated_items_res /
tag_name block.

---

356-378: Image loading blocks the draw loop on the async executor.

manager.load_cclip_image(rowid).await is called synchronously within the main TUI loop iteration, before terminal.draw(...). While the subprocess I/O is async (with a 1.5s timeout), this still delays the frame draw by up to ~2s in the worst case (1.5s read + 0.5s wait timeout). During this time, the UI is unresponsive — no input events are processed and no frames are drawn.

For a "Chill" review, this is acceptable as a known trade-off (the timeout bounds the delay), but consider noting this limitation in a comment or TODO for future improvement (e.g., spawning the load as a background task and rendering a "Loading..." placeholder while in progress).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/modes/cclip/run.rs` around lines 356 - 378, The image load is awaited
inline (manager.load_cclip_image(rowid).await) which blocks the TUI draw/input
loop; instead, turn the synchronous await into an asynchronous background task:
when a new image rowid is detected in the current_is_image branch, immediately
set DISPLAY_STATE to DisplayState::Loading (or similar), clone/convert the
image_manager handle into a thread-safe shareable handle (e.g., Arc/Mutex or a
channel-friendly wrapper), spawn a tokio task (tokio::spawn or equivalent) to
call manager.load_cclip_image(rowid).await off the main loop, and on completion
update DISPLAY_STATE to Image(rowid)/Failed(error) and update failed_rowids via
a thread-safe mechanism or by sending back a message to the main loop; ensure
displayed_rowid_opt/failed_rowids logic is updated from the main loop when the
background task reports results so the main draw loop is not blocked.

---

471-473: Duplicate comment.

Lines 471 and 473 both say // Layout calculation. Remove the duplicate.

Proposed fix

             // Re-detect graphics adapter with the shared picker
             let graphics = crate::ui::GraphicsAdapter::detect(picker.as_ref());
 
-            // Layout calculation
-
             // Layout calculation
             let total_height = f.area().height;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/modes/cclip/run.rs` around lines 471 - 473, Remove the duplicated comment
line "// Layout calculation" so only a single instance remains; locate the two
consecutive "// Layout calculation" comments in run.rs (the ones inside the run
function / layout calculation block) and delete the redundant one to keep
comments concise.

src/ui/graphics.rs (1)

151-160: Env-var fallback heuristic: term.contains("xterm") may over-detect Sixel.

Many terminals set TERM=xterm-256color without actually supporting Sixel. This fallback only activates when Picker::from_query_stdio() failed (so protocol auto-detection wasn't possible), meaning the user is already in a degraded path. The consequence is that GraphicsAdapter::detect might return Sixel for terminals that don't support it, which could affect the clear/sync logic in the draw loop (Sixel-specific clearing paths).

This is low-risk since the actual image rendering goes through the picker's protocol (which would be halfblocks if detection failed), and GraphicsAdapter is only used for clear/sync heuristics. But worth noting.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/graphics.rs` around lines 151 - 160, The current heuristic in
GraphicsAdapter::detect that treats term.contains("xterm") as Sixel is too
permissive; update the detection logic (the block using TERM and TERM_PROGRAM)
to be conservative: remove or restrict the generic term.contains("xterm") check
and instead check for explicit indicators of sixel support (e.g., specific TERM
values like "xterm-kitty", presence of "sixel" in TERM, or known TERM_PROGRAM
values such as "WezTerm" or "kitty"); keep Kitty detection as-is (Self::Kitty)
and ensure this fallback only applies when Picker::from_query_stdio()
auto-detection has failed by leaving that flow unchanged. Ensure you modify the
function where Self::Sixel is returned so only explicit sixel-capable term
strings or trusted TERM_PROGRAM names map to Self::Sixel.

src/modes/cclip/run.rs (1)

251-283: Simplify unreachable image-manager warning branch.

image_manager is always Some due to the halfblocks fallback, so the later warning if image_preview_enabled && picker.is_none() && image_manager.is_none() can never fire. Consider removing the dead branch or folding the logic into the earlier message to avoid confusion.

♻️ Suggested cleanup

-    // warn if image preview is enabled but terminal graphics detection failed (and no fallback)
-    if image_preview_enabled && picker.is_none() && image_manager.is_none() {
-        ui.set_temp_message(
-            "image_preview enabled but terminal graphics detection failed".to_string(),
-        );
-    }
+    // (optional) warning already surfaced above when picker is None

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/modes/cclip/run.rs` around lines 251 - 283, Remove the unreachable
warning that checks image_manager.is_none() because image_manager is always Some
(due to the halfblocks fallback); delete the final if block that reads "if
image_preview_enabled && picker.is_none() && image_manager.is_none()" and, if
you still want to warn when image preview is enabled but terminal graphics
aren't available, fold that check into the earlier picker/is_none branch (use
image_preview_enabled and picker.is_none() there) so the warning is reachable
and uses the existing ui.set_temp_message call instead of a dead branch.

src/modes/cclip/run.rs (1)

1686-1698: Consider preserving scroll position relative to selection.

The current implementation sets ui.scroll_offset = pos (line 1694), which scrolls the selected item to the top of the visible area. If the item was previously in the middle of the viewport, this can be disorienting after tag operations.

A more seamless UX would preserve the item's relative position within the viewport when possible.

♻️ Optional: Preserve relative scroll position

     // Restore selection by finding the same rowid
     if let Some(ref rowid) = selected_rowid {
         if let Some(pos) = ui
             .shown
             .iter()
             .position(|item| item.original_line.split('\t').next() == Some(rowid.as_str()))
         {
             ui.selected = Some(pos);
-            // Restore scroll offset by scrolling selected item to top
-            ui.scroll_offset = pos;
+            // Keep item visible - only adjust scroll if item would be out of view
+            // (Scroll adjustment logic depends on knowing visible height, 
+            // which may require passing it as a parameter)
         } else if !ui.shown.is_empty() {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/modes/cclip/run.rs` around lines 1686 - 1698, When restoring selection
for selected_rowid, preserve the item's relative position in the viewport
instead of forcing it to the top: capture the previous selected index
(ui.selected) and previous ui.scroll_offset, compute relative_offset =
previous_selected_index.saturating_sub(previous_scroll_offset) (use pos when
there was no previous selection), then set ui.selected = Some(pos) and set
ui.scroll_offset = pos.saturating_sub(relative_offset) clamped to valid bounds
(0..=shown.len().saturating_sub(1)). Update the block handling selected_rowid
(refs: selected_rowid, ui.shown, ui.selected, ui.scroll_offset, pos) to perform
that calculation instead of assigning ui.scroll_offset = pos.

src/ui/graphics.rs (1)

7-7: Remove the commented-out import.

Dead commented-out code adds noise.

-// use std::sync::Mutex; (removed)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/graphics.rs` at line 7, Remove the dead commented-out import line "use
std::sync::Mutex;" from src/ui/graphics.rs; simply delete that commented line so
the file contains only active imports and no commented-out dead code.

src/modes/cclip/run.rs (2)

494-495: GraphicsAdapter::detect() re-reads environment variables every frame inside the draw closure.

picker and the env vars (TERM, TERM_PROGRAM) don't change at runtime. Cache the result before the main loop to avoid per-frame overhead.

♻️ Suggested fix

Move detection before the loop:

+    let graphics = crate::ui::GraphicsAdapter::detect(picker.as_ref());
+
     // Main TUI loop
     loop {

Then inside the draw closure, remove the per-frame call:

-            // Re-detect graphics adapter with the shared picker
-            let graphics = crate::ui::GraphicsAdapter::detect(picker.as_ref());

And capture graphics in the closure instead (it's Copy).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/modes/cclip/run.rs` around lines 494 - 495, GraphicsAdapter::detect
currently gets called every frame inside the draw closure using picker.as_ref();
move the call to GraphicsAdapter::detect(picker.as_ref()) outside and before the
main event/render loop (cache the returned graphics value once), then remove the
per-frame call inside the draw closure and capture the cached graphics (it is
Copy) in the closure so the draw path uses the cached GraphicsAdapter instead of
re-reading environment variables (TERM/TERM_PROGRAM) each frame.

---

790-804: Image rendering overlays the status text — verify this is intentional.

When current_is_image is true, both the content paragraph (showing "IMAGE PREVIEW - Ready" header) and the image widget are rendered to overlapping areas. The image renders on top, which means the status text will be hidden when the image is ready. If the image doesn't fill the entire content area, status text may peek through at the edges. If this is intentional (status text is only visible during loading/failure), this is fine. Just confirming the UX intent.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/modes/cclip/run.rs` around lines 790 - 804, The image render currently
draws over the content paragraph when current_is_image is true, hiding the
"IMAGE PREVIEW - Ready" status; fix by either skipping the content paragraph
when an image is available or drawing the status after the image so it remains
visible. Concretely, in run.rs around the image_preview_enabled/current_is_image
block that uses image_manager and manager_lock.render, check whether the manager
has a ready image (via the same manager.try_lock() path) and if so either (A)
avoid rendering the content paragraph that prints the status, or (B) move the
status draw call to execute after manager_lock.render so the status is on top;
update the code paths that produce the content paragraph/status accordingly.

src/modes/cclip/run.rs (2)

265-276: Warning fires for all users when picker detection fails, even those who don't use image preview.

When picker.is_none() and image_preview_enabled is false, displaying "Terminal graphics detection failed (using half-block fallback)" is noisy for users who never use image preview. Consider suppressing the message entirely when image_preview_enabled is false.

Suggested fix

     // Show initialization warnings/errors
-    if picker.is_none() {
-        if image_preview_enabled {
-            ui.set_temp_message(
-                "image_preview enabled but terminal graphics detection failed (using half-block fallback)".to_string(),
-            );
-        } else {
-            ui.set_temp_message(
-                "Terminal graphics detection failed (using half-block fallback)".to_string(),
-            );
-        }
+    if picker.is_none() && image_preview_enabled {
+        ui.set_temp_message(
+            "image_preview enabled but terminal graphics detection failed (using half-block fallback)".to_string(),
+        );
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/modes/cclip/run.rs` around lines 265 - 276, The warning is shown to all
users when picker.is_none(); update the condition so the UI temp message is only
set when image_preview_enabled is true. In the block referencing picker and
ui.set_temp_message (in run.rs), remove or skip the else branch and only call
ui.set_temp_message with the graphics-detection/fallback text when
image_preview_enabled is true, leaving silence when image_preview_enabled is
false.

---

1654-1716: reload_and_restore is a good extraction, but scroll_offset can become stale after reload.

When the restored item's position changes (e.g., after deletion or re-ordering), scroll_offset retains its previous value and isn't adjusted. This can cause the selected item to be off-screen until the user navigates. The comment on lines 1705-1707 acknowledges this.

A simple clamp would help:

Suggested fix

         if let Some(pos) = ui
             .shown
             .iter()
             .position(|item| item.original_line.split('\t').next() == Some(rowid.as_str()))
         {
             ui.selected = Some(pos);
-            // Keep item visible only adjust scroll if item would be out of view
-            // (Scroll adjustment logic depends on knowing visible height,
-            // which may require passing it as a parameter)
+            // Ensure scroll_offset doesn't exceed the new list bounds
+            if ui.scroll_offset > pos {
+                ui.scroll_offset = pos;
+            }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/modes/cclip/run.rs` around lines 1654 - 1716, reload_and_restore
currently doesn't adjust ui.scroll_offset so the restored selection can be
off-screen; update reload_and_restore to accept a visible_height parameter
(e.g., fn reload_and_restore(..., visible_height: usize)) and after you set
ui.selected compute and clamp ui.scroll_offset so the selected index is visible:
if selected < scroll_offset set scroll_offset = selected, else if selected >=
scroll_offset + visible_height set scroll_offset = selected + 1 -
visible_height; also ensure ui.scroll_offset is bounded between 0 and
ui.shown.len().saturating_sub(visible_height). Apply this logic in the branch
where you restore selection (the block referencing ui.selected and
ui.scroll_offset) and in the branch that selects the first item.

src/ui/graphics.rs (4)

26-27: Split state between DISPLAY_STATE and ImageManager.current_rowid risks divergence.

DISPLAY_STATE (global tokio Mutex) and current_rowid (struct field) independently track overlapping concerns. Nothing in ImageManager updates DISPLAY_STATE, so callers must keep them in sync manually. A missed update leaves one stale — e.g. DISPLAY_STATE showing Image("5") while current_rowid is None after a clear().

Consider driving DISPLAY_STATE transitions from within ImageManager methods (load_cclip_image, set_image, clear) so state is always consistent, or consolidate everything into a single source of truth.

Also applies to: 30-34

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/graphics.rs` around lines 26 - 27, DISPLAY_STATE and
ImageManager.current_rowid are two independent sources of truth and can diverge;
modify ImageManager so its methods drive DISPLAY_STATE transitions instead of
updating only current_rowid (or collapse to a single state field). Specifically,
update ImageManager::load_cclip_image, ImageManager::set_image, and
ImageManager::clear to acquire the tokio Mutex on DISPLAY_STATE and update it to
the correct DisplayState value whenever current_rowid changes (or remove
current_rowid and use DISPLAY_STATE exclusively), ensuring all reads/writes go
through the same symbol (DISPLAY_STATE or the single consolidated field) to
prevent stale state.

---

138-138: Redundant use ratatui_image::picker::ProtocolType — already imported at Line 9.

♻️ Proposed fix

     pub fn detect(picker: Option<&Picker>) -> Self {
         if let Some(picker) = picker {
-            use ratatui_image::picker::ProtocolType;
             match picker.protocol_type() {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/graphics.rs` at line 138, There is a duplicate import of ProtocolType
(use ratatui_image::picker::ProtocolType); remove the redundant import statement
so only the original import at the top of the module remains; ensure no other
code relies on the duplicate and run a build to confirm the single import
satisfies all references to ProtocolType.

---

33-33: Unbounded cache growth — StatefulProtocol objects accumulate without eviction.

self.cache only ever inserts entries (Line 101) and clear() doesn't purge it. The resizing and encoding is blocking by default, meaning each StatefulProtocol holds pre-encoded image data. For long clipboard sessions with many image items, memory grows without bound.

Consider capping the cache (e.g. a simple LRU with a fixed limit, or evicting on clear()):

♻️ Simple eviction option — flush cache on clear()

     pub fn clear(&mut self) {
         self.current_rowid = None;
+        self.cache.clear();
     }

For a more granular approach, use an LRU crate (e.g. lru) or cap insertions to a fixed N most-recently-used entries.

Also applies to: 100-102

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/graphics.rs` at line 33, The cache field (cache: HashMap<String,
StatefulProtocol>) currently only ever inserts and never evicts, causing
unbounded memory growth because each StatefulProtocol holds pre-encoded image
data; update the cache to a bounded collection or flush it on clear(): either
replace the HashMap with an LRU (e.g., use the lru crate and a fixed capacity)
and update all cache.insert/cache.get usages accordingly, or modify the clear()
method to purge self.cache (and drop StatefulProtocol entries) and add a capped
insertion policy that evicts the least-recently-used entry when adding beyond N;
refer to the cache field, StatefulProtocol type, the clear() method, and the
places using cache.insert/cache.get to make these changes.

---

107-118: render() silently discards encoding/resize errors — change the signature to Result<()>.

The library explicitly recommends handling the encoding result by calling last_encoding_result() on the StatefulProtocol after each render. The current void signature makes it impossible to surface these errors to the caller (e.g. failed Sixel encoding, Kitty write failure).

♻️ Proposed fix

-    pub fn render(&mut self, f: &mut Frame, area: Rect) {
+    pub fn render(&mut self, f: &mut Frame, area: Rect) -> Result<()> {
         if let Some(rowid) = &self.current_rowid {
             if let Some(protocol) = self.cache.get_mut(rowid) {
                 f.render_stateful_widget(
                     StatefulImage::default().resize(Resize::Fit(None)),
                     area,
                     protocol,
                 );
+                protocol.last_encoding_result()?;
             }
         }
+        Ok(())
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/graphics.rs` around lines 107 - 118, Change render() to return a
Result and propagate encoding/resize errors: update the signature of render(&mut
self, f: &mut Frame, area: Rect) to return Result<(), E> (e.g. anyhow::Error or
Box<dyn std::error::Error>) and after f.render_stateful_widget(..., protocol)
call protocol.last_encoding_result() on the StatefulProtocol instance (the
variable protocol returned from cache.get_mut(rowid)); if last_encoding_result()
is Err, return that error to the caller instead of silently discarding it, and
update any call sites of render() accordingly.

src/ui/graphics.rs (3)

143-160: Render-time resize/encode is still blocking on the draw thread.

The resizing and encoding done by StatefulImage is blocking, and since it happens at render-time, it is a good idea to offload that to another thread or async task if the UI must be responsive. The spawn_blocking in load_cclip_image only covers image::load_from_memory; the first-render encode (and any re-encode after a terminal resize) still runs synchronously inside the ratatui draw callback here.

For large images or frequent resize events, consider the needs_resize + background resize_encode pattern from examples/async.rs in the ratatui-image repo to move this work off the draw thread.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/graphics.rs` around lines 143 - 160, The render method is performing
blocking resize/encode work inside the draw thread via StatefulImage (see render
and StatefulImage::default().resize(...)), so refactor to offload that work:
detect when an image needs (re)encoding/resizing (use a needs_resize flag on the
cached protocol entry keyed by current_rowid), spawn a background task
(spawn_blocking or async task) to run the resize/encode logic (see
load_cclip_image which already spawn_blocking image::load_from_memory) and
expose a resize_encode function to update the cache entry when complete; then
make render only paint ready frames from cache (skip/queue when
protocol.last_encoding_result() is pending) so the heavy work no longer runs
synchronously in render.

---

162-167: clear() destroys the entire LRU cache — consider a separate hide() method.

clear() is the replacement for image_hide(), but it also wipes all 50 cached StatefulProtocol entries. Any previously loaded image must be fully re-fetched and re-decoded the next time it's previewed, defeating the purpose of the LRU cache.

Consider splitting the responsibility:

♻️ Proposed refactor

+    /// Hide the current image without evicting the cache.
+    pub fn hide(&mut self) {
+        self.current_rowid = None;
+        self.update_display_state(DisplayState::Empty);
+    }
+
     pub fn clear(&mut self) {
         self.current_rowid = None;
         self.cache.clear();
         self.cache_order.clear();
         self.update_display_state(DisplayState::Empty);
     }

Use hide() for dismiss/navigation and reserve clear() for full teardown (e.g., on mode exit).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/graphics.rs` around lines 162 - 167, The clear() method currently
nukes the entire LRU cache (cache and cache_order) when used as an image_hide
replacement; instead add a new hide() method that only sets self.current_rowid =
None and calls self.update_display_state(DisplayState::Empty) without touching
self.cache or self.cache_order, and leave clear() as the full-teardown that
still clears cache and cache_order; update any callsites that intend to merely
hide/dismiss previews to call hide() (not clear()), and keep clear() for
mode-exit/full reset semantics.

---

101-115: No timeout on wait_with_output() — hung cclip get leaves Loading state permanently.

wait_with_output().await blocks the async task indefinitely if the subprocess stalls. The TUI itself stays responsive (other tasks continue), but DISPLAY_STATE never transitions out of Loading(rowid) and the task leaks. Wrapping with tokio::time::timeout bounds this:

♻️ Proposed fix

+        use tokio::time::{timeout, Duration};
+
-        let output = child.wait_with_output().await?;
+        let output = timeout(Duration::from_secs(5), child.wait_with_output())
+            .await
+            .map_err(|_| eyre!("cclip get timed out for rowid: {}", rowid))??;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/ui/graphics.rs` around lines 101 - 115, The code calls
child.wait_with_output().await for the "cclip" process (spawned via
tokio::process::Command::new("cclip")...) which can block indefinitely; wrap the
await in tokio::time::timeout with a sensible duration, and on timeout ensure
you kill the child process (child.kill()/child.wait() or use Child::start_kill)
and return an error so DISPLAY_STATE doesn’t remain Loading(rowid); update the
logic around wait_with_output() to handle Ok(output), Err(timeout), and Err(io)
cases and include the timeout branch in the existing error handling that
currently returns eyre!("cclip get failed for rowid: {}", rowid).

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/ui/graphics.rs">

<violation number="1" location="src/ui/graphics.rs:82">
P2: Silently ignoring a poisoned `Mutex` will cause display state updates to be lost without any diagnostic output. Other callers of `DISPLAY_STATE` in `run.rs` use `.lock().unwrap()` (which panics on poison), making the error-handling strategy inconsistent across the codebase. Consider recovering the inner value from the poison error to keep the state consistent:
```rust
let mut lock = DISPLAY_STATE.lock().unwrap_or_else(|e| e.into_inner());
*lock = state;

This ensures the state is always updated, even after a panic in another thread.