Skip to content

refactor: extract file-tree key handling into key_actions module#2042

Open
sximelon wants to merge 3 commits into
Hmbown:mainfrom
sximelon:refactor_key_actions
Open

refactor: extract file-tree key handling into key_actions module#2042
sximelon wants to merge 3 commits into
Hmbown:mainfrom
sximelon:refactor_key_actions

Conversation

@sximelon
Copy link
Copy Markdown

Summary

Move the file-tree keyboard event handler (Up/Down/Enter/Esc) out of ui::run_event_loop into a dedicated key_actions module.

The new handle_file_tree_key function returns bool to signal whether the key was consumed, keeping the call site a single-line delegation.

Testing

  • cargo fmt --all -- --check
  • cargo clippy --workspace --all-targets --all-features
  • cargo test --workspace --all-features

Checklist

  • Updated docs or comments as needed
  • Added or updated tests where relevant
  • Verified TUI behavior manually if UI changes

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the file-tree keyboard event handling logic into a dedicated key_actions module to improve the maintainability of the main event loop. The review highlights a regression where the omission of the file_tree_visible check causes keyboard events to be intercepted even when the file tree is hidden. Further feedback points out stale documentation referencing an undefined KeyActionResult type and suggests consistent use of inlined variables in format strings.

Comment thread crates/tui/src/tui/key_actions.rs
Comment thread crates/tui/src/tui/key_actions.rs Outdated
Comment on lines +5 to +6
//! control-flow change (shutdown, return to caller) communicate via
//! [`KeyActionResult`].
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The module documentation refers to KeyActionResult for signaling control-flow changes, but the function currently returns a bool and KeyActionResult is neither defined nor used in this module. This documentation appears to be stale.

Comment thread crates/tui/src/tui/key_actions.rs Outdated
if let Some(rel_path) = file_tree.activate() {
let path_str = rel_path.to_string_lossy().to_string();
app.status_message = Some(format!("Attached @{path_str}"));
app.insert_str(&format!("@{} ", path_str));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Inconsistent format string style. Line 44 uses inlined variables (@{path_str}), while line 45 uses positional placeholders (@{} ). It is better to use the inlined style for consistency and to match the original code.

Suggested change
app.insert_str(&format!("@{} ", path_str));
app.insert_str(&format!("@{path_str} "));

Restore file_tree_visible guard, remove stale KeyActionResult docs, unify format string style.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant