Skip to content

feat: alt+enter fill-only - #40

Merged
sunlei merged 1 commit into
sunlei:masterfrom
kr-nn:alt-enter-fill-only
Aug 30, 2025
Merged

feat: alt+enter fill-only#40
sunlei merged 1 commit into
sunlei:masterfrom
kr-nn:alt-enter-fill-only

Conversation

@kr-nn

@kr-nn kr-nn commented Aug 23, 2025

Copy link
Copy Markdown
Contributor

Adds --expect and alt+enter will only fill the cli buffer instead of connecting immediately
useful when you're doing something besides just connecting to the server like:
ssh server -j otherserver
or
ssh server nixos-version

@claude

claude Bot commented Aug 30, 2025

Copy link
Copy Markdown

PR Review: feat: alt+enter fill-only

🎯 Summary

This PR adds a useful feature that allows Alt+Enter to fill the command buffer without immediately executing the SSH connection, providing more flexibility for complex SSH commands.

Code Quality & Best Practices

Strengths:

  • Good variable naming: key and selection are clear and descriptive
  • Proper Zsh practices: Uses setopt localoptions correctly
  • Consistent style: Follows the existing code formatting and indentation patterns
  • Clear logic flow: The key detection and handling logic is easy to follow

Suggestions:

  • Consider adding inline comments to explain the key parsing logic in zsh-ssh.zsh:244-254 for future maintainability
  • The variable selected_host is declared but not used - consider removing it from line 201

🐛 Potential Issues

Logic concerns:

  • Edge case handling: The condition if [[ "$key" == "$result" ]] (line 244) could be more explicit about what scenario this handles
  • String parsing robustness: The newline-based parsing assumes fzf output format won't change

Recommendations:

  • Add validation to ensure selection is not empty before calling _set_lbuffer
  • Consider what happens if fzf returns unexpected output format

Performance Considerations

Good:

  • No performance regressions identified
  • String operations are lightweight
  • No additional external command calls

🔒 Security Assessment

Status: ✅ Secure

  • No security concerns identified
  • No user input validation issues
  • No command injection vectors introduced
  • Maintains existing security posture

🧪 Test Coverage

Current state: No automated tests found in repository
Impact: Medium risk - feature works as intended but lacks regression protection

Recommendations:

  • Consider adding basic functional tests for the new Alt+Enter behavior
  • Test edge cases: empty selection, malformed fzf output
  • Verify behavior with various SSH command patterns mentioned in PR description

🎯 Feature Implementation

Excellent work on:

  • User experience: Great addition for power users who need to modify SSH commands
  • Backward compatibility: Maintains existing Enter key behavior perfectly
  • Use case coverage: Addresses the specific need for complex SSH commands like ssh server -j otherserver

📝 Minor Suggestions

  1. Documentation: Consider updating README.md to document the new Alt+Enter functionality
  2. Error handling: Add fallback behavior if key parsing fails
  3. Code organization: The key parsing logic could be extracted to a helper function for clarity

🏆 Overall Assessment

Rating: ⭐⭐⭐⭐ (4/5)

This is a well-implemented feature that adds genuine value without breaking existing functionality. The code is clean, follows project conventions, and solves a real user need. The implementation is straightforward and maintainable.

Recommendation: ✅ Approve with minor suggestions

The PR is ready to merge. The suggested improvements are enhancement opportunities rather than blocking issues.

@sunlei
sunlei requested a review from Copilot August 30, 2025 10:46

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a feature that allows users to fill the command line buffer without immediately executing the SSH connection. The addition of the --expect=alt-enter,enter flag to fzf enables two interaction modes: pressing Enter connects immediately (existing behavior), while pressing Alt+Enter only fills the buffer for further editing.

  • Adds dual key binding support with --expect flag in fzf
  • Implements conditional execution based on the key pressed (Enter vs Alt+Enter)
  • Preserves existing immediate connection behavior while adding fill-only option

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread zsh-ssh.zsh
Comment on lines +244 to +249
if [[ "$key" == "$result" ]]; then
selection="$result"
key=""
else
selection=${result#*$'\n'}
fi

Copilot AI Aug 30, 2025

Copy link

Choose a reason for hiding this comment

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

[nitpick] This logic for parsing the fzf result with expect keys is complex and could benefit from a comment explaining the format. When --expect is used, fzf returns the key on the first line and the selection on subsequent lines, but this isn't immediately clear from the code.

Copilot uses AI. Check for mistakes.
Comment thread zsh-ssh.zsh
fi
fi

zle reset-prompt

Copilot AI Aug 30, 2025

Copy link

Choose a reason for hiding this comment

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

The zle reset-prompt call at line 261 is now redundant since it's already called at line 254 when alt-enter is pressed. This could lead to unnecessary prompt resets in the alt-enter case.

Suggested change
zle reset-prompt
# Only reset prompt if not already done for alt-enter
if [[ "$key" != "alt-enter" ]]; then
zle reset-prompt
fi

Copilot uses AI. Check for mistakes.
@sunlei
sunlei merged commit 2449bf9 into sunlei:master Aug 30, 2025
1 check passed
@sunlei

sunlei commented Aug 30, 2025

Copy link
Copy Markdown
Owner

@kr-nn Thank you!

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.

3 participants