refactor(input): EventContext, dispatch split, bugfixes & test quality

[leszek3737]

Implements docs/pr-plan/pr-08-input-main-infra.md (PR8).

Bugs (with value-asserting tests):

• dialogs: pass trimmed dest to resolve_user_path (extract/create archive)
• menu: surface menu-bar width overflow (debug_log + saturate) not silent 0
• mouse: guard panel_bounds for tiny terminals (no impossible row range)
• main: recover_terminal_state no longer reports a working terminal as a failure

Refactor:

• EventContext bundles loop state; collapses dispatch_event/dispatch_key_event (drops too_many_arguments), per-mode dispatch table, run_app split
• handlers migrated to &mut EventContext; drop dead _viewer_state params
• mouse/mode_dispatch/normal/dialogs/pickers/menu_actions/directory_tree: helper extraction, shared matchers, named constants, fewer hot-path clones
• lib.rs: drop dead root re-exports (module-path public API surface)
• input/mod.rs: pub(crate)

Build:

• Cargo.toml: [profile.release] lto + codegen-units=1, [profile.dev] opt-level=1

Tests:

• value-asserting key/mouse outcomes, picker/history coverage, parameterized dialog cases, loader-exposing harness

No behavior change beyond the listed fixes. Gate green: fmt, clippy -D warnings, 1225 tests, release build.

[sourcery-ai]

Sorry @leszek3737, your pull request is larger than the review limit of 150000 diff characters

[gemini-code-assist]

Code Review

This pull request refactors the input handling layer by introducing an EventContext struct to consolidate long-lived, per-frame mutable state, simplifying parameter lists across various event dispatch and mode handlers. It also deduplicates archive dialog logic, optimizes string building to reduce allocations, cleans up public API exports, and updates the integration test suite to match the new signatures. Feedback on the changes suggests trimming the dialog input text to prevent accidental leading or trailing whitespace during input-based actions.

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]

Trimming the input here ensures that all input-based actions (such as creating directories, renaming files, filtering, and quick-cd) are protected against accidental leading or trailing whitespace. This is especially helpful when users copy-paste names or paths into the TUI dialogs, preventing the accidental creation of files or directories with trailing spaces which can be difficult to manage on some filesystems.

Suggested change

	let input = state.input.dialog_input.text().to_owned();
	let input = state.input.dialog_input.text().trim().to_owned();

[greptile-apps]

Greptile Summary

Large input-layer refactor that introduces EventContext to bundle per-frame mutable state, eliminating the 7–8 argument parameter lists carried through dispatch_event and all mode handlers. Four pre-existing bugs are fixed along the way: archive destination paths are now trimmed before resolve_user_path, menu-bar width overflow now saturates and logs instead of silently collapsing, panel_bounds guards against tiny terminals producing an impossible (1, 0) row range, and recover_terminal_state no longer propagates a leave failure when enter succeeded.

• EventContext / dispatch split: all mode handlers migrated from individual &mut parameters to &mut EventContext; dispatch_event and dispatch_key_event drop their #[allow(clippy::too_many_arguments)] suppression; AppLoop struct and poll_async/render_frame helpers break up the 300-line run_app.
• Refactors across mouse, normal, menu, directory-tree, dialogs: named constants replace magic numbers, shared matchers (is_viewer_exit_key, archive_dialog_nav, open_confirm_dialog) replace repeated arms, and dead _viewer_state parameters are removed.
• Tests: value-asserting key/mouse outcomes (Ok(true/false) instead of is_ok()), new picker/history coverage, parameterised dialog cases, and a loader-exposing DispatchResult harness.

Confidence Score: 4/5

Safe to merge after fixing the archive dialog Left/Right dual-action regression; all four advertised bug fixes are correct, the EventContext refactor is well-structured, and the test suite is genuinely improved.

The archive extract and create dialogs now respond to Left/Right by both toggling the OK/Cancel button focus and moving the text cursor in the destination path field. In the old code those keys returned early after toggling the button and never reached apply_dialog_text_edit. The change is unintentional (the PR description says no behaviour change beyond the listed fixes), and no test covers Left/Right navigation in archive dialogs, so the regression was invisible to the test suite.

src/input/dialogs.rs — specifically the ArchiveNav::Continue return value for KeyCode::Left | KeyCode::Right in archive_dialog_nav

Important Files Changed

