From 695aae9830a2eb560d3311fcd88ae8790e2ab5ef Mon Sep 17 00:00:00 2001 From: Jonathan Liebig Date: Sat, 5 Sep 2026 17:00:41 +0200 Subject: [PATCH] fix: keep retained selection copies from interrupting agents --- src/client/mod.rs | 3 - src/client/shell/actions.rs | 56 +++++----------- src/client/shell/input.rs | 3 +- src/client/shell/state.rs | 5 +- src/client/shell/tests/copy.rs | 114 +++++++++++++++++++++++++-------- src/client/shell/worktrees.rs | 2 +- 6 files changed, 108 insertions(+), 75 deletions(-) diff --git a/src/client/mod.rs b/src/client/mod.rs index ea789431..e2225051 100644 --- a/src/client/mod.rs +++ b/src/client/mod.rs @@ -541,9 +541,6 @@ fn dispatch_client_shell_actions( shell::ClientShellAction::ClipboardWrite(bytes) => { crate::selection::write_osc52_bytes(&bytes); } - shell::ClientShellAction::Request(request) => { - write_to_server(write_stream, &request).map_err(ClientError::ConnectionLost)?; - } shell::ClientShellAction::OpenSafeWebUrl(url) => { if crate::app::actions::safe_web_url(&url).is_some() { match crate::platform::open_url(&url) { diff --git a/src/client/shell/actions.rs b/src/client/shell/actions.rs index 1ebd4e29..db1feaa4 100644 --- a/src/client/shell/actions.rs +++ b/src/client/shell/actions.rs @@ -245,45 +245,27 @@ impl ClientShellState { } pub(super) fn request_selection_copy(&mut self, outcome: &mut ClientShellInput) { - self.request_selection_copy_with_fallback(outcome, None); + let content_revision = self.selection.as_ref().and_then(|selection| { + self.pane_surface + .as_ref()? + .panes + .iter() + .find(|pane| pane.pane_id == selection.pane_id) + .map(|pane| pane.content_revision) + }); + self.request_selection_copy_at_revision(outcome, content_revision); } - pub(super) fn request_selection_copy_with_fallback( + pub(super) fn request_selection_copy_at_revision( &mut self, outcome: &mut ClientShellInput, - fallback_key: Option, + content_revision: Option, ) { let Some(selection) = self.selection.as_ref() else { return; }; let pane_id = selection.pane_id.clone(); - let content_revision = self - .pane_surface - .as_ref() - .and_then(|surface| surface.panes.iter().find(|pane| pane.pane_id == pane_id)) - .map(|pane| pane.content_revision); let (anchor, cursor) = selection.ordered_cells(); - let fallback = fallback_key.and_then(|key| { - let press = ClientPaneInputEvent::from_terminal_key(key.clone())?; - let tracks_release = matches!( - &press, - ClientPaneInputEvent::Key { - tracks_release: true, - .. - } - ); - let mut message = - super::target_event_message(ClientInputTarget::Pane(pane_id.clone()), press); - if tracks_release { - let release = ClientPaneInputEvent::from_terminal_key( - key.with_kind(crossterm::event::KeyEventKind::Release), - )?; - if let ClientMessage::ClientShellPaneInput { events, .. } = &mut message { - events.push(release); - } - } - Some(message) - }); self.push_endpoint_method_with_kind( crate::api::schema::Method::PaneSelectionRead( crate::api::schema::PaneSelectionReadParams { @@ -299,7 +281,7 @@ impl ClientShellState { content_revision, }, ), - PendingEndpointKind::SelectionCopy { fallback }, + PendingEndpointKind::SelectionCopy, outcome, ); } @@ -588,13 +570,7 @@ impl ClientShellState { let repaint = self.complete_pane_scroll(pane_id, serial, result, &mut outcome); return (repaint, outcome.actions); } - PendingEndpointKind::SelectionCopy { fallback } => { - let fallback = || { - fallback - .map(ClientShellAction::Request) - .into_iter() - .collect::>() - }; + PendingEndpointKind::SelectionCopy => { return match result { Ok(crate::api::schema::ResponseResult::PaneSelection { text, .. }) if !text.is_empty() => @@ -606,14 +582,14 @@ impl ClientShellState { ) } Ok(crate::api::schema::ResponseResult::PaneSelection { .. }) => { - (false, fallback()) + (false, Vec::new()) } Ok(_) => { self.endpoint_error = Some("endpoint returned an unexpected selection result".to_owned()); - (true, fallback()) + (true, Vec::new()) } - Err(_) => (true, fallback()), + Err(_) => (true, Vec::new()), }; } PendingEndpointKind::WordSelection { diff --git a/src/client/shell/input.rs b/src/client/shell/input.rs index d2ca3a82..4d191048 100644 --- a/src/client/shell/input.rs +++ b/src/client/shell/input.rs @@ -520,7 +520,8 @@ impl ClientShellState { .as_ref() .is_some_and(crate::selection::Selection::is_visible) { - self.request_selection_copy_with_fallback(outcome, Some(key.clone())); + // Unrelated live output must not invalidate a retained selection's copy. + self.request_selection_copy_at_revision(outcome, None); self.selection = None; self.stop_selection_autoscroll(); self.selection_highlight_clear_deadline = None; diff --git a/src/client/shell/state.rs b/src/client/shell/state.rs index 60307f66..a43ad717 100644 --- a/src/client/shell/state.rs +++ b/src/client/shell/state.rs @@ -283,7 +283,6 @@ pub(crate) enum ClientShellAction { request: Box, }, ClipboardWrite(Vec), - Request(ClientMessage), OpenSafeWebUrl(String), ReplayMouse(Vec), Keybind(crate::input::KeybindAction), @@ -657,9 +656,7 @@ pub(super) enum PendingEndpointKind { WorktreeRemove { forced: bool, }, - SelectionCopy { - fallback: Option, - }, + SelectionCopy, PaneScroll { pane_id: String, serial: u64, diff --git a/src/client/shell/tests/copy.rs b/src/client/shell/tests/copy.rs index e8555db6..06a5e780 100644 --- a/src/client/shell/tests/copy.rs +++ b/src/client/shell/tests/copy.rs @@ -115,6 +115,7 @@ fn client_mouse_selection_highlights_and_copies_through_endpoint_extraction() { if params.pane_id == "pane_1" && params.anchor == crate::api::schema::PaneTextPoint { row: 0, col: 0 } && params.cursor == crate::api::schema::PaneTextPoint { row: 0, col: 2 } + && params.content_revision == Some(0) )); let (repaint, actions) = state.handle_endpoint_result( @@ -164,8 +165,8 @@ fn clipboard_feedback_is_client_local_and_respects_config() { assert!(state.copy_feedback_deadline.is_none()); } -#[test] -fn retained_mouse_selection_copies_only_on_exact_copy_shortcut() { +#[tokio::test] +async fn retained_mouse_selection_copies_only_on_exact_copy_shortcut() { let mut state = ClientShellState::new(ClientShellConfig::from_config(&Config::default())); state.config.copy_on_select = false; state.set_snapshot(Box::new(snapshot())); @@ -220,39 +221,99 @@ fn retained_mouse_selection_copies_only_on_exact_copy_shortcut() { if matches!(request.method, crate::api::schema::Method::PaneSelectionRead(_)) )); assert!(copy.requests.is_empty()); - let request_id = match ©.actions[0] { - ClientShellAction::Endpoint { request, .. } => request.id.clone(), - _ => unreachable!(), + let ClientShellAction::Endpoint { request, .. } = ©.actions[0] else { + unreachable!(); }; - let (_, fallback) = state.handle_endpoint_result( + let crate::api::schema::Method::PaneSelectionRead(params) = &request.method else { + unreachable!(); + }; + let (_tx, rx) = tokio::sync::mpsc::unbounded_channel(); + let mut app = crate::app::App::new( + &Config::default(), + crate::app::AppPolicy::TEST, + None, + rx, + crate::api::EventHub::default(), + ); + app.state.workspaces = vec![crate::workspace::Workspace::test_new("copy")]; + app.state.ensure_test_terminals(); + let pane_id = app.state.workspaces[0].tabs[0].root_pane; + let runtime = + crate::terminal::TerminalRuntime::test_with_scrollback_bytes(4, 2, 1000, b"LIVE\r\nPANE"); + runtime.test_process_pty_bytes(b"\r-ANE"); + // More output arrives after Ctrl+C, before the server extracts the selection. + runtime.test_process_pty_bytes(b"\rMORE"); + app.state.insert_test_runtime(pane_id, runtime); + let mut params = params.clone(); + params.pane_id = app.public_pane_id(0, pane_id).expect("pane id"); + let mut checked = params.clone(); + checked.content_revision = Some(2); + assert_eq!( + app.pane_selection_text(&checked).unwrap_err().0, + "stale_content" + ); + let text = app + .pane_selection_text(¶ms) + .expect("copy during output"); + assert_eq!(text, "LIV"); + let (_, actions) = state.handle_endpoint_result( "boot-1", - &request_id, + &request.id, Ok(crate::api::schema::ResponseResult::PaneSelection { pane_id: "pane_1".into(), - text: String::new(), + text, }), ); assert!(matches!( - &fallback[..], - [ClientShellAction::Request(ClientMessage::ClientShellPaneInput { - pane_id, - events, - })] if pane_id == "pane_1" - && matches!( - &events[..], - [ClientPaneInputEvent::Key { - code: crate::protocol::ClientKeyCode::Char('c'), - kind: crate::protocol::ClientKeyKind::Press, - .. - }, ClientPaneInputEvent::Key { - code: crate::protocol::ClientKeyCode::Char('c'), - kind: crate::protocol::ClientKeyKind::Release, - .. - }] - ) + &actions[..], + [ClientShellAction::ClipboardWrite(bytes)] if bytes == b"LIV" )); } +#[test] +fn retained_selection_copy_failures_never_send_terminal_input() { + use crate::api::schema::ResponseResult; + + for result in [ + Ok(ResponseResult::PaneSelection { + pane_id: "pane_1".into(), + text: String::new(), + }), + Ok(ResponseResult::Ok {}), + Err(ClientShellEndpointError { + code: Some("stale_content".into()), + message: "pane content changed".into(), + }), + Err(ClientShellEndpointError { + code: Some("endpoint_timeout".into()), + message: "timed out".into(), + }), + Err(ClientShellEndpointError { + code: Some("selection_unavailable".into()), + message: "selection unavailable".into(), + }), + ] { + let mut state = ClientShellState::new(ClientShellConfig::from_config(&Config::default())); + state.config.copy_on_select = false; + state.set_snapshot(Box::new(snapshot())); + state.set_pane_surface(surface()); + let mut selection = + crate::selection::Selection::absolute_range("pane_1".to_owned(), (0, 0), (0, 2)); + assert!(selection.finish()); + state.selection = Some(selection); + let copy = state.handle_input_bytes(b"\x03"); + assert!(copy.requests.is_empty()); + let [ClientShellAction::Endpoint { request, .. }] = ©.actions[..] else { + panic!("Ctrl+C should request a copy"); + }; + let (_, actions) = state.handle_endpoint_result("boot-1", &request.id, result); + assert!( + actions.is_empty(), + "copy failure must never interrupt the pane" + ); + } +} + #[test] fn selection_edge_drag_requests_scroll_and_timer_continues_it() { let mut state = ClientShellState::new(ClientShellConfig::from_config(&Config::default())); @@ -412,7 +473,8 @@ fn keyboard_copy_mode_owns_cursor_selection_copy_and_scroll_restore() { assert!(copy.actions.iter().any(|action| matches!( action, ClientShellAction::Endpoint { request, .. } - if matches!(request.method, crate::api::schema::Method::PaneSelectionRead(_)) + if matches!(&request.method, crate::api::schema::Method::PaneSelectionRead(params) + if params.content_revision == Some(0)) ))); assert!(copy.actions.iter().any(|action| matches!( action, diff --git a/src/client/shell/worktrees.rs b/src/client/shell/worktrees.rs index bcadf1c8..99b9376c 100644 --- a/src/client/shell/worktrees.rs +++ b/src/client/shell/worktrees.rs @@ -538,7 +538,7 @@ impl ClientShellState { | PendingEndpointKind::ReloadConfig | PendingEndpointKind::IntegrationList | PendingEndpointKind::IntegrationInstall - | PendingEndpointKind::SelectionCopy { .. } + | PendingEndpointKind::SelectionCopy | PendingEndpointKind::PaneScroll { .. } | PendingEndpointKind::WordSelection { .. } | PendingEndpointKind::PaneLinkActivate { .. }