From 30f2bc243d39b4fc2a69eed748ee8a3e53fbbc9f Mon Sep 17 00:00:00 2001 From: Ed Zynda Date: Sat, 16 May 2026 13:48:51 +0300 Subject: [PATCH] fix(ui): correct mouse selection drift with extension widgets - Match View() and getItemAndLineAtY() row counts for empty items so streaming-reasoning placeholders no longer offset hit-testing by one row each (exposed when extension widgets like subagent-monitor shrink the scrollback). - Honor IsLineInRange's endCol=-1 'to end of line' sentinel in HighlightLine and ExtractText so the start row of a multi-line drag actually renders highlighted and is included in clipboard copies. - Add regression tests for both invariants in scrolllist and selection. --- internal/ui/scrolllist.go | 15 ++++++++ internal/ui/scrolllist_test.go | 49 +++++++++++++++++++++++++ internal/ui/selection/selection.go | 21 ++++++++++- internal/ui/selection/selection_test.go | 48 ++++++++++++++++++++++++ 4 files changed, 131 insertions(+), 2 deletions(-) diff --git a/internal/ui/scrolllist.go b/internal/ui/scrolllist.go index b28ba1d7..6110826c 100644 --- a/internal/ui/scrolllist.go +++ b/internal/ui/scrolllist.go @@ -538,6 +538,21 @@ func (s *ScrollList) View() string { for idx := s.offsetIdx; idx < len(s.items) && remainingHeight > 0; idx++ { item := s.items[idx] content := item.Render(s.width) + + // Items that render to an empty string contribute zero height to + // the viewport. This MUST match renderedHeight()'s semantics — + // otherwise getItemAndLineAtY (which uses renderedHeight) treats + // the item as 0 lines while View() emits one blank line via + // strings.Split("", "\n") = [""], producing a 1-row downward + // drift in mouse hit-testing per empty item between offsetIdx + // and the cursor (most visibly streaming-reasoning items before + // any reasoning has streamed, which extension widgets surface by + // shrinking the scrollback). + if content == "" { + s.heightCache[item.ID()] = 0 + continue + } + contentLines := strings.Split(content, "\n") // Refresh height cache from the actual render (authoritative). diff --git a/internal/ui/scrolllist_test.go b/internal/ui/scrolllist_test.go index a88f9d23..83b14039 100644 --- a/internal/ui/scrolllist_test.go +++ b/internal/ui/scrolllist_test.go @@ -130,3 +130,52 @@ func TestScrollList_SetItemsRespectsMouseDown(t *testing.T) { } } } + +// TestScrollList_EmptyItemsDoNotShiftMouseMapping is the regression test +// for the second drift bug: items that render to "" must contribute the +// same number of rows in View() (zero) as in renderedHeight(), or mouse +// hit-testing drifts by one row per empty item between offsetIdx and the +// cursor. This was surfaced by extension widgets (e.g. subagent-monitor) +// that shrink the scrollback so empty streaming-reasoning items end up +// in the visible window. +// +// Setup: 1 normal item + 1 empty item + 1 normal item. Click on the line +// where the third item begins. With the bug, getItemAndLineAtY skips the +// empty item (renderedHeight=0) and reports lineIdx pointing one row +// past where View() actually painted that line. +func TestScrollList_EmptyItemsDoNotShiftMouseMapping(t *testing.T) { + sl := NewScrollList(80, 10) + sl.SetItems([]MessageItem{ + &fakeItem{id: "a", lines: 2}, // viewY 0–1 + &fakeItem{id: "empty", lines: 0}, // renders "" — contributes 0 rows + &fakeItem{id: "b", lines: 2}, // viewY 2–3 + }) + + // Render the viewport once so the cache reflects what View() actually + // emits (this is the path that previously diverged from renderedHeight + // for empty items). + rendered := sl.View() + lines := strings.Split(rendered, "\n") + + // Sanity: View() must emit exactly height lines. + if len(lines) != 10 { + t.Fatalf("View() returned %d lines, want 10", len(lines)) + } + // Item b's first line should appear at viewY=2, NOT viewY=3. + if !strings.Contains(lines[2], "b-line-0") { + t.Errorf("viewY=2 should render b-line-0 (empty item contributes 0 rows), got %q", lines[2]) + } + + // Now the actual hit-test contract: clicking on viewY=2 must map to + // item b line 0 — the same coordinate View() rendered there. + idx, line := sl.getItemAndLineAtY(2) + if idx != 2 || line != 0 { + t.Errorf("getItemAndLineAtY(2) = (%d,%d), want (2,0)", idx, line) + } + + // And clicking on the second line of b (viewY=3) must map to b line 1. + idx, line = sl.getItemAndLineAtY(3) + if idx != 2 || line != 1 { + t.Errorf("getItemAndLineAtY(3) = (%d,%d), want (2,1)", idx, line) + } +} diff --git a/internal/ui/selection/selection.go b/internal/ui/selection/selection.go index 1b432e7a..7c4c81ba 100644 --- a/internal/ui/selection/selection.go +++ b/internal/ui/selection/selection.go @@ -230,8 +230,10 @@ func FindWordBoundaries(line string, col int) (startCol, endCol int) { // HighlightLine applies reverse-video highlighting to a portion of a rendered // line (which may contain ANSI escape codes). startCol/endCol are in display -// columns. If startCol == -1, the entire line is highlighted. If startCol == -// endCol, returns the line unchanged. +// columns. If startCol == -1, the entire line is highlighted. If endCol == +// -1, the highlight runs from startCol to the end of the line (the sentinel +// returned by IsLineInRange for the first line of a multi-line selection). +// If startCol == endCol, returns the line unchanged. // // Uses ultraviolet ScreenBuffer for cell-level ANSI manipulation. func HighlightLine(line string, startCol, endCol int) string { @@ -250,6 +252,16 @@ func HighlightLine(line string, startCol, endCol int) string { endCol = lineWidth } + // "From startCol to end of line" sentinel (returned by IsLineInRange + // for the first line of a multi-line selection). Without this branch, + // the start line of a multi-line drag would never be highlighted — + // the user perceives this as the selection being shifted one row down + // from the cursor, especially when extension widgets shrink the + // scrollback and make the start line land on a tall styled block. + if endCol < 0 { + endCol = lineWidth + } + if startCol >= endCol || startCol >= lineWidth { return line } @@ -296,6 +308,11 @@ func ExtractText(line string, startCol, endCol int) string { endCol = lineWidth } + // "From startCol to end of line" sentinel (see HighlightLine). + if endCol < 0 { + endCol = lineWidth + } + if startCol >= endCol || startCol >= lineWidth { return "" } diff --git a/internal/ui/selection/selection_test.go b/internal/ui/selection/selection_test.go index b6d91562..75210f84 100644 --- a/internal/ui/selection/selection_test.go +++ b/internal/ui/selection/selection_test.go @@ -357,6 +357,54 @@ func TestHighlightLine_NoSelection(t *testing.T) { } } +// TestHighlightLine_EndOfLineSentinel verifies that endCol=-1 is interpreted +// as "highlight from startCol to end of line", matching the sentinel +// returned by IsLineInRange for the first line of a multi-line selection. +// +// Regression: without this contract, the start line of any multi-line drag +// would silently fall through HighlightLine's startCol >= endCol guard and +// render unstyled, making the selection appear to begin one row below the +// cursor — the exact "tracking gets shifted" symptom users reported when +// extension widgets shrank the scrollback enough that the click landed on a +// styled tool-result block. +func TestHighlightLine_EndOfLineSentinel(t *testing.T) { + line := "Hello, World!" + result := HighlightLine(line, 0, -1) + if result == line { + t.Errorf("endCol=-1 should highlight from startCol to end of line; got unchanged input") + } + if len(result) <= len(line) { + t.Errorf("highlighted result should be longer than plain input (ANSI codes added); got len=%d want > %d", len(result), len(line)) + } +} + +// TestExtractText_EndOfLineSentinel mirrors TestHighlightLine_EndOfLineSentinel +// for the extraction path used by the clipboard copy. +func TestExtractText_EndOfLineSentinel(t *testing.T) { + line := "Hello, World!" + got := ExtractText(line, 7, -1) + want := "World!" + if got != want { + t.Errorf("ExtractText(line, 7, -1) = %q, want %q", got, want) + } +} + +// TestIsLineInRange_StartLineSentinelHighlights composes IsLineInRange with +// HighlightLine end-to-end: the start line of a multi-line, single-item +// selection must actually emit highlight ANSI codes. This is the contract +// the rendering path in scrolllist.View() relies on. +func TestIsLineInRange_StartLineSentinelHighlights(t *testing.T) { + r := Range{StartItemIdx: 5, EndItemIdx: 5, StartLine: 0, EndLine: 2, StartCol: 0, EndCol: 10} + inRange, sc, ec := IsLineInRange(r, 5, 0) + if !inRange { + t.Fatalf("item 5 line 0 should be in range") + } + highlighted := HighlightLine("first line of selection", sc, ec) + if highlighted == "first line of selection" { + t.Errorf("first line of multi-line selection was not highlighted (sc=%d ec=%d)", sc, ec) + } +} + // TestMultiClickDetection verifies the click counting logic. func TestMultiClickDetection(t *testing.T) { s := NewState()