Filename	Overview
src/input/dialogs.rs	Refactors archive dialogs into shared handle_archive_dialog/archive_dialog_nav helpers; contains the archive-path trim bugfix. The Left/Right arm returns Continue instead of a key-consumed variant, causing apply_dialog_text_edit to additionally move the text cursor.
src/input/mod.rs	Introduces EventContext struct bundling per-frame mutable state; all submodules tightened to pub(crate).
src/main.rs	run_app split into init_app_state/init_watcher/AppLoop/poll_async/render_frame; recover_terminal_state bug fixed.
src/input/mouse.rs	panel_bounds guards tiny terminals; visible_rows/panel_height helpers centralise scroll math; all arithmetic is saturating.
src/menu.rs	MENU_BAR_TEXT_WIDTH computed at compile time; menu_title_width overflow now debug-logs and saturates; ASCII compile-time guard added.
src/input/normal.rs	launch_editor factored into write_terminal_state_marker/spawn_editor/clear_terminal_state_marker/editor_status_message helpers.
src/input/mode_dispatch.rs	All mode handlers migrated to &mut EventContext; is_viewer_exit_key extracted as single source of truth.
src/tests/keyevents.rs	Tests upgraded to value-asserting Ok(true/false); new tests cover confirmed Delete job, F3 viewer loader, and repeat-destructive guard.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[crossterm Event] --> B[dispatch_event]
    B -->|Key| C[dispatch_key_event]
    B -->|Mouse| D[dispatch_mouse_event]
    B -->|Resize| E[ctx.term_size updated]
    C --> F{ctx.state.mode}
    F -->|Normal| G[handle_normal_mode]
    F -->|Viewing| H[handle_viewer_mode]
    F -->|Dialog| I[handle_dialog]
    F -->|Menu| J[handle_menu_mode]
    F -->|Search| K[handle_search_mode]
    F -->|DirectoryTree| L[handle_directory_tree]
    F -->|ListPicker| M[handle_list_picker]
    F -->|CommandLine| N[handle_command_line]
    D --> O[handle_mouse_event]
    O -->|NormalKey| G
    O -->|MenuAction| Q[run_selected_menu_action]
    G -.->|ctx| S[(EventContext)]
    I -.->|ctx| S
    J -.->|ctx| S

Loading

_{Reviews (1): Last reviewed commit: "refactor(input): EventContext, dispatch ..." | Re-trigger Greptile}

[greptile-apps]

Left/Right keys in the archive extract/create dialogs now trigger two independent actions in sequence: archive_dialog_nav toggles dialog_selection (OK→Cancel or Cancel→OK) and returns ArchiveNav::Continue, then apply_dialog_text_edit also moves the text cursor one position left or right in the destination path field. In the old code both handle_archive_extract_dialog and handle_archive_create_dialog returned early on Left/Right, so the text cursor was never touched. The Continue variant's doc comment explicitly says "fall through to text edit", which makes the dual-action behaviour look intentional, but it produces confusing UX and contradicts the PR description's "No behaviour change beyond the listed fixes".

Suggested change

	KeyCode::Left | KeyCode::Right => {
	state.input.dialog_selection = if state.input.dialog_selection == 0 {
	1
	} else {
	0
	};
	return;
	ArchiveNav::Continue
	}
	KeyCode::Left | KeyCode::Right => {
	state.input.dialog_selection = if state.input.dialog_selection == 0 {
	1
	} else {
	0
	};
	ArchiveNav::Handled
	}

[opencode-agent]

PR Review: refactor(input): EventContext, dispatch split, bugfixes & test quality

Validation

Gate	Result
cargo fmt --check	clean
cargo clippy --locked --all-targets -- -D warnings	no warnings
cargo test --locked	1225 passed (933 lib + 292 integration), 0 failed
cargo build --release --locked	success

Overall Assessment

High-quality refactor. Approve with minor comments. The EventContext introduction cleanly collapses the 7-8 argument parameter lists that were spreading through the dispatch layer, the per-mode dispatch table reads well, and the test harness upgrades (assert_eq! handled, Ok(true) instead of handled.is_ok(), named menu coordinates, parameterized dialog cases) materially raise signal.

Strengths

• EventContext design (src/input/mod.rs:30): Correctly keeps Terminal<B> and per-event data (key/mouse) out of the struct so the B parameter doesn't leak into the call graph. Doc comment makes this intentional boundary explicit.
• Discriminant-only match (src/main.rs:483): Matching ctx.state.mode with non-binding patterns (Dialog(_), ListPicker(_)) to avoid the previous per-keypress mode.clone() deep-allocation is a real hot-path win. Well-documented.
• Bug fixes:
  • commit_archive_* trimming dest before resolve_user_path (src/input/dialogs.rs:580,601) — real correctness fix (trailing spaces in path would otherwise create literal "foo " files).
  • recover_terminal_state now prioritizes the enter result over leave (src/main.rs:439-447) — correct; the old leave.and(enter)? would abort startup even when the terminal was actually usable.
