fix: resolve 33 confirmed issues from scan audit#57
Conversation
- Fala 1: Magic numbers, doc comments, redundant ops, deprecated constants - Fala 2: Extract shared helpers, unify duplicate functions, DRY patterns - Fala 3: Ghost test assertions, production code in tests, isolated config test - Fala 4: Zero-alloc render paths, iterator-based dialogs, repeat_n padding - Fala 5: O(1) path_index lookups, zero-alloc sort comparators, O(n) dedup - Fala 6: TOCTOU watcher fix, async image preview, unconditional cleanup - Review: Image preview race guard, loader cleanup, DRY fallback, tests 957 tests pass, zero clippy warnings.
Reviewer's GuideRefactors multiple subsystems (search, sorting, viewer image preview, dialogs, shell/history, panel indexing, config loading, watcher sync, deletion safety, and tests) to remove duplication, improve performance/robustness, and tighten API shapes while fixing race conditions and TOCTOU-style issues surfaced by a scan audit. Sequence diagram for asynchronous image preview loading and race guardsequenceDiagram
participant Main as run_app
participant VS as viewer::ViewerState
participant IPL as viewer::ImagePreviewLoader
participant Chafa as run_chafa
Main->>Main: start_image_preview_if_needed(viewer_state, image_preview_loader, terminal_size)
alt VS.view_mode == ViewMode::Image && VS.needs_image_preview()
opt existing loader
Main->>IPL: cancel()
Main->>Main: image_preview_loader.take()
end
Main->>IPL: ImagePreviewLoader::start(VS.file_path.clone(), cw, ch)
activate IPL
IPL->>Chafa: run_chafa(path, width, height)
Chafa-->>IPL: Text
IPL-->>Main: tx.send((width, height, text))
deactivate IPL
end
loop main loop
Main->>Main: poll_image_preview(&mut viewer_state, &mut image_preview_loader)
alt loader.try_recv() == Ok((w, h, text))
Main->>VS: compare vs.file_path == loader.file_path
alt paths match
Main->>VS: set_image_preview(w, h, text)
Main->>Main: image_preview_loader = None
else paths differ
Main->>Main: image_preview_loader = None (discard stale preview)
end
else Disconnected
Main->>Main: image_preview_loader = None
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements several performance optimizations and architectural refactorings, including the introduction of asynchronous image previews using chafa and the transition of DialogKind::Progress to a struct variant. Significant optimizations were made to UI rendering string allocations, path deduplication logic, and file listing operations via a path index. Review feedback identifies a high-severity security risk where the removal of path validation in recursive deletions exposes the system to directory traversal attacks. Further feedback notes efficiency issues in bulk file entry removals and suggests short-circuiting logic for case-insensitive sorting tie-breakers.
I am having trouble creating individual review comments. Click here to see my feedback.
src/ops/file_ops.rs (635-646)
The removal of the path validation logic significantly weakens the safety of recursive deletions. This check ensured that the canonical path of every subdirectory being processed remained within the intended root directory. Removing it makes the function vulnerable to symlink-based directory traversal attacks or race conditions (TOCTOU), where a directory is replaced by a symlink to an external location during the deletion process. If the goal was to improve performance by avoiding repeated canonicalization, consider a more efficient way to track the allowed path boundaries rather than removing the safety guard entirely.
src/fs/reader.rs (285-294)
This implementation is inefficient for bulk removals. Calling rebuild_path_index (which is remove_entry makes repeated removals ensure_path_index at line 285 is redundant because the index is unconditionally rebuilt at line 294. Consider updating the path_index incrementally or deferring the rebuild until after a batch of operations is complete.
src/ops/sorting.rs (147-148)
The tie-breaker logic for case-insensitive sorting is inefficient. chars().all(|c| !c.is_uppercase()) scans the entire string even if an uppercase character is found early. Using !any(|c| c.is_uppercase()) would allow for short-circuiting. Additionally, since this is only evaluated when cmp_ignore_case returns Equal, the strings are already known to be identical except for case, making a full scan unnecessary.
let a_lower = !a.chars().any(|c| c.is_uppercase());
let b_lower = !b.chars().any(|c| c.is_uppercase());
![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 found 4 issues, and left some high level feedback:
- The refactor of
delete_dir_contents_impldrops the canonical-root boundary check (the previouslexical_path_starts_with(root, &canonical)guard); it would be safer to keep an equivalent root confinement check to prevent recursive deletion from ever escaping the originally requested directory in the presence of races or unusual filesystem setups. run_chafainviewer.rsandupdate_panel_read_errorsinpanel_opsare nowpub/pub(crate)helpers but appear to be used only internally; consider tightening their visibility (e.g.pub(crate)/fn) if you don't intend them as part of the public API surface.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The refactor of `delete_dir_contents_impl` drops the canonical-root boundary check (the previous `lexical_path_starts_with(root, &canonical)` guard); it would be safer to keep an equivalent root confinement check to prevent recursive deletion from ever escaping the originally requested directory in the presence of races or unusual filesystem setups.
- `run_chafa` in `viewer.rs` and `update_panel_read_errors` in `panel_ops` are now `pub`/`pub(crate)` helpers but appear to be used only internally; consider tightening their visibility (e.g. `pub(crate)`/`fn`) if you don't intend them as part of the public API surface.
## Individual Comments
### Comment 1
<location path="src/ui/panels/mod.rs" line_range="422-425" />
<code_context>
let padding = available_name_width.saturating_sub(name_actual_width);
- format!("{marker}{name}{}{}", " ".repeat(padding), suffix.text)
+ let mut s = String::with_capacity(1 + name.len() + padding + suffix.text.len());
+ s.push(marker);
+ s.push_str(&name);
+ s.extend(std::iter::repeat_n(' ', padding));
+ s.push_str(&suffix.text);
+ s
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `std::iter::repeat_n` here is likely to fail on stable Rust; prefer `repeat().take()` or a helper.
`std::iter::repeat_n` isn’t in stable `std` and will fail to compile there. Instead, use something like:
```rust
s.extend(std::iter::repeat(' ').take(padding));
```
or keep the previous `
</issue_to_address>
### Comment 2
<location path="src/ui/panels/mod.rs" line_range="599-601" />
<code_context>
let info_line_width = UnicodeWidthStr::width(info_line.as_str());
let padding = remaining.saturating_sub(info_line_width);
- let full_text = format!("{info_line}{}{right_info}", " ".repeat(padding));
+ let mut full_text = String::with_capacity(info_line.len() + padding + right_info.len());
+ full_text.push_str(&info_line);
+ full_text.extend(std::iter::repeat_n(' ', padding));
+ full_text.push_str(&right_info);
</code_context>
<issue_to_address>
**issue (bug_risk):** Same `repeat_n` compilation issue in status bar construction as in `format_entry_line`.
Please update this to use a stable approach (and match whatever you do in `format_entry_line`), for example:
```rust
full_text.extend(std::iter::repeat(' ').take(padding));
```
or build and reuse a `String` of spaces.
</issue_to_address>
### Comment 3
<location path="src/tests/history.rs" line_range="5-12" />
<code_context>
#[test]
fn test_dialog_kind_progress() {
- let dialog = DialogKind::Progress("Copying...".to_string(), 0.5, true);
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for push_history ignoring empty or whitespace-only commands
The tests cover duplicates and max-length capping, but don’t assert the behavior for empty/whitespace-only commands, which are currently ignored via `cmd.trim().is_empty()`. Please add tests that:
- Call `push_history` with only whitespace and with an empty string on a fresh `AppState`, asserting `command_history` remains empty.
- Optionally, push a valid command, then a whitespace-only command, and assert the length and last entry are unchanged.
This will lock in the intended “no blank commands in history” behavior.
</issue_to_address>
### Comment 4
<location path="src/tests/selection.rs" line_range="151" />
<code_context>
+fn selected_or_current_paths_uses_selection_when_present() {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider tests for selected_or_current_paths when there is no selection or only ".."
The current test only covers the `selected_count > 0` path. To exercise the rest of the logic, please add tests for:
1. `selected_count == 0` with a valid current entry (not `".."`): expect a single path for the current entry.
2. `selected_count == 0` with the current entry `".."`: expect an empty vec.
3. (If representable) `selected_count > 0` where all selected entries are `".."`, to validate the fallback to `current_entry_fallback` and the `".."` filter.
This will verify that the function never returns `".."` and falls back correctly when the selection is effectively empty.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| let mut s = String::with_capacity(1 + name.len() + padding + suffix.text.len()); | ||
| s.push(marker); | ||
| s.push_str(&name); | ||
| s.extend(std::iter::repeat_n(' ', padding)); |
There was a problem hiding this comment.
issue (bug_risk): Using std::iter::repeat_n here is likely to fail on stable Rust; prefer repeat().take() or a helper.
std::iter::repeat_n isn’t in stable std and will fail to compile there. Instead, use something like:
s.extend(std::iter::repeat(' ').take(padding));or keep the previous `
| let mut full_text = String::with_capacity(info_line.len() + padding + right_info.len()); | ||
| full_text.push_str(&info_line); | ||
| full_text.extend(std::iter::repeat_n(' ', padding)); |
There was a problem hiding this comment.
issue (bug_risk): Same repeat_n compilation issue in status bar construction as in format_entry_line.
Please update this to use a stable approach (and match whatever you do in format_entry_line), for example:
full_text.extend(std::iter::repeat(' ').take(padding));or build and reuse a String of spaces.
| #[test] | ||
| fn history_dedup_consecutive() { | ||
| let tmp = tempfile::tempdir().unwrap(); | ||
| let mut state = AppState::default(); | ||
| state.left_panel.set_path(tmp.path().to_path_buf()); | ||
| state.command_history.push_back("echo hi".to_string()); | ||
| let cmd = "echo hi"; | ||
| if state.command_history.back().is_none_or(|l| l != cmd) { | ||
| state.command_history.push_back(cmd.to_string()); | ||
| } | ||
| shell::push_history(&mut state, "echo hi"); | ||
| shell::push_history(&mut state, "echo hi"); | ||
| assert_eq!(state.command_history.len(), 1); | ||
| assert_eq!(state.command_history[0], "echo hi"); | ||
| } |
There was a problem hiding this comment.
suggestion (testing): Add tests for push_history ignoring empty or whitespace-only commands
The tests cover duplicates and max-length capping, but don’t assert the behavior for empty/whitespace-only commands, which are currently ignored via cmd.trim().is_empty(). Please add tests that:
- Call
push_historywith only whitespace and with an empty string on a freshAppState, assertingcommand_historyremains empty. - Optionally, push a valid command, then a whitespace-only command, and assert the length and last entry are unchanged.
This will lock in the intended “no blank commands in history” behavior.
| state.left_panel.selected_count = 2; | ||
|
|
||
| let paths = selected_or_current_paths(&state); | ||
| assert_eq!(paths.len(), 2); |
There was a problem hiding this comment.
suggestion (testing): Consider tests for selected_or_current_paths when there is no selection or only ".."
The current test only covers the selected_count > 0 path. To exercise the rest of the logic, please add tests for:
selected_count == 0with a valid current entry (not".."): expect a single path for the current entry.selected_count == 0with the current entry"..": expect an empty vec.- (If representable)
selected_count > 0where all selected entries are"..", to validate the fallback tocurrent_entry_fallbackand the".."filter.
This will verify that the function never returns ".." and falls back correctly when the selection is effectively empty.
Greptile SummaryThis PR resolves 33 confirmed scan-audit issues across six passes, touching 36 files. The changes span naming improvements (magic numbers, deprecated constants), DRY refactoring (shared helpers, unified duplicate functions), ghost-assertion fixes in tests, zero-alloc render paths, O(1) data-structure lookups, and a genuine TOCTOU fix in the filesystem watcher.
Confidence Score: 3/5Mostly safe to merge; the watcher TOCTOU fix, zero-alloc paths, and DRY consolidations are all correct. Two behavioral changes in The src/main.rs — Important Files Changed
Sequence DiagramsequenceDiagram
participant EL as Event Loop
participant PVL as poll_viewer_loader
participant PIP as poll_image_preview
participant SIPN as start_image_preview_if_needed
participant IPL as ImagePreviewLoader (thread)
participant Chafa as chafa process
EL->>PVL: file loaded?
PVL-->>EL: "dirty=true (ViewerState ready)"
EL->>PIP: result ready?
PIP-->>EL: false (loader not started yet)
EL->>SIPN: needs_image_preview?
SIPN->>IPL: ImagePreviewLoader::start(path, w, h)
IPL->>Chafa: Command::output() [blocking]
note over IPL,Chafa: cancel flag checked before and after, not during
Chafa-->>IPL: ANSI text
IPL-->>EL: tx.send(w, h, text)
EL->>PIP: try_recv()
PIP->>PIP: "guard: loader.file_path == vs.file_path?"
PIP-->>EL: "dirty=true, sets cached_image_text"
EL->>SIPN: needs_image_preview? (cached_image_size matches, false)
EL->>EL: terminal.draw() with image text
note over EL,SIPN: On rapid navigation: old loader cancelled, new loader started, but old chafa still runs
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
|
|
||
| fn selected_or_current_paths(state: &AppState) -> Vec<std::path::PathBuf> { | ||
| let selected: Vec<std::path::PathBuf> = state | ||
| .active_panel() | ||
| let panel = state.active_panel(); | ||
|
|
||
| let current_entry_fallback = || { | ||
| panel | ||
| .current_entry() | ||
| .filter(|entry| entry.name != "..") | ||
| .map(|entry| vec![entry.path.clone()]) | ||
| .unwrap_or_default() | ||
| }; | ||
|
|
||
| if panel.selected_count == 0 { | ||
| return current_entry_fallback(); | ||
| } | ||
|
|
||
| let selected: Vec<std::path::PathBuf> = panel | ||
| .selected_entries() | ||
| .into_iter() | ||
| .filter(|entry| entry.name != "..") | ||
| .map(|entry| entry.path.clone()) | ||
| .collect(); | ||
|
|
||
| if selected.is_empty() { | ||
| state | ||
| .active_panel() | ||
| .current_entry() | ||
| .filter(|entry| entry.name != "..") | ||
| .map(|entry| vec![entry.path.clone()]) | ||
| .unwrap_or_default() | ||
| } else { | ||
| selected | ||
| return current_entry_fallback(); |
There was a problem hiding this comment.
selected_or_current_paths now silently depends on selected_count accuracy
The function now short-circuits at if panel.selected_count == 0 { return current_entry_fallback(); } before ever calling selected_entries(). This is a semantic shift: previously the function checked actual entry state; now it trusts the counter. The companion test fix in src/tests/selection.rs had to manually add state.left_panel.selected_count = 2 to keep the test passing, which confirms the new coupling.
If any code path sets entry.selected = true without incrementing selected_count (or decrements the counter without clearing the field), this function will silently fall back to the cursor entry and silently skip the selection — with no error or warning.
- history: empty command, whitespace command, whitespace after valid - selection: no-selection returns current, dotdot current returns empty, all-dotdot-selected falls back to current entry
1 participant
Summary
Refine file search, sorting, viewer image preview, and panel behavior while tightening filesystem watcher safety and configuration handling, and expand tests to cover new helpers and edge cases.
New Features:
Bug Fixes:
Enhancements:
Tests: