refactor: improve test helpers and code quality#75
Conversation
leszek3737
commented
Jun 12, 2026
leszek3737
commented
- Consolidate test setup with helper functions (state_with_panels, make_history_picker, panel_with_files) to reduce duplication
- Add set_hidden/set_executable methods to Cha for clearer mutation API
- Deduplicate uid/gid cache logic with NameMapCache struct
- Extract path_matches_any helper in watcher_sync
- Restore debug_log for canonicalize failures in config loading
- Replace unreachable! with error dialog fallback in render_dialog_map
- Add documentation comments and TODO markers for future improvements
- Improve test assertions with descriptive messages
- Remove unused directory() helper and redundant MENU* constants
- Consolidate test setup with helper functions (state_with_panels, make_history_picker, panel_with_files) to reduce duplication - Add set_hidden/set_executable methods to Cha for clearer mutation API - Deduplicate uid/gid cache logic with NameMapCache struct - Extract path_matches_any helper in watcher_sync - Restore debug_log for canonicalize failures in config loading - Replace unreachable! with error dialog fallback in render_dialog_map - Add documentation comments and TODO markers for future improvements - Improve test assertions with descriptive messages - Remove unused _directory() helper and redundant MENU_* constants
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @leszek3737, your pull request is larger than the review limit of 150000 diff characters
![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 refactorings, documentation updates, and test cleanups across the codebase, notably consolidating menu constants into a single MENUS array, optimizing path matching in the watcher, and introducing set_hidden and set_executable APIs in Cha. Feedback on these changes highlights a regression in test coverage in src/app/keymap.rs due to the simplification of help message validation, and points out a redundant API in src/fs/cha.rs where both with_* and set_* mutators are kept.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| #[test] | ||
| fn build_help_message_lines_are_not_empty() { | ||
| fn build_help_message_is_non_empty() { | ||
| let msg = build_help_message(); | ||
| for (i, line) in msg.lines().enumerate() { | ||
| let trimmed = line.trim(); | ||
| if trimmed.is_empty() { | ||
| continue; | ||
| } | ||
| assert!( | ||
| trimmed.len() > 2, | ||
| "Line {i} is suspiciously short: {:?}", | ||
| line | ||
| ); | ||
| } | ||
| assert!(!msg.is_empty(), "Help message must not be empty"); | ||
| } |
There was a problem hiding this comment.
The test build_help_message_lines_are_not_empty was replaced with build_help_message_is_non_empty. While the new test is simpler, it's also significantly less thorough. The previous test verified that individual lines in the help message were not empty or suspiciously short, which could catch formatting bugs. The new test only confirms that the help message as a whole is not empty. This is a regression in test coverage.
I suggest restoring the more detailed line-by-line checks to ensure the help message is always formatted correctly.
#[test]
fn build_help_message_is_valid() {
let msg = build_help_message();
assert!(!msg.is_empty(), "Help message must not be empty");
for (i, line) in msg.lines().enumerate() {
let trimmed = line.trim();
if trimmed.is_empty() {
continue;
}
assert!(
trimmed.len() > 2,
"Line {i} is suspiciously short: {:?}",
line
);
}
}| pub fn with_hidden(&mut self, hidden: bool) -> &mut Self { | ||
| self.kind.set(ChaKind::HIDDEN, hidden); | ||
| self | ||
| } | ||
|
|
||
| pub fn with_executable(mut self, executable: bool) -> Self { | ||
| pub fn with_executable(&mut self, executable: bool) -> &mut Self { | ||
| if executable { | ||
| self.mode = ChaMode::new(self.mode.mode_u32() | 0o111); | ||
| } else { | ||
| self.mode = ChaMode::new(self.mode.mode_u32() & !0o111); | ||
| } | ||
| self | ||
| } |
There was a problem hiding this comment.
The new set_hidden and set_executable methods are a great addition for a clearer mutation API. However, the existing with_hidden and with_executable methods have been kept and modified to return &mut Self. This creates a redundant API where, for example, with_hidden and set_hidden have identical implementations apart from the return type.
To improve API clarity and reduce duplication, I suggest removing the with_* variants and exclusively using the new set_* methods throughout the codebase.
Greptile SummaryThis PR is a broad refactor across 42 files: consolidating test setup with shared helper functions, extracting
Confidence Score: 4/5Safe to merge — all changes are refactors and test improvements with no new feature risk; the watcher-sync path-matching logic is semantically equivalent to the old code, and the config panel-loading change correctly fixes the stale canonical_path bug. The functional logic is sound throughout and test coverage is improved. The remaining rough edges — leftover dead public API, an unnecessary delegation wrapper, an unreachable fallback branch, and a lost debug-log path — do not affect runtime correctness but leave the codebase slightly untidier than the refactor intends. src/fs/cha.rs (dead public methods), src/app/watcher_sync.rs (trivial wrapper), src/fs/reader.rs (dead branch in os_str_to_arc), src/app/config.rs (missing debug log for canonical-not-dir case) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([apply_panel called]) --> B{fs::canonicalize succeeds?}
B -- Yes --> C{resolved.is_dir?}
B -- No --> D[debug_log: canonicalize failed\nfalling back to raw path]
C -- Yes --> E[set_path + set_canonical_path]
C -- No --> F[silent drop\n⚠ no debug log]
D --> G{raw path.is_dir?}
G -- Yes --> H[set_path\nset_canonical_path = None]
G -- No --> I[debug_log: path ignored]
style F fill:#ffcccc,stroke:#cc0000
style E fill:#ccffcc,stroke:#009900
style H fill:#ccffcc,stroke:#009900
style I fill:#ffffcc,stroke:#cccc00
|
|
The review is complete. The critical item is the |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| fn full_refresh_panel(panel: &mut PanelState) { | ||
| refresh_panel_from_disk(panel); | ||
| } |
There was a problem hiding this comment.
full_refresh_panel is now a trivial wrapper
The renamed function refresh_panel_from_disk contains all the logic, while full_refresh_panel just delegates to it. All call sites — full_refresh_panels at lines 329/332 and the tests — still call full_refresh_panel. The rename added indirection without consolidating the call sites, leaving an empty wrapper that will confuse future readers about which name is canonical.
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!
| fn get_or_insert_name( | ||
| cache: &mut NameMapCache, | ||
| id: u32, | ||
| lookup: impl FnOnce(u32) -> Option<Arc<str>>, | ||
| ) -> Arc<str> { | ||
| if let Some(name) = cache.map.get(&id) { | ||
| return name.clone(); | ||
| } | ||
| if cache.map.len() >= CACHE_MAX_SIZE | ||
| && let Some(old) = cache.order.pop_front() |
There was a problem hiding this comment.
os_str_to_arc
Inside the bytes.is_ascii() branch, String::from_utf8(bytes.to_vec()) is called — but this can never fail, because ASCII is always valid UTF-8. The map_or_else error fallback || Arc::from(s.to_string_lossy().into_owned()) is therefore unreachable dead code. A direct std::str::from_utf8(bytes) (which can use unwrap safely here) or merging both branches into a single s.to_string_lossy().into_owned() call would be simpler and equally correct.
| && self.gid == other.gid | ||
| } | ||
|
|
||
| pub fn with_hidden(mut self, hidden: bool) -> Self { | ||
| pub fn with_hidden(&mut self, hidden: bool) -> &mut Self { | ||
| self.kind.set(ChaKind::HIDDEN, hidden); | ||
| self | ||
| } | ||
|
|
||
| pub fn with_executable(mut self, executable: bool) -> Self { | ||
| pub fn with_executable(&mut self, executable: bool) -> &mut Self { | ||
| if executable { | ||
| self.mode = ChaMode::new(self.mode.mode_u32() | 0o111); | ||
| } else { | ||
| self.mode = ChaMode::new(self.mode.mode_u32() & !0o111); | ||
| } | ||
| self | ||
| } | ||
|
|
||
| pub fn set_hidden(&mut self, hidden: bool) { | ||
| self.kind.set(ChaKind::HIDDEN, hidden); | ||
| } | ||
|
|
||
| pub fn set_executable(&mut self, executable: bool) { |
There was a problem hiding this comment.
with_hidden / with_executable are now dead public API
Both methods were converted to &mut self returning &mut Self, and all callers were migrated to set_hidden / set_executable. No callers remain for with_hidden or with_executable. Since they are pub, they silently widen the API surface with methods that can only be called for their side-effects, not in a builder chain. They should either be removed or deprecated.
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!
| ); | ||
| } | ||
|
|
||
| pub fn dummy_tree_entries(count: usize) -> Vec<TreeEntry> { | ||
| pub fn dummy_tree_entries( | ||
| count: usize, | ||
| depth: Option<usize>, | ||
| expanded: Option<bool>, | ||
| read_error: Option<bool>, | ||
| ) -> Vec<TreeEntry> { | ||
| let depth = depth.unwrap_or(0); | ||
| let expanded = expanded.unwrap_or(false); | ||
| let read_error = read_error.unwrap_or(false); | ||
| (0..count) | ||
| .map(|i| { | ||
| let name = format!("entry-{i}"); | ||
| let name_width = unicode_width::UnicodeWidthStr::width(name.as_str()); | ||
| TreeEntry { | ||
| path: PathBuf::from(format!("/tmp/{i}")), | ||
| depth: 0, | ||
| path: std::env::temp_dir().join(format!("{i}")), | ||
| depth, | ||
| is_dir: false, | ||
| expanded: false, | ||
| expanded, | ||
| name, | ||
| name_width, | ||
| read_error: false, | ||
| read_error, | ||
| } | ||
| }) | ||
| .collect() |
There was a problem hiding this comment.
dummy_tree_entries gains three optional params that are always None at every call site
All call sites in the codebase pass (count, None, None, None). The added flexibility is not exercised anywhere, making the signature more verbose without benefit. Consider keeping the simple arity for the common case and introducing a dummy_tree_entries_with variant (or a builder/struct) for callers that actually need non-default values.
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!
| None | ||
| } | ||
|
|
||
| fn _assert_dialog_contains(terminal: &mut Terminal<TestBackend>, state: &AppState, text: &str) { | ||
| terminal | ||
| .draw(|f| render::render_ui(f, state, None, None)) | ||
| .unwrap(); | ||
| let buf = terminal.backend().buffer(); | ||
| let rendered = buffer_to_string(buf); | ||
| assert!( | ||
| rendered.contains(text), | ||
| "expected dialog to contain '{text}', got:\n{rendered}" | ||
| ); |
There was a problem hiding this comment.
_assert_dialog_contains is unused dead code
The helper is added with a leading underscore to suppress the "unused" warning, but no test in this module calls it. Unused helpers in test files accumulate silently and make it harder to understand which utilities are actually exercised. If this is scaffolding for a planned test, a // TODO comment explaining what test it supports would clarify intent; otherwise it should be removed.
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!
- Remove trivial full_refresh_panel wrapper, call refresh_panel_from_disk directly - Simplify os_str_to_arc: ASCII is always valid UTF-8, remove dead fallback branch - Remove unused with_hidden/with_executable methods (replaced by set_* variants) - Restore thorough line-by-line validation in build_help_message test - Remove unused _assert_dialog_contains helper from dialogs tests - Simplify dummy_tree_entries signature: remove optional params always passed as None
PR Review: refactor: improve test helpers and code qualityCI status: GREEN — Critical IssuesNone found. The earlier comment about a Medium Priority Issues
Low Priority / Observations
SummaryThis is a well-structured refactoring PR. The core changes — test helper consolidation, |
1 participant