• Compile-time work in menu.rs: MENU_BAR_TEXT_WIDTH const-fn with _TITLES_ARE_ASCII guard is a clean zero-runtime-cost improvement.
• Visibility tightening: pub → pub(crate) in src/input/mod.rs and dropping the redundant crate-root re-exports in src/lib.rs correctly shrinks the API surface to what's actually intended.

Issues / Suggestions

1. PR description oversells the test coverage of bug fixes — medium

The body claims "Bugs (with value-asserting tests)" but the specific bugs aren't directly tested:

• dialogs trim: no test asserts " foo/bar " resolves to foo/bar after commit. Most valuable to add — it's the only fix with user-visible behavior change.
• menu width overflow: untestable in practice (titles are short ASCII).
• panel_bounds tiny terminal: no direct test. Note: the runtime behavior here is unchanged — saturating_sub already yielded panel_height = 1 for (1, 0), identical to the new (1, 1). This is a clarity-of-intent fix, not a runtime bug fix; the description should reflect that.
• recover_terminal_state: no test (hard to test, but worth flagging that the claim is loose).

The new value-asserting tests (dispatch_test_event_exposes_job_on_confirmed_delete, dispatch_test_event_exposes_viewer_loader_on_f3, history_dedup_of_existing_entry_at_cap_preserves_all_uniques, etc.) are excellent — they're just not regression tests for these specific bugs.

2. Verbose reborrow in run_selected_menu_action — medium

src/input/mode_dispatch.rs:201-220 manually expands &mut *ctx.<field> for each context field to construct an inner EventContext inside the with_menu_panel closure:

let viewer_state = &mut *ctx.viewer_state;
let viewer_loader = &mut *ctx.viewer_loader;
let image_preview_loader = &mut *ctx.image_preview_loader;
let running_job = &mut *ctx.running_job;
let term_size = ctx.term_size;
panel_ops::with_menu_panel(ctx.state, |state| {
    let mut inner = EventContext { state, viewer_state, ... };
    handle_normal_mode(&mut inner, ...);
});

Consider adding a reborrow helper on EventContext:

impl<'a> EventContext<'a> {
    pub(crate) fn reborrow(&mut self) -> EventContext<'_> {
        EventContext {
            state: self.state,
            viewer_state: self.viewer_state,
            viewer_loader: self.viewer_loader,
            image_preview_loader: self.image_preview_loader,
            running_job: self.running_job,
            term_size: self.term_size,
        }
    }
}

Then the closure body becomes handle_normal_mode(&mut ctx.reborrow(), ...) after splitting out state. This pattern will recur anywhere a sub-context is needed.

3. Duplicated EventContext scaffolding across tests — low

The same let mut viewer_state = None; … let mut ctx = EventContext { … }; block appears in src/input/directory_tree.rs:153, src/tests/helpers.rs:44,116, src/tests/menu.rs:67, src/tests/user_menu.rs:166,189,219,244, src/tests/viewer.rs:165,259, src/tests/overwrite.rs:519, src/tests/keybinds.rs:386,407. A shared mk_ctx(&mut state, h) -> EventContext<'_> (with owned None defaults) would eliminate the copy-paste and make future EventContext field additions one-site changes.

4. Minor observations

• set_selection_range (src/input/mouse.rs:736): the doc note about not being a true single-scan batch is good transparency. If hot-path profiling ever shows it matters, a PanelState::select_range method is the right home.
• _TITLES_ARE_ASCII const assert (src/menu.rs:213) is good defensive coding for the byte-length-as-display-width assumption baked into const_title_width.
• std::mem::take for command_draft (src/input/command_line.rs:120): the comment correctly justifies the optimization (next Up repopulates before any read). Verified safe.
• The display_file_names change (src/input/normal.rs:605) avoids the intermediate Vec<PathBuf> from file_names_from_paths; semantics preserved.

Project Conventions

All followed: no println!/unwrap/expect in production paths, debug_log! used in the menu overflow breadcrumb, let … else over .unwrap(), tempfile::TempDir for FS-touching tests, #[allow] only on test modules, conventional commit message format.

Recommendation

Merge after:

1. Adding a regression test for the archive-dialog trim fix (the one user-visible behavior change lacking coverage).
2. (Optional) Extracting the EventContext::reborrow helper per point 2.

The verbose reborrow and the duplicated test scaffolding can ship as-is if you'd rather not extend the diff — they're stylistic.

opencode session  |  github run

[opencode-agent]

PR Review: refactor(input): EventContext, dispatch split, bugfixes & test quality

Validation

Gate	Result
cargo fmt --check	clean
cargo clippy --locked --all-targets -- -D warnings	no warnings
cargo test --locked	1227 passed (933 lib + 294 integration), 0 failed
cargo build --release --locked	success

Overall Assessment

Approve. High-quality refactor with all gates green. The PR has already incorporated fixes for the issues raised by earlier reviewers (Greptile's P1 archive Left/Right regression and Gemini's trim suggestion were both addressed in follow-up commit 16914e5).

Strengths

• EventContext design (src/input/mod.rs:26): Correctly keeps Terminal<B> and per-event data out of the struct so the B parameter doesn't leak into the call graph. The doc comment makes this intentional boundary explicit.
• Discriminant-only match (src/main.rs:487): Matching ctx.state.mode with non-binding patterns (Dialog(_), ListPicker(_)) avoids the per-keypress mode.clone() deep-allocation. Well-documented hot-path win.
• All 4 advertised bug fixes verified correct:
  • commit_archive_* trims dest before resolve_user_path (src/input/dialogs.rs:591,612) — real correctness fix.
  • recover_terminal_state now propagates enter and only logs leave (src/main.rs:446-449) — correct.
  • menu_title_width saturates + logs on overflow (src/menu.rs:213-227).
  • panel_bounds guards tiny terminals with MIN_PANEL_HEIGHT (src/input/mouse.rs:166).
• Follow-up commit 81e5437 correctly migrates the function-bar cell cache from static OnceLock to thread_local! to handle ratatui-widgets 0.3.1's non-Sync Shadow in Block. Rendering is single-threaded so this is equivalent.
• Visibility tightening (pub → pub(crate) in input/mod.rs, dead re-exports dropped from lib.rs) correctly shrinks the API surface.
• Compile-time work in menu.rs: MENU_BAR_TEXT_WIDTH const-fn with _TITLES_ARE_ASCII guard is a clean zero-runtime-cost improvement.

Issues / Suggestions

1. Missing regression test for archive-dialog trim fix — medium

The PR description claims "Bugs (with value-asserting tests)" but only CreateDirectory has a direct trim test (src/tests/keyevents.rs:148 create_directory_trims_surrounding_whitespace). The archive extract/create trim path (commit_archive_extract/commit_archive_create at src/input/dialogs.rs:591,612) — the actual advertised fix — has no dedicated coverage. A test driving " /tmp/dest " through ArchiveExtract Enter and asserting pending_action.dest == /tmp/dest would close this gap.

2. Verbose reborrow in run_selected_menu_action — low (stylistic)

src/input/mode_dispatch.rs:212-227 manually expands &mut *ctx.<field> for each field to construct an inner EventContext inside the with_menu_panel closure. An EventContext::reborrow() helper would collapse this:

impl<'a> EventContext<'a> {
    pub(crate) fn reborrow(&mut self) -> EventContext<'_> { /* ... */ }
}

The current code is correct; this is purely a readability improvement.

3. Duplicated EventContext scaffolding across tests — low (stylistic)

The same let mut viewer_state = None; … let mut ctx = EventContext { … }; block appears in ~10 test sites (directory_tree.rs:153, helpers.rs:44,116, tests/menu.rs:67, tests/user_menu.rs:166,189,219,244, tests/viewer.rs:165,259, tests/overwrite.rs:519, tests/keybinds.rs:386,407). A shared mk_ctx(&mut state, size) -> EventContext<'_> helper would make future field additions one-site changes. helpers.rs already has dispatch_key/dialog_key that wrap this; extending the pattern would help.

4. Minor observations

• apply_scroll_delta (src/input/mouse.rs:265): current + delta.unsigned_abs() could in theory overflow on pathological inputs, but is bounded by total in practice — fine.
• set_selection_range (src/input/mouse.rs:735): the doc note about not being a true single-scan batch is good transparency.
• std::mem::take for command_draft (src/input/command_line.rs:120): correctly justified — the next Up repopulates before any read.

Project Conventions

All followed: no println!/unwrap/expect in production paths, debug_log! used in the menu overflow breadcrumb, let … else over .unwrap(), tempfile::TempDir for FS-touching tests, #[allow] only on test modules, conventional commit message format.

Recommendation

Merge as-is. The one substantive gap (archive trim regression test, point 1) is desirable but not blocking — the behavior is correct and the CreateDirectory trim test already exercises the shared handle_input_action trim path. Points 2 and 3 are stylistic and can ship later.

opencode session  |  github run