Add endless-scrolling TUI table for interactive list commands#4729
Add endless-scrolling TUI table for interactive list commands#4729simonfaltum wants to merge 46 commits intomainfrom
Conversation
|
Commit: be88c2e |
53b85d2 to
64f2b98
Compare
64f2b98 to
ef8c413
Compare
Co-authored-by: Isaac
1bf0772 to
e672906
Compare
shreyas-goenka
left a comment
There was a problem hiding this comment.
Note: This review was posted by Claude (AI assistant). Shreyas will do a separate, more thorough review pass.
Priority: HIGH — Performance and design concerns in TUI table infrastructure
MAJOR: O(n) re-render on every keystroke
The search/filter functionality appears to re-render the entire table on each keystroke. For large result sets, this will cause noticeable lag. Consider debouncing the search input or using incremental filtering.
MAJOR: Partially-consumed iterator after search
When a user searches/filters, the iterator may have already been partially consumed. If the search is cleared, previously consumed items that didn't match won't be re-fetched. This could lead to confusing behavior where clearing a search shows fewer results than expected.
MEDIUM: Entire feature unreachable until follow-up PRs
This is 1535 lines of new code that is unreachable until the override PRs (#4731, #4732) land. Consider whether the infrastructure could be reviewed more effectively alongside at least one consumer.
What looks good
- Clean separation of concerns between model, view, and update
- Good use of Bubble Tea framework patterns
- Table column configuration is flexible and well-designed
8fa51c9 to
d7a7104
Compare
Search input now triggers server-side filtering automatically after the user stops typing for 200ms, instead of waiting for Enter. This prevents redundant API calls on each keystroke while keeping the text input responsive. Enter still executes search immediately, bypassing the debounce. Uses Bubble Tea's tick-based message pattern with a sequence counter to discard stale debounce ticks when the user types additional characters before the delay expires.
Fix four issues in the paginated TUI: 1. Entering search mode now sets loading=true to prevent maybeFetch from starting new fetches against the shared iterator while in search mode. In-flight fetches are discarded via the generation check. 2. executeSearch sets loading=true (was false) to prevent overlapping fetch commands when a quick scroll triggers maybeFetch before the first search fetch returns. 3. Pressing esc to close search now restores savedRows, savedIter, and savedExhaust (same as clearing the query via enter with empty input). 4. RenderIterator now checks the final model for application-level errors via the new FinalModel interface, since tea.Program.Run() only returns framework errors.
Approval status: pending
|
Init() returns a tea.Cmd that fetches rows in a goroutine but cannot modify the model (Bubbletea's Init() only returns a Cmd, not a Model). Without loading=true in the constructor, a keypress arriving before the first rowsFetchedMsg would pass maybeFetch's guard and fire a second concurrent fetch on the same non-thread-safe iterator. Co-authored-by: Isaac
There was a problem hiding this comment.
Hard to do a full review. I ran through the UX in the snapshot build and ran a separate review (see below)
How we do the plumbing is most important to me because this adds a second mode.
Review finding:
renderTableLines and paginatedModel.renderContent use "─" (box-drawing horizontal), but RenderStaticTable uses "-" (ASCII hyphen)
|
|
||
| cmdIO := cmdio.NewIO(ctx, f.output, cmd.InOrStdin(), cmd.OutOrStdout(), cmd.ErrOrStderr(), headerTemplate, template) | ||
| ctx = cmdio.InContext(ctx, cmdIO) | ||
| ctx = cmdio.WithCommand(ctx, cmd) |
There was a problem hiding this comment.
The existing mechanism to pass information from the autogen'd commands (or the overrides) into the command is via the annotations. This adds another mechanism.
I'm not biased to either but we should maintain a single system for associating static info with commands.
There was a problem hiding this comment.
Agreed. Removed WithCommand/CommandFromContext and now thread *cobra.Command as an explicit parameter to RenderIterator. The registry (RegisterConfig/GetConfig) is the only new mechanism, annotations still handle templates.
| func listOverride(listCmd *cobra.Command, _ *sql.ListAlertsRequest) { | ||
| listCmd.Annotations["template"] = cmdio.Heredoc(` | ||
| {{range .}}{{green "%s" .Id}} {{.DisplayName}} {{.State}} | ||
| {{end}}`) |
There was a problem hiding this comment.
Do we still need the template if we also use the tableview?
There was a problem hiding this comment.
Yes, the template is the fallback for non-interactive output (piped, --output text, non-TTY). The TUI only runs when SupportsTUI() is true. Added a comment to all overrides clarifying this.
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestListTableConfig(t *testing.T) { |
There was a problem hiding this comment.
Why test here and not other commands?
There was a problem hiding this comment.
Apps has nested pointer dereferences (ComputeStatus.State, ActiveDeployment.Status.State) that can nil-panic if the Extract functions aren't careful. The other overrides are single-field type assertions so there's less to go wrong.
| // AutoDetect creates a TableConfig by reflecting on the element type of the iterator. | ||
| // It picks up to maxAutoColumns top-level scalar fields. | ||
| // Returns nil if no suitable columns are found. | ||
| func AutoDetect[T any](iter listing.Iterator[T]) *TableConfig { |
| if m.cfg.Search != nil { | ||
| m.searching = true | ||
| m.searchInput = "" | ||
| m.viewport.Height-- |
There was a problem hiding this comment.
Why does this alter the viewport size?
There was a problem hiding this comment.
Shrinks by one row to make room for the search input bar at the bottom. Added a comment.
|
Btw, the initial "Fetching results..." and then the experience when waiting for more results in the table are different. I recommend putting the user immediately in the table view and clearly showing a marker that rows are being fetched. When using a terminal with >50 rows, the initial page loads but doesn't continue loading. I have to first navigate to the last row before it triggers fetching the next set of rows. Forward slash to enter search doesn't work for me. |
…mprove consistency - Remove useless TestNewPaginatedProgramSetsLoadingTrue (linter unusedwrite) - Make ctrl+c quit from search mode instead of canceling search - Add comments explaining viewport height adjustments for search bar - Remove unused AutoDetect function and its tests - Add comments explaining template+tableview coexistence in all overrides - Thread cobra.Command through RenderIterator parameter instead of context, removing WithCommand/CommandFromContext context mechanism - Unify separator character to box-drawing (U+2500) in RenderStaticTable Co-authored-by: Isaac
|
Good points. I'll look into showing the table immediately with a loading indicator instead of the separate "Fetching results..." screen. Will debug the forward slash issue too, might be terminal-specific. |
Co-authored-by: Isaac
…eRunE Co-authored-by: Isaac
Co-authored-by: Isaac
Co-authored-by: Isaac
Co-authored-by: Isaac
- Keep previous rows visible during search (show "Searching..." footer instead of clearing rows immediately) - Clone TableConfig before mutation in disableSearchIfFilterSet - Remove unused cmd parameter from RenderIterator and update all callers - Export and deduplicate SanitizeControlWhitespace - Fix inaccurate highlightSearch comment about case folding Co-authored-by: Isaac
Each cmd/workspace/*/overrides.go had repeated type assertions like `v.(jobs.BaseJob).Field` in every column Extract closure. Introduce a generic `tableview.Col[T]` helper (and `ColMax[T]` for width-capped columns) that hides the assertion, then convert all 18 override files to use it. Each column definition drops from a 3-line func literal with an explicit type assertion to a single-line generic call, making the shape of each table at a glance. Co-authored-by: Isaac
paginatedModel had 9 scattered fields handling server-side search (searching, searchLoading, searchInput, debounceSeq, hasSearchState, savedRows, savedIter, savedExhaust, limitReached was left as its own thing). Many of the recent fixes in this PR's history were races between these fields getting out of sync. Group them into a searchState struct with an optional *savedSearch sub-struct for the pre-search snapshot. restorePreSearchState and executeSearch become markedly simpler because "there is no saved state" is one nil check instead of four zeroed fields. Also consolidate a few trivial table tests (View, renderFooter, search input keys, fetch) into table-driven form. Co-authored-by: Isaac
ba1b90a to
e269ea3
Compare
When stdin and stdout are both TTYs and a TableConfig is registered, list commands now stream their first page of rows to stdout and prompt: [space] more [enter] all [q|esc] quit Piped output and `--output json` keep the existing behavior. Alternative approach to #4729: instead of a full Bubble Tea TUI with scrolling, search, and cursor navigation, this renders a plain text table page-by-page and reads a single key in raw terminal mode to drive paging. Same TableConfig / Col[T] infrastructure, minus the interactive browser. Wiring: - `libs/tableview/config.go`, `context.go`, `wrap.go`: TableConfig, RowIterator, Col[T]/ColMax[T] generic helpers, cobra PreRunE wiring. - `libs/tableview/common.go`: extracted `computeColumnWidths` and `renderTableToLines` so they can be shared between the existing static aitools browser and the new pager. - `libs/tableview/simple.go`: the pager itself (raw-mode key reader, fetch-on-demand, per-column MaxWidth truncation). - `libs/cmdio/render.go`: `RenderIterator` routes to the pager when `Capabilities.SupportsPager()` returns true. - `cmd/workspace/*/overrides.go`: TableConfigs for the 18 most common list commands. Test plan: - `go test ./libs/tableview/... ./libs/cmdio/... ./cmd/workspace/...` - `make checks` passes - `make lintfull` passes (0 issues) - Manual smoke in a TTY: space pages through, enter drains, q/esc exits. Co-authored-by: Isaac
|
Decided to go with another approach that solves pagination initially. Can always re-open to extend this functionality if needed. |
## Why List commands with a row template (`jobs list`, `clusters list`, `apps list`, `pipelines list`, `workspace list`, etc.) drain the full iterator and render every row at once. In workspaces with hundreds of resources, the output scrolls past before you can read it. An interactive terminal should get a chance to step through the output. This PR is an alternative to #4729 (the Bubble Tea TUI). It only solves pagination, nothing else. Smaller diff, no new public API, no override file changes. Interactive I/O is consolidated on bubbletea (already a direct dep via the spinner), so no new dependency is added. ## Changes **Before:** `databricks <resource> list` drained the full iterator through the existing template + tabwriter pipeline before showing anything. **Now:** when stdin, stdout, and stderr are all TTYs and the command has a row template, the CLI streams 50 rows at a time. While a batch is being fetched, the view shows a loading spinner: ``` ⣾ loading… ``` Between batches the prompt takes over: ``` [space] more [enter] all [q|esc] quit ``` `SPACE` fetches the next page. `ENTER` drains the rest (still interruptible by `q`/`esc`/`Ctrl+C` between pages, with the spinner staying up while fetching). `q`/`esc`/`Ctrl+C` stop immediately. Piped output and `--output json` keep the existing non-paged behavior. Rendering reuses the existing `Annotations["template"]` and `Annotations["headerTemplate"]`: colors, alignment, and row format come from the same code path as today's non-paged `jobs list`. No new `TableConfig`, no new `ColumnDef`, no changes to any override files. Files under `libs/cmdio/`: - `capabilities.go`: `SupportsPager()` (stdin + stdout + stderr all TTYs, not Git Bash). - `pager.go`: `pagerModel` (a `tea.Model`) that drives the paged render loop. Captures keys as `tea.KeyMsg`, emits rendered rows with `tea.Println` (which prints above the TUI area), and in `View()` shows either a spinner (while fetching) or the prompt (between pages). Same braille frames + green color as `cmdio.NewSpinner`. Only one fetch runs at a time; SPACE during an in-flight fetch is dropped, ENTER flips `drainAll` and lets the pending `batchMsg` chain the next fetch, so the iterator is never read from two goroutines. - `paged_template.go`: the template pager. Executes the header + row templates into an intermediate buffer per batch, splits by tab, computes visual column widths (stripping ANSI SGR so colors don't inflate), locks those widths from the first page, and pads every subsequent page to the same widths. Single-page output matches tabwriter's alignment; columns stay aligned across pages for longer lists. - `render.go`: `RenderIterator` routes to the template pager when the capability check passes and a row template is set. No `cmd/` changes. No new public API beyond `Capabilities.SupportsPager`. Notes on the bubbletea approach: - Using `tea.Model` + `tea.Println` means we don't call `term.MakeRaw` ourselves: tea enters and restores raw mode on its own, so the earlier `crlfWriter` workaround for the cleared `OPOST` flag is gone. - The header and row templates parse into independent `*template.Template` instances. Sharing one receiver causes the second `Parse` to overwrite the first, which made `apps list` render the header in place of every data row. - Empty iterators still flush their header: the first fetch returns `done=true` with header lines, and the pager prints them before quitting. - Tabwriter computes column widths per-flush and resets them. The pager does the padding itself with widths locked from the first batch, so a short final batch does not compress visually against wider pages above it. History: this consolidates #5016 (shared pager infrastructure) and drops an earlier JSON-output pager. JSON output is mostly consumed by scripts, so paging it adds complexity without a clear win. ## Test plan - [x] `go test ./libs/cmdio/...` passes. Coverage: state-machine unit tests on `pagerModel.Update` (init, batch handling, drain chaining, error propagation, every key path, in-flight-fetch serialization, spinner visibility) plus end-to-end tests via `tea.Program` for full drain, `--limit` integration, header-once, empty iterator, header + rows, cross-batch column stability, and content parity with the non-paged path for single-page lists. - [x] `make checks` passes. - [x] `make lint` passes (0 issues). - [ ] Manual smoke in a TTY: `apps list`, `jobs list`, `clusters list`, `workspace list /`. First page renders after a brief spinner, SPACE fetches next (spinner reappears), ENTER drains (spinner stays up), `Ctrl+C`/`esc`/`q` quit (and interrupt a drain). - [ ] Manual smoke with piped stdout and `--output json`: output unchanged from `main`.
Why
Large list commands are hard to use in a terminal today. They print the full result set before users can do anything, which is slow for big lists and makes it hard to browse results. Interactive terminals should get a faster, more navigable experience without changing scripted output.
Changes
Before: list commands drained the full paginated iterator and rendered everything through text templates or JSON.
Now: when the CLI is running interactively (TTY stdin + stdout + stderr, color enabled), list commands with a registered table configuration open a scrollable TUI table that loads more rows on demand. Piped output and
--output jsonkeep the existing behavior. Commands without an explicit table config continue to use template rendering.Implementation:
tableview.gointocommon.gotableviewconfig, registry, and wrapper typesexperimental/aitoolsto the shared static table rendererPost-review fixes
utf8.DecodeLastRuneInString)RunPaginatedsilently dropping model-level fetch errors (now checksFinalModel.Err()likerender.go)restorePreSearchStateto eliminate duplicated search-restore logictea.KeySpacein both paginated and non-paginated tables)loadingandsearchingconcerns:loadingis purely "fetch in-flight",searchingflag blocksmaybeFetchduring search input mode (fixes stuck pagination and overlapping fetch races)Simplification pass
tableview.Col[T]/ColMax[T]helpers so each column drops from a 3-line func literal with explicit type assertion to a single-line generic call. Converted all 18cmd/workspace/*/overrides.gofiles.searching,searchLoading,searchInput,debounceSeq,hasSearchState,savedRows,savedIter,savedExhaust) into onesearchStatestruct with an optional*savedSearchsub-struct for the pre-search snapshot.restorePreSearchStateandexecuteSearchshrink significantly; the state lifecycle becomes self-documenting.Test plan
go test ./libs/tableview/... ./libs/cmdio/... ./cmd/root/... ./experimental/aitools/cmd/...--output json: verify existing output is unchangedmake checkspassesmake lintfullpasses (0 issues)