Refactor UI: Fix bugs, optimize rendering, and reduce tech debt

[leszek3737]

• Fix off-by-one in confirm dialog file list and capacity
• Fix scrollbar/text desync for >65535 lines in help dialog
• Fix CJK/emoji capacity bug in input dialog (bytes vs columns)
• Fix control char width counting in text dialog (unwrap_or(0))
• Fix overflow in visual_row_to_logical (checked_add)
• Fix scroll_right max_offset when effective_width == 0
• Fix viewer render heights bounds check (panic prevention)
• Fix viewer loader Drop blocking event thread (detach instead of join)
• Fix viewer open.rs capacity mismatch (MAX_VIEW_SIZE + 1)
• Fix search overlapping/non-overlapping inconsistency
• Fix toggle.rs recomputing line_offsets for Image mode
• Extract helpers: collect_graphemes_up_to_width, build_visible, styled_padded_line, HelpGeometry, centered_paragraph, viewer_title, write_line_number, CancellableLoader, ViewerRenderCache
• Consolidate mime constants to shared app/mime.rs
• Simplify theme macro, remove deprecated icon_theme()
• Add reusable buffers for panel rendering (~50 allocs/frame saved)
• Pipe read limit 50MiB, join timeout 2s in viewer loader
• Extract dialog tests from mod.rs to tests.rs
• Strengthen test assertions, add missing negative/edge-case tests
• 1011 tests pass, clippy clean, release build OK

[sourcery-ai]

Sorry @leszek3737, your pull request is larger than the review limit of 150000 diff characters

[gemini-code-assist]

Code Review

This pull request introduces significant refactoring and performance optimizations across the UI and viewer modules, notably extracting a ViewerRenderCache to isolate render-specific caches, consolidating MIME type constants, and introducing a CancellableLoader abstraction to reduce duplication. It also replaces tuple-based geometries with structured types, optimizes string formatting with reusable scratch buffers, and migrates inline tests to dedicated test files. Feedback on these changes highlights a debug_assert! in the search module that will always panic in debug builds when handling expanded lowercase characters, as well as a potential layout overflow in the confirmation dialog when the visible height is extremely small.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[opencode-agent]

PR Review: Refactor UI — Fix bugs, optimize rendering, reduce tech debt

Overall Assessment: High quality PR. Well-structured refactoring with genuine bug fixes, good test coverage, and clean elimination of tech debt. No blocking issues found.

---

Genuine Bug Fixes (All Correct)

1. loader.rs — Drop blocking event thread (CRITICAL FIX) — Previously ViewerLoader::drop() called handle.join(), which could block the event loop for seconds. Now it just sets cancel + detaches. The join_pipe_reader_with_timeout with a 2s deadline is a good fallback. PIPE_READ_LIMIT of 50MiB is a sensible guard.

2. scroll.rs — Overflow in visual_row_to_logical — checked_add prevents overflow when summing visual heights for very large files. The extracted past_end_logical() helper is clean.

3. text.rs — Control char width counting — Changed unwrap_or(1) to unwrap_or(0) for control characters in wrapped_line_count. This is correct: control chars (e.g., \t, \r in text content) should not contribute display width. Note this means col = cw on wrap could set col = 0, which is fine since the line just wrapped.

4. search.rs — Overlapping/non-overlapping inconsistency — Fixed to advance by one char width (char_width) instead of full match length, preventing missed overlapping matches after multi-char lowercase expansion (e.g., ß → ss). Good use of debug_assert!.

5. scroll.rs — scroll_right max_offset when effective_width == 0 — The old code returned max_line as fallback when effective_width == 0, allowing nonsensical horizontal scroll. New code uses saturating_sub consistently: max_line.saturating_sub(0) == max_line — wait, actually saturating_sub(0) still returns max_line. This means the fix is only effective when effective_width > max_line, in which case it correctly returns 0. The old code had an explicit branch that returned max_line when effective_width == 0, which is unchanged. The real fix is removing the else { max_line } branch for effective_width == 0 — but the new code still produces max_line.saturating_sub(0) == max_line. Minor: the behavior when effective_width == 0 is unchanged from before. Not a regression, but the "fix" is effectively a simplification.

6. help.rs — Scrollbar/text desync for >65535 lines — Added .min(u16::MAX as usize) clamp on clamped_offset before casting to u16. Correct fix.

7. confirm.rs — Off-by-one in file list — Fixed capacity and visible file count calculation. Removed the saturating_sub(1).max(1) that was off by one.

8. open.rs — Capacity mismatch — Vec::with_capacity(file_size.min(MAX_VIEW_SIZE + 1)) now correctly matches the file.take((MAX_VIEW_SIZE + 1) as u64) limit.

9. input.rs — CJK/emoji capacity bug — Rewrote compute_visible_window to compute widths per-grapheme upfront, then use width-based indexing. This fixes the old code that conflated grapheme count with column count.

10. toggle.rs — Unnecessary line_offsets recomputation in Image mode — Removed the if !self.is_hex_mode() { self.view_mode = ViewMode::Text; } side effect from toggle_line_numbers and toggle_wrap, which was forcing a mode change when toggling in Image mode.

---

Code Quality & Design

Strengths:

• ViewerRenderCache extraction — Clean consolidation of 6 RefCell/Option fields into a single struct with private new(). All accesses updated consistently.
• CancellableLoader<T> generic — Eliminates the duplication between ViewerLoader and ImagePreviewLoader that was flagged as a TODO. Clean design.
• Theme macro simplification — Removed 30+ boilerplate accessor methods (Theme::xxx() + Theme::xxx_with_colors() pairs), replaced with direct colors.field access. The ColorPalette fields are now pub, which is appropriate since the struct already served as the public color configuration.
• Reusable scratch buffers in panels — scratch: &mut String threaded through format_entry_line, format_brief_entry_line, build_suffix_into, render_scrollbar_with_colors, panel_status_summary. Real allocation reduction.
• HelpGeometry struct — Replaces ad-hoc (usize, u16) tuple return, improving readability.
• Mime constants consolidation — Moved TEXT_APPLICATION_MIMES, KNOWN_BINARY_MIMES, KNOWN_BINARY_PREFIXES from ui/viewer/mime.rs to app/mime.rs where they logically belong.
• collect_graphemes_up_to_width helper in archive.rs — DRYs up repeated grapheme-width logic.
• Tests extracted from dialogs/mod.rs (390 lines!) to tests.rs with additional edge-case coverage.

Observations / Non-blocking suggestions:

1. render.rs:175-185 — Redundant match arms — Both Cow::Borrowed and Cow::Owned call the same format_line_with_highlight. The only difference is the Owned arm converts to owned spans. This could be simplified to:

   let spans = format_line_with_highlight(&line, line_matches, state.current_match, colors);
   if matches!(line, Cow::Owned(_)) {
       spans.into_iter().map(|sp| Span::styled(sp.content.into_owned(), sp.style)).collect()
   } else {
       spans
   }

   But the current form is explicit and clear enough.

2. search.rs:65 — debug_assert! in production path — The debug_assert! on lowercase mapping is correct (stripped in release), but it runs inside the hot loop of search(). In debug builds this adds overhead. Since it's a debug_assert (not assert), this is acceptable.

3. loader.rs — join_pipe_reader_with_timeout busy-waits with 10ms sleeps — This is a minor concern. The total wait is at most 2 seconds with 200 iterations. For the use case (waiting for chafa stdout/stderr to drain after kill), this is fine.

4. simple.rs — PERCENT_LABELS OnceLock — Pre-computing 101 "N%" strings to avoid per-frame format allocation. Good optimization for TUI rendering. Minor: the strings live for the process lifetime, but 101 small strings is negligible.

5. panels/mod.rs — format_brief_entry_line clones scratch — At the end: scratch.clone(). This allocates a new String per entry. The scratch optimization still helps avoid intermediate allocations within the function, but the clone could potentially be avoided by returning the scratch buffer + borrowing it. Not a blocker — the current approach is correct and simpler.

6. open.rs — new_normal and new_text_listing are private but not #[inline] — These are called once per file open, so inlining doesn't matter. Fine as-is.

---

Project Convention Compliance

Rule	Status
No unsafe	Pass
No println!/eprintln!/dbg!	Pass
No state mutation in ui::* draw code	Pass
No blocking in event thread	Pass (loader Drop now detaches)
No tokio	Pass
unicode_width for column math	Pass (used correctly throughout)
No #[allow(...)] outside tests	Pass
Tests use tempfile::TempDir	Pass
UI tests use TestBackend	Pass

---

Summary

This is a well-executed refactoring PR. The bug fixes are real and address edge cases (CJK width, overflow, event thread blocking). The structural changes (render cache, CancellableLoader, theme macro simplification, mime consolidation) all reduce tech debt without introducing new complexity. Test coverage is strong with 385 new lines of tests extracted and expanded. No blocking issues — approve.

opencode session  |  github run

[greptile-apps]

Greptile Summary

This PR is a broad refactor and bug-fix sweep across the viewer, dialogs, panels, and theme layers. It extracts ViewerRenderCache, CancellableLoader, and several helper functions to reduce duplication, fixes a set of well-described rendering and overflow bugs, consolidates MIME constants, and moves dialog tests to their own file.

• Viewer: introduces ViewerRenderCache to centralise RefCell-wrapped layout state; fixes scroll overflow, bounds panics, Drop-blocking-event-thread, and search overlap inconsistency; adds pipe-read limits and join timeouts for the chafa loader.
• Dialogs: fixes the confirm file-list off-by-one, the help-dialog scrollbar desync for >65535 lines, CJK/emoji capacity in the input dialog, and control-char width counting.
• Panels: reuses a single scratch buffer per frame to save ~50 allocations; inlines Theme::*_with_colors calls directly to colors.* fields; refactors build_suffix and status_metadata to write into a caller-owned buffer.

Confidence Score: 2/5

Not safe to merge as-is: two rendering bugs ship on every frame, and search produces wrong results for all multi-character queries.

Three independent issues affect core, always-exercised paths: the status bar unconditionally appends a stale scratch buffer on every frame (doubling the right-side summary for every active panel), the text-mode and hex-fallback search both advance by one character instead of one match-length (producing duplicate/spurious match highlights for any query longer than one character), and the wrapped_line_count control-character fix inverts the intended behaviour. A fourth issue — toggle_hex_mode skipping clear_search_results for non-binary files — can leave a stale current_match index pointing past the end of the new match list after a mode toggle. The refactor work itself is well-structured and the underlying fixes for scroll overflow, the help-dialog desync, and the confirm off-by-one are correct.

src/ui/panels/mod.rs (status bar scratch-buffer reuse), src/ui/viewer/search.rs (search stride in text and hex fallback), src/ui/dialogs/text.rs (wrapped_line_count control-char width), src/ui/viewer/toggle.rs (clear_search_results scope narrowed too far)

Important Files Changed

