refactor(app): AppState aggregates, panel entries-as-indices, type/config debt#83
Conversation
…nfig debt PR-07: tackle the AppState god-object and types/* modeling debt, fix type-level bugs, and dedup config/paths/shell/keymap/mime boilerplate. App state & types: - Split AppState's 36 flat fields into UiState/InputState/TreeState/ InteractionState aggregates; merge last_click_time/last_click_position into one Option<(Instant,(u16,u16))>; add invariant accessors (set_mode/set_active_panel/request_quit) and a generic string-cache helper. - Redesign PanelListing into a single backing store with a filtered index view (entries: Vec<usize>), removing the dual-store sync bug; selection is single-sourced; toggle_selection returns ToggleResult; selected_entries is now a zero-alloc iterator. Migrate all consumers. Type-level bugs: - FileEntry::build() now returns Result<_, BuildError> instead of panicking. - Fix size formatting: pad the formatted String (FileSize's Display ignores formatter width flags). - text_input: keep the cursor visible on the right when the last grapheme is wider than the viewport; centralize scroll recompute and the cursor invariant. - test_helpers: compose raw_mode + size; define explicit symlink precedence. Sorting & config: - sorting: canonical field/direction names + FromStr + serde round-trip tests; resolve the cross-naming with config (canonical "sensitive"). - config/paths/shell/keymap/mime/file_type: dedup wrappers via generics/macros, fold MIME constants, mode-name consts; mime returns Option<&'static str>. - file_type: make the .env. prefix check follow the exact-name case policy. Safety & misc: - shell: document the by-design shell-injection threat model (local menu is attacker-controlled, gated by the confirm dialog). - job_runner: extract handle_worker_result; document the reaper timeout trade-off. - debug_log: batch BufWriter flushes; cache the per-second timestamp. - Fix the Windows test cfg gate in misc.rs. CI gate green: build + clippy -D warnings + 1212 tests passing.
![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 refactors state management by splitting the monolithic AppState into focused sub-states (InputState, UiState, TreeState, and InteractionState) and introduces a single-source-of-truth PanelListing model to prevent selection desynchronization. Feedback on these changes highlights a performance regression from deriving source_display on demand during frame rendering, ineffective panic payload logging that should downcast to extract the actual message, and an unidiomatic builder pattern for PendingAction that would be cleaner as an in-place mutator.
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.
| impl CopyMoveDetails { | ||
| /// Per-source display labels (file name, falling back to the full path). | ||
| /// Derived on demand from `source` instead of being stored as a parallel | ||
| /// `source_display` field that could drift out of sync. | ||
| pub fn source_display(&self) -> Vec<String> { | ||
| self.source | ||
| .iter() | ||
| .map(|p| { | ||
| p.file_name() | ||
| .map(|n| n.to_string_lossy().into_owned()) | ||
| .unwrap_or_else(|| p.display().to_string()) | ||
| }) | ||
| .collect() | ||
| } | ||
| } |
There was a problem hiding this comment.
Deriving source_display on demand on every frame render introduces a significant performance regression. Since to_ui_dialog is called on every frame tick (especially with animations like the spinner), this method will repeatedly iterate over self.source, perform multiple heap allocations (to_string_lossy().into_owned(), to_string()), and collect them into a new Vec<String>.
Since CopyMoveDetails is immutable once the dialog is opened, there is no risk of the display labels drifting out of sync. It is much more efficient to pre-compute source_display once during construction and store it as a field (as it was before) to avoid high memory churn during rendering.
| fn handle_worker_result(handle: JoinHandle<()>, context: &str) -> bool { | ||
| match handle.join() { | ||
| Ok(()) => true, | ||
| Err(panic_payload) => { | ||
| debug_log!("{context}: {:?}", panic_payload); | ||
| false | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The panic_payload is of type Box<dyn Any + Send>. Formatting it with {:?} simply prints "Any", which does not provide the actual panic message (e.g., "index out of bounds"). To make this log useful for troubleshooting, we should downcast the payload to &str or String to extract and log the actual panic message.
fn handle_worker_result(handle: JoinHandle<()>, context: &str) -> bool {
match handle.join() {
Ok(()) => true,
Err(panic_payload) => {
let msg = if let Some(s) = panic_payload.downcast_ref::<&str>() {
*s
} else if let Some(s) = panic_payload.downcast_ref::<String>() {
s.as_str()
} else {
"unknown panic payload"
};
debug_log!("{context}: {msg}");
false
}
}
}| impl PendingAction { | ||
| pub fn set_overwrite(&mut self) { | ||
| match self { | ||
| Self::Copy(t) | Self::Move(t) => { | ||
| t.overwrite = true; | ||
| } | ||
| /// Builder that marks the action as "overwrite existing targets". | ||
| /// | ||
| /// Replaces the former `set_overwrite(&mut self)` mutator. Unifies the | ||
| /// duplicated `overwrite` flag across every variant in one place. `Delete` | ||
| /// has no destination to overwrite, so it is a no-op. | ||
| #[must_use] | ||
| pub fn with_overwrite(mut self) -> Self { | ||
| match &mut self { | ||
| Self::Copy(t) | Self::Move(t) => t.overwrite = true, | ||
| Self::Delete { .. } => {} | ||
| Self::ExtractArchive { overwrite, .. } | Self::CreateArchive { overwrite, .. } => { | ||
| *overwrite = true; | ||
| } | ||
| } | ||
| self | ||
| } |
There was a problem hiding this comment.
Using a builder pattern (with_overwrite(self) -> Self) for PendingAction forces callers to move the action out of state.ui.pending_action via take(), apply the mutation, and then re-insert it back into the Option.
Using an in-place mutator (set_overwrite(&mut self)) is much more idiomatic and cleaner in Rust, as it allows callers to mutate the action directly inside the Option using if let Some(a) = &mut state.ui.pending_action { a.set_overwrite(); }, avoiding the take() and re-assignment boilerplate.
impl PendingAction {
/// Marks the action as "overwrite existing targets".
///
/// Unifies the duplicated `overwrite` flag across every variant in one place.
/// `Delete` has no destination to overwrite, so it is a no-op.
pub fn set_overwrite(&mut self) {
match self {
Self::Copy(t) | Self::Move(t) => t.overwrite = true,
Self::Delete { .. } => {}
Self::ExtractArchive { overwrite, .. } | Self::CreateArchive { overwrite, .. } => {
*overwrite = true;
}
}
}
}| KeyCode::Char('o' | 'O') => { | ||
| if let Some(action) = state.pending_action.as_mut() { | ||
| action.set_overwrite(); | ||
| if let Some(a) = state.ui.pending_action.take() { | ||
| state.ui.pending_action = Some(a.with_overwrite()); | ||
| } | ||
| } |
There was a problem hiding this comment.
With set_overwrite(&mut self) on PendingAction, we can mutate the action in-place without taking it out of the Option.
| KeyCode::Char('o' | 'O') => { | |
| if let Some(action) = state.pending_action.as_mut() { | |
| action.set_overwrite(); | |
| if let Some(a) = state.ui.pending_action.take() { | |
| state.ui.pending_action = Some(a.with_overwrite()); | |
| } | |
| } | |
| KeyCode::Char('o' | 'O') => { | |
| if let Some(a) = &mut state.ui.pending_action { | |
| a.set_overwrite(); | |
| } | |
| } |
| KeyCode::Enter => match state.input.dialog_selection { | ||
| 0 => { | ||
| if let Some(action) = state.pending_action.as_mut() { | ||
| action.set_overwrite(); | ||
| if let Some(a) = state.ui.pending_action.take() { | ||
| state.ui.pending_action = Some(a.with_overwrite()); | ||
| } | ||
| } |
There was a problem hiding this comment.
With set_overwrite(&mut self) on PendingAction, we can mutate the action in-place without taking it out of the Option.
| KeyCode::Enter => match state.input.dialog_selection { | |
| 0 => { | |
| if let Some(action) = state.pending_action.as_mut() { | |
| action.set_overwrite(); | |
| if let Some(a) = state.ui.pending_action.take() { | |
| state.ui.pending_action = Some(a.with_overwrite()); | |
| } | |
| } | |
| KeyCode::Enter => match state.input.dialog_selection { | |
| 0 => { | |
| if let Some(a) = &mut state.ui.pending_action { | |
| a.set_overwrite(); | |
| } | |
| } |
| 0 => { | ||
| if let Some(action) = state.pending_action.as_mut() { | ||
| action.set_overwrite(); | ||
| if let Some(a) = state.ui.pending_action.take() { | ||
| state.ui.pending_action = Some(a.with_overwrite()); | ||
| } |
There was a problem hiding this comment.
With set_overwrite(&mut self) on PendingAction, we can mutate the action in-place without taking it out of the Option.
| 0 => { | |
| if let Some(action) = state.pending_action.as_mut() { | |
| action.set_overwrite(); | |
| if let Some(a) = state.ui.pending_action.take() { | |
| state.ui.pending_action = Some(a.with_overwrite()); | |
| } | |
| 0 => { | |
| if let Some(a) = &mut state.ui.pending_action { | |
| a.set_overwrite(); | |
| } |
Greptile SummaryThis refactor tackles AppState's 36-field god-object, the dual-store PanelListing sync bug, and config/sorting/MIME/keymap boilerplate. It is a large but well-scoped mechanical migration with clear separation between structural changes and behaviour fixes.
Confidence Score: 4/5The refactor is mechanically correct and well-tested; the AppState decomposition and PanelListing redesign can go in as-is. One issue in text_input.rs needs attention before merge. The text_input scroll-offset fallback leaves the cursor invisible in the specific case the PR description states it fixes: a cursor positioned after a grapheme whose display width exceeds the visible viewport. The remaining ~3500 lines of changes are solid mechanical migrations with thorough test coverage. src/app/types/text_input.rs — the current_scroll_offset fallback path Important Files Changed
Reviews (1): Last reviewed commit: "refactor(app): AppState aggregates, pane..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -92,28 +108,25 @@ impl TextInput { | |||
| Some(c) | |||
| }) | |||
| .position(|cum| cum >= raw_scroll) | |||
| .unwrap_or(0); | |||
| self.scroll_offset = widths[..start_idx].iter().sum(); | |||
| .unwrap_or(widths.len().saturating_sub(1)); | |||
| widths[..start_idx].iter().sum() | |||
There was a problem hiding this comment.
The fallback .unwrap_or(widths.len().saturating_sub(1)) resolves to the start of the last grapheme, not the end. When the cursor is placed after a grapheme whose display width exceeds visible_width, cursor_display can easily exceed scroll_offset + visible_width, leaving the cursor outside the viewport.
Concrete: widths = [3, 10], visible_width = 5, cursor = 2 (end of text).
cursor_display = 13,raw_scroll = 9.- No boundary ≥ 9 → current fallback gives
start_idx = 1,scroll_offset = 3. - Viewport
[3, 7]; cursor at display 13 — not visible.
With widths.len() as the fallback, scroll_offset = 13 = cursor_display. Viewport [13, 17]; cursor at relative 0 — visible on the left edge. This matches the stated goal in the PR description: "keep the cursor visible on the right when the last grapheme is wider than the viewport".
| const MSG_COMMAND_SUCCEEDED: &str = "\n[Command succeeded. Press Enter to return]"; | ||
| const PRESS_ENTER_TO_RETURN: &str = "Press Enter to return]"; |
There was a problem hiding this comment.
PRESS_ENTER_TO_RETURN contains a trailing ] but no leading [; the opening bracket lives in the format strings that embed it. This splits a single syntactic bracket pair across two source locations, which makes the strings hard to parse at a glance and risks mismatched brackets if either side is edited independently.
| const MSG_COMMAND_SUCCEEDED: &str = "\n[Command succeeded. Press Enter to return]"; | |
| const PRESS_ENTER_TO_RETURN: &str = "Press Enter to return]"; | |
| const MSG_COMMAND_SUCCEEDED: &str = "\n[Command succeeded. Press Enter to return]"; | |
| const MSG_COMMAND_FAILED_SUFFIX: &str = " Press Enter to return]"; |
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!
|
I've completed a thorough review of this PR. Here's my assessment: SummaryThis is a high-quality refactoring PR with excellent documentation, comprehensive test coverage (1212 tests), and the CI gate is fully green (build + clippy Strengths
Issues & SuggestionsMinor: Naming collision on
|
- text_input: fix the scroll fallback for an end-of-line cursor past a
grapheme wider than the viewport. The scan only yields grapheme *start*
offsets, so `position` returns None exactly in that case; fall back to
widths.len() (full width) instead of widths.len()-1 so the cursor stays
visible. Add a regression test.
- job_runner: log the real panic message by downcasting the join() payload
to &str/String ({:?} on Box<dyn Any> only prints "Any").
- modes: revert PendingAction::with_overwrite(self) back to an in-place
set_overwrite(&mut self) mutator; callers now edit the action inside the
Option directly instead of take()/re-insert.
- panel_ops::set_active_panel delegates to AppState::set_active_panel to
avoid two same-named functions diverging.
- Fix broken intra-doc links introduced by the refactor (set_overwrite,
PanelListing::set_filtered/set_filtered_all/upsert/remove, debug_log #[allow]).
Gate: clippy -D warnings clean; 1214 tests passing.
|
I've completed my independent review by examining the actual code (both commits), running the CI gate, and verifying each prior reviewer's claims. Here's my assessment. Review SummaryVerdict: Approve. High-quality, well-documented refactor with the CI gate fully green (fmt clean, clippy Prior issues already fixed in commit 2 ✅
Remaining issues (minor/nits)
Strengths
The remaining items are stylistic and suitable for follow-up; none block merge. |
1 participant
Tackle the AppState god-object and types/* modeling debt, fix type-level bugs, and dedup config/paths/shell/keymap/mime boilerplate.
App state & types:
Type-level bugs:
Sorting & config:
Safety & misc:
CI gate green: build + clippy -D warnings + 1212 tests passing.