refactor(tui): split tui module#288
Open
Darkroom4364 wants to merge 1 commit intomainfrom
Open
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughImplements a full interactive terminal UI: new TUI state, rendering, input/mouse dispatch, modal dialogs (launch/search/help/action/export/severity), export writers (CBOM/JSON/SARIF), terminal session lifecycle, external-open integration, source-context caching/worker loads, and comprehensive unit tests. ChangesTUI feature (single cohesive DAG)
Sequence Diagram(s)sequenceDiagram
participant User
participant Terminal as Terminal (crossterm/ratatui)
participant TUI as TuiApp
participant Worker as Background Worker
participant FS as Filesystem
participant Editor as External Editor
User->>Terminal: key / mouse event
Terminal->>TUI: deliver event
TUI->>TUI: handle_key / handle_mouse (modal routing)
alt Rescan requested
TUI->>Worker: start_tui_execution (spawn thread)
Worker->>TUI: WorkerMessage::Scan via channel
TUI->>TUI: handle_worker_messages (store result, update cache)
TUI->>Terminal: redraw frame
end
alt Open target requested
TUI->>FS: resolve_finding_path / read source context (optional)
TUI->>Terminal: suspend TerminalSession
TUI->>Editor: spawn external command
Editor-->>TUI: exit
TUI->>Terminal: resume TerminalSession, push runtime notice
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 10 minutes and 33 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/rules/manifest.rs`:
- Around line 329-332: The selection of best from reached_seeds using only
a.confidence.total_cmp makes the result non-deterministic when confidences tie;
update the comparator in the max_by call (the expression that computes best) to
add a deterministic tie-breaker such as comparing a stable field (e.g., seed id,
seed text, or another canonical key) after the confidence comparison — for
example chain the Ordering from total_cmp with then(a.seed_id.cmp(&b.seed_id))
or then(a.seed.cmp(&b.seed)) so reached_seeds.values().max_by(...) always yields
a stable best.
In `@src/tui/input.rs`:
- Line 136: The handler that maps KeyCode::Char('r') to ControlFlow::Rescan must
be changed to check whether a scan is already running and only return
ControlFlow::Rescan when no scan is in progress; otherwise return a no-op flow
(e.g., ControlFlow::None or ControlFlow::Ignore). Locate the input handling
branch that matches KeyCode::Char('r') in src/tui/input.rs and consult the
shared scan state or an accessor like is_scanning()/scan_in_progress flag
exposed by the scan controller (or add one) to gate the Rescan emission; ensure
the scan-starting path sets the flag and clears it on completion so repeated 'r'
presses are debounced/blocked while a run is active.
In `@src/tui/mod.rs`:
- Around line 257-263: The code incorrectly treats a path with an extension as a
file (scan_root_is_file = scan_root.is_file() ||
scan_root.extension().is_some()), which misclassifies dotted directory names;
remove the extension-based check and base file detection solely on filesystem
metadata (e.g., use scan_root.is_file() or std::fs::metadata/metadata.is_file()
to handle symlinks/exists cases) so that scan_root_is_file, base, and later
resolve_finding_path use the actual file/dir state of scan_root rather than its
extension.
- Around line 179-183: The current split_whitespace() call on the editor string
will break quoted paths/flags; replace this with a shell-aware tokenizer (e.g.,
use shlex::split or the shell-words crate) to parse the editor command into
tokens while preserving quoted segments, then collect those tokens into the
parts Vec<String> used downstream (keep existing variable names like editor and
parts and ensure you handle the Result/Option from the tokenizer and fall back
or error appropriately).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 958e93b8-0df1-484e-aa33-1878c168d446
📒 Files selected for processing (9)
src/report/sarif.rssrc/rules/manifest.rssrc/tui.rssrc/tui/input.rssrc/tui/mod.rssrc/tui/state.rssrc/tui/tests.rssrc/tui/views.rssrc/tui/widgets.rs
Darkroom4364
force-pushed
the
273-tui-submodules
branch
from
82c5a15 to
f028491
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tui/widgets.rs`:
- Around line 479-570: The code currently indexes and slices source lines by
char count which breaks alignment for tabs, wide glyphs, and combining marks;
update render_context_line, highlight_range_for_line, and context_caret_line to
operate on terminal display-cell widths instead of .chars(): iterate the line as
grapheme clusters (unicode_segmentation::UnicodeSegmentation::graphemes) and
measure each cluster's display width
(unicode_width::UnicodeWidthStr/UnicodeWidthChar) to compute
window_start/window_end, visible_highlight offsets, and caret_offset; ensure
highlight_range_for_line returns character-indexed boundaries that you then
convert to cumulative display-cell offsets when rendering; keep using
CONTEXT_LINE_MAX_CHARS and CONTEXT_FOCUS_LEAD but treat them as cell counts, and
adjust context_caret_line to accept caret_offset/caret_width in display cells so
the "^" markers line up with the rendered text.
- Around line 232-272: The current dataflow step pushes for source/sink only
when both finding.source_line and finding.source_description (and same for sink)
are Some, which hides entries if either field is present alone; update the
conditional blocks that build the steps (the ones referencing
finding.source_line, finding.source_description, finding.sink_line,
finding.sink_description and pushing tuples with
OpenFocus::Source/OpenFocus::Sink) to accept either field being Some: if line is
Some use format!("{}:{}", display_path(&finding.file), line) as the label,
otherwise use display_path(&finding.file) (or another sensible fallback) and
still pass description.clone() when present (or None when absent); keep the same
OpenFocus variants and colors (Color::Yellow/Color::Red) so entries render when
only a location or only a description exists and align with
available_open_focuses/open_target_lines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9ec4181a-15c3-4936-a3ea-4fb3813650d8
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.tomlsrc/tui.rssrc/tui/input.rssrc/tui/mod.rssrc/tui/state.rssrc/tui/tests.rssrc/tui/views.rssrc/tui/widgets.rs
✅ Files skipped from review due to trivial changes (2)
- src/tui/tests.rs
- src/tui/views.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/tui/mod.rs
- src/tui/state.rs
- src/tui/input.rs
Darkroom4364
force-pushed
the
273-tui-submodules
branch
from
f028491 to
024a02e
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
1 similar comment
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/tui/widgets.rs (1)
15-22:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep open-target selection aligned with description-only dataflow steps.
dataflow_lines()now renders source/sink entries when only the description exists, butavailable_open_focuses()still only checkssource_line/sink_line. That leaves detail nodes that are visible but unreachable via Tab/Enter, andopen_target_lines()omits them entirely.♻️ Minimal fix
pub(super) fn available_open_focuses(finding: &Finding) -> Vec<OpenFocus> { let mut focuses = vec![OpenFocus::Finding]; - if finding.source_line.is_some() { + if finding.source_line.is_some() || finding.source_description.is_some() { focuses.push(OpenFocus::Source); } - if finding.sink_line.is_some() { + if finding.sink_line.is_some() || finding.sink_description.is_some() { focuses.push(OpenFocus::Sink); } focuses }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tui/widgets.rs` around lines 15 - 22, available_open_focuses currently only adds Source/Sink when finding.source_line / sink_line exist, but dataflow_lines also renders entries when only source_description / sink_description exist; update available_open_focuses (and mirror the same predicate in open_target_lines) to consider description-only cases by checking finding.source_line.is_some() || finding.source_description.is_some() and finding.sink_line.is_some() || finding.sink_description.is_some() so the focus options match what dataflow_lines renders and open_target_lines exposes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tui/views.rs`:
- Around line 700-721: The sync fs::read_to_string call inside
source_context_lines (invoked from detail_text) must be removed and replaced
with a cache-driven async load: on selection change (where detail_text is
triggered) or via a background worker, call a loader that uses spawn_blocking/ a
threadpool to read the file found by resolve_finding_path and then compute
render_source_context, storing the resulting Vec<Line<'static>> into
source_context_cache (or a new enum state like Cached/Loading/Error) keyed by
SourceContextCacheKey; have source_context_lines simply read from
source_context_cache and return None or the cached lines immediately (no IO),
and ensure the loader posts an update/redraw when the cache is filled or on
error so the UI updates.
In `@src/tui/widgets.rs`:
- Around line 52-60: drain_queued_scroll_events is currently consuming the first
non-scroll Event and losing it; change the logic so that when event::read()
returns a non-scroll event you do not drop it but store it into a small shared
stash (e.g., a one-slot Mutex<Option<Event>> like STASHED_EVENT) and then break,
and update the global event-reading path to check and consume STASHED_EVENT
first before calling event::read(); implement helper functions (e.g.,
stash_event(event: Event) and pop_stashed_event() -> Option<Event>) and update
any callers to use pop_stashed_event_or_read() so the first non-scroll event is
preserved instead of being discarded by drain_queued_scroll_events.
---
Duplicate comments:
In `@src/tui/widgets.rs`:
- Around line 15-22: available_open_focuses currently only adds Source/Sink when
finding.source_line / sink_line exist, but dataflow_lines also renders entries
when only source_description / sink_description exist; update
available_open_focuses (and mirror the same predicate in open_target_lines) to
consider description-only cases by checking finding.source_line.is_some() ||
finding.source_description.is_some() and finding.sink_line.is_some() ||
finding.sink_description.is_some() so the focus options match what
dataflow_lines renders and open_target_lines exposes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 78526d66-bba9-4bb6-8f6f-99025a31efcf
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.tomlsrc/tui.rssrc/tui/input.rssrc/tui/mod.rssrc/tui/state.rssrc/tui/tests.rssrc/tui/views.rssrc/tui/widgets.rs
✅ Files skipped from review due to trivial changes (3)
- Cargo.toml
- src/tui/state.rs
- src/tui/mod.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tui/input.rs
- src/tui/tests.rs
Darkroom4364
force-pushed
the
273-tui-submodules
branch
from
024a02e to
a9e2f53
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tui/mod.rs`:
- Around line 127-137: The enter() function (and the similar block around
clear()) activates raw mode and switches to the alternate screen but returns
early on errors from execute!, Terminal::new, or clear(), leaving the terminal
in a modified state; update enter() (and the 156-170 block) to perform rollback
on every error path by undoing what succeeded: if enable_raw_mode() succeeded
but execute!(EnterAlternateScreen, EnableMouseCapture) later fails, call
execute!(LeaveAlternateScreen, DisableMouseCapture) and disable_raw_mode()
before returning; likewise, if execute! succeeded but Terminal::new or clear()
fails, run execute!(LeaveAlternateScreen, DisableMouseCapture) and
disable_raw_mode() to restore the terminal; factor this into a small cleanup
helper or use a scope guard around the EnterAlternateScreen/EnableMouseCapture
sequence to ensure consistent rollback on all early returns.
- Around line 248-270: The match against editor names uses basename (derived
from program) but doesn't normalize Windows variants like Code.exe or code.cmd,
so update the code that computes basename to strip common executable extensions
and lowercase it before the match (e.g., derive file_name as before, then call
file_stem() or strip_suffix for ".exe", ".cmd", ".bat" and then to_lowercase()).
Modify the variable used in the match (the existing basename) so the branches
(e.g., the "code"/"nvim"/"vim" arms) receive the normalized, lowercased stem and
will correctly match Windows $EDITOR values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: eeaa8b88-74d7-44e3-9f3e-b47766d27afd
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.tomlsrc/tui.rssrc/tui/input.rssrc/tui/mod.rssrc/tui/state.rssrc/tui/tests.rssrc/tui/views.rssrc/tui/widgets.rs
✅ Files skipped from review due to trivial changes (2)
- Cargo.toml
- src/tui/state.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/tui/widgets.rs
- src/tui/views.rs
- src/tui/input.rs
Darkroom4364
force-pushed
the
273-tui-submodules
branch
from
a9e2f53 to
9c589b8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Refs #273
Summary by CodeRabbit
New Features
Tests