fix: Keep the table highlight on screen when the cursor is placed, not moved - #981
Merged
Merged
Conversation
…t moved 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) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
|
Warning Review limit reachedNext included review available in 14 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… case costs 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) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
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) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
mrsabath
approved these changes
Sep 14, 2026
mrsabath
left a comment
mrsabath
left a comment
Contributor
There was a problem hiding this comment.
一针见血 (yī zhēn jiàn xiě — "one needle draws blood," i.e. straight to the heart of it). Excellent, self-contained fix, Hai.
I verified the root-cause analysis against charmbracelet/bubbles v1.0.0 (pinned in cmd/abctl/go.mod): SetCursor clamps and calls UpdateViewport but never reconciles viewport.YOffset, while GotoTop/MoveUp/MoveDown do — so setCursorVisible's GotoTop() + MoveDown(n) is exactly the relative-move mechanism the library itself uses for jumps (GotoBottom = MoveDown(len(rows))). The clamp matches SetCursor's own, so it's behavior-preserving on the index while fixing the offset.
The divider-nudge conversion from SetCursor(±1) to MoveUp/MoveDown(1) is index-equivalent including the end clamps (pinned by TestPipelineTable_DividerNudgeClampsAtTheEnds). Making the rebuildEventsTable restore unconditional correctly closes the rows-shrink case the old else if prevRow < len(rows) skipped.
Tests are a highlight: they assert on the rendered View() (what the operator sees) rather than the cursor index (which was always correct), and cover both sides of the height boundary, both shrink levers, resize, live-tail, and empty-table edge cases.
Areas reviewed: Go (TUI / bubbles table), tests, commit/PR conventions, security.
Commits: 3, all signed-off (DCO ✓).
CI status: all green.
github-project-automation
Bot
moved this from New/ToDo
to Done
in Rossoctl Issue Prioritization
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
In the events pane, scrolling to the bottom and pressing ↑ scrolls the rows one at a time
with no row highlighted anywhere.
Root cause
table.SetCursormoves the cursor without reconciling the viewport's scroll offset.UpdateViewportrenders a window of rows around the cursor —start = clamp(cursor−height, 0, cursor)— and the viewport then showsheightlines of thatwindow beginning at its own
YOffset, which onlyMoveUp/MoveDownmaintain. So withYOffset0 and a cursor at or past one screenful,startlands oncursor−heightand thecursor sits at window line
height— exactly one line past the last visible one. The row isrendered and the highlight is drawn on it, off the bottom edge.
MoveUpcannot recover from that state either: its cases arestart == 0,start < height,and
YOffset >= 1, and none match, soYOffsetstays 0 whilestartwalks up with thecursor. Hence the symptom — rows scroll under the arrow key, nothing is ever highlighted.
Measured on a 40-row session at height 11, driving the real table through the real key path:
MoveDown)G/ end (SetCursor(n-1))G, then ↑ ×6Same targets, two APIs — the boundary is exactly the table height:
Solution
setCursorVisible(newtui/table_cursor.go) expresses the jump as relative movement, theonly cursor API in bubbles that maintains the offset:
GotoTopto normalize it, then oneMoveDownofn— a distance, not a loop, so two O(height) re-renders however far the cursortravels. Its doc comment carries the mechanics above so the next person does not re-derive
them.
Every programmatic cursor placement now goes through it:
rebuildEventsTableauto-follow and position-restore. This is why the bug appeared evenwhen 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 now unconditional — the
old
else if prevRow < len(rows)skipped it when the rows shrank under the cursor (afilter typed,
hideInactivetoggled), leaving the cursor whereverSetRowshad clamped itwith an unreconciled offset.
goTop/goBottom(gandG) for all four tables; the empty-row guards fold intothe helper, which clamps.
which become
MoveUp/MoveDownof 1.PgUp/PgDnwere already relative moves and were never affected.Testing
tui/table_cursor_test.goasserts what an operator sees, not the cursor index — the indexwas always correct, it was the rendered window that excluded it. Each fixture row gets a
unique host, and the selected row's text must appear in the table's
View():TestSetCursorVisible_LandsOnScreen— targets on both sides of the height boundaryTestSetCursorVisible_ClampsAndSurvivesEmpty— out-of-range targets and an empty tableTestEventsTable_AutoFollowKeepsSelectionVisible— the reported sequence: rebuild, ↑,rebuild, ×6
TestEventsTable_RebuildMidListKeepsSelectionVisible— a new event while parked mid-listmust move neither the cursor nor the highlight off screen
TestGoBottom_KeepsSelectionVisible—G, then ↑ ×3, thengTestEventsTable_ResizeKeepsSelectionVisible—SetHeightre-windows rows the same wayAgainst a stub that kept the old behaviour these fail 31 times, with the boundary exactly
at the table height.
go test ./...incmd/abctlpasses exceptTestRunExec_BeforeFirstStartRunsAndSaysWhatIsLost("the child inherited a bundle path that does not exist"), which fails identically on an
unmodified
upstream/main— pre-existing and unrelated.go vetclean;golangci-lint --new-from-rev=upstream/main0 issues;gofmtclean.Notes
mainand touch differentregions of
events_pane.go, so they merge in either order.SetCursoris a reasonable-looking API with no hintthat it leaves the offset stale. Worth an issue against
charmbracelet/bubbles— the helperis the local guard either way.
Assisted-By: Claude (Anthropic AI) noreply@anthropic.com