fix: address comprehensive audit findings across 34 files#60
Merged
Conversation
File safety: - Fix symlink self-overwrite on move (move_ops.rs) - Fix TOCTOU race in temp-file reservation for symlink copy (temp.rs, copy.rs) - Fix Windows destructive replacement in chunked copy path (temp.rs) - Fix Windows directory symlink removal in delete walk (delete.rs) State & types: - Fix size formatting rounding bug (1024.0 KB -> 1.0 MB) (file_entry.rs) - Fix pre-1970 timestamp rendering via chrono (file_entry.rs) - Fix combining character cursor invariant in TextInput (text_input.rs) - Remove dead drag fields, keep only drag_anchor_index (app_state.rs) - Add hotlist() getter, fix cache desync in menu_actions (app_state.rs) - Deduplicate compute_category via file_type::category delegation (file_entry.rs) - Deduplicate owner/group strings with Arc<str> (file_entry.rs, reader.rs) Input handling: - Fix drag-selection bypassing selection stats (mouse.rs) - Fix dropdown scroll offset and clipped-area click mismatch (mouse.rs) - Fix dialog hitbox min-clamp mismatch via shared centered_rect (mouse.rs) - Fix menu action dispatch dropping previous mode on no-op (mode_dispatch.rs) - Fix path validation double-wrapping and platform-aware backslash (dialogs.rs) - Add Home/End key support to all ListPicker handlers (pickers.rs) - Fix hotlist push to use API instead of direct field mutation (menu_actions.rs) Rendering: - Move viewer wrap layout computation from render to pre-render step (render.rs, main.rs) - Eliminate per-frame image preview Text clone (render.rs) - Reduce hex view and text viewer span allocation churn (render.rs) - Fix fullscreen fallback on tiny terminals (layout.rs) - Fix adaptive dialog heights for small terminals (confirm.rs, simple.rs) Performance & reliability: - Reduce poll timeout from 100ms to 33ms (main.rs) - Add 5s cooldown for failed watcher canonicalize retries (watcher_sync.rs) - Reset panel viewport on failed refresh (watcher_sync.rs) - Switch debug_log to blocking lock, fix double-open on log clear (debug_log.rs) - Remove redundant BufReader/BufWriter in chunk_copy, use heap buffer (chunk_copy.rs) - Simplify case-insensitive search, remove SmallCharBuf (pattern.rs) - Use DirEntry::file_type() to avoid extra metadata syscalls in delete walk (delete.rs) - Add Windows inode loop detection via MetadataExt (helpers.rs) - Remove unused blocking detect_mime function (mime.rs) - Add dotless config file recognition (mime.rs, file_type.rs) - Add undocumented Ctrl+A/E/U/W keybindings to F1 help (keymap.rs) - Log ignored panel paths via debug_log (config.rs)
Reviewer's GuideThis PR addresses a broad audit across file safety, state/types, input handling, rendering, and watcher/logging, focusing on closing race conditions and platform bugs, simplifying code paths, and improving selection, dialogs, viewer rendering, and watcher robustness. 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 introduces various optimizations, refactorings, and bug fixes across the application. Key changes include transitioning FileEntry owner and group fields to Arc<str> to reduce allocations, implementing a cooldown mechanism in the file watcher to prevent redundant sync attempts on failure, adding Windows-specific handling for symlinks and filesystem identity, and optimizing rendering in the viewer. The review feedback highlights several critical areas for improvement: refining the file watcher's cooldown logic to be path-specific, using fs::symlink_metadata on Windows to correctly handle broken symlinks, avoiding heap allocations in case-insensitive pattern matching, using platform-aware path parsing for mime detection, and ensuring selection stats are recalculated when directory refreshes fail.
![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 2 issues, and left some high level feedback:
- The new
contains_case_insensitiveimplementation inpattern.rsallocates aVec<char>and then runs.windows()over it for every call; if this is used in hot search paths, consider preserving a streaming, allocation-free approach similar to the previous ring buffer to avoid extra allocations and O(n) char copies per search. - In
debug_log::log, switching to a blockingMutexand reopening/truncating the log file while holding the lock may increase contention and latency for other logging calls; it might be worth minimizing the critical section (e.g., prepare/open the new file outside the lock) so high-frequency logging or slow I/O does not stall unrelated code.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `contains_case_insensitive` implementation in `pattern.rs` allocates a `Vec<char>` and then runs `.windows()` over it for every call; if this is used in hot search paths, consider preserving a streaming, allocation-free approach similar to the previous ring buffer to avoid extra allocations and O(n) char copies per search.
- In `debug_log::log`, switching to a blocking `Mutex` and reopening/truncating the log file while holding the lock may increase contention and latency for other logging calls; it might be worth minimizing the critical section (e.g., prepare/open the new file outside the lock) so high-frequency logging or slow I/O does not stall unrelated code.
## Individual Comments
### Comment 1
<location path="src/main.rs" line_range="228-229" />
<code_context>
+ image_preview_loader,
+ (size.width, size.height),
+ );
+ if state.mode == AppMode::Viewing
+ && let Some(vs) = viewer_state
+ {
+ vs.update_wrap_layout(size.width as usize);
</code_context>
<issue_to_address>
**issue (bug_risk):** The `if state.mode == AppMode::Viewing && let Some(vs) = viewer_state` expression likely does not compile on stable Rust.
`let_chains` was stabilized only for `if let pat = expr && guard` forms; `if cond && let pat = expr` is not supported on many stable toolchains. To keep this portable, please rewrite as either:
```rust
if state.mode == AppMode::Viewing {
if let Some(vs) = viewer_state {
vs.update_wrap_layout(size.width as usize);
}
}
```
or using `if let` first with a guard:
```rust
if let Some(vs) = viewer_state && state.mode == AppMode::Viewing {
vs.update_wrap_layout(size.width as usize);
}
```
</issue_to_address>
### Comment 2
<location path="src/app/types/file_entry.rs" line_range="55-63" />
<code_context>
}
pub(crate) fn format_system_time(modified: SystemTime) -> Option<String> {
- let duration = modified.duration_since(std::time::UNIX_EPOCH).ok()?;
- let ts = i64::try_from(duration.as_secs()).ok()?;
- let dt = DateTime::from_timestamp(ts, 0)?;
- Some(
- dt.with_timezone(&chrono::Local)
- .format("%d-%m-%y %H:%M")
- .to_string(),
- )
+ let dt = DateTime::<Local>::from(modified);
+ Some(dt.format("%d-%m-%y %H:%M").to_string())
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Changing to `DateTime::<Local>::from(modified)` may panic for `SystemTime` values outside the representable range instead of returning `None`.
Previously, out-of-range or pre-epoch times produced `None` via `duration_since(UNIX_EPOCH)` and `DateTime::from_timestamp`. `DateTime::<Local>::from(modified)` can instead panic for those values, especially with unusual filesystems or corrupted mtimes. To keep the old, non-panicking behavior, use the checked `duration_since`/`from_timestamp` path (or another checked conversion) and map failures to `None` rather than relying on `From<SystemTime>`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Greptile SummaryThis PR applies a comprehensive sweep of bug fixes and clean-ups across 34 files, covering filesystem-safety hazards (symlink self-overwrite, TOCTOU in temp reservation, Windows directory-symlink deletion), state-management correctness (size-format rounding, pre-1970 timestamps, combining-character cursor, dead drag fields, hotlist encapsulation), input/UI correctness (dialog hitbox alignment, dropdown scroll offset, mode-dispatch no-op, Home/End in pickers), and several performance improvements (33 ms poll, pre-draw wrap layout, Arc dedup for owner/group strings, hex-view pre-allocation).
Confidence Score: 3/5Safe to merge after fixing the symlink-rename guard in move_ops.rs; all other changes are conservative improvements or targeted bug fixes. The symlink self-overwrite guard in src/ops/file_ops/move_ops.rs (both move_entry and move_entry_with_progress need the narrower symlink guard); src/ops/file_ops/temp.rs (reserve_temp_path_for TOCTOU noted for awareness); src/app/debug_log.rs (blocking lock tradeoff). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[move_entry / move_entry_with_progress] --> B{src.canonicalize == dest.canonicalize?}
B -- No --> C[Normal rename / cross-device copy+delete path]
B -- Yes --> D{src == dest?}
D -- Yes --> E[Ok - no-op]
D -- No --> F{src is symlink?}
F -- No --> G[fs::rename - case-only rename]
F -- Yes --> H["NEW CHECK (this PR)"]
H --> I["Error: cannot move symlink onto its own target"]
H -.->|"Missing: dest is also symlink?"| J["Should also allow: fs::rename\n(link1->real renamed over link2->real)"]
style I fill:#f88,color:#000
style J fill:#ffa,color:#000
style H fill:#fca,color:#000
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- watcher_sync: path-specific cooldown stores failed paths, bypasses on navigation
- watcher_sync: recalculate_selection_stats on failed directory refresh
- delete: Windows remove_symlink uses symlink_metadata for broken symlinks
- mime: platform-aware basename via Path::new().file_name() instead of rsplit('/')
- move_ops: symlink guard refined to only block src-symlink onto non-symlink dest
- file_entry: checked DateTime conversion returns None instead of panicking
- main: reorder let_chains to stable Rust if-let-first form
- copy: inline retry loop for symlink creation eliminates TOCTOU race
- temp: remove dead reserve_temp_path_for function
- pattern: ring buffer replaces Vec<char> allocation in case-insensitive search
- debug_log: document blocking lock behavior
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.
File safety:
State & types:
Input handling:
Rendering:
Performance & reliability:
Summary by Sourcery
Tighten file operations, viewer rendering, and watcher syncing for better safety, correctness, and responsiveness across platforms.
Bug Fixes:
Enhancements:
Documentation:
Tests: