refactor(input): reduce tech debt across 6 files — helpers, dedup, bugfix

[leszek3737]

command_line.rs: reset_history() helper (6 call sites), mem::take
instead of clone, intent comment on Up idx==0 clamp

directory_tree.rs: _viewer_state intent comment, Home is_empty guard,
ensure_selected_visible() extracted, diagnostics join(), match
instead of .map().unwrap_or(), removed unused fmt::Write import

menu_actions.rs: separator pattern Vec + join(" | ")

mode_dispatch.rs: fix apply_search_filter to call refresh_or_rebuild
(was filtering stale unfiltered_entries), extract
set_active_panel_filter helper

normal.rs: open_in_viewer() dedup F3/Enter, is_not_parent_dir()
helper, editor errors restructured to show both errors on
spawn fail + resume fail

pickers.rs: LIFO reversal comment + len-1-idx simplification,
compare_mode bounds check uses .get() for consistency

Summary by Sourcery

Refine input handling and search behavior while reducing duplication and clarifying status messaging across command line, directory tree, menus, mode dispatch, normal mode, and pickers.

Bug Fixes:

• Guard Home key handling in the directory tree when no entries are present to avoid invalid selection state.
• Fix search filtering to rebuild or refresh panel entries after updating the active panel filter, preventing use of stale unfiltered entries.
• Use safe bounds-checked access in the compare mode picker to avoid out-of-range selection when Enter is pressed.

Enhancements:

• Centralize command-line history reset logic and streamline history navigation and word deletion behavior.
• Extract helpers for keeping the directory tree selection visible and for computing target directories when changing directories, and simplify diagnostic status message construction.
• Deduplicate viewer-opening logic for normal mode (F3/Enter) and add a helper for excluding parent directory entries in path selection.
• Simplify user menu warning status composition using string joins for clearer, more maintainable formatting.
• Clarify history picker indexing with an explicit LIFO reversal and more direct index math for command history lookup.
• Extract a helper for setting the active panel filter to share logic between search initiation and filter application, and consistently route through the existing refresh path.
• Improve editor error reporting so both editor exit status and terminal restore failures can be surfaced together when applicable.

[sourcery-ai]

Reviewer's Guide

Refactors input-related modules to reduce duplication, clarify intent, and fix subtle behavioral issues in history handling, directory tree navigation, search filtering, menu warnings, and editor error reporting.

Sequence diagram for the updated search filter application

sequenceDiagram
    actor User
    participant ModeDispatch as mode_dispatch
    participant AppState
    participant Panel as ActivePanel
    participant PanelOps as panel_ops

    User->>ModeDispatch: apply_search_filter(state, visible)
    ModeDispatch->>AppState: read search_query

    alt after_refactor
        ModeDispatch->>ModeDispatch: set_active_panel_filter(state, filter_query)
        ModeDispatch->>AppState: active_panel_mut()
        AppState->>Panel: set_filter(Some(filter_query))
        ModeDispatch->>ModeDispatch: refresh_or_rebuild(state, visible)
        ModeDispatch->>PanelOps: refresh_or_rebuild(state, visible)
    else before_refactor
        ModeDispatch->>AppState: active_panel_mut()
        AppState->>Panel: set_filter(Some(filter_query))
        ModeDispatch->>PanelOps: rebuild_visible_entries(panel, visible)
    end

Loading

File-Level Changes

Change	Details	Files
Centralize command-line history reset logic and improve history draft handling.	Introduced reset_history helper and replaced multiple direct history_index resets with this helper across command-line handlers. Used std::mem::take when capturing the current draft before moving into history navigation, avoiding an extra clone and making ownership explicit. Added an intent comment documenting the behavior when navigating beyond the oldest history item with Up (clamping at index 0).	src/input/command_line.rs
Improve directory tree UX and simplify diagnostics/status construction.	Guarded Home key handling so it only resets selection/scroll when there are tree entries, preventing unnecessary state changes on empty trees. Extracted ensure_selected_visible to encapsulate scroll/selection clamping logic, passing scroll as &mut to decouple from AppState. Reimplemented diagnostic status aggregation using map + join over a Vec rather than manual string building and write!, clarifying the format. Replaced an Option parent() handling pattern from map().unwrap_or() to an explicit match, making the non-parent case clearer. Removed an unused fmt::Write import and added an intent comment on viewer state where applicable.	src/input/directory_tree.rs
Refactor menu warning/status formatting to use a vector + join pattern.	Collect user menu warnings into a Vec and join with "	" instead of manually concatenating to a String. Append the local menu confirmation message as a separate element in the same vector, simplifying conditional concatenation and only setting status_message when there is at least one part.
Fix search filter application to refresh correctly and centralize panel filter setting.	Introduced set_active_panel_filter helper to encapsulate setting the active panel's filter. Changed apply_search_filter to call refresh_or_rebuild instead of panel_ops::rebuild_visible_entries directly, ensuring filtered view is consistent with other search operations. Updated initiate_search to use set_active_panel_filter for consistency with apply_search_filter.	src/input/mode_dispatch.rs
Deduplicate viewer-opening logic and clarify editor error reporting and parent-dir filtering.	Extracted open_in_viewer helper and reused it for both F3 and Enter archive handling paths, ensuring consistent mode transitions when opening the viewer. Introduced is_not_parent_dir helper and used it in both current-entry fallback and selected entries filters to consistently skip ".." entries. Restructured launch_editor error handling to compose both editor exit status and terminal restore failure into a single status message when both occur, while keeping simple cases readable.	src/input/normal.rs
Clarify picker index behavior and harden compare-mode picker bounds.	Simplified history picker index math to len - 1 - picker_selected with an explicit LIFO reversal comment, assuming picker_selected is in range. Used modes.get(...) when resolving the compare-mode picker selection to avoid out-of-bounds access and gracefully return to Normal when the index is invalid.	src/input/pickers.rs

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• In handle_history_picker, replacing the previous saturating_sub logic with let idx = len - 1 - state.picker_selected can panic if picker_selected >= len; consider either clamping/validating picker_selected before this calculation or keeping saturating arithmetic to avoid a potential out-of-bounds panic.
• In set_tree_diagnostic_status, you can avoid the intermediate Vec<String> allocation by joining directly over an iterator (e.g., using itertools::Itertools::join or manual writing) to reduce allocations on hot paths with many diagnostics.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `handle_history_picker`, replacing the previous `saturating_sub` logic with `let idx = len - 1 - state.picker_selected` can panic if `picker_selected >= len`; consider either clamping/validating `picker_selected` before this calculation or keeping saturating arithmetic to avoid a potential out-of-bounds panic.
- In `set_tree_diagnostic_status`, you can avoid the intermediate `Vec<String>` allocation by joining directly over an iterator (e.g., using `itertools::Itertools::join` or manual writing) to reduce allocations on hot paths with many diagnostics.

## Individual Comments

### Comment 1
<location path="src/input/pickers.rs" line_range="53-54" />
<code_context>
                 return;
             }
