fix: scope tmux history copy to visible pane#1435
Conversation
|
⏳ Code review in progress. Analyzing for code quality issues and best practices. You can monitor the review status in the checks section at the bottom of this pull request. Detailed findings will be posted upon completion. Using Amazon Q Developer for GitHubAmazon Q Developer1 is an AI-powered assistant that integrates directly into your GitHub workflow, enhancing your development process with intelligent features for code development, review, and transformation. Slash Commands
FeaturesAgentic Chat Code Review CustomizationYou can create project-specific rules for Amazon Q Developer to follow:
Example rule: FeedbackTo provide feedback on Amazon Q Developer, create an issue in the Amazon Q Developer public repository. For more detailed information, visit the Amazon Q for GitHub documentation. Footnotes
|
There was a problem hiding this comment.
Review Summary
This PR improves the tmux history copy functionality by replacing a simple alias with a more sophisticated function that properly handles pane targeting and limits capture to visible content. The implementation is sound and addresses the stated goals.
Key Changes
- ✅ Replaces global history capture with visible-only content using
-S -0 - ✅ Adds proper pane targeting (current pane inside tmux, last pane outside)
- ✅ Includes error handling for missing tmux sessions
Issues Found
- Shell Quoting: Three instances need proper quoting to prevent word splitting and pathname expansion
The changes are functionally correct and improve the user experience by scoping the copy operation to visible content while maintaining cross-platform clipboard support.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| local target_pane | ||
|
|
||
| if [ -n "$TMUX" ]; then | ||
| target_pane="$(tmux display-message -p '#{pane_id}')" |
There was a problem hiding this comment.
Quote the variable expansion to prevent word splitting and pathname expansion.
| target_pane="$(tmux display-message -p '#{pane_id}')" | |
| target_pane="$(tmux display-message -p "#{pane_id}")" |
| if [ -n "$TMUX" ]; then | ||
| target_pane="$(tmux display-message -p '#{pane_id}')" | ||
| else | ||
| if ! target_pane="$(tmux display-message -p -F '#{pane_id}' -t '{last}' 2>/dev/null)"; then |
There was a problem hiding this comment.
Quote the variable expansion to prevent word splitting and pathname expansion.
| if ! target_pane="$(tmux display-message -p -F '#{pane_id}' -t '{last}' 2>/dev/null)"; then | |
| if ! target_pane="$(tmux display-message -p -F "#{pane_id}" -t "{last}" 2>/dev/null)"; then |
| # -S -0 captures from the top of the visible pane (respects a recent clear) | ||
| tmux capture-pane -p -J -S -0 -t "${target_pane}" | clipboard_copy | ||
| } | ||
| alias tmux-copy-history='tmux_copy_history' |
There was a problem hiding this comment.
Quote the variable expansion to prevent word splitting and pathname expansion.
| alias tmux-copy-history='tmux_copy_history' | |
| alias tmux-copy-history="tmux_copy_history" |
Git Statistics
Summary
Testing