From 5f27b6327d6011b5b75107574d7466a0ed893486 Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Sun, 13 Sep 2026 15:04:04 -0400 Subject: [PATCH 1/3] fix: Keep the table highlight on screen when the cursor is placed, not moved MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Scrolling to the bottom of the events pane and pressing up scrolled the rows one at a time with no row highlighted anywhere. table.SetCursor moves the cursor without reconciling the viewport's scroll offset. UpdateViewport renders a WINDOW of rows around the cursor — start = clamp(cursor-height, 0, cursor) — and the viewport shows `height` lines of that window beginning at its own YOffset, which only MoveUp/MoveDown maintain. So with YOffset 0 and a cursor at or past one screenful, start lands on cursor-height and the cursor sits at window line `height`: exactly one line past the last visible one. The row is rendered and the highlight is drawn on it, off the bottom edge. MoveUp cannot recover from that state — its cases are start == 0, start < height, and YOffset >= 1, and none match — so YOffset stays 0 while start walks up with the cursor. That is the reported symptom: the rows scroll under the arrow key and no row is ever highlighted. Measured on a 40-row session at height 11: SetCursor(10) leaves the cursor on screen, SetCursor(11) and every target above it do not. setCursorVisible expresses the jump as relative movement instead — GotoTop to normalize the offset, then one MoveDown of n, which is a distance and not a loop, so it costs two O(height) re-renders however far the cursor travels. Every programmatic cursor placement now goes through it: - rebuildEventsTable's auto-follow and position-restore. This is why the bug showed up even when the bottom had been reached with the arrow keys: the pane rebuilds on every incoming event, and the restore lost the highlight again. The restore is also unconditional now — the old `else if prevRow < len(rows)` skipped it entirely when the rows shrank under the cursor (a filter typed, hideInactive toggled), leaving the cursor whereever SetRows had clamped it with an offset nobody reconciled. - goTop / goBottom, i.e. the g and G keys, for all four tables. The empty-row guards fold into the helper, which clamps. - the sessions pane's restore-by-session-id, and the pipeline pane's divider-skip nudges, which become MoveUp/MoveDown of 1. PgUp/PgDn were already relative moves and were never affected. Tests assert what an operator sees rather than the cursor index — the index was always correct, it was the rendered window that excluded it — by giving each fixture row a unique host and requiring the selected row's text to appear in the table's View(). Against the stub that kept the old behaviour they fail 31 times, with the boundary exactly at the table height. Verified: go test ./... in cmd/abctl passes except TestRunExec_BeforeFirstStartRunsAndSaysWhatIsLost, which fails identically on an unmodified upstream/main and is unrelated; go vet clean; golangci-lint --new-from-rev=upstream/main reports 0 issues; gofmt clean. Assisted-By: Claude (Anthropic AI) Signed-off-by: Hai Huang --- authbridge/cmd/abctl/tui/events_pane.go | 23 +- authbridge/cmd/abctl/tui/keys.go | 37 ++-- authbridge/cmd/abctl/tui/pipeline_pane.go | 5 +- authbridge/cmd/abctl/tui/sessions_pane.go | 10 +- authbridge/cmd/abctl/tui/table_cursor.go | 47 ++++ authbridge/cmd/abctl/tui/table_cursor_test.go | 209 ++++++++++++++++++ 6 files changed, 300 insertions(+), 31 deletions(-) create mode 100644 authbridge/cmd/abctl/tui/table_cursor.go create mode 100644 authbridge/cmd/abctl/tui/table_cursor_test.go diff --git a/authbridge/cmd/abctl/tui/events_pane.go b/authbridge/cmd/abctl/tui/events_pane.go index bb1616ebc..be1ae3f47 100644 --- a/authbridge/cmd/abctl/tui/events_pane.go +++ b/authbridge/cmd/abctl/tui/events_pane.go @@ -154,11 +154,24 @@ func (m *model) rebuildEventsTable() { // Auto-follow: if user was at the bottom, stay at the bottom. Otherwise // preserve position so reading isn't disturbed by new events. - if wasAtEnd && len(rows) > 0 { - m.eventsTbl.SetCursor(len(rows) - 1) - } else if prevRow < len(rows) { - m.eventsTbl.SetCursor(prevRow) - } + // + // Unconditional now, and through setCursorVisible rather than SetCursor. Two + // reasons, both about the highlight rather than the index: + // + // - SetCursor does not reconcile the viewport's offset, so restoring any row + // at or past one screenful left the cursor one line below the rendered + // window. On a live session that meant the highlight disappeared on the very + // next event, for every session long enough to scroll. setCursorVisible + // carries the mechanics. + // - the old `else if prevRow < len(rows)` skipped the restore entirely when + // the rows SHRANK under the cursor — a filter typed, hideInactive toggled. + // SetRows had clamped the index by then, so the cursor was left wherever + // that landed, with an offset nobody reconciled. + target := prevRow + if wasAtEnd { + target = len(rows) - 1 + } + setCursorVisible(&m.eventsTbl, target) } // selectedEvent returns the event at the cursor row, or nil. The cursor diff --git a/authbridge/cmd/abctl/tui/keys.go b/authbridge/cmd/abctl/tui/keys.go index 7a1348bad..5df302c1f 100644 --- a/authbridge/cmd/abctl/tui/keys.go +++ b/authbridge/cmd/abctl/tui/keys.go @@ -605,12 +605,14 @@ func (m *model) handleKey(msg tea.KeyMsg) tea.Cmd { prev := m.pipelineTbl.Cursor() var cmd tea.Cmd m.pipelineTbl, cmd = m.pipelineTbl.Update(msg) - // Skip over the divider row when navigating. + // Skip over the divider row when navigating. One more step in the direction + // of travel, as a relative move so the offset stays reconciled — see + // setCursorVisible for why SetCursor is not used for cursor placement. if isDividerRow(m.pipelineTbl.Rows(), m.pipelineTbl.Cursor()) { if m.pipelineTbl.Cursor() > prev { - m.pipelineTbl.SetCursor(m.pipelineTbl.Cursor() + 1) + m.pipelineTbl.MoveDown(1) } else { - m.pipelineTbl.SetCursor(m.pipelineTbl.Cursor() - 1) + m.pipelineTbl.MoveUp(1) } } return cmd @@ -640,16 +642,21 @@ func (m *model) refreshActivePane() { } } +// goTop and goBottom place the cursor through setCursorVisible, not SetCursor: a +// jump to the last row is exactly the case where SetCursor leaves the highlight one +// line below the rendered window, so `G` on any list longer than the screen used to +// scroll to the bottom with nothing highlighted. The empty-table guards live in +// setCursorVisible now, and it clamps, so goBottom does not need the row count. func (m *model) goTop() { switch m.pane { case paneCatalog: - m.catalogTbl.SetCursor(0) + setCursorVisible(&m.catalogTbl, 0) case paneSessions: - m.sessionsTbl.SetCursor(0) + setCursorVisible(&m.sessionsTbl, 0) case paneEvents: - m.eventsTbl.SetCursor(0) + setCursorVisible(&m.eventsTbl, 0) case panePipeline: - m.pipelineTbl.SetCursor(0) + setCursorVisible(&m.pipelineTbl, 0) case paneDetail, panePluginDetail: m.detailVp.GotoTop() } @@ -658,21 +665,13 @@ func (m *model) goTop() { func (m *model) goBottom() { switch m.pane { case paneSessions: - if n := len(m.sessionsTbl.Rows()); n > 0 { - m.sessionsTbl.SetCursor(n - 1) - } + setCursorVisible(&m.sessionsTbl, len(m.sessionsTbl.Rows())-1) case paneEvents: - if n := len(m.eventsTbl.Rows()); n > 0 { - m.eventsTbl.SetCursor(n - 1) - } + setCursorVisible(&m.eventsTbl, len(m.eventsTbl.Rows())-1) case panePipeline: - if n := len(m.pipelineTbl.Rows()); n > 0 { - m.pipelineTbl.SetCursor(n - 1) - } + setCursorVisible(&m.pipelineTbl, len(m.pipelineTbl.Rows())-1) case paneCatalog: - if n := len(m.catalogTbl.Rows()); n > 0 { - m.catalogTbl.SetCursor(n - 1) - } + setCursorVisible(&m.catalogTbl, len(m.catalogTbl.Rows())-1) case paneDetail, panePluginDetail: m.detailVp.GotoBottom() } diff --git a/authbridge/cmd/abctl/tui/pipeline_pane.go b/authbridge/cmd/abctl/tui/pipeline_pane.go index 0bd7c4a49..36ee64db2 100644 --- a/authbridge/cmd/abctl/tui/pipeline_pane.go +++ b/authbridge/cmd/abctl/tui/pipeline_pane.go @@ -49,9 +49,10 @@ func (m *model) rebuildPipelineTable() { rows = append(rows, pipelineRow(p, counts[p.Name], m.pipeline.Outbound)) } m.pipelineTbl.SetRows(rows) - // If cursor is on the divider row, nudge to the next plugin. + // If cursor is on the divider row, nudge to the next plugin. A relative move so + // the viewport offset stays reconciled — see setCursorVisible. if isDividerRow(rows, m.pipelineTbl.Cursor()) { - m.pipelineTbl.SetCursor(m.pipelineTbl.Cursor() + 1) + m.pipelineTbl.MoveDown(1) } } diff --git a/authbridge/cmd/abctl/tui/sessions_pane.go b/authbridge/cmd/abctl/tui/sessions_pane.go index c81f1ca52..0abbed56a 100644 --- a/authbridge/cmd/abctl/tui/sessions_pane.go +++ b/authbridge/cmd/abctl/tui/sessions_pane.go @@ -73,18 +73,18 @@ func (m *model) rebuildSessionsTable() { } m.sessionsTbl.SetRows(rows) - // Restore cursor position if possible. + // Restore cursor position if possible. Through setCursorVisible: a restored row + // past the first screenful would otherwise land one line below the rendered + // window, leaving the pane with no highlight — see setCursorVisible. if prev != "" { for i, r := range rows { if r[0] == prev { - m.sessionsTbl.SetCursor(i) + setCursorVisible(&m.sessionsTbl, i) return } } } - if len(rows) > 0 { - m.sessionsTbl.SetCursor(0) - } + setCursorVisible(&m.sessionsTbl, 0) } // cachedOnlySessionIDs lists sessions abctl has events for that the server's diff --git a/authbridge/cmd/abctl/tui/table_cursor.go b/authbridge/cmd/abctl/tui/table_cursor.go new file mode 100644 index 000000000..269c7b7b4 --- /dev/null +++ b/authbridge/cmd/abctl/tui/table_cursor.go @@ -0,0 +1,47 @@ +package tui + +import "github.com/charmbracelet/bubbles/table" + +// setCursorVisible moves a table's cursor to row n and leaves it ON SCREEN. +// +// Use this instead of table.SetCursor everywhere a cursor is placed +// programmatically — following a tail, restoring a saved position, jumping to an +// end. SetCursor alone loses the highlight for any row at or past one screenful. +// +// WHY: bubbles' UpdateViewport renders a WINDOW of rows around the cursor — +// start = clamp(cursor−height, 0, cursor) — and the viewport then shows `height` +// lines of that window beginning at its own YOffset. SetCursor recomputes the +// window but never reconciles YOffset, and only MoveUp/MoveDown do. So with +// YOffset 0 and cursor ≥ height, start lands on cursor−height and the cursor sits +// at window line `height`: exactly one line past the last visible one. The row is +// rendered, the highlight is drawn on it, and it is off the bottom edge. +// +// MoveUp cannot recover from that state either — its three cases are start == 0, +// start < height, and YOffset ≥ 1, and in it none of them match — so YOffset stays +// 0 while start walks up with the cursor. The rows scroll one at a time under an +// arrow key and no row is ever highlighted, which is how this reached us: "the +// lines are getting scrolled up, but the highlight disappears". +// +// The fix is to express the jump as relative movement, the only cursor API in +// bubbles that maintains the offset: GotoTop normalizes it (a MoveUp to row 0), +// then one MoveDown of n walks to the target. n is a distance, not a loop, so this +// is two O(height) re-renders however far the cursor travels. +// +// n is clamped to the row range, as SetCursor's own is. An empty table is left +// alone rather than parked on a row that does not exist. +func setCursorVisible(t *table.Model, n int) { + rows := len(t.Rows()) + if rows == 0 { + return + } + if n < 0 { + n = 0 + } + if n > rows-1 { + n = rows - 1 + } + t.GotoTop() + if n > 0 { + t.MoveDown(n) + } +} diff --git a/authbridge/cmd/abctl/tui/table_cursor_test.go b/authbridge/cmd/abctl/tui/table_cursor_test.go new file mode 100644 index 000000000..1c3e33f6e --- /dev/null +++ b/authbridge/cmd/abctl/tui/table_cursor_test.go @@ -0,0 +1,209 @@ +package tui + +import ( + "fmt" + "strings" + "testing" + + "github.com/charmbracelet/bubbles/table" + tea "github.com/charmbracelet/bubbletea" + + "github.com/rossoctl/cortex/authbridge/authlib/pipeline" +) + +// hostToken is the per-row marker these fixtures use. Unique per row and short +// enough that the 20-wide HOST column does not truncate it, so its presence in the +// rendered view is a reliable "this row is on screen" — which is the property under +// test. Asserting on the cursor INDEX alone cannot catch this bug: the index was +// always right, it was the rendered window that excluded it. +func hostToken(i int) string { return fmt.Sprintf("h%02d.example", i) } + +// cursorRowsFixture builds n request events, each identifiable by its host. +func cursorRowsFixture(n int) []pipeline.SessionEvent { + events := make([]pipeline.SessionEvent, n) + for i := range events { + events[i] = pipeline.SessionEvent{ + Direction: pipeline.Outbound, Phase: pipeline.SessionRequest, + Host: hostToken(i), + Inference: &pipeline.InferenceExtension{Model: "m"}, + } + } + return events +} + +func cursorModel(t *testing.T, n int) *model { + t.Helper() + m := &model{ + pane: paneEvents, selectedSess: "s", bodyHeight: 12, width: 200, + events: map[string][]pipeline.SessionEvent{"s": cursorRowsFixture(n)}, + } + m.eventsTbl = newEventsTable() + m.rebuildEventsTable() + if got := len(m.eventsTbl.Rows()); got != n { + t.Fatalf("fixture built %d rows, want %d", got, n) + } + if h := m.eventsTbl.Height(); h >= n { + t.Fatalf("fixture height %d must be smaller than %d rows to exercise scrolling", h, n) + } + return m +} + +// assertSelectionVisible fails when the highlighted row is outside the window the +// table actually renders. The highlight is drawn on the cursor row wherever it is, +// so a cursor outside the rendered window means a table with no visible highlight +// at all — which is what an operator sees. +func assertSelectionVisible(t *testing.T, tbl table.Model, label string) { + t.Helper() + cur := tbl.Cursor() + if cur < 0 { + t.Errorf("%s: no row selected (cursor=%d)", label, cur) + return + } + view := tbl.View() + want := hostToken(cur) + if strings.Contains(view, want) { + return + } + var onScreen []int + for i := 0; i < len(tbl.Rows()); i++ { + if strings.Contains(view, hostToken(i)) { + onScreen = append(onScreen, i) + } + } + win := "nothing" + if len(onScreen) > 0 { + win = fmt.Sprintf("rows %d..%d", onScreen[0], onScreen[len(onScreen)-1]) + } + t.Errorf("%s: selected row %d is off screen; the view shows %s", label, cur, win) +} + +// TestSetCursorVisible_LandsOnScreen is the unit-level guard. +// +// table.SetCursor re-windows the rendered rows around the new cursor +// (start = cursor − height) but never reconciles the viewport's YOffset, so for any +// target at or past one screenful the cursor lands exactly one line below the last +// visible row. setCursorVisible must put the cursor where it was asked AND leave it +// on screen, for targets on both sides of that boundary. +func TestSetCursorVisible_LandsOnScreen(t *testing.T) { + const n = 40 + for _, target := range []int{0, 1, 5, 10, 11, 12, 20, 33, 38, 39} { + m := cursorModel(t, n) + setCursorVisible(&m.eventsTbl, target) + if got := m.eventsTbl.Cursor(); got != target { + t.Errorf("target %d: cursor=%d", target, got) + } + assertSelectionVisible(t, m.eventsTbl, fmt.Sprintf("target %d", target)) + } +} + +// Out-of-range targets clamp to the row range rather than leaving the table with a +// cursor pointing at nothing — SetCursor's own clamping behaviour, preserved. +func TestSetCursorVisible_ClampsAndSurvivesEmpty(t *testing.T) { + m := cursorModel(t, 40) + setCursorVisible(&m.eventsTbl, 999) + if got := m.eventsTbl.Cursor(); got != 39 { + t.Errorf("cursor past the end = %d, want 39", got) + } + assertSelectionVisible(t, m.eventsTbl, "target past the end") + + setCursorVisible(&m.eventsTbl, -5) + if got := m.eventsTbl.Cursor(); got != 0 { + t.Errorf("negative target = %d, want 0", got) + } + assertSelectionVisible(t, m.eventsTbl, "negative target") + + // An empty table must not panic or park the cursor on a row that is not there. + empty := newEventsTable() + setCursorVisible(&empty, 3) + if got := empty.Cursor(); got > 0 { + t.Errorf("empty table cursor = %d, want <= 0", got) + } +} + +// TestEventsTable_AutoFollowKeepsSelectionVisible is the reported bug. +// +// The pane rebuilds on every incoming event, and the rebuild restores the cursor — +// to the last row while following the tail. That restore used to lose the highlight, +// so on a live session the highlight vanished on the next event even when the +// operator had scrolled to the bottom with the arrow keys. Pressing up then scrolled +// the rows one at a time with no highlight anywhere, because the cursor stayed +// exactly one row below the rendered window. +func TestEventsTable_AutoFollowKeepsSelectionVisible(t *testing.T) { + m := cursorModel(t, 40) + + // The first rebuild already follows the tail. + assertSelectionVisible(t, m.eventsTbl, "after initial rebuild") + if got, want := m.eventsTbl.Cursor(), 39; got != want { + t.Fatalf("auto-follow cursor = %d, want %d", got, want) + } + + // A rebuild while following the tail keeps the highlight on screen. + m.rebuildEventsTable() + assertSelectionVisible(t, m.eventsTbl, "rebuild while following") + + // Arrow up, then more rebuilds: the highlight stays visible and the cursor + // keeps walking up one row at a time. + for i := 1; i <= 6; i++ { + m.eventsTbl, _ = m.eventsTbl.Update(tea.KeyMsg{Type: tea.KeyUp}) + assertSelectionVisible(t, m.eventsTbl, fmt.Sprintf("up #%d", i)) + if got, want := m.eventsTbl.Cursor(), 39-i; got != want { + t.Fatalf("up #%d: cursor = %d, want %d", i, got, want) + } + m.rebuildEventsTable() + assertSelectionVisible(t, m.eventsTbl, fmt.Sprintf("up #%d + rebuild", i)) + if got, want := m.eventsTbl.Cursor(), 39-i; got != want { + t.Fatalf("up #%d + rebuild: cursor = %d, want %d", i, got, want) + } + } +} + +// A new event arriving while the operator reads mid-list must not move the +// highlight, and must not lose it either — the other branch of the rebuild. +func TestEventsTable_RebuildMidListKeepsSelectionVisible(t *testing.T) { + m := cursorModel(t, 40) + setCursorVisible(&m.eventsTbl, 20) + assertSelectionVisible(t, m.eventsTbl, "parked mid-list") + + m.events["s"] = append(m.events["s"], cursorRowsFixture(41)[40]) + m.rebuildEventsTable() + if got := m.eventsTbl.Cursor(); got != 20 { + t.Errorf("cursor moved on a new event: %d, want 20", got) + } + assertSelectionVisible(t, m.eventsTbl, "new event while parked mid-list") +} + +// TestGoBottom_KeepsSelectionVisible covers the G/end key, which jumps straight to +// the last row — the fastest way to reproduce this by hand. +func TestGoBottom_KeepsSelectionVisible(t *testing.T) { + m := cursorModel(t, 40) + setCursorVisible(&m.eventsTbl, 0) + + m.goBottom() + if got, want := m.eventsTbl.Cursor(), 39; got != want { + t.Fatalf("goBottom cursor = %d, want %d", got, want) + } + assertSelectionVisible(t, m.eventsTbl, "goBottom") + + for i := 1; i <= 3; i++ { + m.eventsTbl, _ = m.eventsTbl.Update(tea.KeyMsg{Type: tea.KeyUp}) + assertSelectionVisible(t, m.eventsTbl, fmt.Sprintf("goBottom then up #%d", i)) + } + + m.goTop() + if got := m.eventsTbl.Cursor(); got != 0 { + t.Fatalf("goTop cursor = %d, want 0", got) + } + assertSelectionVisible(t, m.eventsTbl, "goTop") +} + +// A terminal resize re-heights the table mid-session. SetHeight re-windows the rows +// the same way SetCursor does, so the highlight must survive it. +func TestEventsTable_ResizeKeepsSelectionVisible(t *testing.T) { + m := cursorModel(t, 40) + setCursorVisible(&m.eventsTbl, 30) + for _, h := range []int{40, 20, 12, 8, 30} { + m.bodyHeight = h + m.rebuildEventsTable() + assertSelectionVisible(t, m.eventsTbl, fmt.Sprintf("bodyHeight %d", h)) + } +} From 23bf675d2d8ea65fc2261c9d70f68cd0d64ee766 Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Sun, 13 Sep 2026 15:24:12 -0400 Subject: [PATCH 2/3] test: Pin the untested cursor placements, and correct what the shrink case costs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review catch: three of the converted call sites had no test, and the rows-shrink case was mis-described. The description said the unconditional restore fixes a cursor "left wherever SetRows had clamped it, with an offset nobody reconciled". Measured, it is worse than that. rebuildEventsTable clears the table first (SetRows(nil)), which clamps the cursor to len(rows)-1 = -1, and the refill does not restore it because -1 is not greater than the new len-1. The old `else if prevRow < len(rows)` guard was false whenever the rows shrank past the cursor, so nothing restored it either and the cursor stayed at -1: NO ROW SELECTED AT ALL. Not a highlight one line off — SelectedRow returns nil, selectedEventRow reports not-ok, and the detail pane goes blank. Typing a filter or toggling hideInactive while parked below the new row count did that every time, on any list length. Pinned red: parked at row 30: selected row 30 is off screen; the view shows rows 19..29 after the filter shrank the list: no row selected (cursor=-1) no selected row resolves after the shrink New tests, each verified red against the pre-fix files: - FilterShrink, over both a moderate shrink (ten rows) and a drastic one (one row, shorter than the table), plus the round trip back. A drastic shrink is self-correcting for the OFFSET — the viewport clamps when the content gets shorter than YOffset — so only the moderate case exercises that half, while both exercise the lost selection. - HideInactiveShrink, which removes rows from the MIDDLE, so no surviving row keeps its old index. - SessionsTable_RestoreByID: the sessions pane restores by session id on a poll rather than a keystroke, so the same lost highlight was one refresh away there too. Covers the fallback-to-row-0 branch that absorbed the old len(rows) > 0 guard, and an empty list after it. The divider-nudge tests PIN behaviour rather than catch a regression: MoveDown(1) and SetCursor(+1) agree on the index, including the clamp at both ends, so they pass either way. They exist because the offset does move now — skipping the divider can scroll the pane by a line — and because both ends and both directions should stay pinned while that is true. Two nits, also from review: - the empty-table assertion accepted `> 0`, which admits 0 — itself not a row on an empty table. It now pins "the cursor is left exactly as it was", which is all the helper promises, over both ways a table can be empty: fresh (cursor 0) and emptied by SetRows(nil) (cursor -1). This is what caught the stub moving a fresh empty table's cursor from 0 to -1. - stray "in it" in table_cursor.go's MoveUp paragraph. assertSelectionVisible now reads the marker off the SELECTED ROW instead of deriving it from the cursor index, because under a filter row N is no longer event N and the old form would have asserted that some unrelated row was on screen. Verified: go test ./tui/ passes; gofmt clean; go vet clean; golangci-lint --new-from-rev=upstream/main 0 issues. Assisted-By: Claude (Anthropic AI) Signed-off-by: Hai Huang --- authbridge/cmd/abctl/tui/table_cursor.go | 4 +- authbridge/cmd/abctl/tui/table_cursor_test.go | 320 +++++++++++++++++- 2 files changed, 316 insertions(+), 8 deletions(-) diff --git a/authbridge/cmd/abctl/tui/table_cursor.go b/authbridge/cmd/abctl/tui/table_cursor.go index 269c7b7b4..52b3041dc 100644 --- a/authbridge/cmd/abctl/tui/table_cursor.go +++ b/authbridge/cmd/abctl/tui/table_cursor.go @@ -17,8 +17,8 @@ import "github.com/charmbracelet/bubbles/table" // rendered, the highlight is drawn on it, and it is off the bottom edge. // // MoveUp cannot recover from that state either — its three cases are start == 0, -// start < height, and YOffset ≥ 1, and in it none of them match — so YOffset stays -// 0 while start walks up with the cursor. The rows scroll one at a time under an +// start < height, and YOffset ≥ 1, and in that state none of them match — so YOffset +// stays 0 while start walks up with the cursor. The rows scroll one at a time under an // arrow key and no row is ever highlighted, which is how this reached us: "the // lines are getting scrolled up, but the highlight disappears". // diff --git a/authbridge/cmd/abctl/tui/table_cursor_test.go b/authbridge/cmd/abctl/tui/table_cursor_test.go index 1c3e33f6e..2ae1d0246 100644 --- a/authbridge/cmd/abctl/tui/table_cursor_test.go +++ b/authbridge/cmd/abctl/tui/table_cursor_test.go @@ -4,11 +4,14 @@ import ( "fmt" "strings" "testing" + "time" "github.com/charmbracelet/bubbles/table" tea "github.com/charmbracelet/bubbletea" "github.com/rossoctl/cortex/authbridge/authlib/pipeline" + "github.com/rossoctl/cortex/authbridge/authlib/session" + "github.com/rossoctl/cortex/authbridge/cmd/abctl/apiclient" ) // hostToken is the per-row marker these fixtures use. Unique per row and short @@ -18,6 +21,17 @@ import ( // always right, it was the rendered window that excluded it. func hostToken(i int) string { return fmt.Sprintf("h%02d.example", i) } +// markerOf finds the fixture marker among a row's cells, so an assertion can name the row +// the cursor is actually on rather than the row its index would have been before a filter. +func markerOf(row table.Row) (string, bool) { + for _, cell := range row { + if c := strings.TrimSpace(cell); strings.HasSuffix(c, ".example") { + return c, true + } + } + return "", false +} + // cursorRowsFixture builds n request events, each identifiable by its host. func cursorRowsFixture(n int) []pipeline.SessionEvent { events := make([]pipeline.SessionEvent, n) @@ -60,7 +74,14 @@ func assertSelectionVisible(t *testing.T, tbl table.Model, label string) { return } view := tbl.View() - want := hostToken(cur) + // Read the marker off the SELECTED ROW, not from the cursor index. Once a filter is + // on, row N is no longer event N, so deriving the token from the index would assert + // that some unrelated row is on screen. + want, ok := markerOf(tbl.SelectedRow()) + if !ok { + t.Errorf("%s: selected row %d carries no marker cell: %q", label, cur, tbl.SelectedRow()) + return + } if strings.Contains(view, want) { return } @@ -112,11 +133,30 @@ func TestSetCursorVisible_ClampsAndSurvivesEmpty(t *testing.T) { } assertSelectionVisible(t, m.eventsTbl, "negative target") - // An empty table must not panic or park the cursor on a row that is not there. - empty := newEventsTable() - setCursorVisible(&empty, 3) - if got := empty.Cursor(); got > 0 { - t.Errorf("empty table cursor = %d, want <= 0", got) + // An empty table is left exactly as it was, which is the most the helper can promise: + // with no rows there is no row to select, and where a bubbles table parks its cursor + // when empty depends on how it emptied — a fresh one sits at 0, one whose rows were + // taken away by SetRows(nil) at -1. Neither index addresses a row, so the helper + // declines to move rather than picking one, and this pins "unchanged" rather than a + // value that would only describe one of the two ways in. + for _, tc := range []struct { + name string + build func() table.Model + }{ + {"fresh", newEventsTable}, + {"emptied by SetRows(nil)", func() table.Model { + tbl := newEventsTable() + tbl.SetRows([]table.Row{make(table.Row, len(tbl.Columns()))}) + tbl.SetRows(nil) + return tbl + }}, + } { + empty := tc.build() + before := empty.Cursor() + setCursorVisible(&empty, 3) + if got := empty.Cursor(); got != before { + t.Errorf("%s empty table: cursor moved %d -> %d, want it left alone", tc.name, before, got) + } } } @@ -207,3 +247,271 @@ func TestEventsTable_ResizeKeepsSelectionVisible(t *testing.T) { assertSelectionVisible(t, m.eventsTbl, fmt.Sprintf("bodyHeight %d", h)) } } + +// TestEventsTable_FilterShrinkKeepsSelectionVisible covers the rows-SHRINK case, the one the +// old `else if prevRow < len(rows)` skipped outright: typing a filter or toggling +// hideInactive can pull the list out from under the cursor, and with no restore the cursor +// was left wherever SetRows had clamped it, with an offset nobody reconciled. +// +// Both shrink paths are exercised, and both directions of the round trip, because the +// interesting state is the one where the stale offset still addresses a line of the new, +// shorter content — a drastic shrink is self-correcting, a moderate one is not. +func TestEventsTable_FilterShrinkKeepsSelectionVisible(t *testing.T) { + for _, tc := range []struct { + name string + shrink func(m *model) + wantRows int + }{ + // "h3" keeps h30..h39: ten rows, every one of them below the parked cursor. + {"filter to ten rows", func(m *model) { m.filter = "h3" }, 10}, + // A single row is the drastic case: shorter than the table height. + {"filter to one row", func(m *model) { m.filter = "h07" }, 1}, + } { + t.Run(tc.name, func(t *testing.T) { + m := cursorModel(t, 40) + setCursorVisible(&m.eventsTbl, 30) + assertSelectionVisible(t, m.eventsTbl, "parked at row 30") + + tc.shrink(m) + m.rebuildEventsTable() + if got := len(m.eventsTbl.Rows()); got != tc.wantRows { + t.Fatalf("filter left %d rows, want %d", got, tc.wantRows) + } + assertSelectionVisible(t, m.eventsTbl, "after the filter shrank the list") + if got, want := m.eventsTbl.Cursor(), tc.wantRows-1; got > want { + t.Errorf("cursor %d addresses no row in a %d-row list", got, tc.wantRows) + } + // The detail pane reads the selection through this, so a cursor the table + // clamped but the model did not follow shows an empty pane. + if _, ok := m.selectedEventRow(); !ok { + t.Error("no selected row resolves after the shrink") + } + + // Clearing the filter grows the list back; the cursor must still be on screen. + m.filter = "" + m.rebuildEventsTable() + if got := len(m.eventsTbl.Rows()); got != 40 { + t.Fatalf("clearing the filter left %d rows, want 40", got) + } + assertSelectionVisible(t, m.eventsTbl, "after clearing the filter") + if _, ok := m.selectedEventRow(); !ok { + t.Error("no selected row resolves after clearing the filter") + } + }) + } +} + +// hideInactive is the other shrink lever, and it hides rows from the MIDDLE of the list +// rather than the ends, so the surviving rows keep no relationship to their old indices. +func TestEventsTable_HideInactiveShrinkKeepsSelectionVisible(t *testing.T) { + events := cursorRowsFixture(40) + // Give a handful of events a plugin invocation so they survive the inactive filter. + active := map[int]bool{2: true, 9: true, 17: true, 26: true, 33: true, 38: true} + for i := range events { + if active[i] { + events[i].Invocations = &pipeline.Invocations{Outbound: []pipeline.Invocation{ + {Plugin: "tool-prune", Action: pipeline.ActionModify, Reason: "tools_pruned"}, + }} + } + } + m := &model{ + pane: paneEvents, selectedSess: "s", bodyHeight: 12, width: 200, + events: map[string][]pipeline.SessionEvent{"s": events}, + } + m.eventsTbl = newEventsTable() + m.rebuildEventsTable() + setCursorVisible(&m.eventsTbl, 30) + assertSelectionVisible(t, m.eventsTbl, "parked at row 30") + + m.hideInactive = true + m.rebuildEventsTable() + if got, want := len(m.eventsTbl.Rows()), len(active); got != want { + t.Fatalf("hideInactive left %d rows, want %d", got, want) + } + assertSelectionVisible(t, m.eventsTbl, "after hideInactive shrank the list") + if _, ok := m.selectedEventRow(); !ok { + t.Error("no selected row resolves after hideInactive") + } + + m.hideInactive = false + m.rebuildEventsTable() + assertSelectionVisible(t, m.eventsTbl, "after hideInactive off") +} + +// sessionsModel builds a sessions pane with n sessions, taller than the table, and the +// cursor parked on one of them. IDs carry the marker so the same visibility assertion works. +func sessionsModel(t *testing.T, n int) *model { + t.Helper() + m := &model{pane: paneSessions, width: 200} + m.sessionsTbl = newSessionsTable() + m.sessionsTbl.SetHeight(12) + for i := 0; i < n; i++ { + m.sessions = append(m.sessions, session.SessionSummary{ + ID: hostToken(i), UpdatedAt: time.Now(), EventCount: 3, + }) + } + m.rebuildSessionsTable() + if got := len(m.sessionsTbl.Rows()); got != n { + t.Fatalf("fixture built %d session rows, want %d", got, n) + } + if h := m.sessionsTbl.Height(); h >= n { + t.Fatalf("fixture height %d must be smaller than %d rows", h, n) + } + return m +} + +// The sessions pane restores its cursor BY SESSION ID on every refresh, which arrives on a +// poll rather than a keystroke — so the same lost highlight was one refresh away there too, +// for any session list longer than the pane. +func TestSessionsTable_RestoreByIDKeepsSelectionVisible(t *testing.T) { + m := sessionsModel(t, 40) + setCursorVisible(&m.sessionsTbl, 30) + assertSelectionVisible(t, m.sessionsTbl, "parked at session 30") + want := m.selectedSessionID() + + // A refresh that changes nothing must not move or lose the selection. + m.rebuildSessionsTable() + if got := m.selectedSessionID(); got != want { + t.Errorf("refresh moved the selection to %q, want %q", got, want) + } + assertSelectionVisible(t, m.sessionsTbl, "after a refresh") + + // A refresh that drops the selected session falls back to the first row — the branch + // that absorbed the old len(rows) > 0 guard, so it must survive an empty list too. + m.sessions = m.sessions[:20] + m.rebuildSessionsTable() + if got, want := m.sessionsTbl.Cursor(), 0; got != want { + t.Errorf("cursor after the selected session vanished = %d, want %d", got, want) + } + assertSelectionVisible(t, m.sessionsTbl, "after the selected session vanished") + + m.sessions = nil + m.rebuildSessionsTable() + if got := len(m.sessionsTbl.Rows()); got != 0 { + t.Fatalf("expected an empty sessions table, got %d rows", got) + } + if got := m.selectedSessionID(); got != "" { + t.Errorf("empty sessions table reports %q selected", got) + } +} + +// pipelineModel builds a pipeline pane with an inbound and an outbound chain either side of +// the divider, long enough that the table scrolls. +func pipelineModel(t *testing.T, inbound, outbound int) *model { + t.Helper() + m := &model{pane: panePipeline, width: 200, pipeline: &apiclient.PipelineView{}} + m.pipelineTbl = newPipelineTable() + m.pipelineTbl.SetHeight(12) + for i := 0; i < inbound; i++ { + m.pipeline.Inbound = append(m.pipeline.Inbound, apiclient.PipelinePlugin{ + Name: fmt.Sprintf("in-%02d", i), Direction: "inbound", Position: i + 1, + }) + } + for i := 0; i < outbound; i++ { + m.pipeline.Outbound = append(m.pipeline.Outbound, apiclient.PipelinePlugin{ + Name: fmt.Sprintf("out-%02d", i), Direction: "outbound", Position: i + 1, + }) + } + m.rebuildPipelineTable() + return m +} + +// The divider nudge moves by one row in the direction of travel. As MoveUp/MoveDown that is +// index-equivalent to SetCursor(±1) — including the clamp at both ends — but it also moves +// the viewport offset, which is both the point and a visible change: skipping the divider can +// now scroll the pane by a line. Pinned here so neither half drifts. +func TestPipelineTable_DividerNudgeKeepsSelectionVisible(t *testing.T) { + m := pipelineModel(t, 20, 20) + rows := m.pipelineTbl.Rows() + divider := -1 + for i := range rows { + if isDividerRow(rows, i) { + divider = i + break + } + } + if divider <= 0 || divider >= len(rows)-1 { + t.Fatalf("divider at %d in %d rows; fixture needs plugins on both sides", divider, len(rows)) + } + + // Travelling DOWN onto the divider skips to the row after it. + setCursorVisible(&m.pipelineTbl, divider-1) + m.pipelineTbl, _ = m.pipelineTbl.Update(tea.KeyMsg{Type: tea.KeyDown}) + if isDividerRow(m.pipelineTbl.Rows(), m.pipelineTbl.Cursor()) { + m.pipelineTbl.MoveDown(1) // what handleKey does for panePipeline + } + if got, want := m.pipelineTbl.Cursor(), divider+1; got != want { + t.Errorf("down onto the divider left cursor %d, want %d", got, want) + } + if p := m.selectedPlugin(); p == nil { + t.Error("cursor rests on the divider after a downward nudge") + } + assertPipelineSelectionVisible(t, m, "after a downward nudge") + + // Travelling UP onto it skips to the row before. + setCursorVisible(&m.pipelineTbl, divider+1) + m.pipelineTbl, _ = m.pipelineTbl.Update(tea.KeyMsg{Type: tea.KeyUp}) + if isDividerRow(m.pipelineTbl.Rows(), m.pipelineTbl.Cursor()) { + m.pipelineTbl.MoveUp(1) + } + if got, want := m.pipelineTbl.Cursor(), divider-1; got != want { + t.Errorf("up onto the divider left cursor %d, want %d", got, want) + } + if p := m.selectedPlugin(); p == nil { + t.Error("cursor rests on the divider after an upward nudge") + } + assertPipelineSelectionVisible(t, m, "after an upward nudge") + + // A rebuild whose cursor lands on the divider nudges past it and keeps it on screen. + setCursorVisible(&m.pipelineTbl, divider) + m.rebuildPipelineTable() + if isDividerRow(m.pipelineTbl.Rows(), m.pipelineTbl.Cursor()) { + t.Errorf("rebuild left the cursor on the divider at %d", m.pipelineTbl.Cursor()) + } + assertPipelineSelectionVisible(t, m, "after a rebuild off the divider") +} + +// The clamp at both ends: an inbound-only chain puts the divider last, where nudging DOWN +// has nowhere to go. SetCursor(+1) clamped there and so does MoveDown(1) — the cursor stays +// on the divider, and selectedPlugin returning nil is what keeps the detail pane honest. +func TestPipelineTable_DividerNudgeClampsAtTheEnds(t *testing.T) { + m := pipelineModel(t, 20, 0) + rows := m.pipelineTbl.Rows() + if last := len(rows) - 1; !isDividerRow(rows, last) { + t.Fatalf("expected the divider last in an inbound-only chain, rows=%d", len(rows)) + } + last := len(rows) - 1 + m.pipelineTbl.SetCursor(last) + m.pipelineTbl.MoveDown(1) + if got := m.pipelineTbl.Cursor(); got != last { + t.Errorf("nudge past the last row moved cursor to %d, want %d", got, last) + } + if p := m.selectedPlugin(); p != nil { + t.Errorf("divider row reported plugin %q", p.Name) + } + + // And an outbound-only chain puts it first, where nudging UP has nowhere to go. + m = pipelineModel(t, 0, 20) + if !isDividerRow(m.pipelineTbl.Rows(), 0) { + t.Fatal("expected the divider first in an outbound-only chain") + } + m.pipelineTbl.SetCursor(0) + m.pipelineTbl.MoveUp(1) + if got := m.pipelineTbl.Cursor(); got != 0 { + t.Errorf("nudge above the first row moved cursor to %d, want 0", got) + } +} + +// assertPipelineSelectionVisible is assertSelectionVisible for the pipeline table, whose rows +// carry plugin names rather than the host marker. +func assertPipelineSelectionVisible(t *testing.T, m *model, label string) { + t.Helper() + row := m.pipelineTbl.SelectedRow() + if len(row) < 3 { + t.Errorf("%s: no row selected", label) + return + } + if name := strings.TrimSpace(row[2]); !strings.Contains(m.pipelineTbl.View(), name) { + t.Errorf("%s: selected row %d (%q) is off screen", label, m.pipelineTbl.Cursor(), name) + } +} From 2e8f7034b0e94d342582e8b7d4c5192d9ac3da4f Mon Sep 17 00:00:00 2001 From: Hai Huang Date: Sun, 13 Sep 2026 15:32:20 -0400 Subject: [PATCH 3/3] test: Pin the live tail, where the window lags the cursor by one row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reported from a running session: scrolling to the very last message highlights it, then a second later "it deselects that last row, goes one line up, and the highlight disappears" — consistently, on a ~1s cadence. Same root cause as this branch already fixes, and the measurement says so precisely: the cursor never moves. Against the pre-fix files, with an event arriving each tick, tick 1: selected row 40 is off screen; the view shows rows 29..39 tick 2: selected row 41 is off screen; the view shows rows 30..40 Auto-follow puts the cursor on the new last row every time and the index assertion never fires — what is wrong is that the rendered window ends one row short of it. The selection is on row 40 while the screen shows up to 39, which is indistinguishable from a deselection that scrolled up a line. The one-second cadence is the poll's rebuild, which is when SetCursor re-windows the rows. Kept as its own test because the existing auto-follow test rebuilds WITHOUT adding events, so it never exercised the tail growing under the cursor — the shape every live session is in. Assisted-By: Claude (Anthropic AI) Signed-off-by: Hai Huang --- authbridge/cmd/abctl/tui/table_cursor_test.go | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/authbridge/cmd/abctl/tui/table_cursor_test.go b/authbridge/cmd/abctl/tui/table_cursor_test.go index 2ae1d0246..b999a8b31 100644 --- a/authbridge/cmd/abctl/tui/table_cursor_test.go +++ b/authbridge/cmd/abctl/tui/table_cursor_test.go @@ -515,3 +515,39 @@ func assertPipelineSelectionVisible(t *testing.T, m *model, label string) { t.Errorf("%s: selected row %d (%q) is off screen", label, m.pipelineTbl.Cursor(), name) } } + +// TestEventsTable_TailWithArrivingEventsKeepsSelectionVisible is the live-session shape: +// parked on the last row while the pane keeps rebuilding as events arrive. +// +// Reported as "it highlights the last row, then a second later deselects it, goes one line +// up, and the highlight disappears". The cursor does NOT actually move — what moves is the +// rendered window, by exactly one row, which takes the selected row off the bottom edge. So +// the index stays right while the screen looks deselected, and both halves are asserted here. +func TestEventsTable_TailWithArrivingEventsKeepsSelectionVisible(t *testing.T) { + m := cursorModel(t, 40) + for i := 0; i < 40; i++ { // arrive at the tail the way an operator does + m.eventsTbl, _ = m.eventsTbl.Update(tea.KeyMsg{Type: tea.KeyDown}) + } + assertSelectionVisible(t, m.eventsTbl, "at the tail by arrow key") + + // Each tick: one more event, then a rebuild. Auto-follow must keep the selection on + // the new last row AND on screen. + for tick := 1; tick <= 4; tick++ { + next := len(m.events["s"]) + m.events["s"] = append(m.events["s"], pipeline.SessionEvent{ + Direction: pipeline.Outbound, Phase: pipeline.SessionRequest, + Host: hostToken(next), + Inference: &pipeline.InferenceExtension{Model: "m"}, + }) + m.rebuildEventsTable() + label := fmt.Sprintf("tick %d", tick) + if got, want := m.eventsTbl.Cursor(), len(m.eventsTbl.Rows())-1; got != want { + t.Errorf("%s: cursor = %d, want the new last row %d", label, got, want) + } + assertSelectionVisible(t, m.eventsTbl, label) + } + + // And a tick that adds nothing still must not drop the highlight. + m.rebuildEventsTable() + assertSelectionVisible(t, m.eventsTbl, "idle tick at the tail") +}