-            let idx = len.saturating_sub(1).saturating_sub(state.picker_selected);
+            // History displays most-recent-first; reverse visual index to VecDeque position
+            let idx = len - 1 - state.picker_selected;
             if let Some(cmd) = state.command_history.get(idx).cloned() {
                 state.command_line.text = cmd;
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against picker_selected >= len to avoid potential panic from subtraction underflow.

With the previous `saturating_sub` logic we were safe even if `picker_selected` drifted out of range. Now, `let idx = len - 1 - state.picker_selected;` will panic in debug (and wrap in release) when `picker_selected >= len`. Consider clamping `picker_selected` to `< len` or early-returning in that case, similar to the bounds check you use in `handle_compare_mode_picker` with `modes.get(...)`.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

issue (bug_risk): Guard against picker_selected >= len to avoid potential panic from subtraction underflow.

With the previous saturating_sub logic we were safe even if picker_selected drifted out of range. Now, let idx = len - 1 - state.picker_selected; will panic in debug (and wrap in release) when picker_selected >= len. Consider clamping picker_selected to < len or early-returning in that case, similar to the bounds check you use in handle_compare_mode_picker with modes.get(...).

[gemini-code-assist]

Code Review

This pull request refactors several input handling modules by extracting helper functions (such as reset_history, ensure_selected_visible, open_in_viewer, and set_active_panel_filter), simplifying warning formatting with join, and improving safety in pickers. A review comment suggests rewriting a scroll visibility check to prevent potential integer overflow.

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.

[gemini-code-assist]

To prevent any potential integer overflow panic in debug mode when adding *scroll + visible_height, you can rewrite the condition as selected - *scroll >= visible_height. Since the first if branch ensures selected >= *scroll, this subtraction is guaranteed not to underflow.

Suggested change

	} else if selected >= *scroll + visible_height {
	} else if selected - *scroll >= visible_height {

[opencode-agent]

All tests pass (226/226), clippy is clean. There's a minor formatting issue (3 lines need cargo fmt). Here's my review:

---

PR Review: refactor(input): reduce tech debt across 6 files

Verdict: Approve with minor nits

All changes are well-scoped, correct, and consistent with project conventions. The clippy gate passes, all 226 tests pass. One formatting issue to fix before merge.

---

Bug Fixes (all correct)

1. mode_dispatch.rs:41-44 — search filter stale entries bugfix. This is the most impactful fix. Previously apply_search_filter called rebuild_visible_entries directly on the panel after setting the filter, which could operate on stale unfiltered_entries. Now it routes through refresh_or_rebuild, which checks the unfiltered_dirty flag. Good catch.

2. directory_tree.rs:29 — Home key guard. KeyCode::Home now has if !state.tree_entries.is_empty(), preventing setting tree_selected = 0 on an empty list. Correct defensive guard matching the pattern on End and PageDown.

3. pickers.rs:112-115 — compare mode bounds check. Changed from direct indexing (modes[state.picker_selected.min(len.saturating_sub(1))]) to modes.get(state.picker_selected). The old code was actually safe due to move_cursor clamping, but .get() is more idiomatic and consistent with the rest of the file.

---

Refactoring (all clean)

4. command_line.rs — reset_history() helper. Extracts state.history_index = None into a named function used at 6 call sites. Trivial but improves readability and makes future changes to reset logic single-point.

5. command_line.rs:84 — std::mem::take instead of clone. state.command_draft = std::mem::take(&mut state.command_line.text) avoids cloning when the command line text is about to be replaced anyway. Correct — the text is immediately replaced by history entry on the next lines.

6. directory_tree.rs:57-73 — ensure_selected_visible() extraction. Clean extraction of scroll-clamping logic into a standalone function. The function is pure (takes selected, &mut scroll, visible_height) which is good for testability.

7. directory_tree.rs:87-94 — diagnostics join(). Replaces a manual loop with fmt::Write (and the now-removed use std::fmt::Write) with a Vec<String> + join("] ["). Cleaner and the removed import is correct.

8. directory_tree.rs:127-130 — match instead of .map().unwrap_or(). Stylistic preference; both are fine. The match is arguably more readable here.

9. normal.rs:522-526 — open_in_viewer() dedup. Extracts the 3-line viewer-opening pattern used by F3, Enter-on-archive, and now shares one implementation. Clean dedup.

10. normal.rs:518-520 — is_not_parent_dir() helper. Extracts entry.name != ".." used in two filter closures. Named predicate is clearer.

11. menu_actions.rs:271-288 — Vec<String> + join(" | "). Replaces manual string building with push/separator logic. Cleaner and equivalent.

12. pickers.rs:53-54 — LIFO comment + len - 1 - idx. Replaces len.saturating_sub(1).saturating_sub(state.picker_selected) with len - 1 - state.picker_selected. Safe because picker_selected < len is guarded above. The comment explaining LIFO reversal is helpful.

---

One Issue: Formatting

cargo fmt --check shows 3 diffs in the PR code:

• directory_tree.rs:54 — function call should be on one line
• menu_actions.rs:272-280 — closure body and format! should be on one line
• normal.rs:522 — function signature should wrap across multiple lines

Fix: run cargo fmt and commit the result.

---

Minor Observations (non-blocking)

• command_line.rs:7-9: The reset_history function is a single statement. It's on the edge of being too trivial for a helper, but with 6 call sites it earns its place.
• mode_dispatch.rs:37-39: set_active_panel_filter is also a one-liner wrapper, but it centralizes the pattern and is used in two places (apply_search_filter and initiate_search). Acceptable.
• normal.rs:174-190: The editor error restructuring is a genuine improvement — the old code had a (Ok(s), Err(e)) if !s.success() arm that combined both messages, but a (Ok(s), Err(e)) with s.success() would fall through to (_, Err(e)) and only show the terminal restore error, losing the editor exit status. The new code correctly handles both.

---

Summary

Solid tech debt reduction PR. Three real bugfixes (stale filter, empty tree Home, bounds-checked compare mode), several clean extractions, and no regressions. Just needs cargo fmt before merge.

opencode session  |  github run

[opencode-agent]

Review complete. CI is green (all 1024 tests, clippy clean, fmt clean, release build succeeds). The PR is ready to merge — three real bugs fixed, clean dedup, and all project conventions followed.

opencode session  |  github run

[greptile-apps]

Greptile encountered an error while reviewing this PR. Please reach out to support@greptile.com for assistance.

[opencode-agent]

CI gate is fully green. Here is my review:

---

PR Review: refactor(input): reduce tech debt across 6 files

Verdict: Approve

CI gate passes completely: 226/226 tests, cargo fmt --check clean, clippy clean, release build succeeds.

---

Bug Fixes (all correct and well-targeted)

1. mode_dispatch.rs:41-44 — stale filter entries. The most impactful fix. Previously apply_search_filter called rebuild_visible_entries directly, which could operate on stale unfiltered_entries if the filesystem had changed. Now it routes through refresh_or_rebuild, which checks the unfiltered_dirty flag first. This ensures filtered views stay consistent with disk state.

2. directory_tree.rs:29 — Home key on empty tree. Adds if !state.tree_entries.is_empty() guard, consistent with the existing guards on End and PageDown. Prevents setting tree_selected = 0 when there are no entries to select.

3. pickers.rs:112-115 — compare mode bounds. Changed from direct indexing with manual clamp to .get() with let else. The old code was technically safe (due to move_cursor clamping), but .get() is more idiomatic and makes the defensive intent explicit.

4. normal.rs:174-189 — editor error handling logic bug. The old match had (Ok(s), Err(e)) if !s.success() handling both messages, but a (Ok(s), Err(e)) with s.success() would fall through to (_, Err(e)) and only show the terminal restore error — losing the editor exit status information. The new code correctly reports both conditions via a Vec<String> + join("; ").

---

Refactoring (all clean)

5. command_line.rs:7-9 — reset_history() helper. Single-statement helper used at 6 call sites. Earns its place by centralizing the reset pattern.

6. command_line.rs:84 — std::mem::take over clone. Correct — the command line text is about to be replaced by a history entry, so taking ownership avoids a redundant allocation.

7. directory_tree.rs:60-68 — ensure_selected_visible() extraction. Clean, testable, pure function. Takes selected, &mut scroll, visible_height — decoupled from AppState. The change from selected >= scroll + visible_height to selected - scroll >= visible_height (line 63) avoids a potential scroll + visible_height overflow in the else if branch, since the first if guarantees selected >= *scroll. This is a minor safety improvement.

8. directory_tree.rs:86-90 — diagnostics join(). Cleaner than the old fmt::Write loop, and the unused use std::fmt::Write import is correctly removed.

9. directory_tree.rs:123-126 — match over .map().unwrap_or(). Stylistic improvement; both are equivalent.

10. normal.rs:522-528 — open_in_viewer() dedup. Eliminates duplicated 3-line blocks from F3 and Enter-on-archive paths.

11. normal.rs:518-520 — is_not_parent_dir() helper. Named predicate used in two filter closures — clearer intent.

12. menu_actions.rs:271-288 — Vec<String> + join(" | "). Replaces manual separator logic. Equivalent behavior, more maintainable.

13. mode_dispatch.rs:37-39 — set_active_panel_filter() helper. Two-line wrapper used in both apply_search_filter and initiate_search. Centralizes the panel filter pattern.

14. pickers.rs:53-54 — LIFO index math. len - 1 - state.picker_selected is safe because of the guard on line 49 (if state.picker_selected >= len { return; }). The comment explaining LIFO reversal is helpful context.

---

Observations (non-blocking)

• command_line.rs:103 still uses .clone() to restore command_draft on Down-arrow past the end. This is fine — command_draft needs to remain valid for future Up/Down navigation, so take isn't appropriate here.
• directory_tree.rs:36-38 — PageUp has no empty-list guard, meaning tree_selected and tree_scroll stay at 0 via saturating_sub. This is consistent and correct, just noting the asymmetry with PageDown/Home/End which do have guards.
• The reset_history and set_active_panel_filter helpers are borderline trivial (single statements), but both have multiple call sites that justify their existence.

---

Summary

Solid tech debt reduction. Three real bugfixes (stale filter data, empty tree Home, editor error reporting logic), one overflow safety improvement in scroll math, and several clean extractions with zero regressions. Ready to merge.

opencode session  |  github run