Filename	Overview
src/ui/panels/mod.rs	Reuses a scratch buffer across the render loop to save allocations, but the final out.push_str(&scratch) at line 544 unconditionally appends whichever string happens to be in scratch last — duplicating the right-side summary on every frame.
src/ui/viewer/search.rs	Text-mode search advances by one character per match instead of one match-length, turning intended non-overlapping search into single-character sliding. Same bug exists in the hex fallback path.
src/ui/viewer/toggle.rs	toggle_hex_mode now only calls clear_search_results for the binary-originally path, leaving stale current_match after text↔hex or image→text toggles.
src/ui/dialogs/text.rs	wrapped_line_count changed control-char fallback from unwrap_or(1) to unwrap_or(0), making lines of control characters never wrap. The truncate_suffix refactor itself is correct.
src/ui/viewer/loader.rs	Extracts CancellableLoader<T> to remove duplicated structure; switches Drop to detach instead of join (fixing event-thread blocking); adds 50 MiB pipe limit and 2 s join timeout. join_pipe_reader_with_timeout busy-polls at 10 ms which is minor but worth a comment.
src/ui/viewer/open.rs	Extracts ViewerRenderCache struct; splits open_with_cancel into new_normal/new_text_listing helpers. Capacity pre-allocation fix (MAX_VIEW_SIZE + 1) is correct. Clean refactor overall.
src/ui/viewer/scroll.rs	Fixes checked_add overflow in visual_row_to_logical, fixes scroll_right max_offset when effective_width == 0, and migrates cache field accesses to render_cache. All changes are correct.
src/ui/dialogs/confirm.rs	Fixes off-by-one in file list capacity and +N more count. Clean and correct.
src/ui/dialogs/help.rs	Fixes scrollbar/text desync by clamping scroll_offset to u16::MAX before casting. Logic is correct and well-tested.
src/ui/dialogs/input.rs	Pre-computes grapheme widths into widths[]; replaces two helper functions with unified build_visible. Logic is correct; fixes CJK/emoji capacity calculation.
src/ui/viewer/render.rs	Extracts viewer_title, write_line_number, resolve_overlap helpers; reuses a line_num_buf across the render loop. Bounds check heights.get(end).copied().unwrap_or(1) is correct.
src/ui/theme.rs	Simplifies impl_default_colors! macro; removes deprecated icon_theme(). Direct field accesses replace Theme::*_with_colors forwarding calls. Clean.
src/ui/dialogs/tests.rs	New file: moved 390 lines of dialog tests from mod.rs, added negative/edge-case coverage. Tests are well-structured.
src/app/mime.rs	New file consolidating MIME-type constants. Straightforward extraction with no logic changes.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[ViewerLoader::start] -->|CancellableLoader::spawn| B[background thread]
    B -->|ViewerState::open_with_cancel| C{file type}
    C -->|archive| D[new_text_listing]
    C -->|image| E[new_normal - Image mode]
    C -->|text| F[new_normal - Text mode]
    C -->|binary| G[new_normal - Hex mode]
    D & E & F & G --> H[ViewerState with ViewerRenderCache]
    H --> I[visual_heights RefCell]
    H --> J[visual_offsets RefCell]
    H --> K[cached_line_num_col_width Cell]
    H --> L[cached_image_text Option]

    subgraph panel_render[Panel status bar - BUG]
        P1[panel_status_summary into scratch] --> P2[right_width computed]
        P2 --> P3[entry info written into out]
        P3 -->|scratch still appended unconditionally| P4[duplicate right summary]
    end

    subgraph search_flow[Search - BUG]
        S1[query] --> S2[clear_search_results]
        S2 --> S3[find match in lower_buf]
        S3 -->|advance by 1 char not query len| S3
    end

Loading

Comments Outside Diff (1)

1. src/ui/dialogs/text.rs, line 87-96 (link)

   Control-char width falls back to 0, causing the wrapping counter to stall

   UnicodeWidthChar::width returns None for control characters (U+0000–U+001F, DEL, etc.). The original code used unwrap_or(1) so control chars occupied one column and the wrap counter advanced. With unwrap_or(0), a line of control characters never triggers a wrap — col + 0 <= w forever — so wrapped_line_count returns 1 regardless of content. The PR description says this commit fixes control-char width counting, but unwrap_or(0) makes the stall worse, not better.

_{Reviews (1): Last reviewed commit: "refactor(ui): fix bugs, eliminate tech d..." | Re-trigger Greptile}

[opencode-agent]

PR Review: Refactor UI — Fix bugs, optimize rendering, reduce tech debt

