fix: resolve 20 bugs and performance issues from scan audit#56
Conversation
Reviewer's GuideFixes multiple bugs and polish items identified by a scan audit: normalizes and hardens path/env expansion semantics, corrects reverse sort key implementations, tightens watcher and watcher_sync behavior, improves MIME detection and config classification, fixes viewer edge cases and mouse handling, adjusts chunked copy publishing semantics, clarifies UI messages and dialog behavior, and plugs several small panics/logic gaps across input, dialogs, hotlist, menu, and mode dispatch paths. Sequence diagram for updated chunked copy publish_temp behaviorsequenceDiagram
participant App as copy_with_progress
participant Temp as copy_to_temp
participant Pub as publish_temp
participant FS as fs
App->>Temp: copy_to_temp(src, temp_dest, cancel)
Temp-->>App: io::Result<()> or Interrupted
App->>Pub: publish_temp(temp_dest, dest, cancel, overwrite)
alt rename succeeds
Pub->>FS: rename(temp_dest, dest)
FS-->>Pub: Ok(())
Pub-->>App: Ok(())
else dest already exists
Pub->>FS: rename(temp_dest, dest)
FS-->>Pub: Err(AlreadyExists)
Pub->>Pub: cleanup_file(temp_dest)
Pub-->>App: Err(AlreadyExists)
else other error
Pub->>FS: rename(temp_dest, dest)
FS-->>Pub: Err(other)
Pub->>Pub: cleanup_file(temp_dest)
Pub-->>App: Err(other)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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 5 issues, and left some high level feedback:
- In
app/mime::detect_mime, switching frompath.is_dir()to checkingfile.metadata().is_ok_and(|m| m.is_dir())means directories that can't be opened (permission errors, special files) will no longer be classified asinode/directoryand will fall back to extension-based detection; consider keeping the cheappath.is_dir()fast path or handling metadata errors explicitly to preserve the previous behavior. - In
ops/chunk_copy::publish_tempyou removedpreserve_permissions(temp_dest, src_metadata)and the subsequentset_file_timescall, which changes copy semantics so the destination no longer inherits the source's permissions (and only indirectly gets timestamps viacopy_with_progress); if this behavior change isn't intentional, it may be better to keep a single, clearly located permissions/timestamps preservation step.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `app/mime::detect_mime`, switching from `path.is_dir()` to checking `file.metadata().is_ok_and(|m| m.is_dir())` means directories that can't be opened (permission errors, special files) will no longer be classified as `inode/directory` and will fall back to extension-based detection; consider keeping the cheap `path.is_dir()` fast path or handling metadata errors explicitly to preserve the previous behavior.
- In `ops/chunk_copy::publish_temp` you removed `preserve_permissions(temp_dest, src_metadata)` and the subsequent `set_file_times` call, which changes copy semantics so the destination no longer inherits the source's permissions (and only indirectly gets timestamps via `copy_with_progress`); if this behavior change isn't intentional, it may be better to keep a single, clearly located permissions/timestamps preservation step.
## Individual Comments
### Comment 1
<location path="src/app/mime.rs" line_range="8-14" />
<code_context>
use crate::app::types::FileCategory;
pub fn detect_mime(path: &Path) -> Option<String> {
- if path.is_dir() {
- return Some("inode/directory".to_string());
- }
</code_context>
<issue_to_address>
**issue (bug_risk):** Directory MIME detection can now fail for plain directory paths
The new flow opens the path before checking if it’s a directory, so on platforms where `File::open` fails for directories, directory paths now go through the `Err(_) => fallback()` branch and are guessed by filename instead of returning `inode/directory`. To preserve the previous behaviour, consider checking `path.metadata().map(|m| m.is_dir())` / `path.is_dir()` before `File::open`, or falling back to path-based metadata when `open` fails with `IsADirectory`.
</issue_to_address>
### Comment 2
<location path="src/ops/search.rs" line_range="905" />
<code_context>
}
};
let match_found = if case_sensitive {
- line_text.contains(pattern)
+ true
</code_context>
<issue_to_address>
**issue (bug_risk):** Case-sensitive content search now matches every line unconditionally
With this change, `case_sensitive` mode no longer uses `pattern` and instead treats every line as a match, breaking case-sensitive search. To preserve the intended behavior while keeping the refactor, this should remain:
```rust
let match_found = if case_sensitive {
line_text.contains(pattern)
} else {
Self::contains_case_insensitive(&line_text, pattern_lower)
};
```
</issue_to_address>
### Comment 3
<location path="scan-results.md" line_range="26" />
<code_context>
+- [line 113-120] `delete_word_backward()` — podwójna iteracja graphemes.
+- [line 1022-1027] `sync_unfiltered_selection()` tworzy HashMap co wywołanie. Cache/dirty flag.
+
+**Summary:** Zdrowy strukturalnie. Jeden potential panic, kilka niepotrzebnych klonowań.
+
+---
</code_context>
<issue_to_address>
**nitpick (typo):** Poprawa wyrażenia „potential panic” na polską formę „potencjalny panic” / „potencjalny błąd/panic”.
W zdaniu „Jeden potential panic, kilka niepotrzebnych klonowań” słowo „potential” wybija się z polskiego kontekstu. Warto zmienić je na „potencjalny panic” lub „potencjalny błąd (panic)”, spójnie z resztą dokumentu.
```suggestion
**Summary:** Zdrowy strukturalnie. Jeden potencjalny błąd (panic), kilka niepotrzebnych klonowań.
```
</issue_to_address>
### Comment 4
<location path="scan-results.md" line_range="69" />
<code_context>
+- [line 234-238] `WildcardSimple::matches` tworzy `Vec<char>` z name每次 wywołania.
</code_context>
<issue_to_address>
**issue (typo):** Usunięcie chińskich znaków („每次”, „每个”) użytych omyłkowo w opisach.
Znaki „每次” / „每个” (np. w „Vec<char> z name每次 wywołania”) wyglądają na artefakt wklejania/enkodowania, a nie zamierzone słownictwo. Proponuję zastąpić je polskimi odpowiednikami („każdorazowo”, „dla każdego wywołania” itp.), żeby opis był czytelny.
```suggestion
- [line 234-238] `WildcardSimple::matches` tworzy `Vec<char>` z name przy każdym wywołaniu.
```
</issue_to_address>
### Comment 5
<location path="scan-results.md" line_range="582" />
<code_context>
+**Technical Debt**
+- Brak log rotation — 10 MB danych znika.
+- `Ordering::SeqCst` w teście vs `Relaxed` w produkcji — niespójność.
+- `format!` na error path — `write!` bez allokuje mniej.
+
+**Optimizations**
</code_context>
<issue_to_address>
**nitpick (typo):** Poprawka literówki „allokuje” na „alokuje”.
W innych miejscach używasz formy „alokuje”, więc dla spójności zastosuj ją także tutaj („write! bez alokuje mniej”).
```suggestion
- `format!` na error path — `write!` bez alokuje mniej.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -904,7 +903,7 @@ impl FileSearch { | |||
| } | |||
| }; | |||
There was a problem hiding this comment.
issue (bug_risk): Case-sensitive content search now matches every line unconditionally
With this change, case_sensitive mode no longer uses pattern and instead treats every line as a match, breaking case-sensitive search. To preserve the intended behavior while keeping the refactor, this should remain:
let match_found = if case_sensitive {
line_text.contains(pattern)
} else {
Self::contains_case_insensitive(&line_text, pattern_lower)
};| - [line 113-120] `delete_word_backward()` — podwójna iteracja graphemes. | ||
| - [line 1022-1027] `sync_unfiltered_selection()` tworzy HashMap co wywołanie. Cache/dirty flag. | ||
|
|
||
| **Summary:** Zdrowy strukturalnie. Jeden potential panic, kilka niepotrzebnych klonowań. |
There was a problem hiding this comment.
nitpick (typo): Poprawa wyrażenia „potential panic” na polską formę „potencjalny panic” / „potencjalny błąd/panic”.
W zdaniu „Jeden potential panic, kilka niepotrzebnych klonowań” słowo „potential” wybija się z polskiego kontekstu. Warto zmienić je na „potencjalny panic” lub „potencjalny błąd (panic)”, spójnie z resztą dokumentu.
| **Summary:** Zdrowy strukturalnie. Jeden potential panic, kilka niepotrzebnych klonowań. | |
| **Summary:** Zdrowy strukturalnie. Jeden potencjalny błąd (panic), kilka niepotrzebnych klonowań. |
| - [line 827] `search_in_file` — 106 linii, przekracza limit 100. | ||
|
|
||
| **Optimizations** | ||
| - [line 234-238] `WildcardSimple::matches` tworzy `Vec<char>` z name每次 wywołania. |
There was a problem hiding this comment.
issue (typo): Usunięcie chińskich znaków („每次”, „每个”) użytych omyłkowo w opisach.
Znaki „每次” / „每个” (np. w „Vec z name每次 wywołania”) wyglądają na artefakt wklejania/enkodowania, a nie zamierzone słownictwo. Proponuję zastąpić je polskimi odpowiednikami („każdorazowo”, „dla każdego wywołania” itp.), żeby opis był czytelny.
| - [line 234-238] `WildcardSimple::matches` tworzy `Vec<char>` z name每次 wywołania. | |
| - [line 234-238] `WildcardSimple::matches` tworzy `Vec<char>` z name przy każdym wywołaniu. |
| **Technical Debt** | ||
| - Brak log rotation — 10 MB danych znika. | ||
| - `Ordering::SeqCst` w teście vs `Relaxed` w produkcji — niespójność. | ||
| - `format!` na error path — `write!` bez allokuje mniej. |
There was a problem hiding this comment.
nitpick (typo): Poprawka literówki „allokuje” na „alokuje”.
W innych miejscach używasz formy „alokuje”, więc dla spójności zastosuj ją także tutaj („write! bez alokuje mniej”).
| - `format!` na error path — `write!` bez allokuje mniej. | |
| - `format!` na error path — `write!` bez alokuje mniej. |
![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 a wide range of fixes and optimizations based on a repository scan, including bounds checking for hotlist removal, corrected reverse sorting logic, and improved date formatting for future-dated files. It also refactors path expansion, MIME detection, and environment variable parsing while addressing various technical debts. Review feedback identifies a potential logic error in file renaming that could bypass overwrite protections on certain filesystems and recommends explicit zero-checks in menu dispatching to prevent arithmetic errors.
| match fs::rename(temp_dest, dest) { | ||
| Ok(()) => {} | ||
| Err(err) if err.kind() == io::ErrorKind::AlreadyExists => { | ||
| cleanup_file(temp_dest); | ||
| return Err(err); | ||
| } | ||
| Err(err) => { | ||
| cleanup_file(temp_dest); | ||
| return Err(err); | ||
| } | ||
| } |
There was a problem hiding this comment.
On Unix-like systems, fs::rename silently overwrites the destination if it exists. Since the try_exists check was removed to avoid TOCTOU, and AlreadyExists is typically not returned by rename on Unix, the overwrite=false requirement will be violated if hard_link fails (e.g., on filesystems like FAT32). Consider performing a try_exists check before rename in this fallback path to respect the overwrite flag, even if it is not perfectly atomic.
References
- Verify code functionality, handle edge cases, and ensure alignment between function descriptions and implementations.
| terminal_height: u16, | ||
| terminal: &mut ratatui::Terminal<B>, | ||
| ) { | ||
| let total = menu_total_count(); |
There was a problem hiding this comment.
While max_items is checked later, it is safer to explicitly check if total is zero here. This prevents potential underflow at line 243 (total - 1) or division by zero at line 250 (% total) if the menu state is inconsistent.
References
- Ensure that invalid inputs or states are safely handled in all cases.
Greptile SummaryThis PR addresses 20 bugs and performance issues discovered during a repository-wide audit (
Confidence Score: 4/5Safe to merge; all 20 stated fixes are correct and no new breakage was introduced. Every fix was verified against its context: the src/ops/chunk_copy.rs (duplicate Err arms after rename), src/input/dialogs.rs (explicit wildcard pattern in is_same_file) Important Files Changed
Reviews (1): Last reviewed commit: "fix: resolve 20 bugs and performance iss..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| match fs::rename(temp_dest, dest) { | ||
| Ok(()) => {} | ||
| Err(err) if err.kind() == io::ErrorKind::AlreadyExists => { | ||
| cleanup_file(temp_dest); | ||
| return Err(err); | ||
| } | ||
| Err(err) => { | ||
| cleanup_file(temp_dest); | ||
| return Err(err); | ||
| } | ||
| } |
There was a problem hiding this comment.
Err arms in this match perform identical cleanup (cleanup_file + return Err), so the AlreadyExists branch is redundant. On POSIX, rename(2) atomically replaces existing files and never returns EEXIST for a regular-file destination, making that arm unreachable on Linux/macOS. The two arms can be collapsed without changing behaviour on any platform where AlreadyExists is actually returned.
| match fs::rename(temp_dest, dest) { | |
| Ok(()) => {} | |
| Err(err) if err.kind() == io::ErrorKind::AlreadyExists => { | |
| cleanup_file(temp_dest); | |
| return Err(err); | |
| } | |
| Err(err) => { | |
| cleanup_file(temp_dest); | |
| return Err(err); | |
| } | |
| } | |
| match fs::rename(temp_dest, dest) { | |
| Ok(()) => {} | |
| Err(err) => { | |
| cleanup_file(temp_dest); | |
| return Err(err); | |
| } | |
| } |
| (Err(_), Err(_)) | (Ok(_), _) | (_, Ok(_)) => { | ||
| crate::fs::path::clean_path(src) == crate::fs::path::clean_path(dest) | ||
| } |
There was a problem hiding this comment.
(Err(_), Err(_)) | (Ok(_), _) | (_, Ok(_)) is logically equivalent to _ — the sub-pattern (Ok(_), _) already covers (Ok, Ok) which is handled by the first arm, and (_, Ok(_)) overlaps it further. While Rust handles this safely by ordering, clippy's match_overlapping_arm lint (treated as an error via -D warnings per the project's CI setup) may flag the overlapping alternatives within the | chain. A plain _ is both correct and unambiguous.
| (Err(_), Err(_)) | (Ok(_), _) | (_, Ok(_)) => { | |
| crate::fs::path::clean_path(src) == crate::fs::path::clean_path(dest) | |
| } | |
| _ => crate::fs::path::clean_path(src) == crate::fs::path::clean_path(dest), |
| # LibreCommander — Skan Repozytorium | ||
|
|
||
| Data: 2026-05-24 | ||
| Cel: błędy, dług techniczny, optymalizacje | ||
|
|
||
| --- | ||
|
|
||
| ## Fala 1 | ||
|
|
||
| ### app/types.rs (2463 lines) |
There was a problem hiding this comment.
This 765-line Polish-language scan report is now versioned in the repository. If the intent is to track outstanding work publicly, a GitHub Issue or a file under .claude/ / docs/ would keep the repo root clean and separate concerns from source code. As committed, cargo test and cargo clippy won't flag it, but it adds noise to git log and future contributors may be puzzled by it at the root.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
sorting: wrap full tuple in Reverse() for tiebreaker ordering
viewer: fix empty file line_offsets, remove RefCell render mutation
main: log leave error before resume, drop leave propagation
reader: signed_duration_since prevents panic on future timestamps
mime: eliminate double I/O in detect_mime, .lock->text/config
chunk_copy: remove duplicate set_file_times, cancel flush, TOCTOU
watcher: fix path_cache self-mapping, remove dead fallback branch
watcher_sync: skip non-canonicalizable paths in desired set
search: remove redundant contains, limit visibility, drop allow
path: add clean_path normalization, reject ${123} env var names
command_line: clear command_draft on execute/cancel/esc
menu_actions: join warning and local-menu messages
mouse: rename _viewer_state, replace unreachable!() with no-op
user_menu: strip_prefix char-aware, single-strip leading +
theme: remove redundant hex digit pre-validation
dialogs: saturate u16 scroll, fix status message double-wrap
types: bounds check in hotlist_remove before Vec::remove
pickers: clarify hotlist error message for non-directory case
input/dialogs: fix is_same_file fallback exhaustiveness
mode_dispatch: guard menu_total_count % 0 and underflow
chunk_copy: add try_exists before rename fallback to prevent silent overwrite on POSIX when hard_link fails (e.g. FAT32); collapse duplicate Err arms into inspect_err dialogs: simplify is_same_file fallback pattern to wildcard _ mime: add path.is_dir() fallback when File::open fails for directories
1 participant
sorting: wrap full tuple in Reverse() for tiebreaker ordering
viewer: fix empty file line_offsets, remove RefCell render mutation
main: log leave error before resume, drop leave propagation
reader: signed_duration_since prevents panic on future timestamps
mime: eliminate double I/O in detect_mime, .lock->text/config
chunk_copy: remove duplicate set_file_times, cancel flush, TOCTOU
watcher: fix path_cache self-mapping, remove dead fallback branch
watcher_sync: skip non-canonicalizable paths in desired set
search: remove redundant contains, limit visibility, drop allow
path: add clean_path normalization, reject ${123} env var names
command_line: clear command_draft on execute/cancel/esc
menu_actions: join warning and local-menu messages
mouse: rename _viewer_state, replace unreachable!() with no-op
user_menu: strip_prefix char-aware, single-strip leading +
theme: remove redundant hex digit pre-validation
dialogs: saturate u16 scroll, fix status message double-wrap
types: bounds check in hotlist_remove before Vec::remove
pickers: clarify hotlist error message for non-directory case
input/dialogs: fix is_same_file fallback exhaustiveness
mode_dispatch: guard menu_total_count % 0 and underflow
Summary by Sourcery
Fix multiple scan-audit issues across sorting, file operations, paths, UI, and input handling to improve correctness, robustness, and UX.
Bug Fixes:
Enhancements:
Documentation: