From b229fd18f0a02ef8a77c98a19de85cafec6dd94d Mon Sep 17 00:00:00 2001 From: thomas Date: Mon, 27 Jul 2026 09:35:26 +0800 Subject: [PATCH] fix(terminal): refuse a stale row in smart select, and drop the stale link MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps left by the hover fix, both the same crash class. `grid_smart_range` indexes `grid[click.line]` the moment it starts — `hyperlink_run`, then `logical_line_at` — with the row a double-click carried in from the frame that dispatched it. That row can outlive its grid the same way a hovered cell can: a split, a window drag, or the reader thread applying the daemon's replayed attach geometry between paints. `Grid`'s `Index` only `debug_assert`s the bound, so a release build walks off the storage, and the click arrives in a gpui `extern "C"` callback where the panic aborts instead of unwinding. Guard the row up front, exactly as `TerminalView::grid_line` does. `set_grid_size` forgot the hovered *cell* but kept the link it had resolved. That link is stored in grid coordinates, so after the reflow it underlines whatever text now sits at those coordinates and holds the pointing-hand cursor over it until the pointer moves again — clamping's wrong-but-silent hover, one step removed. Drop it with the cell. Both halves get a regression test; the smart-select one panics on the storage `debug_assert` without the guard. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WCb8ZDmvdA5xbVtvs647tD --- src/terminal/smart_select.rs | 24 ++++++++++++++++++++++++ src/terminal/view.rs | 32 +++++++++++++++++++++++++++++++- 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/src/terminal/smart_select.rs b/src/terminal/smart_select.rs index d71cbf84..e36ab0f5 100644 --- a/src/terminal/smart_select.rs +++ b/src/terminal/smart_select.rs @@ -71,6 +71,16 @@ pub(super) fn grid_smart_range( term: &Term, click: Point, ) -> Option { + // 0) The click carries the geometry of the frame that dispatched it, and + // the grid can shrink out from under it (a split, a window drag, a + // replayed attach size landing on the reader thread). Both walks below + // index `grid[click.line]` straight away, and `Grid`'s `Index` + // only `debug_assert`s the bound — a release build walks off the + // storage. Same guard, same reason, as `TerminalView::grid_line`. + if click.line < term.topmost_line() || click.line > term.bottommost_line() { + return None; + } + // 1) An explicit OSC 8 hyperlink run wins outright — the program told us // the exact extent, no guessing needed. if let Some((start, end)) = hyperlink_run(term, click) { @@ -805,6 +815,20 @@ mod tests { assert!(grid_smart_range(&term, Point::new(Line(0), Column(99))).is_none()); } + /// A double-click dispatched with the previous frame's geometry can name a + /// row the grid has since dropped. Indexing it walks off the storage, and + /// the click arrives in a gpui `extern "C"` callback where that panic + /// aborts instead of unwinding — so the row has to be refused first. + #[test] + fn click_outside_the_grid_rows_yields_no_range() { + let term = term_with(10, 2, "hello"); + // Below the last row of a shrunken grid... + assert!(grid_smart_range(&term, Point::new(Line(2), Column(0))).is_none()); + assert!(grid_smart_range(&term, Point::new(Line(9_000), Column(0))).is_none()); + // ...and above the top of a scrollback this short. + assert!(grid_smart_range(&term, Point::new(Line(-1), Column(0))).is_none()); + } + fn selected(text: &str, click: usize) -> Option { let chars: Vec = text.chars().collect(); range(text, click).map(|(s, e)| chars[s..=e].iter().collect()) diff --git a/src/terminal/view.rs b/src/terminal/view.rs index cff8f9a7..eeea58af 100644 --- a/src/terminal/view.rs +++ b/src/terminal/view.rs @@ -1196,9 +1196,13 @@ impl TerminalView { // its row may not exist any more, and the pointer sits over a different // cell regardless. Forget it — the next mouse move records a fresh one. // (`grid_line` also refuses a stale row, so this is about not underlining - // the wrong cell, not about safety.) + // the wrong cell, not about safety.) The link that cell resolved to goes + // with it: it is held in grid coordinates the reflow just moved text + // under, so keeping it would underline whatever now sits there (and hold + // the pointing-hand cursor over it) until the pointer moves again. if (cols, rows) != (self.terminal.size().cols, self.terminal.size().rows) { self.last_hover_cell = None; + self.hovered_link = None; } self.cell_width = cell_width; self.line_height = line_height; @@ -6871,6 +6875,32 @@ mod gpui_tests { .unwrap(); } + /// The other half of the fix: the pane that shrank forgets the hover it was + /// holding, rather than carrying a cell (and the underline it resolved) that + /// now names different text. + #[gpui::test] + fn a_resize_forgets_the_hovered_cell(cx: &mut TestAppContext) { + let (window, _daemon) = harness(cx); + window + .update(cx, |view, _, cx| { + view.hover_link_at(0, 23, true, cx); + assert_eq!(view.last_hover_cell, Some((0, 23))); + view.hovered_link = Some(HoveredLink { + line: 23, + start: 0, + end: 3, + }); + // The same geometry again changes nothing... + view.set_grid_size(80, 24, px(8.), px(17.)); + assert_eq!(view.last_hover_cell, Some((0, 23))); + // ...but a split (or a window drag) that shrinks the pane does. + view.set_grid_size(80, 8, px(8.), px(17.)); + assert!(view.last_hover_cell.is_none(), "the cell is stale"); + assert!(view.hovered_link.is_none(), "so is the link it resolved"); + }) + .unwrap(); + } + #[gpui::test] fn title_events_drive_the_tab_title(cx: &mut TestAppContext) { let (window, _daemon) = harness(cx);