Overall Assessment: Approve. This is a well-structured refactoring PR with genuine bug fixes, clean structural improvements, and strong test coverage. All 226 tests pass, clippy is clean.

---

Evaluation of Flagged Issues from Other Reviewers

Several P1 claims from automated reviewers are false positives. I verified each against the actual code:

Claim	Source	Verdict
Status bar duplicates scratch content	Greptile	False. render_status_bar_with_colors clones scratch to right_summary on line 503 before reusing scratch. Line 545 appends &right_summary, not &scratch.
toggle_hex_mode skips clear_search_results	Greptile	False. clear_search_results() is called unconditionally at toggle.rs:49. The if block at line 51 only contains line_offset recomputation.
debug_assert! always panics	Gemini	False. debug_assert_eq!(mapped_end, orig_byte_start, ...) — inside the if mapped_end <= orig_byte_start branch, and byte_map is monotonically non-decreasing, so mapped_end >= orig_byte_start. Combined: mapped_end == orig_byte_start. Assert always passes.
Confirm dialog layout overflow at max_visible=1	Gemini	False. file_slots = 1.saturating_sub(1) = 0, so zero files shown + one "... +N more" line = 1 line in height-1 area. Fits correctly.

Genuine Design Observation (Non-blocking)

search.rs — overlapping match stride (search.rs:100): The text search now advances by 1 character width instead of full match length, producing overlapping matches. This is an intentional fix for characters that expand on lowercase (e.g., ß → ss), and the behavior is now consistent across all three search paths (text, hex-bytes, hex-text-fallback). However, this is a semantic change in behavior — searching for "aa" in "aaa" now yields 2 matches instead of 1. This seems deliberate but worth being aware of.

text.rs — unwrap_or(0) for control chars (text.rs:87): Control characters that return None from UnicodeWidthChar::width now contribute 0 columns. This is correct for ratatui rendering (control chars are zero-width in TUI). The old unwrap_or(1) was overcounting. A line of only control characters will report as 1 visual line, which is correct since they render as invisible.

Verified Bug Fixes (All Correct)

• loader.rs — Drop detaches instead of joining — prevents event thread blocking
• scroll.rs — checked_add overflow — prevents panic with very large files
• scroll.rs — scroll_right max_offset — saturating_sub handles edge cases cleanly
• help.rs — u16::MAX clamp — prevents scrollbar/text desync for >65535 lines
• confirm.rs — off-by-one file list — correct capacity and visible count
• open.rs — capacity mismatch — MAX_VIEW_SIZE + 1 now matches file.take() limit
• input.rs — CJK/emoji width — pre-computed widths[] array fixes byte-vs-column conflation
• toggle.rs — no-op in image mode — early return for toggle_line_numbers/toggle_wrap

Structural Improvements

• ViewerRenderCache consolidates 6 RefCell/Option/Cell fields into one struct — cleaner ownership
• CancellableLoader<T> eliminates duplication between ViewerLoader and ImagePreviewLoader
• Reusable scratch buffers in panels — scratch: &mut String threaded through formatting functions
• HelpGeometry, centered_paragraph, viewer_title, write_line_number — good helper extractions
• MIME constants moved to app/mime.rs — proper layering
• Theme macro simplification — pub fields on ColorPalette replace 30+ forwarding methods
• Tests extracted from dialogs/mod.rs (390 lines) to tests.rs with additional edge-case coverage

Project Convention Compliance

Rule	Status
No unsafe	Pass
No println!/eprintln!/dbg!	Pass
No state mutation in ui::*	Pass
No blocking event thread	Pass (loader Drop now detaches)
No tokio	Pass
unicode_width for columns	Pass
No #[allow(...)] outside tests	Pass
CI gate passes	Pass (tests + clippy clean)

Summary

High-quality refactoring PR. All flagged "P1 bugs" from other reviewers are false positives. The bug fixes are genuine and address real edge cases. The structural changes reduce tech debt without introducing new complexity. Recommended merge.

opencode session  |  github run