From 8b42905fca5eef09bcf13d982958f173059724bb Mon Sep 17 00:00:00 2001 From: Hongwei Qin <122079993+shihuaidexianyu@users.noreply.github.com> Date: Thu, 13 Aug 2026 15:54:28 +0800 Subject: [PATCH] fix(terminal): stop menu click-through and surface failed file opens (#541, #542) (#575) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(terminal): stop terminal pop-up menus leaking clicks into the grid (#541) The completion menu and the reverse-search menu (the floating panel and the input-bar row alike) carry no click handlers of their own, and in gpui an element without handlers, cursor or occlude inserts no hitbox — the same rule the app pins with a test pair in app.rs. A press that missed every row therefore fell straight through to the live grid: it moved the cursor and cleared or started a selection there, and a modified click even opened whatever link happened to sit under the menu, so the menu read as broken while the damage landed elsewhere. All three menu roots now occlude, the same remedy the terminal search bar already uses, so a click on menu background is swallowed where it lands. Making the individual candidates clickable instead is a separate feature, not part of this fix. * fix(terminal): toast a file link that fails to open instead of dying silently (#542) The external half of open_file_link has no failure channel: a misspelled link_file_command or a missing xdg-open only produces a log::warn, and the click reads as a dead link — while the path was only ever underlined because the pane's own host verified it exists, so "nothing happens" is the worst possible answer. The URL half at least toasts a failed loopback forward; the built-in editor arm reports downstream of OpenFileRequested; the two spawn arms had nothing. open_file_path and run_file_command now return io::Result, and open_file_link turns an Err into the same kind of notification a failed image paste raises, naming the path and the error. The file tree's directory fallback — the one other caller, handing a path to the OS association — gets the same toast instead of silence. A template whose tokens all expand to nothing (a lone {line} on a link with no line number — a blank template never gets this far, sanitize maps it to None) reports as an InvalidInput config error rather than a silent no-op. Spawn is still all that is reported: an opener that spawns fine and then exits non-zero is nobody's to see, and a test pins both error paths. * test(terminal): pin the press a pop-up menu has to swallow (#541) The menus occlude now, but nothing held them to it: a bare div over the grid renders the same and only the mouse can tell the difference. This presses on a history row and asks the grid whether it started selecting, then takes the menu away and presses again — the second half is what keeps the first from passing on a pane the mouse never reached. * docs(changelog): say what a leaked press actually did (#541) A press on a menu never moved the terminal cursor and the menus have no buttons to miss; what it did was clear the selection, drag out a new one, underline the text under the row on hover, and open the link beneath it on Ctrl+click. --------- Co-authored-by: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> --- CHANGELOG.md | 12 +++ src/terminal/view.rs | 199 +++++++++++++++++++++++++++++++++++++++---- src/ui/file_tree.rs | 16 +++- src/ui/i18n/en.rs | 1 + src/ui/i18n/ja.rs | 1 + src/ui/i18n/mod.rs | 1 + src/ui/i18n/zh.rs | 1 + 7 files changed, 212 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 075bb9dd..1ef7f465 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -235,6 +235,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 already staged; the toast now carries the plan's actual reason ("Write a commit message first"), the same words the panel's own button shows on its tooltip. (#546) +- **Terminal pop-up menus no longer leak clicks into the grid behind them** — + the completion menu and the reverse-search menu (both the floating panel and + the input-bar row) inserted no hitbox of their own, so a press on one fell + straight through to the terminal: it cleared whatever was selected and + dragged out a new selection, merely moving over a row underlined the text + beneath it, and a Ctrl+click opened the link the menu was covering. All + three occlude now, so a press on a menu stops at the menu. (#541) +- **A file link that fails to open says so** — clicking a file path whose + opener is missing, or whose `link_file_command` template expands to nothing, + used to fail into a logfile line and nothing else; the click now raises the + same kind of toast a failed image upload does, naming the path and the + error. (#542) ## [26.8.3] - 2026-08-12 diff --git a/src/terminal/view.rs b/src/terminal/view.rs index 3d1fc94d..843bc526 100644 --- a/src/terminal/view.rs +++ b/src/terminal/view.rs @@ -2810,6 +2810,29 @@ impl TerminalView { ); } + /// Same shape as `warn_image_upload_failed`: one toast per failed open, so + /// a broken `link_file_command` surfaces as a config problem instead of a + /// "dead link" (#542). Spawn is all that is reported — a spawned opener + /// that exits non-zero is nobody's to see. + fn warn_file_open_failed( + &self, + path: &std::path::Path, + reason: &std::io::Error, + window: &mut Window, + cx: &mut Context, + ) { + log::warn!("failed to open file link {}: {reason}", path.display()); + let path = path.display().to_string(); + let reason = reason.to_string(); + window.push_notification( + crate::ui::i18n::t_fmt( + crate::ui::i18n::L10nKey::LinkFileOpenFailed, + &[("path", path.as_str()), ("error", reason.as_str())], + ), + cx, + ); + } + pub fn clear_scrollback(&mut self, cx: &mut Context) { use alacritty_terminal::vte::ansi::{ClearMode, Handler as _}; @@ -4716,7 +4739,7 @@ impl TerminalView { is_dir, }, .., - ) => self.open_file_link(path, line, column, is_dir, cx), + ) => self.open_file_link(path, line, column, is_dir, window, cx), LinkAt::Unresolved { candidate, pending } => { return self.report_unresolved_link(&candidate, pending, window, cx); } @@ -4737,6 +4760,7 @@ impl TerminalView { line: Option, column: Option, is_dir: bool, + window: &mut Window, cx: &mut Context, ) { let cfg = cx.global::(); @@ -4752,19 +4776,30 @@ impl TerminalView { true => mode, false => LinkFileOpen::Internal, }; - match (mode, command) { + // Both external arms have to answer for a failed spawn (#542): a + // misspelled `link_file_command` or a missing opener used to hit only + // the log, and the link read as dead — while the path was only ever + // underlined because it verifiably exists. The built-in editor has + // its own error path downstream of OpenFileRequested. + let outcome = match (mode, command) { (LinkFileOpen::Command, Some(template)) => { run_file_command(&template, &path, line, column) } (LinkFileOpen::System, _) => open_file_path(&path), // Told to run a command, with no command left to run: falling back // to the built-in editor beats the click doing nothing. - (LinkFileOpen::Internal | LinkFileOpen::Command, _) => cx.emit(OpenFileRequested { - path, - line, - column, - is_dir, - }), + (LinkFileOpen::Internal | LinkFileOpen::Command, _) => { + cx.emit(OpenFileRequested { + path, + line, + column, + is_dir, + }); + return; + } + }; + if let Err(e) = outcome { + self.warn_file_open_failed(&path, &e, window, cx); } } @@ -5166,6 +5201,10 @@ impl TerminalView { .top(cy_top) .right_4() .h(self.line_height) + // The row floats over live grid cells; a bare div inserts no + // hitbox, so a click aimed at it started a selection in the + // text underneath (#541). + .occlude() .flex() .items_center() .font_family(self.font.family.clone()) @@ -5461,6 +5500,11 @@ impl TerminalView { .absolute() .left(x) .top(y) + // The menu has no click handlers of its own, and a handlerless + // element inserts no hitbox — so without this the press falls + // through to the grid: the selection there is cleared, or a + // modified click opens whatever link lies under the row (#541). + .occlude() .flex() .flex_col() .py_1() @@ -5594,6 +5638,10 @@ impl TerminalView { .absolute() .left(px(GRID_PAD_X)) .top(y) + // Same fall-through as the completion menu (#541): a bare div + // inserts no hitbox, so a click on a history row landed on the + // grid beneath it. + .occlude() .flex() .flex_col() .py_1() @@ -6064,7 +6112,7 @@ fn select_end_copy(enabled: bool, grid: bool, editor: bool) -> SelectEndCopy { /// Hands a path to whatever the OS has it associated with. Also the fallback /// for a directory the file tree cannot reach. -pub(crate) fn open_file_path(path: &std::path::Path) { +pub(crate) fn open_file_path(path: &std::path::Path) -> std::io::Result<()> { let opener = if cfg!(target_os = "macos") { "open" } else if cfg!(windows) { @@ -6072,9 +6120,8 @@ pub(crate) fn open_file_path(path: &std::path::Path) { } else { "xdg-open" }; - if let Err(e) = std::process::Command::new(opener).arg(path).spawn() { - log::warn!("failed to open {}: {e}", path.display()); - } + std::process::Command::new(opener).arg(path).spawn()?; + Ok(()) } fn run_file_command( @@ -6082,15 +6129,19 @@ fn run_file_command( path: &std::path::Path, line: Option, column: Option, -) { +) -> std::io::Result<()> { let argv = expand_file_command_template(template, path, line, column); let Some((program, args)) = argv.split_first() else { - log::warn!("link_file_command is empty; ignoring file link"); - return; + // Sanitize maps a blank template to None, so the only way here is a + // template whose tokens all expand to nothing (a lone `{line}` on a + // link with no line number). Still a config error worth reporting. + return Err(std::io::Error::new( + std::io::ErrorKind::InvalidInput, + "link_file_command expanded to nothing", + )); }; - if let Err(e) = std::process::Command::new(program).args(args).spawn() { - log::warn!("failed to run link_file_command {template:?}: {e}"); - } + std::process::Command::new(program).args(args).spawn()?; + Ok(()) } fn expand_file_command_template( @@ -7065,6 +7116,18 @@ mod tests { assert_eq!(argv, vec!["code", "--goto", "{other}"]); } + /// #542's contract: a failed open is an `Err` the click site can toast, + /// not a line in a logfile nobody is watching. + #[test] + fn a_file_command_that_cannot_spawn_comes_back_as_an_error() { + let path = Path::new("/tmp/wherever.rs"); + // The binary does not exist, so the spawn itself fails. + assert!(super::run_file_command("tty7-no-such-binary {path}", path, None, None).is_err()); + // A template whose tokens all expand to nothing is a config error, + // not a silent no-op. + assert!(super::run_file_command("{line}", path, None, None).is_err()); + } + #[test] fn clipboard_image_transcodes_bmp_to_png_and_passes_png_through() { use gpui::{Image, ImageFormat}; @@ -11739,4 +11802,104 @@ mod gpui_tests { }) .unwrap(); } + + /// #541: the history menu floats over live grid cells, and a `div` that + /// carries no handler of its own inserts no hitbox — so the press went + /// straight through to the grid behind the menu, which cleared the + /// selection, dragged out a new one, or (with the link modifier held) + /// opened whatever link the menu was covering. + /// + /// The second half is the other side of the pair `src/ui/app.rs` keeps for + /// the resize handle: with the menu gone the very same press must still + /// reach the grid, or this would pass on a pane that never sees a mouse. + #[gpui::test] + fn a_press_on_the_history_menu_never_reaches_the_grid(cx: &mut TestAppContext) { + use gpui::{MouseMoveEvent, PlatformInput}; + + crate::core::config::pin_test_config_dir(); + let (window, mut daemon) = harness(cx); + prompt_ready(&window, cx, &mut daemon); + wait_for_input_active(&window, cx); + + window + .update(cx, |view, window, cx| { + window.activate_window(); + view.focus_handle.focus(window, cx); + view.history = vec!["echo one".to_string(), "echo two".to_string()]; + view.history_frecency = vec![1.0, 1.0]; + view.start_reverse_search(); + cx.notify(); + }) + .unwrap(); + + let mut vcx = gpui::VisualTestContext::from_window(window.into(), cx); + vcx.update(|window, _| window.refresh()); + vcx.run_until_parked(); + + // The menu is laid out one row under the cursor, plus the gap + // `render_reverse_search_menu` leaves; this lands in the middle of its + // first row, where a candidate is drawn. + let lh = window.update(cx, |view, _, _| view.line_height).unwrap(); + let at = point( + px(GRID_PAD_X) + px(10.), + px(GRID_PAD_Y) + lh * 2.0 + px(10.), + ); + + let press = |vcx: &mut gpui::VisualTestContext| { + vcx.update(|window, cx| { + window.dispatch_event( + PlatformInput::MouseMove(MouseMoveEvent { + position: at, + pressed_button: None, + modifiers: Modifiers::none(), + }), + cx, + ); + window.dispatch_event( + PlatformInput::MouseDown(MouseDownEvent { + button: MouseButton::Left, + position: at, + modifiers: Modifiers::none(), + click_count: 1, + first_mouse: false, + }), + cx, + ); + }); + vcx.run_until_parked(); + }; + + press(&mut vcx); + window + .update(cx, |view, _, _| { + assert!( + view.reverse_search.is_some(), + "the press must not have closed the menu either" + ); + assert!( + !view.selecting, + "the menu swallowed the press, so no selection started under it" + ); + }) + .unwrap(); + + window + .update(cx, |view, _, cx| { + view.reverse_search = None; + cx.notify(); + }) + .unwrap(); + vcx.update(|window, _| window.refresh()); + vcx.run_until_parked(); + + press(&mut vcx); + window + .update(cx, |view, _, _| { + assert!( + view.selecting, + "with no menu over it the same press is the grid's to take" + ); + }) + .unwrap(); + } } diff --git a/src/ui/file_tree.rs b/src/ui/file_tree.rs index e594548a..6794b2e7 100644 --- a/src/ui/file_tree.rs +++ b/src/ui/file_tree.rs @@ -1024,7 +1024,21 @@ impl Tty7App { // another machine, where handing the name to a local file manager // would open whatever this one keeps at that path, or nothing. if self.can_spawn_locally(cx) { - crate::terminal::view::open_file_path(path); + // The OS association can fail to spawn like any other opener + // (#542): say so with the same words a failed file link uses. + if let Err(e) = crate::terminal::view::open_file_path(path) { + log::warn!("failed to open {}: {e}", path.display()); + window.push_notification( + t_fmt( + L10nKey::LinkFileOpenFailed, + &[ + ("path", &path.display().to_string()), + ("error", &e.to_string()), + ], + ), + cx, + ); + } } else { window.push_notification( t_fmt( diff --git a/src/ui/i18n/en.rs b/src/ui/i18n/en.rs index 1e6e4470..a035a8e0 100644 --- a/src/ui/i18n/en.rs +++ b/src/ui/i18n/en.rs @@ -892,6 +892,7 @@ pub fn translate_en(key: L10nKey) -> &'static str { L10nKey::SftpImagePasteUploadFailed => { "Could not upload the pasted image to {host}: {error}" } + L10nKey::LinkFileOpenFailed => "Could not open {path}: {error}", L10nKey::ForwardPanelTitle => "Forwards", L10nKey::ForwardDisconnected => "Disconnected", L10nKey::ForwardDisconnectedFrom => "Disconnected from {host}", diff --git a/src/ui/i18n/ja.rs b/src/ui/i18n/ja.rs index 4d42121d..c46c02f5 100644 --- a/src/ui/i18n/ja.rs +++ b/src/ui/i18n/ja.rs @@ -942,6 +942,7 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> { L10nKey::SftpImagePasteUploadFailed => { "貼り付けた画像を {host} にアップロードできませんでした: {error}" } + L10nKey::LinkFileOpenFailed => "{path} を開けませんでした: {error}", L10nKey::ForwardPanelTitle => "ポートフォワード", L10nKey::ForwardDisconnected => "切断済み", L10nKey::ForwardDisconnectedFrom => "{host} から切断されました", diff --git a/src/ui/i18n/mod.rs b/src/ui/i18n/mod.rs index a06915e0..441ebcb8 100644 --- a/src/ui/i18n/mod.rs +++ b/src/ui/i18n/mod.rs @@ -662,6 +662,7 @@ l10n_keys! { SftpTransferError, SftpTransferListFailed, SftpImagePasteUploadFailed, + LinkFileOpenFailed, ForwardPanelTitle, ForwardDisconnected, ForwardDisconnectedFrom, diff --git a/src/ui/i18n/zh.rs b/src/ui/i18n/zh.rs index e2fea02d..d6ad26ea 100644 --- a/src/ui/i18n/zh.rs +++ b/src/ui/i18n/zh.rs @@ -848,6 +848,7 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { L10nKey::SftpTransferError => "错误", L10nKey::SftpTransferListFailed => "无法获取传输状态：{error}", L10nKey::SftpImagePasteUploadFailed => "无法将粘贴的图片上传到 {host}：{error}", + L10nKey::LinkFileOpenFailed => "无法打开 {path}：{error}", L10nKey::ForwardPanelTitle => "端口转发", L10nKey::ForwardDisconnected => "已断开", L10nKey::ForwardDisconnectedFrom => "与 {host} 的连接已断开",