Skip to content

perf(tui): only render visible session rows in /sessions dialog#2830

Merged
dgageot merged 2 commits into
docker:mainfrom
dgageot:board/b0775ed33c5e1fa1
May 20, 2026
Merged

perf(tui): only render visible session rows in /sessions dialog#2830
dgageot merged 2 commits into
docker:mainfrom
dgageot:board/b0775ed33c5e1fa1

Conversation

@dgageot
Copy link
Copy Markdown
Member

@dgageot dgageot commented May 20, 2026

The /sessions dialog was visibly slow when scrolling or moving the
selection with arrow keys, especially with a large session history.

Every render of sessionBrowserDialog.View() was pre-rendering all
filtered session rows into styled lines (two lipgloss.Style.Render
calls plus string concatenation per row), even though the scrollview
only displays a small visible window. With hundreds of sessions, every
keystroke and scroll tick incurred hundreds of style renders before
the scrollview sliced the result down to the viewport.

The scrollview already exposes ViewWithLines(visible) for the
virtualised case (used by picker.go and working_dir_picker.go).
This change uses it to render only [offset, offset+visibleHeight),
making render cost O(visible rows) (~10–20) instead of O(total
sessions).

filterSessions also now calls SetContent(nil, len(filtered)) so
the scrollview's totalHeight is always in sync — this fixes a
latent bug where EnsureLineVisible (called on Up/Down keypress)
could clamp against a stale totalHeight between a filter change and
the next View().

@dgageot dgageot requested a review from a team as a code owner May 20, 2026 09:12
Copy link
Copy Markdown

@docker-agent docker-agent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assessment: 🟢 APPROVE

Virtualized session-row rendering — the O(visible) rendering refactor is correct and well-structured.

What was checked:

  • Negative capacity in make([]string, 0, end-offset): Safe. After SetContent(nil, total) + SetScrollOffset(ScrollOffset()), the offset is clamped to [0, max(0, total−height)]. Since total > 0 in the else-branch and end = min(offset+visibleLines, total), we always have end ≥ offset. No panic path.
  • Selected-row highlight index: i == d.selected uses the absolute index into d.filtered on both sides — correct for virtual windows.
  • syncScrollbar() desync: ViewWithLines internally calls syncScrollbar() after the offset variable is captured. Both SetScrollOffset (called in View()) and syncScrollbar use the same clamping formula max(0, total−height) with the same already-updated totalHeight, so the read-back value is stable. No observable desync in the single-threaded TUI render path.
  • nil lines stored in scrollview: ViewWithLines never reads m.lines; only the old View() path does. Since ViewWithLines is always called in the new path, the nil value is harmless.
  • filterSessions() double SetContent: The call in filterSessions() primes totalHeight for EnsureLineVisible; the call in View() is idempotent given the same len(d.filtered). The scroll-reset-to-0 in filterSessions() is consistent with pre-existing behaviour.
  • regionWidth variable: Still used at line 358 (NewContent(regionWidth)). No dead-variable compile error.

The change cleanly delivers the stated O(visible) goal with no regressions found.

@dgageot dgageot merged commit 1466528 into docker:main May 20, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants