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
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)