chore(ui): debt cleanup — input, dialogs, viewer, render - #112
Conversation
Reviewer's GuideThis PR performs a small, targeted UI/maintainability cleanup focused on fixing a real hex-color parsing bug, removing a handful of dead/redundant code paths, and applying a few one-line style/sanity improvements in hot UI/rendering files without changing UX or layout. Sequence diagram for updated hex color parsing in theme configsequenceDiagram
actor User
participant Config as ThemeConfig
participant Theme as parse_color
participant HexParser as parse_hex_color
User->>Config: edit hex color value
Config->>Theme: parse_color(hex_string)
Theme->>HexParser: parse_hex_color(hex_string)
alt [hex is ASCII and length is 6 or 3]
HexParser-->>Theme: Some(Color::Rgb)
Theme-->>Config: Some(Color)
else [hex is non-ASCII or invalid length]
HexParser-->>Theme: None
Theme-->>Config: None (no panic)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Greptile SummaryThis PR performs targeted UI debt cleanup without changing established behavior.
Confidence Score: 5/5The PR appears safe to merge, with no concrete behavioral, build, or security regressions identified. The functional changes preserve command-history and byte-search semantics, while hexadecimal color parsing now safely rejects malformed non-ASCII configuration values instead of panicking.
|
| Filename | Overview |
|---|---|
| src/ui/theme.rs | Replaces potentially unsafe UTF-8 string slicing with guarded byte slicing and adds focused non-ASCII regression tests. |
| src/input/command_line.rs | Removes a redundant clear immediately before history text and cursor state are fully overwritten. |
| src/ui/viewer/hex.rs | Delegates oversized-needle handling to memmem while preserving the existing empty-needle behavior. |
| src/ui/viewer/render.rs | Computes the base status-bar style once and conditionally applies the warning foreground color. |
| src/ui/mod.rs | Removes unused convenience re-exports while retaining the theme types under the public theme module. |
Reviews (1): Last reviewed commit: "chore(ui): debt — input, dialogs, viewer..." | Re-trigger Greptile
![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
parse_hex_color, you callhex.is_ascii()twice; consider doing a singleif !hex.is_ascii() { return None; }early and then branching only onbytes.len()to avoid repeated checks and make the guard logic clearer. - The new
parse_hex_color_non_ascii_does_not_panictest only assertsNone; it might be worth adding at least one positive hex case in this test or nearby to ensure the ASCII path still behaves as expected after the change.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `parse_hex_color`, you call `hex.is_ascii()` twice; consider doing a single `if !hex.is_ascii() { return None; }` early and then branching only on `bytes.len()` to avoid repeated checks and make the guard logic clearer.
- The new `parse_hex_color_non_ascii_does_not_panic` test only asserts `None`; it might be worth adding at least one positive hex case in this test or nearby to ensure the ASCII path still behaves as expected after the change.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
CI gate is fully green. Here's my review. PR Review:
|
| Change | Verification |
|---|---|
command_line.rs dead set_text removal |
set_text_at_end (line 110) immediately overwrites it; draft is preserved separately in command_draft (line 98). Existing comment at lines 122–123 confirms the invariant. ✅ |
hex.rs redundant guard |
memchr::memmem::find returns None when needle.len() > haystack.len(); existing test needle_longer_than_haystack confirms. ✅ |
ui/mod.rs re-export removal |
No consumer uses ui::Theme/ColorPalette/IconTheme anywhere — all use the full ui::theme:: path. ✅ |
panels/tests.rs allow removal |
No .expect() in the file. ✅ |
theme.rs #[serde(default)] removal |
Struct-level #[serde(default)] (line 51) already covers all fields. ✅ |
theme.rs Eq derive |
All ColorPalette fields (Color, IconTheme) are Eq — sound. ✅ |
viewer/render.rs hoist |
Semantically identical; Style is Copy, shadowing pattern is idiomatic. ✅ |
Minor observations (non-blocking, optional)
-
parse_hex_coloris slightly more verbose than necessary (theme.rs:227–235). Afterhex.is_ascii()passes,std::str::from_utf8(&bytes[..]).ok()?can never returnNone(ASCII ⊆ UTF-8), so those?are dead. A more minimal fix keeps str slicing and just adds the guard:if !hex.is_ascii() { return None; } // existing &hex[0..2] slicing is now char-boundary-safe
The byte-based approach chosen is equally correct and arguably more explicit about the safety reasoning, so this is style preference only. Not worth changing given the surgical scope and that it's config-load-time (not hot path).
-
PR body test-count discrepancy: body states "982 lib + 302 integration" but actual is 981 lib + 302 integration. Cosmetic — the new
parse_hex_color_non_ascii_does_not_panictest is present and passing.
Rust best practices / project conventions
- No
println!/eprintln!/dbg!introduced. ✅ - No state mutation in
ui::*draw code. ✅ - No
unsafe. ✅ - Conventional commit (
chore(ui):). ✅ assert_eq!overassert!(==). ✅- Imports grouped at file top per
render.rsconvention. ✅
No blocking issues. The deferred-items rationale in the PR body is well-reasoned — the architectural refactors (ValidationResult → Result, DialogKind consolidation, etc.) correctly belong in dedicated structural PRs rather than a debt tier.
1 participant
chore(ui): debt cleanup — input, dialogs, viewer, render
Audit PR-17 (
PR-17-debt-ui-input). P3 maintainability scope: high-value subset of 106 findings — the real bug, dead/redundant code, and one-line style fixes in hot files. No UX/layout change.Fixed (10 findings)
Bug (reachable from user config):
theme.rs #1—parse_hex_colorpanicked on non-ASCII hex config values (&hex[n..m]split a multi-byte codepoint). Now operates on bytes with anis_ascii()guard. Added regression testparse_hex_color_non_ascii_does_not_panic.Dead / redundant code:
command_line.rs #2— removed deadset_text(String::new())immediately overwritten byset_text_at_endon history Up.hex.rs #4— removed redundantneedle.len() > haystack.len()early-return infind_bytes(memmem handles it).ui/mod.rs #4— removed unusedpub use theme::{ColorPalette, IconTheme, Theme}re-export (all consumers use full path).panels/tests.rs #5— removed redundant#in file).Style (hot files, one-liners):
theme.rs #8— removed redundant field-level#[serde(default)](struct already has it).theme.rs #10— merged tworatatui::style::imports.theme.rs #11— addedEqtoColorPalettederive (fields are allEq).render.rs #11— grouped the mid-fileuse lc::{app, ui}block with top-of-file imports.viewer/render.rs #11— hoistedTheme::status_bar_with_colors(colors)out of both if/else branches.viewer/toggle.rs #9— dropped stale "tracked in PR8" comment reference.viewer/tests.rs #11—assert!(x == 0)→assert_eq!(x, 0).Deferred (~96 findings, by category)
Deferred to keep this PR surgical (debt tier, no UX change). Each is documented below for follow-up:
ValidationResultenum →Result(dialogs.rs Harden file operations and improve terminal UI reliability #6),DialogKind9-variant consolidation (mod 0.0.5 #4/refactor: ownership, perf, error handling, domain extraction, clippy … #5/Harden file operations and improve terminal UI reliability #6),EventContext4×Option<T>sub-struct (input/mod 0.0.2 #1),MenuAction #[non_exhaustive]removal + exhaustive match (menu_actions 0.0.4 #3), parallel arrays →&[(&str, MenuAction)](menu refactor: ownership, perf, error handling, domain extraction, clippy … #5),from_toml_table/Deserialize duplication (theme 0.0.5 #4),SearchMatch/SearchLineMatchByteRangefactor (viewer/mod 0.0.2 #1). Each touches multiple call sites and cross-cuts dispatch; belongs in a dedicated structural PR.MenuActionwildcarddebug_assert!/warn (menu_actions 0.0.4 #3),dialog_selectioncross-kind reset (render_dialog_map refactor: ownership, perf, error handling, domain extraction, clippy … #5),DialogKind::Infofor stats display (pickers 0.0.4 #3),CANCELING_PREFIXstructured flag (simple 0.0.2 #1),parse_colorindexed-color prefix (theme Harden file operations and improve terminal UI reliability #6), IconTheme Serialize/Deserialize asymmetry (theme Fix critical issues, enhance performance, and improve code quality #9), viewer scroll pinning (ui/menu refactor: ownership, perf, error handling, domain extraction, clippy … #5),find_bytes→ ops/ move (hex 0.0.4 #3). These change runtime contracts beyond a debt cleanup.centered_rectmagic-number → const (dialogs/tests 0.0.4 #3), full-path → import (dialogs/tests 0.0.8 - Fix dialog/popup rendering, Implement shift-selection for contiguous file ranges #7),Path::newvsPathBuf(panels Harden file operations and improve terminal UI reliability #6). Low ROI comment/rename work; bundled test-hardening is a separate PR.///docs on text.rs (4/5), layout.rs thin-accessor removal (1),PIPE_JOIN_TIMEOUTcomment trim (loader 0.0.5 #4), let-chain style (loader refactor: ownership, perf, error handling, domain extraction, clippy … #5),clamp_scroll_offset/total_rows#[must_use](scroll 0.0.8 - Fix dialog/popup rendering, Implement shift-selection for contiguous file ranges #7/0.0.9 - Add async file operations, file watching, and UI workflow improvements #8),MAX_VISUAL_LINESscope (toggle 0.0.11 - Integrate Serena AI with project context, guidelines, and tests #10),next_view_modematch (toggle 0.0.11 #11),current_linerename (render 0.0.8 - Fix dialog/popup rendering, Implement shift-selection for contiguous file ranges #7),viewer_titlesimplification (render Implement natural name sorting and enhance file handling features #12/Fix file selection behavior and update versioning #13),lowercase_queryCow (search 0.0.8 - Fix dialog/popup rendering, Implement shift-selection for contiguous file ranges #7), bool→usize consistency (search Harden file operations and improve terminal UI reliability #6), magic1status-bar const (render Harden file operations and improve terminal UI reliability #6),HexQueryErrorcollapse (search 0.0.5 #4),output_from_threadstype (loader Harden file operations and improve terminal UI reliability #6). Pure style with no correctness impact.menu.rs #1usize→u16 (titles capped well below 65535),panels/mod #1pos*100overflow (pos ≤ filtered_len, bounded),ui/menu #1empty-MENUSpanic (const len=5). Unreachable given existing caps; flagged here rather than guarded.hex.rs #5(HEX_PART_WIDTHalready usesHEX_COLS_PER_BYTEviaHEX_BYTES_PER_LINE * 3) — no-op on HEAD.Definition of Done
cargo fmtcargo clippy --locked --all-targets -- -D warningscargo test --locked— 982 lib + 302 integration, 0 failedcargo build --release --lockedNo merge — agent opens, user merges.
Summary by Sourcery
Improve UI theming and viewer behavior while cleaning up small bits of technical debt without changing UX.
Bug Fixes:
Enhancements:
Tests: