feat(composer): mouse + keyboard text selection with copy/cut#2228
feat(composer): mouse + keyboard text selection with copy/cut#2228imkingjh999 wants to merge 10 commits into
Conversation
Mouse: click to position cursor, drag to select text. Ctrl+C copies composer selection. Ctrl+X cuts when selection exists, toggles Plan/Agent mode otherwise.
Add keyboard selection with Shift+arrow keys. Plain arrows clear selection, Ctrl/Alt arrows move by word without affecting selection. Also gitignore docs/superpowers/ and .serena/.
- Remove needless borrows flagged by clippy - Auto-copy selection on mouse release (consistent with transcript) - Use theme selection_bg instead of hardcoded color
Covered by .gitignore, should not be in the PR.
- Revert auto-copy on mouse release (preserves selection for cut/delete) - Use actual scroll_offset in visible_line_char_ranges instead of inferring - Use &str slices in line_spans_with_selection to avoid String allocations
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces text selection and mouse coordinate mapping support in the TUI composer, allowing users to select, copy, cut, and delete text ranges. Feedback on the changes highlights issues with selection clearing and word-by-word selection when using cursor movement keys (such as Ctrl+Left/Right with or without Shift), as well as a bug where killing a selection with Ctrl+K (kill_to_end_of_line) fails to copy the deleted text to the kill buffer.
| KeyCode::Left if key.modifiers.contains(KeyModifiers::SHIFT) => { | ||
| if app.selection_anchor.is_none() { | ||
| app.selection_anchor = Some(app.cursor_position); | ||
| } | ||
| app.move_cursor_left(); | ||
| } | ||
| KeyCode::Left if is_word_cursor_modifier(key.modifiers) => { | ||
| app.move_cursor_word_backward(); | ||
| } | ||
| KeyCode::Left => { | ||
| app.clear_selection(); | ||
| app.move_cursor_left(); | ||
| } | ||
| KeyCode::Right if key.modifiers.contains(KeyModifiers::SHIFT) => { | ||
| if app.selection_anchor.is_none() { | ||
| app.selection_anchor = Some(app.cursor_position); | ||
| } | ||
| app.move_cursor_right(); | ||
| } | ||
| KeyCode::Right if is_word_cursor_modifier(key.modifiers) => { | ||
| app.move_cursor_word_forward(); | ||
| } | ||
| KeyCode::Right => { | ||
| app.clear_selection(); | ||
| app.move_cursor_right(); | ||
| } |
There was a problem hiding this comment.
When cursor movement keys like Ctrl+Left or Ctrl+Right are pressed without holding Shift, the active text selection is not cleared. This causes the selection to unexpectedly expand or distort when jumping the cursor, rather than clearing it as standard text editors do.
Additionally, if Shift is held along with a word modifier (e.g., Shift+Ctrl+Left), the cursor only moves by a single character because the Shift modifier arm matches first and overrides the word-movement logic.
We should update these match arms to correctly handle selection clearing for non-Shift movements, and support word-by-word selection when Shift is combined with the word modifier. (Note: You should also apply app.clear_selection() to other non-selecting cursor movements like Home, End, Ctrl+A, and Ctrl+E in their respective match arms).
KeyCode::Left if key.modifiers.contains(KeyModifiers::SHIFT) => {
if app.selection_anchor.is_none() {
app.selection_anchor = Some(app.cursor_position);
}
if is_word_cursor_modifier(key.modifiers) {
app.move_cursor_word_backward();
} else {
app.move_cursor_left();
}
}
KeyCode::Left if is_word_cursor_modifier(key.modifiers) => {
app.clear_selection();
app.move_cursor_word_backward();
}
KeyCode::Left => {
app.clear_selection();
app.move_cursor_left();
}
KeyCode::Right if key.modifiers.contains(KeyModifiers::SHIFT) => {
if app.selection_anchor.is_none() {
app.selection_anchor = Some(app.cursor_position);
}
if is_word_cursor_modifier(key.modifiers) {
app.move_cursor_word_forward();
} else {
app.move_cursor_right();
}
}
KeyCode::Right if is_word_cursor_modifier(key.modifiers) => {
app.clear_selection();
app.move_cursor_word_forward();
}
KeyCode::Right => {
app.clear_selection();
app.move_cursor_right();
}| if self.delete_selection() { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
When kill_to_end_of_line (Ctrl+K) is triggered with an active selection, it deletes the selection but does not copy the deleted text to the kill_buffer. This breaks the standard Emacs/readline behavior where killing a region/selection cuts it to the kill ring so it can be yanked (pasted) later.
We should copy the selected text to self.kill_buffer before deleting the selection.
| if self.delete_selection() { | |
| return true; | |
| } | |
| if self.selection_range().is_some() { | |
| self.kill_buffer = self.selected_text(); | |
| self.delete_selection(); | |
| return true; | |
| } |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- Ctrl+Left/Right word jump now clears active selection - Ctrl+K with selection saves deleted text to kill_buffer - Ctrl+C/Ctrl+X check clipboard write result before clearing/deleting
|
@gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces text selection capabilities to the TUI composer, supporting mouse-based dragging, Shift+Arrow keys, and standard clipboard operations (copy, cut, paste, and delete). The review feedback highlights critical issues with cursor navigation: cursor-jump keys (like Home, End, Ctrl+A, and Ctrl+E) fail to clear the active selection, which can lead to accidental data loss on subsequent edits. Additionally, suggestions are provided to support word-based selection when Shift is combined with Control or Alt modifiers.
| app.move_cursor_word_forward(); | ||
| } | ||
| KeyCode::Right => { | ||
| app.clear_selection(); |
There was a problem hiding this comment.
While Left and Right (and their word-jump variants) correctly call app.clear_selection() to clear any active selection when moving the cursor, other cursor-jump keys like Home, End, Ctrl+A, and Ctrl+E do not.
If a user selects some text and then presses Home, End, Ctrl+A, or Ctrl+E, the selection_anchor remains set, causing the selection to silently re-span from the new cursor position to the old anchor. Any subsequent edit operation (such as typing, backspace, or pasting) will then trigger delete_selection() and erase the unintended range, leading to accidental data loss.
Please ensure that app.clear_selection() is also called in the match arms for KeyCode::Home, KeyCode::End, KeyCode::Home | KeyCode::Char('a'), and KeyCode::Char('e').
| KeyCode::Left if key.modifiers.contains(KeyModifiers::SHIFT) => { | ||
| if app.selection_anchor.is_none() { | ||
| app.selection_anchor = Some(app.cursor_position); | ||
| } | ||
| app.move_cursor_left(); | ||
| } |
There was a problem hiding this comment.
When SHIFT is combined with CONTROL or ALT (e.g., Ctrl+Shift+Left), the current match arm for SHIFT is matched first, but it only moves the cursor by a single character (app.move_cursor_left()) instead of a whole word. We can easily support word-based selection by checking is_word_cursor_modifier(key.modifiers) inside the SHIFT match arm.
KeyCode::Left if key.modifiers.contains(KeyModifiers::SHIFT) => {
if app.selection_anchor.is_none() {
app.selection_anchor = Some(app.cursor_position);
}
if is_word_cursor_modifier(key.modifiers) {
app.move_cursor_word_backward();
} else {
app.move_cursor_left();
}
}| KeyCode::Right if key.modifiers.contains(KeyModifiers::SHIFT) => { | ||
| if app.selection_anchor.is_none() { | ||
| app.selection_anchor = Some(app.cursor_position); | ||
| } | ||
| app.move_cursor_right(); | ||
| } |
There was a problem hiding this comment.
When SHIFT is combined with CONTROL or ALT (e.g., Ctrl+Shift+Right), the current match arm for SHIFT is matched first, but it only moves the cursor by a single character (app.move_cursor_right()) instead of a whole word. We can easily support word-based selection by checking is_word_cursor_modifier(key.modifiers) inside the SHIFT match arm.
KeyCode::Right if key.modifiers.contains(KeyModifiers::SHIFT) => {
if app.selection_anchor.is_none() {
app.selection_anchor = Some(app.cursor_position);
}
if is_word_cursor_modifier(key.modifiers) {
app.move_cursor_word_forward();
} else {
app.move_cursor_right();
}
}…tion - Home, End, Ctrl+A, Ctrl+E now clear active selection before moving - Shift+Ctrl+Left/Right selects by word instead of by character
|
@gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements text selection, copying, cutting, and mouse-drag selection capabilities within the TUI composer. It updates key bindings (such as Shift+Left/Right, Ctrl+C, Ctrl+X, and deletion keys) and mouse event handling to support selection anchors and ranges. The review feedback highlights two important issues where the raw app.input is used instead of the displayed search query (input_text or app.composer_display_input()) when history search is active, which can lead to mismatched character ranges and incorrect cursor/selection mapping.
| let line_ranges = visible_line_char_ranges( | ||
| &self.app.input, | ||
| &visible_lines, | ||
| content_width, | ||
| scroll_offset, | ||
| ); |
There was a problem hiding this comment.
When history search is active, input_text represents the displayed search query, which can differ from self.app.input. Passing &self.app.input to visible_line_char_ranges while zipping with visible_lines (which are wrapped from input_text) will result in mismatched character ranges and incorrect selection highlighting. We should pass input_text instead.
| let line_ranges = visible_line_char_ranges( | |
| &self.app.input, | |
| &visible_lines, | |
| content_width, | |
| scroll_offset, | |
| ); | |
| let line_ranges = visible_line_char_ranges( | |
| input_text, | |
| &visible_lines, | |
| content_width, | |
| scroll_offset, | |
| ); |
| if app.input.is_empty() { | ||
| return Some(0); | ||
| } | ||
|
|
||
| let width = inner.width.max(1) as usize; | ||
| let wrapped = crate::tui::widgets::wrap_input_lines_for_mouse(&app.input, width); | ||
|
|
||
| // Subtract the vertical top-padding (centering of short inputs). | ||
| let text_row = rel_row.saturating_sub(app.viewport.last_composer_top_padding); | ||
|
|
||
| // Add the scroll offset (lines scrolled out of view). | ||
| let absolute_row = text_row + app.viewport.last_composer_scroll_offset; | ||
|
|
||
| if absolute_row >= wrapped.len() { | ||
| return Some(app.input.chars().count()); | ||
| } |
There was a problem hiding this comment.
In mouse_pos_to_char_index, using app.input directly can cause incorrect cursor positioning and selection mapping when history search is active (since the displayed text is the search query returned by app.composer_display_input(), not the underlying app.input). We should use app.composer_display_input() consistently.
| if app.input.is_empty() { | |
| return Some(0); | |
| } | |
| let width = inner.width.max(1) as usize; | |
| let wrapped = crate::tui::widgets::wrap_input_lines_for_mouse(&app.input, width); | |
| // Subtract the vertical top-padding (centering of short inputs). | |
| let text_row = rel_row.saturating_sub(app.viewport.last_composer_top_padding); | |
| // Add the scroll offset (lines scrolled out of view). | |
| let absolute_row = text_row + app.viewport.last_composer_scroll_offset; | |
| if absolute_row >= wrapped.len() { | |
| return Some(app.input.chars().count()); | |
| } | |
| let input_text = app.composer_display_input(); | |
| if input_text.is_empty() { | |
| return Some(0); | |
| } | |
| let width = inner.width.max(1) as usize; | |
| let wrapped = crate::tui::widgets::wrap_input_lines_for_mouse(input_text, width); | |
| // Subtract the vertical top-padding (centering of short inputs). | |
| let text_row = rel_row.saturating_sub(app.viewport.last_composer_top_padding); | |
| // Add the scroll offset (lines scrolled out of view). | |
| let absolute_row = text_row + app.viewport.last_composer_scroll_offset; | |
| if absolute_row >= wrapped.len() { | |
| return Some(input_text.chars().count()); | |
| } |
Ensures mouse coordinate mapping and selection highlighting use the displayed text (search query) rather than the underlying input when history search is active.
|
@gemini review |
|
Harvested in build/v0.8.47 (commits 8446371 + a554aa9). Changes landed:
Thank you @imkingjh999! Added to CHANGELOG under Added + Community credits. |
There was a problem hiding this comment.
Code Review
This pull request introduces text selection support to the TUI composer, enabling both mouse-based (click and drag) and keyboard-based (Shift + Arrow keys) selections. It integrates selection handling with copy, cut, and delete operations, updates the rendering logic to highlight active selections, and adds comprehensive unit tests. The reviewer suggested a performance optimization in crates/tui/src/tui/widgets/mod.rs to avoid allocating a temporary Vec for byte_offsets on every rendered line by using .char_indices().nth() directly.
| // Build a Vec of byte offsets for each char boundary, plus one past the end. | ||
| let mut byte_offsets: Vec<usize> = line.char_indices().map(|(i, _)| i).collect(); | ||
| byte_offsets.push(line.len()); | ||
|
|
||
| let b0 = byte_offsets | ||
| .get(local_sel_start) | ||
| .copied() | ||
| .unwrap_or(line.len()); | ||
| let b1 = byte_offsets | ||
| .get(local_sel_end) | ||
| .copied() | ||
| .unwrap_or(line.len()); |
There was a problem hiding this comment.
We can avoid allocating a temporary Vec for byte_offsets on every rendered line by using .char_indices().nth() directly to find the byte offsets of local_sel_start and local_sel_end. This improves performance and reduces memory churn during rendering.
| // Build a Vec of byte offsets for each char boundary, plus one past the end. | |
| let mut byte_offsets: Vec<usize> = line.char_indices().map(|(i, _)| i).collect(); | |
| byte_offsets.push(line.len()); | |
| let b0 = byte_offsets | |
| .get(local_sel_start) | |
| .copied() | |
| .unwrap_or(line.len()); | |
| let b1 = byte_offsets | |
| .get(local_sel_end) | |
| .copied() | |
| .unwrap_or(line.len()); | |
| let b0 = if local_sel_start == 0 { | |
| 0 | |
| } else { | |
| line.char_indices() | |
| .nth(local_sel_start) | |
| .map(|(i, _)| i) | |
| .unwrap_or(line.len()) | |
| }; | |
| let b1 = if local_sel_end == 0 { | |
| 0 | |
| } else { | |
| line.char_indices() | |
| .nth(local_sel_end) | |
| .map(|(i, _)| i) | |
| .unwrap_or(line.len()) | |
| }; |
|
Thanks @imkingjh999 — your contribution landed in
Closing this PR now that the code is on If you want to land more work and would prefer your future PRs merge cleanly without a harvest step, the |
Add mouse drag selection and Shift+Arrow text selection in the composer input box. Ctrl+C copies selected text; Ctrl+X cuts (or toggles mode if no selection). Selection highlighting uses the theme's selection_bg color. Mouse coordinate mapping accounts for wrapping, scroll offset, and padding. Also fix Home, End, Ctrl+A, and Ctrl+E to clear the selection anchor before jumping, matching the existing Left/Right behavior. Without these calls a stale anchor silently reforms a selection and can cause unintended deletions on the next keystroke. Harvested from Hmbown#2228. Co-authored-by: imkingjh999 <imkingjh999@users.noreply.github.com>
Add entries for deadlock fix (Hmbown#1856), composer text selection (Hmbown#2228), and project context loading tracing (Hmbown#2227). Add community credits for @Fire-dtx and @imkingjh999.
2 participants
Summary
Rebased and reopened per @Hmbown's request (#1726).
selection_bgcolorReview feedback addressed
scroll_offsetfromViewportStateinstead of inferring from length differenceapp.ui_theme.selection_bg)line_spans_with_selectionuses byte offsets to return&strslices (no String/Vec allocations)Test plan
Greptile Summary
This PR adds mouse drag and Shift+Arrow keyboard text selection to the composer input box, along with Ctrl+C copy and Ctrl+X cut support. Previously-flagged issues (clipboard-failure guards on cut/copy, kill-buffer population in
kill_to_end_of_line, stale selection on Home/End/Ctrl+A/Ctrl+E) are all correctly addressed.selection_anchoronComposerStateand a set of selection helpers (selection_range,selected_text,delete_selection,clear_selection) with good unit-test coverage; all edit operations (insert, delete, yank) properly calldelete_selectionfirst.handle_composer_mousefor click/drag coordinate mapping using a newwrap_input_lines_for_mouseutility; the wrapping logic mirrorswrap_textexactly, keeping rendered lines and char-range indices consistent.handle_composer_mousedoes not guard against active history-search mode, wherecomposer_display_input()returns the search query instead ofapp.input; a mouse click during Ctrl+R search writes a query-relative char index intocursor_position, which refers toapp.inputonce the search is dismissed.Confidence Score: 3/5
Safe to merge with the history-search mouse guard resolved; without it a mouse click during Ctrl+R can silently corrupt cursor position and trigger an unintended delete when the search is dismissed.
The core selection machinery in app.rs and the keyboard paths in ui.rs look correct and well-tested. The one concrete defect is in mouse_ui.rs: handle_composer_mouse uses composer_display_input() (which returns the search query string during history search) to derive a char index, then writes that index into cursor_position and selection_anchor, which are coordinates relative to app.input. When the user dismisses the history search, those stale indices persist and can cause delete_selection to erase an unintended range on the next keystroke.
crates/tui/src/tui/mouse_ui.rs — the handle_composer_mouse function needs an is_history_search_active guard before processing any mouse event.
Important Files Changed
Sequence Diagram
sequenceDiagram participant User participant handle_mouse_event participant handle_composer_mouse participant mouse_pos_to_char_index participant App User->>handle_mouse_event: MouseDown in composer area handle_mouse_event->>handle_composer_mouse: mouse event handle_composer_mouse->>App: viewport.last_composer_area / last_composer_content handle_composer_mouse->>mouse_pos_to_char_index: col, row, inner rect mouse_pos_to_char_index->>App: composer_display_input() note over mouse_pos_to_char_index: wraps text via wrap_input_lines_for_mouse mouse_pos_to_char_index-->>handle_composer_mouse: char index handle_composer_mouse->>App: "cursor_position = char_index" handle_composer_mouse->>App: "selection_anchor = None / Some(old_pos)" User->>handle_mouse_event: MouseDrag in composer area handle_mouse_event->>handle_composer_mouse: drag event handle_composer_mouse->>mouse_pos_to_char_index: new col, row mouse_pos_to_char_index-->>handle_composer_mouse: new char index handle_composer_mouse->>App: "selection_anchor = Some(old cursor) if None" handle_composer_mouse->>App: "cursor_position = new char index" User->>handle_mouse_event: Ctrl+C / Ctrl+X handle_mouse_event->>App: selected_text() → app.input[anchor..cursor] App-->>handle_mouse_event: selected string handle_mouse_event->>App: clipboard.write_text(sel) alt write ok handle_mouse_event->>App: push_status_toast(Copied/Cut) handle_mouse_event->>App: clear_selection() / delete_selection() else write failed handle_mouse_event->>App: push_status_toast(Copy/Cut failed) endComments Outside Diff (4)
crates/tui/src/tui/mouse_ui.rs, line 481-492 (link)copy_active_selectioncomposer branch is now unreachable from the Ctrl+C handlerThe Ctrl+C path in
ui.rsalready handles a non-empty composer selection before callingcopy_active_selection, soapp.selected_text()will always be empty here when this function is reached from that code path. The composer block insidecopy_active_selectionis only reachable from other future callers. When it does fire, it usesapp.status_messagerather thanpush_status_toast, producing a different UX than the copy key-shortcut path. If this function is meant to be a shared utility, it should either use the same notification mechanism or the two paths should be consolidated.crates/tui/src/tui/ui.rs, line 3533-3546 (link)LeftandRightboth callapp.clear_selection()before moving the cursor, but the four analogous cursor-jump keys do not. After a user selects text and presses Home, End,Ctrl+A, orCtrl+E,selection_anchorremains set and the selection silently re-spans from the new cursor position to the old anchor. A subsequent keystroke (typing, Backspace, Delete, paste) then callsdelete_selection()and erases the unintended range — a data-loss path the user never chose to trigger.crates/tui/src/tui/ui.rs, line 3610-3617 (link)handle_composer_history_arrowroutes bothKeyCode::UpandKeyCode::Downtovim_move_up/vim_move_down/history_up/history_down— none of which callclear_selection(). When a selection is active and the user presses Up or Down,selection_anchorsurvives into the repositioned cursor state. The next edit (insert_char,delete_char, etc.) then callsdelete_selection()and silently erases the range between the stale anchor and the new cursor position — a range that was never intentionally selected. The history navigation path is especially dangerous:history_up()replaces the entireinputbuffer but leavesselection_anchorpointing into the old input's coordinate space.crates/tui/src/tui/mouse_ui.rs, line 413-455 (link)handle_composer_mousecallsmouse_pos_to_char_index, which maps pixel position to a char index usingapp.composer_display_input(). During an active history search, that function returns the search-query string, notapp.input. The resulting index is then written intoapp.cursor_position(and potentiallyapp.selection_anchor), which are coordinates inapp.input's address space. When the user subsequently dismisses the history search (Enter or Escape), those stale, search-query-relative indices are applied against the original, differently-sized input — silently landing the cursor at the wrong position, triggering a phantom selection, or causingdelete_selectionto erase unintended text. Addingif app.is_history_search_active() { return false; }at the top of the function is the minimal guard.Reviews (4): Last reviewed commit: "fix: use composer_display_input() for se..." | Re-trigger Greptile