From cbd549c3df78e7b251bdf074b9f13bd91da37cf7 Mon Sep 17 00:00:00 2001 From: simon Date: Tue, 21 Apr 2026 11:56:36 +0200 Subject: [PATCH 1/9] Add interactive pager for list commands with a row template When stdin, stdout, and stderr are all TTYs and the command has a row template (jobs list, clusters list, apps list, pipelines list, etc.), the CLI streams 50 rows at a time and prompts on stderr: [space] more [enter] all [q|esc] quit SPACE fetches the next page. ENTER drains the rest (interruptible by q/esc/Ctrl+C between pages). q/esc/Ctrl+C quit immediately. Piped output and --output json keep the existing non-paged behavior. Rendering reuses the existing template + headerTemplate annotations (same colors, same alignment as today). Column widths are locked from the first page so they stay stable across batches. Co-authored-by: Isaac --- NEXT_CHANGELOG.md | 1 + NOTICE | 4 + go.mod | 1 + libs/cmdio/capabilities.go | 10 ++ libs/cmdio/paged_template.go | 228 +++++++++++++++++++++++++ libs/cmdio/paged_template_test.go | 274 ++++++++++++++++++++++++++++++ libs/cmdio/pager.go | 133 +++++++++++++++ libs/cmdio/pager_test.go | 72 ++++++++ libs/cmdio/render.go | 9 + 9 files changed, 732 insertions(+) create mode 100644 libs/cmdio/paged_template.go create mode 100644 libs/cmdio/paged_template_test.go create mode 100644 libs/cmdio/pager.go create mode 100644 libs/cmdio/pager_test.go diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index ede82f77791..b7acb6e644e 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -7,6 +7,7 @@ ### CLI * Added `--limit` flag to all paginated list commands for client-side result capping ([#4984](https://github.com/databricks/cli/pull/4984)). +* Added interactive pagination for list commands that have a row template (jobs, clusters, apps, pipelines, etc.). When stdin, stdout, and stderr are all TTYs, `databricks list` now streams 50 rows at a time and prompts `[space] more [enter] all [q|esc] quit` on stderr. ENTER can be interrupted by `q`/`esc`/`Ctrl+C` between pages. Colors and alignment match the existing non-paged output; column widths stay stable across pages. Piped output and `--output json` are unchanged. * Accept `yes` in addition to `y` for confirmation prompts, and show `[y/N]` to indicate that no is the default. ### Bundles diff --git a/NOTICE b/NOTICE index 883c24ab787..baad4b05252 100644 --- a/NOTICE +++ b/NOTICE @@ -115,6 +115,10 @@ golang.org/x/sys - https://github.com/golang/sys Copyright 2009 The Go Authors. License - https://github.com/golang/sys/blob/master/LICENSE +golang.org/x/term - https://github.com/golang/term +Copyright 2009 The Go Authors. +License - https://github.com/golang/term/blob/master/LICENSE + golang.org/x/text - https://github.com/golang/text Copyright 2009 The Go Authors. License - https://github.com/golang/text/blob/master/LICENSE diff --git a/go.mod b/go.mod index bdcacec405f..4cd20d588ce 100644 --- a/go.mod +++ b/go.mod @@ -40,6 +40,7 @@ require ( golang.org/x/oauth2 v0.36.0 // BSD-3-Clause golang.org/x/sync v0.20.0 // BSD-3-Clause golang.org/x/sys v0.43.0 // BSD-3-Clause + golang.org/x/term v0.41.0 // BSD-3-Clause golang.org/x/text v0.35.0 // BSD-3-Clause gopkg.in/ini.v1 v1.67.1 // Apache-2.0 ) diff --git a/libs/cmdio/capabilities.go b/libs/cmdio/capabilities.go index 455acebc772..12b46f61505 100644 --- a/libs/cmdio/capabilities.go +++ b/libs/cmdio/capabilities.go @@ -48,6 +48,16 @@ func (c Capabilities) SupportsColor(w io.Writer) bool { return isTTY(w) && c.color } +// SupportsPager returns true when we can run an interactive pager between +// batches of output: stdin, stdout, and stderr must all be TTYs. Stdin +// carries the user's keystrokes, stdout receives the paged content, and +// stderr carries the "[space] more / [enter] all" prompt — all three +// must be visible for the interaction to make sense. Git Bash is +// excluded because raw-mode stdin reads are unreliable there. +func (c Capabilities) SupportsPager() bool { + return c.stdinIsTTY && c.stdoutIsTTY && c.stderrIsTTY && !c.isGitBash +} + // detectGitBash returns true if running in Git Bash on Windows (has broken promptui support). // We do not allow prompting in Git Bash on Windows. // Likely due to fact that Git Bash does not correctly support ANSI escape sequences, diff --git a/libs/cmdio/paged_template.go b/libs/cmdio/paged_template.go new file mode 100644 index 00000000000..15ff1f9311f --- /dev/null +++ b/libs/cmdio/paged_template.go @@ -0,0 +1,228 @@ +package cmdio + +import ( + "bytes" + "context" + "fmt" + "io" + "os" + "regexp" + "strings" + "text/template" + "unicode/utf8" + + "github.com/databricks/databricks-sdk-go/listing" +) + +// pagerColumnSeparator is the inter-column spacing used when emitting paged +// template output. Matches text/tabwriter.NewWriter(..., 2, ' ', ...) in +// the non-paged path, so single-batch output is visually indistinguishable +// from what renderUsingTemplate produces. +const pagerColumnSeparator = " " + +// ansiCSIPattern matches ANSI SGR escape sequences so we can compute the +// on-screen width of cells that contain colored output. We do not attempt +// to cover every ANSI escape — just the SGR color/style ones (CSI ... m) +// emitted by github.com/fatih/color, which is all our templates use today. +var ansiCSIPattern = regexp.MustCompile("\x1b\\[[0-9;]*m") + +// renderIteratorPagedTemplate streams an iterator through the existing +// template-based renderer one page at a time, prompting the user between +// pages on stderr. The rendering is intentionally identical to the non- +// paged path — same templates, same colors — only the flush cadence, +// the user-facing prompt, and column-width stability across batches +// differ. +// +// SPACE advances by one page; ENTER drains the remaining iterator (still +// interruptible by q/esc/Ctrl+C); q/esc/Ctrl+C stop immediately. +func renderIteratorPagedTemplate[T any]( + ctx context.Context, + iter listing.Iterator[T], + out io.Writer, + headerTemplate, tmpl string, +) error { + keys, restore, err := startRawStdinKeyReader(ctx) + if err != nil { + return err + } + defer restore() + return renderIteratorPagedTemplateCore( + ctx, + iter, + crlfWriter{w: out}, + crlfWriter{w: os.Stderr}, + keys, + headerTemplate, + tmpl, + pagerPageSize, + ) +} + +// renderIteratorPagedTemplateCore is the testable core of +// renderIteratorPagedTemplate: it assumes the caller has already set up +// raw stdin (or any key source) and delivered a channel of keystrokes. +// It never touches os.Stdin directly. +// +// Unlike renderUsingTemplate (the non-paged path) we do not rely on +// text/tabwriter to align columns. Tabwriter computes column widths +// per-flush and resets on Flush(), which produces a jarring +// width-shift when a short final batch follows a wider first batch. +// Here we render each page's template output into an intermediate +// buffer, split it into cells by tab, lock visual column widths from +// the first page, and pad every subsequent page to the same widths. +// The output is visually indistinguishable from tabwriter for single- +// batch lists and stays aligned across batches for longer ones. +func renderIteratorPagedTemplateCore[T any]( + ctx context.Context, + iter listing.Iterator[T], + out io.Writer, + prompts io.Writer, + keys <-chan byte, + headerTemplate, tmpl string, + pageSize int, +) error { + // Use two independent templates so parsing the row template doesn't + // overwrite the header template's parsed body (they would if they + // shared the same *template.Template instance — Parse replaces the + // body in place and returns the receiver). + headerT, err := template.New("header").Funcs(renderFuncMap).Parse(headerTemplate) + if err != nil { + return err + } + rowT, err := template.New("row").Funcs(renderFuncMap).Parse(tmpl) + if err != nil { + return err + } + + limit := limitFromContext(ctx) + drainAll := false + buf := make([]any, 0, pageSize) + total := 0 + + var lockedWidths []int + firstBatchDone := false + + flushPage := func() error { + // Nothing to emit after the first batch is out the door and the + // buffer is empty — we've already written the header. + if firstBatchDone && len(buf) == 0 { + return nil + } + + var rendered bytes.Buffer + if !firstBatchDone && headerTemplate != "" { + if err := headerT.Execute(&rendered, nil); err != nil { + return err + } + rendered.WriteByte('\n') + } + if len(buf) > 0 { + if err := rowT.Execute(&rendered, buf); err != nil { + return err + } + buf = buf[:0] + } + firstBatchDone = true + + text := strings.TrimRight(rendered.String(), "\n") + if text == "" { + return nil + } + rows := strings.Split(text, "\n") + + // Lock column widths from the first batch (header + first page). + // Every subsequent batch pads to these widths so columns do not + // shift between pages. + if lockedWidths == nil { + for _, row := range rows { + for i, cell := range strings.Split(row, "\t") { + if i >= len(lockedWidths) { + lockedWidths = append(lockedWidths, 0) + } + if w := visualWidth(cell); w > lockedWidths[i] { + lockedWidths[i] = w + } + } + } + } + + for _, row := range rows { + line := padRow(strings.Split(row, "\t"), lockedWidths) + if _, err := io.WriteString(out, line+"\n"); err != nil { + return err + } + } + return nil + } + + for iter.HasNext(ctx) { + if limit > 0 && total >= limit { + break + } + n, err := iter.Next(ctx) + if err != nil { + return err + } + buf = append(buf, n) + total++ + + if len(buf) < pageSize { + continue + } + if err := flushPage(); err != nil { + return err + } + if drainAll { + if pagerShouldQuit(keys) { + return nil + } + continue + } + // Show the prompt and wait for a key. + fmt.Fprint(prompts, pagerPromptText) + k, ok := pagerNextKey(ctx, keys) + fmt.Fprint(prompts, pagerClearLine) + if !ok { + return nil + } + switch k { + case ' ': + // Continue to the next page. + case '\r', '\n': + drainAll = true + case 'q', 'Q', pagerKeyEscape, pagerKeyCtrlC: + return nil + } + } + return flushPage() +} + +// visualWidth returns the number of runes a string would occupy on-screen +// if ANSI SGR escape sequences are respected as zero-width (which the +// terminal does). This matches what the user sees and lets us pad colored +// cells to consistent visual widths. +func visualWidth(s string) int { + return utf8.RuneCountInString(ansiCSIPattern.ReplaceAllString(s, "")) +} + +// padRow joins the given cells with pagerColumnSeparator, padding every +// cell except the last to widths[i] visual runes. Cells wider than +// widths[i] are emitted as-is — the extra content pushes subsequent +// columns right for that row only, which is the same behavior tabwriter +// would give and the same behavior the non-paged renderer has today. +func padRow(cells []string, widths []int) string { + var b strings.Builder + for i, cell := range cells { + if i > 0 { + b.WriteString(pagerColumnSeparator) + } + b.WriteString(cell) + if i < len(cells)-1 && i < len(widths) { + pad := widths[i] - visualWidth(cell) + if pad > 0 { + b.WriteString(strings.Repeat(" ", pad)) + } + } + } + return b.String() +} diff --git a/libs/cmdio/paged_template_test.go b/libs/cmdio/paged_template_test.go new file mode 100644 index 00000000000..1b61135108d --- /dev/null +++ b/libs/cmdio/paged_template_test.go @@ -0,0 +1,274 @@ +package cmdio + +import ( + "bytes" + "context" + "errors" + "fmt" + "strings" + "testing" + + "github.com/databricks/cli/libs/flags" + "github.com/databricks/databricks-sdk-go/listing" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type numberIterator struct { + n int + pos int + err error +} + +func (it *numberIterator) HasNext(_ context.Context) bool { + return it.pos < it.n +} + +func (it *numberIterator) Next(_ context.Context) (int, error) { + if it.err != nil { + return 0, it.err + } + it.pos++ + return it.pos, nil +} + +func makeTemplateKeys(bytes ...byte) <-chan byte { + ch := make(chan byte, len(bytes)) + for _, b := range bytes { + ch <- b + } + close(ch) + return ch +} + +func runPagedTemplate(t *testing.T, n, pageSize int, keys []byte) string { + t.Helper() + var out, prompts bytes.Buffer + iter := listing.Iterator[int](&numberIterator{n: n}) + err := renderIteratorPagedTemplateCore( + t.Context(), + iter, + &out, + &prompts, + makeTemplateKeys(keys...), + "", + "{{range .}}{{.}}\n{{end}}", + pageSize, + ) + require.NoError(t, err) + return out.String() +} + +func TestPagedTemplateDrainsWhenFirstPageExhausts(t *testing.T) { + out := runPagedTemplate(t, 3, 10, nil) + require.Equal(t, "1\n2\n3\n", out) +} + +func TestPagedTemplateSpaceFetchesOneMorePage(t *testing.T) { + out := runPagedTemplate(t, 7, 3, []byte{' '}) + lines := strings.Split(strings.TrimRight(out, "\n"), "\n") + assert.Len(t, lines, 6) +} + +func TestPagedTemplateEnterDrainsIterator(t *testing.T) { + out := runPagedTemplate(t, 25, 5, []byte{'\r'}) + lines := strings.Split(strings.TrimRight(out, "\n"), "\n") + assert.Len(t, lines, 25) +} + +func TestPagedTemplateQuitKeyExits(t *testing.T) { + out := runPagedTemplate(t, 100, 5, []byte{'q'}) + lines := strings.Split(strings.TrimRight(out, "\n"), "\n") + assert.Len(t, lines, 5) +} + +func TestPagedTemplateEscExits(t *testing.T) { + out := runPagedTemplate(t, 100, 5, []byte{pagerKeyEscape}) + lines := strings.Split(strings.TrimRight(out, "\n"), "\n") + assert.Len(t, lines, 5) +} + +func TestPagedTemplateCtrlCExits(t *testing.T) { + out := runPagedTemplate(t, 100, 5, []byte{pagerKeyCtrlC}) + lines := strings.Split(strings.TrimRight(out, "\n"), "\n") + assert.Len(t, lines, 5) +} + +func TestPagedTemplateEnterInterruptibleByCtrlC(t *testing.T) { + out := runPagedTemplate(t, 20, 5, []byte{'\r', pagerKeyCtrlC}) + lines := strings.Split(strings.TrimRight(out, "\n"), "\n") + assert.Len(t, lines, 10) +} + +func TestPagedTemplateRespectsLimit(t *testing.T) { + var out, prompts bytes.Buffer + iter := listing.Iterator[int](&numberIterator{n: 200}) + ctx := WithLimit(t.Context(), 7) + err := renderIteratorPagedTemplateCore( + ctx, + iter, + &out, + &prompts, + makeTemplateKeys('\r'), + "", + "{{range .}}{{.}}\n{{end}}", + 5, + ) + require.NoError(t, err) + lines := strings.Split(strings.TrimRight(out.String(), "\n"), "\n") + assert.Len(t, lines, 7) +} + +func TestPagedTemplatePrintsHeaderOnce(t *testing.T) { + var out, prompts bytes.Buffer + iter := listing.Iterator[int](&numberIterator{n: 8}) + err := renderIteratorPagedTemplateCore( + t.Context(), + iter, + &out, + &prompts, + makeTemplateKeys(' '), + `ID`, + "{{range .}}{{.}}\n{{end}}", + 3, + ) + require.NoError(t, err) + assert.Equal(t, 1, strings.Count(out.String(), "ID\n")) + assert.True(t, strings.HasPrefix(out.String(), "ID\n")) +} + +func TestPagedTemplatePropagatesFetchError(t *testing.T) { + var out, prompts bytes.Buffer + iter := listing.Iterator[int](&numberIterator{n: 100, err: errors.New("boom")}) + err := renderIteratorPagedTemplateCore( + t.Context(), + iter, + &out, + &prompts, + makeTemplateKeys(), + "", + "{{range .}}{{.}}\n{{end}}", + 5, + ) + require.Error(t, err) + assert.Contains(t, err.Error(), "boom") +} + +func TestPagedTemplateRendersHeaderAndRowsCorrectly(t *testing.T) { + // Regression guard against the header/row template cross-pollution + // bug: if the two templates share a *template.Template receiver, + // every flush re-emits the header text where rows should be. + var out, prompts bytes.Buffer + iter := listing.Iterator[int](&numberIterator{n: 6}) + err := renderIteratorPagedTemplateCore( + t.Context(), + iter, + &out, + &prompts, + makeTemplateKeys(), + `ID Name`, + "{{range .}}{{.}} item-{{.}}\n{{end}}", + 100, + ) + require.NoError(t, err) + got := out.String() + assert.Contains(t, got, "ID") + assert.Contains(t, got, "Name") + for i := 1; i <= 6; i++ { + assert.Contains(t, got, fmt.Sprintf("item-%d", i)) + } + assert.Equal(t, 1, strings.Count(got, "ID")) +} + +func TestPagedTemplateEmptyIteratorStillFlushesHeader(t *testing.T) { + var out, prompts bytes.Buffer + iter := listing.Iterator[int](&numberIterator{n: 0}) + err := renderIteratorPagedTemplateCore( + t.Context(), + iter, + &out, + &prompts, + makeTemplateKeys(), + `ID Name`, + "{{range .}}{{.}}\n{{end}}", + 10, + ) + require.NoError(t, err) + assert.Contains(t, out.String(), "ID") + assert.Contains(t, out.String(), "Name") +} + +// slicedIterator is a tiny iterator implementation for tests that prefer +// to hand over strongly-typed row structs. +type slicedIterator[T any] struct { + data []T + pos int +} + +func (it *slicedIterator[T]) HasNext(_ context.Context) bool { return it.pos < len(it.data) } +func (it *slicedIterator[T]) Next(_ context.Context) (T, error) { + v := it.data[it.pos] + it.pos++ + return v, nil +} + +func TestPagedTemplateColumnWidthsStableAcrossBatches(t *testing.T) { + type row struct { + Name string + Tag string + } + rows := []row{ + {"wide-name-that-sets-the-width", "a"}, + {"short", "b"}, + } + iter := &slicedIterator[row]{data: rows} + var out, prompts bytes.Buffer + err := renderIteratorPagedTemplateCore( + t.Context(), + iter, + &out, + &prompts, + makeTemplateKeys(' '), + "Name Tag", + "{{range .}}{{.Name}} {{.Tag}}\n{{end}}", + 1, + ) + require.NoError(t, err) + got := out.String() + lines := strings.Split(strings.TrimRight(got, "\n"), "\n") + require.Len(t, lines, 3) + + // Column 1 must start at the same visible offset on every line. + const wantColTwoOffset = 31 + for i, line := range lines { + idx := strings.LastIndex(line, " ") + 1 + assert.Equal(t, wantColTwoOffset, idx, "line %d: col 1 expected at offset %d, got %d (line=%q)", i, wantColTwoOffset, idx, line) + } +} + +// TestPagedTemplateMatchesNonPagedForSmallList asserts that single-batch +// output is byte-identical to the non-paged template renderer, so users +// who never hit a second page see the exact same thing they used to. +func TestPagedTemplateMatchesNonPagedForSmallList(t *testing.T) { + const rows = 5 + tmpl := `{{range .}}{{green "%d" .}} {{.}} +{{end}}` + var expected bytes.Buffer + refIter := listing.Iterator[int](&numberIterator{n: rows}) + require.NoError(t, renderWithTemplate(t.Context(), newIteratorRenderer(refIter), flags.OutputText, &expected, "", tmpl)) + + var actual, prompts bytes.Buffer + pagedIter := listing.Iterator[int](&numberIterator{n: rows}) + require.NoError(t, renderIteratorPagedTemplateCore( + t.Context(), + pagedIter, + &actual, + &prompts, + makeTemplateKeys(), + "", + tmpl, + 100, + )) + assert.Equal(t, expected.String(), actual.String()) + assert.NotEmpty(t, expected.String()) +} diff --git a/libs/cmdio/pager.go b/libs/cmdio/pager.go new file mode 100644 index 00000000000..400aee8c6a8 --- /dev/null +++ b/libs/cmdio/pager.go @@ -0,0 +1,133 @@ +package cmdio + +import ( + "context" + "fmt" + "io" + "os" + + "golang.org/x/term" +) + +// pagerPageSize is the number of items rendered between prompts. +const pagerPageSize = 50 + +// pagerPromptText is shown on stderr between pages. +const pagerPromptText = "[space] more [enter] all [q|esc] quit" + +// pagerClearLine is the ANSI sequence to return to column 0 and erase the +// current line. Used to remove the prompt before writing the next page. +const pagerClearLine = "\r\x1b[K" + +// Key codes we care about when reading single bytes from stdin in raw mode. +const ( + pagerKeyEscape = 0x1b + pagerKeyCtrlC = 0x03 +) + +// startRawStdinKeyReader puts stdin into raw mode and spawns a goroutine +// that publishes each keystroke as a byte on the returned channel. The +// returned restore function must be called (typically via defer) to put +// the terminal back in its original mode; it is safe to call even if +// MakeRaw failed (it's a no-op). +// +// The goroutine exits when stdin returns an error (e.g. EOF on process +// shutdown) or when ctx is cancelled, at which point the channel is +// closed. Leaking the goroutine before that is acceptable because the +// pager is only invoked by short-lived CLI commands: the process exits +// shortly after the caller returns. +// +// Note: term.MakeRaw also clears the TTY's OPOST flag on most Unixes. +// With OPOST off, outbound '\n' is not translated to '\r\n', so callers +// that write newlines while raw mode is active should wrap their output +// stream in crlfWriter to avoid staircase output. +func startRawStdinKeyReader(ctx context.Context) (<-chan byte, func(), error) { + fd := int(os.Stdin.Fd()) + oldState, err := term.MakeRaw(fd) + if err != nil { + return nil, func() {}, fmt.Errorf("failed to enter raw mode on stdin: %w", err) + } + restore := func() { _ = term.Restore(fd, oldState) } + + ch := make(chan byte, 16) + go func() { + defer close(ch) + buf := make([]byte, 1) + for { + n, err := os.Stdin.Read(buf) + if err != nil || n == 0 { + return + } + select { + case ch <- buf[0]: + case <-ctx.Done(): + return + } + } + }() + return ch, restore, nil +} + +// pagerNextKey blocks until a key arrives, the key channel closes, or the +// context is cancelled. Returns ok=false on close or cancellation. +func pagerNextKey(ctx context.Context, keys <-chan byte) (byte, bool) { + select { + case k, ok := <-keys: + return k, ok + case <-ctx.Done(): + return 0, false + } +} + +// pagerShouldQuit drains any buffered keys non-blockingly and returns true +// if one of q/Q/esc/Ctrl+C was pressed. Other keys are consumed and +// dropped. A closed channel means stdin ran out (EOF) — that's not a +// quit signal; the caller should keep draining. +func pagerShouldQuit(keys <-chan byte) bool { + for { + select { + case k, ok := <-keys: + if !ok { + return false + } + if k == 'q' || k == 'Q' || k == pagerKeyEscape || k == pagerKeyCtrlC { + return true + } + default: + return false + } + } +} + +// crlfWriter translates outbound '\n' bytes into '\r\n' so output written +// while the TTY is in raw mode (OPOST cleared) still starts at column 0. +// io.Writer semantics are preserved: the returned byte count is the +// number of bytes from p that were consumed, not the (possibly larger) +// number of bytes written to the underlying writer. +type crlfWriter struct { + w io.Writer +} + +func (c crlfWriter) Write(p []byte) (int, error) { + start := 0 + for i, b := range p { + if b != '\n' { + continue + } + if i > start { + if _, err := c.w.Write(p[start:i]); err != nil { + return start, err + } + } + if _, err := c.w.Write([]byte{'\r', '\n'}); err != nil { + return i, err + } + start = i + 1 + } + if start < len(p) { + if _, err := c.w.Write(p[start:]); err != nil { + return start, err + } + } + return len(p), nil +} diff --git a/libs/cmdio/pager_test.go b/libs/cmdio/pager_test.go new file mode 100644 index 00000000000..d2e6f00463a --- /dev/null +++ b/libs/cmdio/pager_test.go @@ -0,0 +1,72 @@ +package cmdio + +import ( + "bytes" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestCrlfWriterTranslatesNewlines(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + {"no newlines", "abc", "abc"}, + {"single newline mid", "a\nb", "a\r\nb"}, + {"newline at end", "abc\n", "abc\r\n"}, + {"newline at start", "\nabc", "\r\nabc"}, + {"consecutive newlines", "\n\n", "\r\n\r\n"}, + {"multiple lines", "one\ntwo\nthree\n", "one\r\ntwo\r\nthree\r\n"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var buf bytes.Buffer + w := crlfWriter{w: &buf} + n, err := w.Write([]byte(tt.in)) + require.NoError(t, err) + assert.Equal(t, len(tt.in), n, "Write must return the input byte count") + assert.Equal(t, tt.want, buf.String()) + }) + } +} + +func TestPagerShouldQuitDrainsNonQuitKeys(t *testing.T) { + ch := make(chan byte, 4) + ch <- ' ' + ch <- 'x' + ch <- 'y' + assert.False(t, pagerShouldQuit(ch), "non-quit keys must return false") + assert.Empty(t, ch, "non-quit keys must be drained from the channel") +} + +func TestPagerShouldQuitReturnsTrueForQuitKeys(t *testing.T) { + for _, k := range []byte{'q', 'Q', pagerKeyEscape, pagerKeyCtrlC} { + ch := make(chan byte, 1) + ch <- k + assert.Truef(t, pagerShouldQuit(ch), "key %q must trigger quit", k) + } +} + +func TestPagerShouldQuitClosedChannelKeepsDraining(t *testing.T) { + ch := make(chan byte) + close(ch) + assert.False(t, pagerShouldQuit(ch), "closed channel (stdin EOF) must not force quit") +} + +func TestPagerNextKeyReturnsFalseOnClosedChannel(t *testing.T) { + ch := make(chan byte) + close(ch) + _, ok := pagerNextKey(t.Context(), ch) + assert.False(t, ok) +} + +func TestPagerNextKeyReturnsKey(t *testing.T) { + ch := make(chan byte, 1) + ch <- ' ' + k, ok := pagerNextKey(t.Context(), ch) + assert.True(t, ok) + assert.Equal(t, byte(' '), k) +} diff --git a/libs/cmdio/render.go b/libs/cmdio/render.go index 733dd53fa7f..662046531c5 100644 --- a/libs/cmdio/render.go +++ b/libs/cmdio/render.go @@ -271,8 +271,17 @@ func Render(ctx context.Context, v any) error { return renderWithTemplate(ctx, newRenderer(v), c.outputFormat, c.out, c.headerTemplate, c.template) } +// RenderIterator renders the items produced by i. When the terminal is +// fully interactive (stdin + stdout + stderr all TTYs) and the command +// has a row template, we page through the existing template + tabwriter +// pipeline (same colors, same alignment as the non-paged path; widths are +// locked from the first batch so columns stay aligned across pages). +// Piped output and JSON output keep the existing non-paged behavior. func RenderIterator[T any](ctx context.Context, i listing.Iterator[T]) error { c := fromContext(ctx) + if c.capabilities.SupportsPager() && c.outputFormat == flags.OutputText && c.template != "" { + return renderIteratorPagedTemplate(ctx, i, c.out, c.headerTemplate, c.template) + } return renderWithTemplate(ctx, newIteratorRenderer(i), c.outputFormat, c.out, c.headerTemplate, c.template) } From 75189212f785e12b1d16e1751bf398ee8a3002a7 Mon Sep 17 00:00:00 2001 From: simon Date: Tue, 21 Apr 2026 12:09:48 +0200 Subject: [PATCH 2/9] Simplify pager: trim docs, extract flushPage, collapse tests Cuts ~100 lines without behavior changes: - Trim over-long doc blocks on SupportsPager, startRawStdinKeyReader, and renderIteratorPagedTemplateCore. - Drop comments that restate the code. - Extract the flushPage closure into a templatePager struct. - Collapse q/Q/esc/Ctrl+C exit tests into a table-driven test. - Drop the brittle hard-coded-offset column test; single-page equivalence and the header-once test already cover the behavior. Co-authored-by: Isaac --- libs/cmdio/capabilities.go | 9 +- libs/cmdio/paged_template.go | 193 +++++++++++++----------------- libs/cmdio/paged_template_test.go | 75 ++---------- libs/cmdio/pager.go | 25 +--- 4 files changed, 101 insertions(+), 201 deletions(-) diff --git a/libs/cmdio/capabilities.go b/libs/cmdio/capabilities.go index 12b46f61505..56a13562d90 100644 --- a/libs/cmdio/capabilities.go +++ b/libs/cmdio/capabilities.go @@ -48,12 +48,9 @@ func (c Capabilities) SupportsColor(w io.Writer) bool { return isTTY(w) && c.color } -// SupportsPager returns true when we can run an interactive pager between -// batches of output: stdin, stdout, and stderr must all be TTYs. Stdin -// carries the user's keystrokes, stdout receives the paged content, and -// stderr carries the "[space] more / [enter] all" prompt — all three -// must be visible for the interaction to make sense. Git Bash is -// excluded because raw-mode stdin reads are unreliable there. +// SupportsPager returns true when all three std streams are TTYs (stdin +// for keystrokes, stdout for content, stderr for the prompt). Git Bash +// is excluded because raw-mode stdin reads are unreliable there. func (c Capabilities) SupportsPager() bool { return c.stdinIsTTY && c.stdoutIsTTY && c.stderrIsTTY && !c.isGitBash } diff --git a/libs/cmdio/paged_template.go b/libs/cmdio/paged_template.go index 15ff1f9311f..06e5384afe0 100644 --- a/libs/cmdio/paged_template.go +++ b/libs/cmdio/paged_template.go @@ -14,27 +14,14 @@ import ( "github.com/databricks/databricks-sdk-go/listing" ) -// pagerColumnSeparator is the inter-column spacing used when emitting paged -// template output. Matches text/tabwriter.NewWriter(..., 2, ' ', ...) in -// the non-paged path, so single-batch output is visually indistinguishable -// from what renderUsingTemplate produces. -const pagerColumnSeparator = " " - -// ansiCSIPattern matches ANSI SGR escape sequences so we can compute the -// on-screen width of cells that contain colored output. We do not attempt -// to cover every ANSI escape — just the SGR color/style ones (CSI ... m) -// emitted by github.com/fatih/color, which is all our templates use today. +// ansiCSIPattern matches ANSI SGR escape sequences so colored cells +// aren't counted toward column widths. github.com/fatih/color emits CSI +// ... m, which is all our templates use. var ansiCSIPattern = regexp.MustCompile("\x1b\\[[0-9;]*m") -// renderIteratorPagedTemplate streams an iterator through the existing -// template-based renderer one page at a time, prompting the user between -// pages on stderr. The rendering is intentionally identical to the non- -// paged path — same templates, same colors — only the flush cadence, -// the user-facing prompt, and column-width stability across batches -// differ. -// -// SPACE advances by one page; ENTER drains the remaining iterator (still -// interruptible by q/esc/Ctrl+C); q/esc/Ctrl+C stop immediately. +// renderIteratorPagedTemplate pages an iterator through the template +// renderer, prompting between batches. SPACE advances one page, ENTER +// drains the rest, q/esc/Ctrl+C quit. func renderIteratorPagedTemplate[T any]( ctx context.Context, iter listing.Iterator[T], @@ -58,20 +45,52 @@ func renderIteratorPagedTemplate[T any]( ) } -// renderIteratorPagedTemplateCore is the testable core of -// renderIteratorPagedTemplate: it assumes the caller has already set up -// raw stdin (or any key source) and delivered a channel of keystrokes. -// It never touches os.Stdin directly. -// -// Unlike renderUsingTemplate (the non-paged path) we do not rely on -// text/tabwriter to align columns. Tabwriter computes column widths -// per-flush and resets on Flush(), which produces a jarring -// width-shift when a short final batch follows a wider first batch. -// Here we render each page's template output into an intermediate -// buffer, split it into cells by tab, lock visual column widths from -// the first page, and pad every subsequent page to the same widths. -// The output is visually indistinguishable from tabwriter for single- -// batch lists and stays aligned across batches for longer ones. +// templatePager renders accumulated rows to out, locking column widths +// from the first page so layout stays stable across batches. We do not +// use text/tabwriter because it recomputes widths on every Flush. +type templatePager struct { + out io.Writer + headerT *template.Template + rowT *template.Template + headerStr string + widths []int + headerDone bool +} + +func (p *templatePager) flush(buf []any) error { + if p.headerDone && len(buf) == 0 { + return nil + } + var rendered bytes.Buffer + if !p.headerDone && p.headerStr != "" { + if err := p.headerT.Execute(&rendered, nil); err != nil { + return err + } + rendered.WriteByte('\n') + } + if len(buf) > 0 { + if err := p.rowT.Execute(&rendered, buf); err != nil { + return err + } + } + p.headerDone = true + + text := strings.TrimRight(rendered.String(), "\n") + if text == "" { + return nil + } + rows := strings.Split(text, "\n") + if p.widths == nil { + p.widths = computeWidths(rows) + } + for _, row := range rows { + if _, err := io.WriteString(p.out, padRow(strings.Split(row, "\t"), p.widths)+"\n"); err != nil { + return err + } + } + return nil +} + func renderIteratorPagedTemplateCore[T any]( ctx context.Context, iter listing.Iterator[T], @@ -81,10 +100,9 @@ func renderIteratorPagedTemplateCore[T any]( headerTemplate, tmpl string, pageSize int, ) error { - // Use two independent templates so parsing the row template doesn't - // overwrite the header template's parsed body (they would if they - // shared the same *template.Template instance — Parse replaces the - // body in place and returns the receiver). + // Header and row templates must be separate *template.Template + // instances: Parse replaces the receiver's body in place, so sharing + // one makes the second Parse stomp the first. headerT, err := template.New("header").Funcs(renderFuncMap).Parse(headerTemplate) if err != nil { return err @@ -93,68 +111,18 @@ func renderIteratorPagedTemplateCore[T any]( if err != nil { return err } + pager := &templatePager{ + out: out, + headerT: headerT, + rowT: rowT, + headerStr: headerTemplate, + } limit := limitFromContext(ctx) drainAll := false buf := make([]any, 0, pageSize) total := 0 - var lockedWidths []int - firstBatchDone := false - - flushPage := func() error { - // Nothing to emit after the first batch is out the door and the - // buffer is empty — we've already written the header. - if firstBatchDone && len(buf) == 0 { - return nil - } - - var rendered bytes.Buffer - if !firstBatchDone && headerTemplate != "" { - if err := headerT.Execute(&rendered, nil); err != nil { - return err - } - rendered.WriteByte('\n') - } - if len(buf) > 0 { - if err := rowT.Execute(&rendered, buf); err != nil { - return err - } - buf = buf[:0] - } - firstBatchDone = true - - text := strings.TrimRight(rendered.String(), "\n") - if text == "" { - return nil - } - rows := strings.Split(text, "\n") - - // Lock column widths from the first batch (header + first page). - // Every subsequent batch pads to these widths so columns do not - // shift between pages. - if lockedWidths == nil { - for _, row := range rows { - for i, cell := range strings.Split(row, "\t") { - if i >= len(lockedWidths) { - lockedWidths = append(lockedWidths, 0) - } - if w := visualWidth(cell); w > lockedWidths[i] { - lockedWidths[i] = w - } - } - } - } - - for _, row := range rows { - line := padRow(strings.Split(row, "\t"), lockedWidths) - if _, err := io.WriteString(out, line+"\n"); err != nil { - return err - } - } - return nil - } - for iter.HasNext(ctx) { if limit > 0 && total >= limit { break @@ -169,16 +137,16 @@ func renderIteratorPagedTemplateCore[T any]( if len(buf) < pageSize { continue } - if err := flushPage(); err != nil { + if err := pager.flush(buf); err != nil { return err } + buf = buf[:0] if drainAll { if pagerShouldQuit(keys) { return nil } continue } - // Show the prompt and wait for a key. fmt.Fprint(prompts, pagerPromptText) k, ok := pagerNextKey(ctx, keys) fmt.Fprint(prompts, pagerClearLine) @@ -187,39 +155,46 @@ func renderIteratorPagedTemplateCore[T any]( } switch k { case ' ': - // Continue to the next page. case '\r', '\n': drainAll = true case 'q', 'Q', pagerKeyEscape, pagerKeyCtrlC: return nil } } - return flushPage() + return pager.flush(buf) } -// visualWidth returns the number of runes a string would occupy on-screen -// if ANSI SGR escape sequences are respected as zero-width (which the -// terminal does). This matches what the user sees and lets us pad colored -// cells to consistent visual widths. +// visualWidth counts runes ignoring ANSI SGR escape sequences. func visualWidth(s string) int { return utf8.RuneCountInString(ansiCSIPattern.ReplaceAllString(s, "")) } -// padRow joins the given cells with pagerColumnSeparator, padding every -// cell except the last to widths[i] visual runes. Cells wider than -// widths[i] are emitted as-is — the extra content pushes subsequent -// columns right for that row only, which is the same behavior tabwriter -// would give and the same behavior the non-paged renderer has today. +func computeWidths(rows []string) []int { + var widths []int + for _, row := range rows { + for i, cell := range strings.Split(row, "\t") { + if i >= len(widths) { + widths = append(widths, 0) + } + if w := visualWidth(cell); w > widths[i] { + widths[i] = w + } + } + } + return widths +} + +// padRow joins cells with two-space separators matching tabwriter's +// minpad, padding every cell except the last to widths[i] visual runes. func padRow(cells []string, widths []int) string { var b strings.Builder for i, cell := range cells { if i > 0 { - b.WriteString(pagerColumnSeparator) + b.WriteString(" ") } b.WriteString(cell) if i < len(cells)-1 && i < len(widths) { - pad := widths[i] - visualWidth(cell) - if pad > 0 { + if pad := widths[i] - visualWidth(cell); pad > 0 { b.WriteString(strings.Repeat(" ", pad)) } } diff --git a/libs/cmdio/paged_template_test.go b/libs/cmdio/paged_template_test.go index 1b61135108d..82cd72fc14e 100644 --- a/libs/cmdio/paged_template_test.go +++ b/libs/cmdio/paged_template_test.go @@ -76,22 +76,14 @@ func TestPagedTemplateEnterDrainsIterator(t *testing.T) { assert.Len(t, lines, 25) } -func TestPagedTemplateQuitKeyExits(t *testing.T) { - out := runPagedTemplate(t, 100, 5, []byte{'q'}) - lines := strings.Split(strings.TrimRight(out, "\n"), "\n") - assert.Len(t, lines, 5) -} - -func TestPagedTemplateEscExits(t *testing.T) { - out := runPagedTemplate(t, 100, 5, []byte{pagerKeyEscape}) - lines := strings.Split(strings.TrimRight(out, "\n"), "\n") - assert.Len(t, lines, 5) -} - -func TestPagedTemplateCtrlCExits(t *testing.T) { - out := runPagedTemplate(t, 100, 5, []byte{pagerKeyCtrlC}) - lines := strings.Split(strings.TrimRight(out, "\n"), "\n") - assert.Len(t, lines, 5) +func TestPagedTemplateQuitKeysExit(t *testing.T) { + for _, k := range []byte{'q', 'Q', pagerKeyEscape, pagerKeyCtrlC} { + t.Run(fmt.Sprintf("key=%d", k), func(t *testing.T) { + out := runPagedTemplate(t, 100, 5, []byte{k}) + lines := strings.Split(strings.TrimRight(out, "\n"), "\n") + assert.Len(t, lines, 5) + }) + } } func TestPagedTemplateEnterInterruptibleByCtrlC(t *testing.T) { @@ -155,9 +147,6 @@ func TestPagedTemplatePropagatesFetchError(t *testing.T) { } func TestPagedTemplateRendersHeaderAndRowsCorrectly(t *testing.T) { - // Regression guard against the header/row template cross-pollution - // bug: if the two templates share a *template.Template receiver, - // every flush re-emits the header text where rows should be. var out, prompts bytes.Buffer iter := listing.Iterator[int](&numberIterator{n: 6}) err := renderIteratorPagedTemplateCore( @@ -198,54 +187,6 @@ func TestPagedTemplateEmptyIteratorStillFlushesHeader(t *testing.T) { assert.Contains(t, out.String(), "Name") } -// slicedIterator is a tiny iterator implementation for tests that prefer -// to hand over strongly-typed row structs. -type slicedIterator[T any] struct { - data []T - pos int -} - -func (it *slicedIterator[T]) HasNext(_ context.Context) bool { return it.pos < len(it.data) } -func (it *slicedIterator[T]) Next(_ context.Context) (T, error) { - v := it.data[it.pos] - it.pos++ - return v, nil -} - -func TestPagedTemplateColumnWidthsStableAcrossBatches(t *testing.T) { - type row struct { - Name string - Tag string - } - rows := []row{ - {"wide-name-that-sets-the-width", "a"}, - {"short", "b"}, - } - iter := &slicedIterator[row]{data: rows} - var out, prompts bytes.Buffer - err := renderIteratorPagedTemplateCore( - t.Context(), - iter, - &out, - &prompts, - makeTemplateKeys(' '), - "Name Tag", - "{{range .}}{{.Name}} {{.Tag}}\n{{end}}", - 1, - ) - require.NoError(t, err) - got := out.String() - lines := strings.Split(strings.TrimRight(got, "\n"), "\n") - require.Len(t, lines, 3) - - // Column 1 must start at the same visible offset on every line. - const wantColTwoOffset = 31 - for i, line := range lines { - idx := strings.LastIndex(line, " ") + 1 - assert.Equal(t, wantColTwoOffset, idx, "line %d: col 1 expected at offset %d, got %d (line=%q)", i, wantColTwoOffset, idx, line) - } -} - // TestPagedTemplateMatchesNonPagedForSmallList asserts that single-batch // output is byte-identical to the non-paged template renderer, so users // who never hit a second page see the exact same thing they used to. diff --git a/libs/cmdio/pager.go b/libs/cmdio/pager.go index 400aee8c6a8..8dbd3bbb0ec 100644 --- a/libs/cmdio/pager.go +++ b/libs/cmdio/pager.go @@ -25,22 +25,10 @@ const ( pagerKeyCtrlC = 0x03 ) -// startRawStdinKeyReader puts stdin into raw mode and spawns a goroutine -// that publishes each keystroke as a byte on the returned channel. The -// returned restore function must be called (typically via defer) to put -// the terminal back in its original mode; it is safe to call even if -// MakeRaw failed (it's a no-op). -// -// The goroutine exits when stdin returns an error (e.g. EOF on process -// shutdown) or when ctx is cancelled, at which point the channel is -// closed. Leaking the goroutine before that is acceptable because the -// pager is only invoked by short-lived CLI commands: the process exits -// shortly after the caller returns. -// -// Note: term.MakeRaw also clears the TTY's OPOST flag on most Unixes. -// With OPOST off, outbound '\n' is not translated to '\r\n', so callers -// that write newlines while raw mode is active should wrap their output -// stream in crlfWriter to avoid staircase output. +// startRawStdinKeyReader puts stdin into raw mode and streams keystrokes +// onto the returned channel. Callers must defer restore. Raw mode also +// clears OPOST on Unix, so output written while active needs crlfWriter +// to avoid staircase newlines. func startRawStdinKeyReader(ctx context.Context) (<-chan byte, func(), error) { fd := int(os.Stdin.Fd()) oldState, err := term.MakeRaw(fd) @@ -80,9 +68,8 @@ func pagerNextKey(ctx context.Context, keys <-chan byte) (byte, bool) { } // pagerShouldQuit drains any buffered keys non-blockingly and returns true -// if one of q/Q/esc/Ctrl+C was pressed. Other keys are consumed and -// dropped. A closed channel means stdin ran out (EOF) — that's not a -// quit signal; the caller should keep draining. +// if q/Q/esc/Ctrl+C was pressed. A closed channel (stdin EOF) is not a +// quit signal. func pagerShouldQuit(keys <-chan byte) bool { for { select { From 81cb4dce0583e2f681f6f0b4bf476c20fbb90920 Mon Sep 17 00:00:00 2001 From: simon Date: Tue, 21 Apr 2026 12:21:08 +0200 Subject: [PATCH 3/9] Apply go-code-structure patterns to tests Two principles from docs/go-code-structure: - Table-driven tests: collapse four pagination behavior tests into one. - Test the pure logic directly: add unit tests for visualWidth, computeWidths, and padRow instead of exercising them only via the integration path. Failures now point directly at the broken helper. Co-authored-by: Isaac --- libs/cmdio/paged_template_test.go | 107 ++++++++++++++++++++++-------- 1 file changed, 79 insertions(+), 28 deletions(-) diff --git a/libs/cmdio/paged_template_test.go b/libs/cmdio/paged_template_test.go index 82cd72fc14e..b143a005650 100644 --- a/libs/cmdio/paged_template_test.go +++ b/libs/cmdio/paged_template_test.go @@ -59,39 +59,32 @@ func runPagedTemplate(t *testing.T, n, pageSize int, keys []byte) string { return out.String() } -func TestPagedTemplateDrainsWhenFirstPageExhausts(t *testing.T) { - out := runPagedTemplate(t, 3, 10, nil) - require.Equal(t, "1\n2\n3\n", out) -} - -func TestPagedTemplateSpaceFetchesOneMorePage(t *testing.T) { - out := runPagedTemplate(t, 7, 3, []byte{' '}) - lines := strings.Split(strings.TrimRight(out, "\n"), "\n") - assert.Len(t, lines, 6) -} - -func TestPagedTemplateEnterDrainsIterator(t *testing.T) { - out := runPagedTemplate(t, 25, 5, []byte{'\r'}) - lines := strings.Split(strings.TrimRight(out, "\n"), "\n") - assert.Len(t, lines, 25) -} - -func TestPagedTemplateQuitKeysExit(t *testing.T) { - for _, k := range []byte{'q', 'Q', pagerKeyEscape, pagerKeyCtrlC} { - t.Run(fmt.Sprintf("key=%d", k), func(t *testing.T) { - out := runPagedTemplate(t, 100, 5, []byte{k}) +func TestPagedTemplateBehavior(t *testing.T) { + tests := []struct { + name string + items int + pageSize int + keys []byte + wantLines int + }{ + {"drains when first page exhausts iterator", 3, 10, nil, 3}, + {"space fetches one more page", 7, 3, []byte{' '}, 6}, + {"enter drains remaining iterator", 25, 5, []byte{'\r'}, 25}, + {"enter interruptible by ctrl+c", 20, 5, []byte{'\r', pagerKeyCtrlC}, 10}, + {"q exits after first page", 100, 5, []byte{'q'}, 5}, + {"Q exits after first page", 100, 5, []byte{'Q'}, 5}, + {"esc exits after first page", 100, 5, []byte{pagerKeyEscape}, 5}, + {"ctrl+c exits after first page", 100, 5, []byte{pagerKeyCtrlC}, 5}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + out := runPagedTemplate(t, tt.items, tt.pageSize, tt.keys) lines := strings.Split(strings.TrimRight(out, "\n"), "\n") - assert.Len(t, lines, 5) + assert.Len(t, lines, tt.wantLines) }) } } -func TestPagedTemplateEnterInterruptibleByCtrlC(t *testing.T) { - out := runPagedTemplate(t, 20, 5, []byte{'\r', pagerKeyCtrlC}) - lines := strings.Split(strings.TrimRight(out, "\n"), "\n") - assert.Len(t, lines, 10) -} - func TestPagedTemplateRespectsLimit(t *testing.T) { var out, prompts bytes.Buffer iter := listing.Iterator[int](&numberIterator{n: 200}) @@ -187,6 +180,64 @@ func TestPagedTemplateEmptyIteratorStillFlushesHeader(t *testing.T) { assert.Contains(t, out.String(), "Name") } +func TestVisualWidth(t *testing.T) { + tests := []struct { + name string + in string + want int + }{ + {"plain ascii", "hello", 5}, + {"empty", "", 0}, + {"green SGR wraps text", "\x1b[32mhello\x1b[0m", 5}, + {"multiple SGR escapes", "\x1b[1;31mfoo\x1b[0m bar", 7}, + {"multibyte runes count as one each", "héllo", 5}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, visualWidth(tt.in)) + }) + } +} + +func TestComputeWidths(t *testing.T) { + tests := []struct { + name string + rows []string + want []int + }{ + {"empty input", nil, nil}, + {"single row", []string{"a\tbb\tccc"}, []int{1, 2, 3}}, + {"widest wins per column", []string{"a\tbb", "aaa\tb"}, []int{3, 2}}, + {"ragged rows extend column count", []string{"a", "b\tcc"}, []int{1, 2}}, + {"SGR escapes don't inflate widths", []string{"\x1b[31mred\x1b[0m\tplain"}, []int{3, 5}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, computeWidths(tt.rows)) + }) + } +} + +func TestPadRow(t *testing.T) { + tests := []struct { + name string + cells []string + widths []int + want string + }{ + {"single cell is emitted as-is", []string{"only"}, []int{10}, "only"}, + {"pads every cell except the last", []string{"a", "bb", "c"}, []int{3, 3, 3}, "a bb c"}, + {"overflowing cell pushes next column right", []string{"toolong", "b"}, []int{3, 3}, "toolong b"}, + {"no widths means no padding", []string{"a", "b"}, nil, "a b"}, + {"SGR escape doesn't count toward pad", []string{"\x1b[31mred\x1b[0m", "b"}, []int{5, 1}, "\x1b[31mred\x1b[0m b"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, padRow(tt.cells, tt.widths)) + }) + } +} + // TestPagedTemplateMatchesNonPagedForSmallList asserts that single-batch // output is byte-identical to the non-paged template renderer, so users // who never hit a second page see the exact same thing they used to. From 0a1799401c23afe0ea44aa41a7fef634d35c60c1 Mon Sep 17 00:00:00 2001 From: simon Date: Thu, 23 Apr 2026 11:18:10 +0200 Subject: [PATCH 4/9] Replace x/term raw stdin with a bubbletea tea.Model pager Consolidates the interactive I/O on bubbletea (already a direct dependency via the spinner) so the paged list renderer no longer needs its own raw-mode stdin handling. Removes golang.org/x/term as a direct dep. Architecture: - pagerModel implements tea.Model. It owns the iterator and fetches one batch per tea.Cmd; batches are emitted with tea.Println (which prints above the TUI area), and the prompt lives in View() while waiting for a key. - Only one fetch runs at a time (fetching flag). SPACE during an in-flight fetch is dropped; ENTER flips drainAll and lets the pending batchMsg chain the next fetch. This keeps the iterator off two goroutines. Gone: startRawStdinKeyReader, pagerNextKey, pagerShouldQuit, crlfWriter, the OPOST staircase workaround, and the x/term NOTICE entry. Kept: templatePager, computeWidths, padRow, visualWidth, and the cross-page column-width locking. flush() became flushLines() and returns []string instead of writing to an io.Writer, so the tea path can fold rendered rows into tea.Println. Co-authored-by: Isaac --- NEXT_CHANGELOG.md | 2 +- NOTICE | 4 - go.mod | 1 - libs/cmdio/paged_template.go | 113 ++++-------- libs/cmdio/paged_template_test.go | 288 ++++++++++++++++-------------- libs/cmdio/pager.go | 231 +++++++++++++++--------- libs/cmdio/pager_test.go | 253 +++++++++++++++++++++----- 7 files changed, 539 insertions(+), 353 deletions(-) diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index 66c4f97effc..058ae375f71 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -5,7 +5,7 @@ ### CLI * Moved file-based OAuth token cache management from the SDK to the CLI. No user-visible change; part of a three-PR sequence that makes the CLI the sole owner of its token cache. -* Added interactive pagination for list commands that have a row template (jobs, clusters, apps, pipelines, etc.). When stdin, stdout, and stderr are all TTYs, `databricks list` now streams 50 rows at a time and prompts `[space] more [enter] all [q|esc] quit` on stderr. ENTER can be interrupted by `q`/`esc`/`Ctrl+C` between pages. Colors and alignment match the existing non-paged output; column widths stay stable across pages. Piped output and `--output json` are unchanged. +* Added interactive pagination for list commands that have a row template (jobs, clusters, apps, pipelines, etc.). When stdin, stdout, and stderr are all TTYs, `databricks list` now streams 50 rows at a time and prompts `[space] more [enter] all [q|esc] quit`. ENTER can be interrupted by `q`/`esc`/`Ctrl+C` between pages. Colors and alignment match the existing non-paged output; column widths stay stable across pages. Piped output and `--output json` are unchanged. ### Bundles diff --git a/NOTICE b/NOTICE index 3145529c707..1e286df6f91 100644 --- a/NOTICE +++ b/NOTICE @@ -115,10 +115,6 @@ golang.org/x/sys - https://github.com/golang/sys Copyright 2009 The Go Authors. License - https://github.com/golang/sys/blob/master/LICENSE -golang.org/x/term - https://github.com/golang/term -Copyright 2009 The Go Authors. -License - https://github.com/golang/term/blob/master/LICENSE - golang.org/x/text - https://github.com/golang/text Copyright 2009 The Go Authors. License - https://github.com/golang/text/blob/master/LICENSE diff --git a/go.mod b/go.mod index 71e90860379..f376aa0a98d 100644 --- a/go.mod +++ b/go.mod @@ -41,7 +41,6 @@ require ( golang.org/x/oauth2 v0.36.0 // BSD-3-Clause golang.org/x/sync v0.20.0 // BSD-3-Clause golang.org/x/sys v0.43.0 // BSD-3-Clause - golang.org/x/term v0.41.0 // BSD-3-Clause golang.org/x/text v0.35.0 // BSD-3-Clause gopkg.in/ini.v1 v1.67.1 // Apache-2.0 ) diff --git a/libs/cmdio/paged_template.go b/libs/cmdio/paged_template.go index 06e5384afe0..098950f4810 100644 --- a/libs/cmdio/paged_template.go +++ b/libs/cmdio/paged_template.go @@ -3,7 +3,6 @@ package cmdio import ( "bytes" "context" - "fmt" "io" "os" "regexp" @@ -11,6 +10,7 @@ import ( "text/template" "unicode/utf8" + tea "github.com/charmbracelet/bubbletea" "github.com/databricks/databricks-sdk-go/listing" ) @@ -28,28 +28,13 @@ func renderIteratorPagedTemplate[T any]( out io.Writer, headerTemplate, tmpl string, ) error { - keys, restore, err := startRawStdinKeyReader(ctx) - if err != nil { - return err - } - defer restore() - return renderIteratorPagedTemplateCore( - ctx, - iter, - crlfWriter{w: out}, - crlfWriter{w: os.Stderr}, - keys, - headerTemplate, - tmpl, - pagerPageSize, - ) + return renderIteratorPagedTemplateCore(ctx, iter, os.Stdin, out, headerTemplate, tmpl, pagerPageSize) } -// templatePager renders accumulated rows to out, locking column widths -// from the first page so layout stays stable across batches. We do not -// use text/tabwriter because it recomputes widths on every Flush. +// templatePager renders accumulated rows, locking column widths from the +// first page so layout stays stable across batches. We do not use +// text/tabwriter because it recomputes widths on every Flush. type templatePager struct { - out io.Writer headerT *template.Template rowT *template.Template headerStr string @@ -57,46 +42,47 @@ type templatePager struct { headerDone bool } -func (p *templatePager) flush(buf []any) error { +// flushLines renders the header (on the first call) plus any buffered +// rows, then pads each cell to the widths recorded on the first page so +// columns line up across batches. +func (p *templatePager) flushLines(buf []any) ([]string, error) { if p.headerDone && len(buf) == 0 { - return nil + return nil, nil } var rendered bytes.Buffer if !p.headerDone && p.headerStr != "" { if err := p.headerT.Execute(&rendered, nil); err != nil { - return err + return nil, err } rendered.WriteByte('\n') } if len(buf) > 0 { if err := p.rowT.Execute(&rendered, buf); err != nil { - return err + return nil, err } } p.headerDone = true text := strings.TrimRight(rendered.String(), "\n") if text == "" { - return nil + return nil, nil } rows := strings.Split(text, "\n") if p.widths == nil { p.widths = computeWidths(rows) } - for _, row := range rows { - if _, err := io.WriteString(p.out, padRow(strings.Split(row, "\t"), p.widths)+"\n"); err != nil { - return err - } + lines := make([]string, len(rows)) + for i, row := range rows { + lines[i] = padRow(strings.Split(row, "\t"), p.widths) } - return nil + return lines, nil } func renderIteratorPagedTemplateCore[T any]( ctx context.Context, iter listing.Iterator[T], + in io.Reader, out io.Writer, - prompts io.Writer, - keys <-chan byte, headerTemplate, tmpl string, pageSize int, ) error { @@ -112,56 +98,29 @@ func renderIteratorPagedTemplateCore[T any]( return err } pager := &templatePager{ - out: out, headerT: headerT, rowT: rowT, headerStr: headerTemplate, } - - limit := limitFromContext(ctx) - drainAll := false - buf := make([]any, 0, pageSize) - total := 0 - - for iter.HasNext(ctx) { - if limit > 0 && total >= limit { - break - } - n, err := iter.Next(ctx) - if err != nil { - return err - } - buf = append(buf, n) - total++ - - if len(buf) < pageSize { - continue - } - if err := pager.flush(buf); err != nil { - return err - } - buf = buf[:0] - if drainAll { - if pagerShouldQuit(keys) { - return nil - } - continue - } - fmt.Fprint(prompts, pagerPromptText) - k, ok := pagerNextKey(ctx, keys) - fmt.Fprint(prompts, pagerClearLine) - if !ok { - return nil - } - switch k { - case ' ': - case '\r', '\n': - drainAll = true - case 'q', 'Q', pagerKeyEscape, pagerKeyCtrlC: - return nil - } + m := &pagerModel[T]{ + ctx: ctx, + iter: iter, + pager: pager, + pageSize: pageSize, + limit: limitFromContext(ctx), + } + p := tea.NewProgram( + m, + tea.WithInput(in), + tea.WithOutput(out), + // Match spinner: let SIGINT reach the process rather than the TUI + // so Ctrl+C also interrupts a stalled iterator fetch. + tea.WithoutSignalHandler(), + ) + if _, err := p.Run(); err != nil { + return err } - return pager.flush(buf) + return m.err } // visualWidth counts runes ignoring ANSI SGR escape sequences. diff --git a/libs/cmdio/paged_template_test.go b/libs/cmdio/paged_template_test.go index b143a005650..f51f6c00324 100644 --- a/libs/cmdio/paged_template_test.go +++ b/libs/cmdio/paged_template_test.go @@ -5,6 +5,9 @@ import ( "context" "errors" "fmt" + "io" + "regexp" + "strconv" "strings" "testing" @@ -32,105 +35,76 @@ func (it *numberIterator) Next(_ context.Context) (int, error) { return it.pos, nil } -func makeTemplateKeys(bytes ...byte) <-chan byte { - ch := make(chan byte, len(bytes)) - for _, b := range bytes { - ch <- b - } - close(ch) - return ch +// ansiStripPattern removes CSI sequences so we can assert on plain content +// ignoring tea's cursor movement, line-clearing, and DEC-private escapes +// like bracketed-paste toggles (CSI ? 2004 l). +var ansiStripPattern = regexp.MustCompile("\x1b\\[[?]?[0-9;]*[A-Za-z]") + +func stripANSI(s string) string { + return ansiStripPattern.ReplaceAllString(s, "") } -func runPagedTemplate(t *testing.T, n, pageSize int, keys []byte) string { +// pagedOutput drives a full paged render with tea.Program, returning the +// plain-text output after stripping ANSI escape sequences. It feeds the +// enter key via a pre-loaded reader, so drainAll is set either on the +// first KeyMsg or on the first batchMsg (whichever arrives first at the +// tea event loop); in both orderings the final output contains every row. +func pagedOutput( + t *testing.T, + ctx context.Context, + iter listing.Iterator[int], + headerTemplate, tmpl string, + pageSize int, +) string { t.Helper() - var out, prompts bytes.Buffer - iter := listing.Iterator[int](&numberIterator{n: n}) - err := renderIteratorPagedTemplateCore( - t.Context(), - iter, + var out bytes.Buffer + require.NoError(t, renderIteratorPagedTemplateCore( + ctx, iter, + strings.NewReader("\r"), &out, - &prompts, - makeTemplateKeys(keys...), - "", - "{{range .}}{{.}}\n{{end}}", - pageSize, - ) - require.NoError(t, err) - return out.String() + headerTemplate, tmpl, pageSize, + )) + return stripANSI(out.String()) } -func TestPagedTemplateBehavior(t *testing.T) { - tests := []struct { - name string - items int - pageSize int - keys []byte - wantLines int - }{ - {"drains when first page exhausts iterator", 3, 10, nil, 3}, - {"space fetches one more page", 7, 3, []byte{' '}, 6}, - {"enter drains remaining iterator", 25, 5, []byte{'\r'}, 25}, - {"enter interruptible by ctrl+c", 20, 5, []byte{'\r', pagerKeyCtrlC}, 10}, - {"q exits after first page", 100, 5, []byte{'q'}, 5}, - {"Q exits after first page", 100, 5, []byte{'Q'}, 5}, - {"esc exits after first page", 100, 5, []byte{pagerKeyEscape}, 5}, - {"ctrl+c exits after first page", 100, 5, []byte{pagerKeyCtrlC}, 5}, +func countContentLines(s string) int { + count := 0 + for line := range strings.SplitSeq(s, "\n") { + trimmed := strings.TrimSpace(line) + if trimmed == "" || strings.Contains(trimmed, pagerPromptText) { + continue + } + count++ } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - out := runPagedTemplate(t, tt.items, tt.pageSize, tt.keys) - lines := strings.Split(strings.TrimRight(out, "\n"), "\n") - assert.Len(t, lines, tt.wantLines) - }) + return count +} + +func TestPagedTemplateDrainsFullIterator(t *testing.T) { + out := pagedOutput(t, t.Context(), &numberIterator{n: 23}, "", "{{range .}}{{.}}\n{{end}}", 5) + assert.Equal(t, 23, countContentLines(out)) + for i := 1; i <= 23; i++ { + assert.Contains(t, out, strconv.Itoa(i)) } } func TestPagedTemplateRespectsLimit(t *testing.T) { - var out, prompts bytes.Buffer - iter := listing.Iterator[int](&numberIterator{n: 200}) ctx := WithLimit(t.Context(), 7) - err := renderIteratorPagedTemplateCore( - ctx, - iter, - &out, - &prompts, - makeTemplateKeys('\r'), - "", - "{{range .}}{{.}}\n{{end}}", - 5, - ) - require.NoError(t, err) - lines := strings.Split(strings.TrimRight(out.String(), "\n"), "\n") - assert.Len(t, lines, 7) + out := pagedOutput(t, ctx, &numberIterator{n: 200}, "", "{{range .}}{{.}}\n{{end}}", 5) + assert.Equal(t, 7, countContentLines(out)) } func TestPagedTemplatePrintsHeaderOnce(t *testing.T) { - var out, prompts bytes.Buffer - iter := listing.Iterator[int](&numberIterator{n: 8}) - err := renderIteratorPagedTemplateCore( - t.Context(), - iter, - &out, - &prompts, - makeTemplateKeys(' '), - `ID`, - "{{range .}}{{.}}\n{{end}}", - 3, - ) - require.NoError(t, err) - assert.Equal(t, 1, strings.Count(out.String(), "ID\n")) - assert.True(t, strings.HasPrefix(out.String(), "ID\n")) + out := pagedOutput(t, t.Context(), &numberIterator{n: 8}, "ID", "{{range .}}{{.}}\n{{end}}", 3) + assert.Equal(t, 1, strings.Count(out, "ID")) } func TestPagedTemplatePropagatesFetchError(t *testing.T) { - var out, prompts bytes.Buffer - iter := listing.Iterator[int](&numberIterator{n: 100, err: errors.New("boom")}) + var buf bytes.Buffer err := renderIteratorPagedTemplateCore( t.Context(), - iter, - &out, - &prompts, - makeTemplateKeys(), + &numberIterator{n: 100, err: errors.New("boom")}, + strings.NewReader(""), + &buf, "", "{{range .}}{{.}}\n{{end}}", 5, @@ -139,45 +113,114 @@ func TestPagedTemplatePropagatesFetchError(t *testing.T) { assert.Contains(t, err.Error(), "boom") } -func TestPagedTemplateRendersHeaderAndRowsCorrectly(t *testing.T) { - var out, prompts bytes.Buffer - iter := listing.Iterator[int](&numberIterator{n: 6}) - err := renderIteratorPagedTemplateCore( - t.Context(), - iter, - &out, - &prompts, - makeTemplateKeys(), - `ID Name`, - "{{range .}}{{.}} item-{{.}}\n{{end}}", - 100, - ) - require.NoError(t, err) - got := out.String() - assert.Contains(t, got, "ID") - assert.Contains(t, got, "Name") +func TestPagedTemplateRendersHeaderAndRows(t *testing.T) { + out := pagedOutput(t, t.Context(), &numberIterator{n: 6}, "ID\tName", "{{range .}}{{.}}\titem-{{.}}\n{{end}}", 100) + assert.Contains(t, out, "ID") + assert.Contains(t, out, "Name") for i := 1; i <= 6; i++ { - assert.Contains(t, got, fmt.Sprintf("item-%d", i)) + assert.Contains(t, out, fmt.Sprintf("item-%d", i)) } - assert.Equal(t, 1, strings.Count(got, "ID")) + assert.Equal(t, 1, strings.Count(out, "ID")) } func TestPagedTemplateEmptyIteratorStillFlushesHeader(t *testing.T) { - var out, prompts bytes.Buffer - iter := listing.Iterator[int](&numberIterator{n: 0}) - err := renderIteratorPagedTemplateCore( + // Use a blocking reader: empty iterator finishes on the first fetch, so + // the pager quits before it ever needs a keystroke. + pr, pw := io.Pipe() + defer pw.Close() + var out bytes.Buffer + require.NoError(t, renderIteratorPagedTemplateCore( t.Context(), - iter, + &numberIterator{n: 0}, + pr, &out, - &prompts, - makeTemplateKeys(), - `ID Name`, + "ID\tName", "{{range .}}{{.}}\n{{end}}", 10, - ) - require.NoError(t, err) - assert.Contains(t, out.String(), "ID") - assert.Contains(t, out.String(), "Name") + )) + stripped := stripANSI(out.String()) + assert.Contains(t, stripped, "ID") + assert.Contains(t, stripped, "Name") +} + +func TestPagedTemplateColumnsStableAcrossBatches(t *testing.T) { + // First batch of rows is narrow; second batch contains a wider row. + // Widths are locked from the first page, so the wider row in batch 2 + // overflows its column instead of re-flowing prior batches. + it := &numberIterator{n: 6} + tmpl := "{{range .}}col-{{.}}\tval\n{{end}}" + out := pagedOutput(t, t.Context(), it, "", tmpl, 3) + lines := strings.Split(strings.TrimRight(out, "\n"), "\n") + // Find rows that contain "col-" (not the prompt) + var dataRows []string + for _, l := range lines { + if strings.Contains(l, "col-") { + dataRows = append(dataRows, l) + } + } + require.Len(t, dataRows, 6) + // All rows start with "col-" followed by two spaces and then "val". + // The gap before "val" must be >= 2 spaces (tabwriter's minpad). + for _, row := range dataRows { + idx := strings.Index(row, "val") + require.Positive(t, idx) + assert.GreaterOrEqual(t, idx, len("col-N")+2, "row %q should keep minpad gap", row) + } +} + +// TestPagedTemplateMatchesNonPagedForSmallList asserts that single-batch +// rendered content (after stripping tea's cursor-control escapes) carries +// the same rows and columns as the non-paged template renderer, so users +// who never see a second page see the same content they used to. +func TestPagedTemplateMatchesNonPagedForSmallList(t *testing.T) { + const rows = 5 + tmpl := "{{range .}}{{green \"%d\" .}}\t{{.}}\n{{end}}" + + var expected bytes.Buffer + refIter := listing.Iterator[int](&numberIterator{n: rows}) + require.NoError(t, renderWithTemplate(t.Context(), newIteratorRenderer(refIter), flags.OutputText, &expected, "", tmpl)) + + pagedIter := listing.Iterator[int](&numberIterator{n: rows}) + var actual bytes.Buffer + pr, pw := io.Pipe() + defer pw.Close() + require.NoError(t, renderIteratorPagedTemplateCore( + t.Context(), + pagedIter, + pr, + &actual, + "", + tmpl, + 100, + )) + + assertSameContentLines(t, expected.String(), stripANSI(actual.String())) +} + +// assertSameContentLines compares two renderings for semantic equivalence: +// same set of non-empty trimmed lines. tabwriter and the manual padder in +// templatePager produce matching column alignment for a single batch, but +// trailing whitespace and newline counts can differ slightly. +func assertSameContentLines(t *testing.T, want, got string) { + t.Helper() + wantLines := nonEmptyLines(want) + gotLines := nonEmptyLines(got) + require.Equal(t, len(wantLines), len(gotLines), "line count mismatch\nwant:\n%s\ngot:\n%s", want, got) + for i := range wantLines { + assert.Equal(t, wantLines[i], gotLines[i], "line %d", i) + } +} + +func nonEmptyLines(s string) []string { + var out []string + for l := range strings.SplitSeq(s, "\n") { + t := strings.TrimRight(l, " \r\t") + if t == "" { + continue + } + out = append(out, t) + } + return out } func TestVisualWidth(t *testing.T) { @@ -237,30 +280,3 @@ func TestPadRow(t *testing.T) { }) } } - -// TestPagedTemplateMatchesNonPagedForSmallList asserts that single-batch -// output is byte-identical to the non-paged template renderer, so users -// who never hit a second page see the exact same thing they used to. -func TestPagedTemplateMatchesNonPagedForSmallList(t *testing.T) { - const rows = 5 - tmpl := `{{range .}}{{green "%d" .}} {{.}} -{{end}}` - var expected bytes.Buffer - refIter := listing.Iterator[int](&numberIterator{n: rows}) - require.NoError(t, renderWithTemplate(t.Context(), newIteratorRenderer(refIter), flags.OutputText, &expected, "", tmpl)) - - var actual, prompts bytes.Buffer - pagedIter := listing.Iterator[int](&numberIterator{n: rows}) - require.NoError(t, renderIteratorPagedTemplateCore( - t.Context(), - pagedIter, - &actual, - &prompts, - makeTemplateKeys(), - "", - tmpl, - 100, - )) - assert.Equal(t, expected.String(), actual.String()) - assert.NotEmpty(t, expected.String()) -} diff --git a/libs/cmdio/pager.go b/libs/cmdio/pager.go index 8dbd3bbb0ec..37cce9382e1 100644 --- a/libs/cmdio/pager.go +++ b/libs/cmdio/pager.go @@ -2,119 +2,174 @@ package cmdio import ( "context" - "fmt" - "io" - "os" + "strings" - "golang.org/x/term" + tea "github.com/charmbracelet/bubbletea" + "github.com/databricks/databricks-sdk-go/listing" ) // pagerPageSize is the number of items rendered between prompts. const pagerPageSize = 50 -// pagerPromptText is shown on stderr between pages. +// pagerPromptText is shown between pages. const pagerPromptText = "[space] more [enter] all [q|esc] quit" -// pagerClearLine is the ANSI sequence to return to column 0 and erase the -// current line. Used to remove the prompt before writing the next page. -const pagerClearLine = "\r\x1b[K" +// pagerModel drives the paged render loop through bubbletea. It owns the +// iterator, fetches one batch at a time via a tea.Cmd, emits rendered +// lines above the view with tea.Println, and shows the prompt while it +// waits for a key. Using bubbletea lets us skip manual raw-mode setup +// (tea enters/restores raw mode on its own), so we don't need a parallel +// CRLF translator for output written during raw mode. +type pagerModel[T any] struct { + ctx context.Context + iter listing.Iterator[T] + pager *templatePager + pageSize int + limit int + total int -// Key codes we care about when reading single bytes from stdin in raw mode. -const ( - pagerKeyEscape = 0x1b - pagerKeyCtrlC = 0x03 -) + // fetching tracks whether a fetchCmd is in flight. We only ever keep + // one in flight at a time so the iterator isn't read from two + // goroutines; if the user hits SPACE or ENTER while one is running, + // we record the intent in drainAll and let the in-flight fetch + // continue in drain mode when its batchMsg lands. + fetching bool + drainAll bool + firstBatch bool + iterDone bool + err error +} -// startRawStdinKeyReader puts stdin into raw mode and streams keystrokes -// onto the returned channel. Callers must defer restore. Raw mode also -// clears OPOST on Unix, so output written while active needs crlfWriter -// to avoid staircase newlines. -func startRawStdinKeyReader(ctx context.Context) (<-chan byte, func(), error) { - fd := int(os.Stdin.Fd()) - oldState, err := term.MakeRaw(fd) - if err != nil { - return nil, func() {}, fmt.Errorf("failed to enter raw mode on stdin: %w", err) - } - restore := func() { _ = term.Restore(fd, oldState) } +// batchMsg is delivered to Update when a fetched batch has been rendered +// into printable lines. done signals the iterator is exhausted (or the +// limit is reached); err surfaces iteration errors. +type batchMsg struct { + lines []string + done bool + err error +} - ch := make(chan byte, 16) - go func() { - defer close(ch) - buf := make([]byte, 1) - for { - n, err := os.Stdin.Read(buf) - if err != nil || n == 0 { - return +func (m *pagerModel[T]) Init() tea.Cmd { + m.fetching = true + return m.fetchCmd() +} + +// fetchCmd returns a tea.Cmd that reads one page from the iterator and +// renders it into lines. The command runs off the update loop so slow +// network fetches don't stall key handling. +func (m *pagerModel[T]) fetchCmd() tea.Cmd { + return func() tea.Msg { + buf := make([]any, 0, m.pageSize) + done := false + for len(buf) < m.pageSize { + if m.limit > 0 && m.total+len(buf) >= m.limit { + done = true + break + } + if !m.iter.HasNext(m.ctx) { + done = true + break } - select { - case ch <- buf[0]: - case <-ctx.Done(): - return + n, err := m.iter.Next(m.ctx) + if err != nil { + return batchMsg{err: err} } + buf = append(buf, n) } - }() - return ch, restore, nil + lines, err := m.pager.flushLines(buf) + if err != nil { + return batchMsg{err: err} + } + m.total += len(buf) + return batchMsg{lines: lines, done: done} + } } -// pagerNextKey blocks until a key arrives, the key channel closes, or the -// context is cancelled. Returns ok=false on close or cancellation. -func pagerNextKey(ctx context.Context, keys <-chan byte) (byte, bool) { - select { - case k, ok := <-keys: - return k, ok - case <-ctx.Done(): - return 0, false +func (m *pagerModel[T]) Update(msg tea.Msg) (tea.Model, tea.Cmd) { + switch msg := msg.(type) { + case batchMsg: + m.fetching = false + if msg.err != nil { + m.err = msg.err + return m, tea.Quit + } + m.firstBatch = true + // Collapse the batch into a single Println so ordering is trivial: + // tea.Println splits on \n internally, so one Cmd emits all rows. + var printCmd tea.Cmd + if len(msg.lines) > 0 { + printCmd = tea.Println(strings.Join(msg.lines, "\n")) + } + switch { + case msg.done: + m.iterDone = true + return m, tea.Sequence(printCmd, tea.Quit) + case m.drainAll: + m.fetching = true + return m, tea.Sequence(printCmd, m.fetchCmd()) + default: + return m, printCmd + } + + case tea.KeyMsg: + if m.iterDone { + return m, nil + } + return m.handleKey(msg) } + return m, nil } -// pagerShouldQuit drains any buffered keys non-blockingly and returns true -// if q/Q/esc/Ctrl+C was pressed. A closed channel (stdin EOF) is not a -// quit signal. -func pagerShouldQuit(keys <-chan byte) bool { - for { - select { - case k, ok := <-keys: - if !ok { - return false - } - if k == 'q' || k == 'Q' || k == pagerKeyEscape || k == pagerKeyCtrlC { - return true - } - default: - return false +// handleKey routes a keystroke to the right state transition. Keys we +// don't care about are ignored so the user can mash the keyboard without +// affecting the pager. +func (m *pagerModel[T]) handleKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { + switch msg.Type { //nolint:exhaustive // the pager only cares about a few keys + case tea.KeyEnter: + return m, m.startDrain() + case tea.KeyEsc, tea.KeyCtrlC: + return m, tea.Quit + case tea.KeySpace: + return m, m.startAdvance() + case tea.KeyRunes: + switch msg.String() { + case " ": + return m, m.startAdvance() + case "q", "Q": + return m, tea.Quit } } + return m, nil } -// crlfWriter translates outbound '\n' bytes into '\r\n' so output written -// while the TTY is in raw mode (OPOST cleared) still starts at column 0. -// io.Writer semantics are preserved: the returned byte count is the -// number of bytes from p that were consumed, not the (possibly larger) -// number of bytes written to the underlying writer. -type crlfWriter struct { - w io.Writer +// startAdvance handles SPACE: fetch one more page unless we're already +// fetching or draining. If a fetch is in flight we drop the keystroke. +func (m *pagerModel[T]) startAdvance() tea.Cmd { + if m.drainAll || m.fetching { + return nil + } + m.fetching = true + return m.fetchCmd() } -func (c crlfWriter) Write(p []byte) (int, error) { - start := 0 - for i, b := range p { - if b != '\n' { - continue - } - if i > start { - if _, err := c.w.Write(p[start:i]); err != nil { - return start, err - } - } - if _, err := c.w.Write([]byte{'\r', '\n'}); err != nil { - return i, err - } - start = i + 1 +// startDrain handles ENTER: flip into drain-all mode. If a fetch is +// already in flight the batchMsg handler will see drainAll and continue +// fetching; otherwise we kick off the next fetch here. +func (m *pagerModel[T]) startDrain() tea.Cmd { + if m.drainAll { + return nil } - if start < len(p) { - if _, err := c.w.Write(p[start:]); err != nil { - return start, err - } + m.drainAll = true + if m.fetching { + return nil + } + m.fetching = true + return m.fetchCmd() +} + +func (m *pagerModel[T]) View() string { + if m.iterDone || m.drainAll || m.err != nil || !m.firstBatch { + return "" } - return len(p), nil + return pagerPromptText } diff --git a/libs/cmdio/pager_test.go b/libs/cmdio/pager_test.go index d2e6f00463a..b30058a43db 100644 --- a/libs/cmdio/pager_test.go +++ b/libs/cmdio/pager_test.go @@ -1,72 +1,233 @@ package cmdio import ( - "bytes" + "errors" + "reflect" "testing" + "text/template" + tea "github.com/charmbracelet/bubbletea" + "github.com/databricks/databricks-sdk-go/listing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -func TestCrlfWriterTranslatesNewlines(t *testing.T) { - tests := []struct { +func newTestPager(t *testing.T, iter listing.Iterator[int], pageSize int) *pagerModel[int] { + t.Helper() + rowT, err := template.New("row").Funcs(renderFuncMap).Parse("{{range .}}{{.}}\n{{end}}") + require.NoError(t, err) + headerT, err := template.New("header").Funcs(renderFuncMap).Parse("") + require.NoError(t, err) + return &pagerModel[int]{ + ctx: t.Context(), + iter: iter, + pager: &templatePager{ + headerT: headerT, + rowT: rowT, + }, + pageSize: pageSize, + } +} + +// runCmd invokes a tea.Cmd and returns the message it produced. Fails the +// test if the cmd is nil. +func runCmd(t *testing.T, cmd tea.Cmd) tea.Msg { + t.Helper() + require.NotNil(t, cmd) + return cmd() +} + +// unwrapCmds returns the component cmds of a tea.Batch or tea.Sequence +// result. sequenceMsg is private to bubbletea; we identify it by the +// underlying []tea.Cmd slice kind and extract via reflect. +func unwrapCmds(t *testing.T, msg tea.Msg) []tea.Cmd { + t.Helper() + if bm, ok := msg.(tea.BatchMsg); ok { + return []tea.Cmd(bm) + } + rv := reflect.ValueOf(msg) + require.Equal(t, reflect.Slice, rv.Kind(), "expected a slice-of-cmds msg, got %T", msg) + cmds := make([]tea.Cmd, rv.Len()) + for i := range cmds { + c, ok := rv.Index(i).Interface().(tea.Cmd) + require.True(t, ok, "slice element %d is not a tea.Cmd", i) + cmds[i] = c + } + return cmds +} + +// printedText returns the body of a tea.Println message. printLineMessage +// is private to bubbletea, so we pull the first string field via reflect. +func printedText(t *testing.T, msg tea.Msg) string { + t.Helper() + rv := reflect.ValueOf(msg) + require.Equal(t, reflect.Struct, rv.Kind(), "expected a struct msg, got %T", msg) + for i := range rv.NumField() { + if rv.Field(i).Kind() == reflect.String { + return rv.Field(i).String() + } + } + t.Fatalf("no string field in %T", msg) + return "" +} + +func TestPagerModelInitFetchesFirstBatch(t *testing.T) { + m := newTestPager(t, &numberIterator{n: 3}, 10) + b, ok := runCmd(t, m.Init()).(batchMsg) + require.True(t, ok, "init should produce a batchMsg") + assert.True(t, b.done, "small iterator is drained in one batch") + assert.Len(t, b.lines, 3) +} + +func TestPagerModelBatchPrintsAndQuitsWhenDone(t *testing.T) { + m := newTestPager(t, &numberIterator{n: 3}, 10) + _, cmd := m.Update(batchMsg{lines: []string{"1", "2", "3"}, done: true}) + assert.True(t, m.iterDone) + assert.True(t, m.firstBatch) + cmds := unwrapCmds(t, runCmd(t, cmd)) + require.Len(t, cmds, 2) + assert.Contains(t, printedText(t, runCmd(t, cmds[0])), "1\n2\n3") + _, isQuit := runCmd(t, cmds[1]).(tea.QuitMsg) + assert.True(t, isQuit, "final cmd must quit once the iterator is drained") +} + +func TestPagerModelBatchDonePrintsHeaderOnlyEmptyIter(t *testing.T) { + m := newTestPager(t, &numberIterator{n: 0}, 10) + _, cmd := m.Update(batchMsg{lines: []string{"HEADER"}, done: true}) + cmds := unwrapCmds(t, runCmd(t, cmd)) + require.Len(t, cmds, 2) + assert.Equal(t, "HEADER", printedText(t, runCmd(t, cmds[0]))) +} + +func TestPagerModelBatchPromptsWhenMore(t *testing.T) { + m := newTestPager(t, &numberIterator{n: 100}, 5) + _, cmd := m.Update(batchMsg{lines: []string{"1", "2"}, done: false}) + assert.False(t, m.iterDone) + assert.True(t, m.firstBatch) + assert.False(t, m.drainAll) + assert.Equal(t, pagerPromptText, m.View()) + assert.Contains(t, printedText(t, runCmd(t, cmd)), "1\n2") +} + +func TestPagerModelBatchDrainingChainsFetch(t *testing.T) { + m := newTestPager(t, &numberIterator{n: 100}, 5) + m.drainAll = true + _, cmd := m.Update(batchMsg{lines: []string{"1", "2"}, done: false}) + cmds := unwrapCmds(t, runCmd(t, cmd)) + require.Len(t, cmds, 2) + assert.Contains(t, printedText(t, runCmd(t, cmds[0])), "1\n2") + _, isFetch := runCmd(t, cmds[1]).(batchMsg) + assert.True(t, isFetch, "draining must auto-fetch the next batch") +} + +func TestPagerModelBatchErrorTerminates(t *testing.T) { + m := newTestPager(t, &numberIterator{n: 0}, 5) + _, cmd := m.Update(batchMsg{err: errors.New("boom")}) + assert.EqualError(t, m.err, "boom") + _, isQuit := runCmd(t, cmd).(tea.QuitMsg) + assert.True(t, isQuit) +} + +func TestPagerModelSpaceFetchesNext(t *testing.T) { + cases := []struct { name string - in string - want string + key tea.KeyMsg }{ - {"no newlines", "abc", "abc"}, - {"single newline mid", "a\nb", "a\r\nb"}, - {"newline at end", "abc\n", "abc\r\n"}, - {"newline at start", "\nabc", "\r\nabc"}, - {"consecutive newlines", "\n\n", "\r\n\r\n"}, - {"multiple lines", "one\ntwo\nthree\n", "one\r\ntwo\r\nthree\r\n"}, + {"KeySpace", tea.KeyMsg{Type: tea.KeySpace}}, + {"KeyRunes space", tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{' '}}}, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - var buf bytes.Buffer - w := crlfWriter{w: &buf} - n, err := w.Write([]byte(tt.in)) - require.NoError(t, err) - assert.Equal(t, len(tt.in), n, "Write must return the input byte count") - assert.Equal(t, tt.want, buf.String()) + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + m := newTestPager(t, &numberIterator{n: 100}, 5) + m.firstBatch = true + _, cmd := m.Update(tc.key) + _, ok := runCmd(t, cmd).(batchMsg) + assert.True(t, ok, "space should fire a fetch") }) } } -func TestPagerShouldQuitDrainsNonQuitKeys(t *testing.T) { - ch := make(chan byte, 4) - ch <- ' ' - ch <- 'x' - ch <- 'y' - assert.False(t, pagerShouldQuit(ch), "non-quit keys must return false") - assert.Empty(t, ch, "non-quit keys must be drained from the channel") +func TestPagerModelEnterSetsDrainAll(t *testing.T) { + m := newTestPager(t, &numberIterator{n: 100}, 5) + m.firstBatch = true + _, cmd := m.Update(tea.KeyMsg{Type: tea.KeyEnter}) + assert.True(t, m.drainAll) + assert.Empty(t, m.View(), "no prompt while draining") + _, ok := runCmd(t, cmd).(batchMsg) + assert.True(t, ok, "enter should fire a fetch") +} + +func TestPagerModelEnterIsNoOpWhileAlreadyDraining(t *testing.T) { + m := newTestPager(t, &numberIterator{n: 100}, 5) + m.firstBatch = true + m.drainAll = true + _, cmd := m.Update(tea.KeyMsg{Type: tea.KeyEnter}) + assert.Nil(t, cmd, "re-entering drain shouldn't stack another fetch") } -func TestPagerShouldQuitReturnsTrueForQuitKeys(t *testing.T) { - for _, k := range []byte{'q', 'Q', pagerKeyEscape, pagerKeyCtrlC} { - ch := make(chan byte, 1) - ch <- k - assert.Truef(t, pagerShouldQuit(ch), "key %q must trigger quit", k) +func TestPagerModelSpaceIgnoredDuringFetch(t *testing.T) { + // Between Init and the first batchMsg, SPACE must not schedule a second + // fetch: doing so would run the iterator from two goroutines at once. + m := newTestPager(t, &numberIterator{n: 100}, 5) + m.fetching = true + _, cmd := m.Update(tea.KeyMsg{Type: tea.KeySpace}) + assert.Nil(t, cmd, "SPACE while fetching must not dispatch another fetch") +} + +func TestPagerModelEnterDuringFetchDefersFetch(t *testing.T) { + // ENTER during an in-flight fetch flips drainAll but can't issue a new + // fetch; the pending batchMsg will chain one when it arrives. + m := newTestPager(t, &numberIterator{n: 100}, 5) + m.fetching = true + _, cmd := m.Update(tea.KeyMsg{Type: tea.KeyEnter}) + assert.True(t, m.drainAll) + assert.Nil(t, cmd, "ENTER during fetch must defer to batchMsg chaining") +} + +func TestPagerModelQuitKeys(t *testing.T) { + cases := []struct { + name string + key tea.KeyMsg + }{ + {"q", tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'q'}}}, + {"Q", tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'Q'}}}, + {"esc", tea.KeyMsg{Type: tea.KeyEsc}}, + {"ctrl+c", tea.KeyMsg{Type: tea.KeyCtrlC}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + m := newTestPager(t, &numberIterator{n: 100}, 5) + m.firstBatch = true + _, cmd := m.Update(tc.key) + _, ok := runCmd(t, cmd).(tea.QuitMsg) + assert.True(t, ok) + }) } } -func TestPagerShouldQuitClosedChannelKeepsDraining(t *testing.T) { - ch := make(chan byte) - close(ch) - assert.False(t, pagerShouldQuit(ch), "closed channel (stdin EOF) must not force quit") +func TestPagerModelQuitKeysInterruptDrain(t *testing.T) { + for _, key := range []tea.KeyMsg{ + {Type: tea.KeyRunes, Runes: []rune{'q'}}, + {Type: tea.KeyEsc}, + {Type: tea.KeyCtrlC}, + } { + m := newTestPager(t, &numberIterator{n: 100}, 5) + m.firstBatch = true + m.drainAll = true + _, cmd := m.Update(key) + _, ok := runCmd(t, cmd).(tea.QuitMsg) + assert.True(t, ok, "quit keys must interrupt a drain") + } } -func TestPagerNextKeyReturnsFalseOnClosedChannel(t *testing.T) { - ch := make(chan byte) - close(ch) - _, ok := pagerNextKey(t.Context(), ch) - assert.False(t, ok) +func TestPagerModelIgnoresKeysAfterDone(t *testing.T) { + m := newTestPager(t, &numberIterator{n: 0}, 5) + m.iterDone = true + _, cmd := m.Update(tea.KeyMsg{Type: tea.KeyCtrlC}) + assert.Nil(t, cmd, "keys after completion should be no-ops") } -func TestPagerNextKeyReturnsKey(t *testing.T) { - ch := make(chan byte, 1) - ch <- ' ' - k, ok := pagerNextKey(t.Context(), ch) - assert.True(t, ok) - assert.Equal(t, byte(' '), k) +func TestPagerModelViewHiddenUntilFirstBatch(t *testing.T) { + m := newTestPager(t, &numberIterator{n: 10}, 5) + assert.Empty(t, m.View(), "prompt must not flash before any output is rendered") } From eb4058e08c302108c21f7e964fe4ad9ceccea60e Mon Sep 17 00:00:00 2001 From: simon Date: Thu, 23 Apr 2026 11:25:30 +0200 Subject: [PATCH 5/9] Trim narrative comments in the paged renderer Keep the non-obvious WHY comments (separate template.Template instances; WithoutSignalHandler for Ctrl+C) and drop the ones that just restate what the identifiers already say. Co-authored-by: Isaac --- libs/cmdio/paged_template_test.go | 33 +++++++----------------- libs/cmdio/pager.go | 42 +++++++++++-------------------- libs/cmdio/pager_test.go | 12 ++++----- 3 files changed, 29 insertions(+), 58 deletions(-) diff --git a/libs/cmdio/paged_template_test.go b/libs/cmdio/paged_template_test.go index f51f6c00324..24daeb45985 100644 --- a/libs/cmdio/paged_template_test.go +++ b/libs/cmdio/paged_template_test.go @@ -35,20 +35,17 @@ func (it *numberIterator) Next(_ context.Context) (int, error) { return it.pos, nil } -// ansiStripPattern removes CSI sequences so we can assert on plain content -// ignoring tea's cursor movement, line-clearing, and DEC-private escapes -// like bracketed-paste toggles (CSI ? 2004 l). +// ansiStripPattern is broader than ansiCSIPattern: tea emits non-SGR +// sequences (cursor moves, erase-line, bracketed-paste toggles) that +// the production width calculation doesn't need to strip. var ansiStripPattern = regexp.MustCompile("\x1b\\[[?]?[0-9;]*[A-Za-z]") func stripANSI(s string) string { return ansiStripPattern.ReplaceAllString(s, "") } -// pagedOutput drives a full paged render with tea.Program, returning the -// plain-text output after stripping ANSI escape sequences. It feeds the -// enter key via a pre-loaded reader, so drainAll is set either on the -// first KeyMsg or on the first batchMsg (whichever arrives first at the -// tea event loop); in both orderings the final output contains every row. +// pagedOutput runs a full paged render, feeding ENTER to auto-drain, +// and returns the ANSI-stripped output. func pagedOutput( t *testing.T, ctx context.Context, @@ -124,8 +121,6 @@ func TestPagedTemplateRendersHeaderAndRows(t *testing.T) { } func TestPagedTemplateEmptyIteratorStillFlushesHeader(t *testing.T) { - // Use a blocking reader: empty iterator finishes on the first fetch, so - // the pager quits before it ever needs a keystroke. pr, pw := io.Pipe() defer pw.Close() var out bytes.Buffer @@ -144,14 +139,10 @@ func TestPagedTemplateEmptyIteratorStillFlushesHeader(t *testing.T) { } func TestPagedTemplateColumnsStableAcrossBatches(t *testing.T) { - // First batch of rows is narrow; second batch contains a wider row. - // Widths are locked from the first page, so the wider row in batch 2 - // overflows its column instead of re-flowing prior batches. it := &numberIterator{n: 6} tmpl := "{{range .}}col-{{.}}\tval\n{{end}}" out := pagedOutput(t, t.Context(), it, "", tmpl, 3) lines := strings.Split(strings.TrimRight(out, "\n"), "\n") - // Find rows that contain "col-" (not the prompt) var dataRows []string for _, l := range lines { if strings.Contains(l, "col-") { @@ -159,8 +150,7 @@ func TestPagedTemplateColumnsStableAcrossBatches(t *testing.T) { } } require.Len(t, dataRows, 6) - // All rows start with "col-" followed by two spaces and then "val". - // The gap before "val" must be >= 2 spaces (tabwriter's minpad). + // Gap before "val" is the locked column width plus tabwriter minpad. for _, row := range dataRows { idx := strings.Index(row, "val") require.Positive(t, idx) @@ -168,10 +158,9 @@ func TestPagedTemplateColumnsStableAcrossBatches(t *testing.T) { } } -// TestPagedTemplateMatchesNonPagedForSmallList asserts that single-batch -// rendered content (after stripping tea's cursor-control escapes) carries -// the same rows and columns as the non-paged template renderer, so users -// who never see a second page see the same content they used to. +// TestPagedTemplateMatchesNonPagedForSmallList pins parity with the +// non-paged path so users who never see a second page see the same +// content they used to. func TestPagedTemplateMatchesNonPagedForSmallList(t *testing.T) { const rows = 5 tmpl := "{{range .}}{{green \"%d\" .}}\t{{.}}\n{{end}}" @@ -197,10 +186,6 @@ func TestPagedTemplateMatchesNonPagedForSmallList(t *testing.T) { assertSameContentLines(t, expected.String(), stripANSI(actual.String())) } -// assertSameContentLines compares two renderings for semantic equivalence: -// same set of non-empty trimmed lines. tabwriter and the manual padder in -// templatePager produce matching column alignment for a single batch, but -// trailing whitespace and newline counts can differ slightly. func assertSameContentLines(t *testing.T, want, got string) { t.Helper() wantLines := nonEmptyLines(want) diff --git a/libs/cmdio/pager.go b/libs/cmdio/pager.go index 37cce9382e1..bb16708d9fc 100644 --- a/libs/cmdio/pager.go +++ b/libs/cmdio/pager.go @@ -14,12 +14,9 @@ const pagerPageSize = 50 // pagerPromptText is shown between pages. const pagerPromptText = "[space] more [enter] all [q|esc] quit" -// pagerModel drives the paged render loop through bubbletea. It owns the -// iterator, fetches one batch at a time via a tea.Cmd, emits rendered -// lines above the view with tea.Println, and shows the prompt while it -// waits for a key. Using bubbletea lets us skip manual raw-mode setup -// (tea enters/restores raw mode on its own), so we don't need a parallel -// CRLF translator for output written during raw mode. +// pagerModel is the tea.Model that drives the paged render loop: one +// fetchCmd produces a batchMsg, Update prints it via tea.Println, and +// View shows the prompt between pages. type pagerModel[T any] struct { ctx context.Context iter listing.Iterator[T] @@ -28,11 +25,10 @@ type pagerModel[T any] struct { limit int total int - // fetching tracks whether a fetchCmd is in flight. We only ever keep - // one in flight at a time so the iterator isn't read from two - // goroutines; if the user hits SPACE or ENTER while one is running, - // we record the intent in drainAll and let the in-flight fetch - // continue in drain mode when its batchMsg lands. + // Keep only one fetchCmd in flight at a time: the iterator is not + // safe to read from two goroutines. If SPACE or ENTER arrives while + // fetching, drainAll is recorded and the pending batchMsg chains + // the next fetch. fetching bool drainAll bool firstBatch bool @@ -40,9 +36,8 @@ type pagerModel[T any] struct { err error } -// batchMsg is delivered to Update when a fetched batch has been rendered -// into printable lines. done signals the iterator is exhausted (or the -// limit is reached); err surfaces iteration errors. +// batchMsg carries the rendered lines from one fetchCmd. done is true +// when the iterator is exhausted or the limit is reached. type batchMsg struct { lines []string done bool @@ -54,9 +49,8 @@ func (m *pagerModel[T]) Init() tea.Cmd { return m.fetchCmd() } -// fetchCmd returns a tea.Cmd that reads one page from the iterator and -// renders it into lines. The command runs off the update loop so slow -// network fetches don't stall key handling. +// fetchCmd runs off the update loop so a slow network fetch doesn't +// stall key handling. func (m *pagerModel[T]) fetchCmd() tea.Cmd { return func() tea.Msg { buf := make([]any, 0, m.pageSize) @@ -94,8 +88,8 @@ func (m *pagerModel[T]) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, tea.Quit } m.firstBatch = true - // Collapse the batch into a single Println so ordering is trivial: - // tea.Println splits on \n internally, so one Cmd emits all rows. + // One Println cmd (not N) keeps the batch ordered even though + // tea.Sequence dispatches each cmd on its own goroutine. var printCmd tea.Cmd if len(msg.lines) > 0 { printCmd = tea.Println(strings.Join(msg.lines, "\n")) @@ -120,9 +114,6 @@ func (m *pagerModel[T]) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, nil } -// handleKey routes a keystroke to the right state transition. Keys we -// don't care about are ignored so the user can mash the keyboard without -// affecting the pager. func (m *pagerModel[T]) handleKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { switch msg.Type { //nolint:exhaustive // the pager only cares about a few keys case tea.KeyEnter: @@ -142,8 +133,6 @@ func (m *pagerModel[T]) handleKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { return m, nil } -// startAdvance handles SPACE: fetch one more page unless we're already -// fetching or draining. If a fetch is in flight we drop the keystroke. func (m *pagerModel[T]) startAdvance() tea.Cmd { if m.drainAll || m.fetching { return nil @@ -152,14 +141,13 @@ func (m *pagerModel[T]) startAdvance() tea.Cmd { return m.fetchCmd() } -// startDrain handles ENTER: flip into drain-all mode. If a fetch is -// already in flight the batchMsg handler will see drainAll and continue -// fetching; otherwise we kick off the next fetch here. func (m *pagerModel[T]) startDrain() tea.Cmd { if m.drainAll { return nil } m.drainAll = true + // If a fetch is already in flight, its batchMsg will see drainAll + // and chain the next fetch. Otherwise kick one off here. if m.fetching { return nil } diff --git a/libs/cmdio/pager_test.go b/libs/cmdio/pager_test.go index b30058a43db..236fe5e0378 100644 --- a/libs/cmdio/pager_test.go +++ b/libs/cmdio/pager_test.go @@ -29,17 +29,15 @@ func newTestPager(t *testing.T, iter listing.Iterator[int], pageSize int) *pager } } -// runCmd invokes a tea.Cmd and returns the message it produced. Fails the -// test if the cmd is nil. func runCmd(t *testing.T, cmd tea.Cmd) tea.Msg { t.Helper() require.NotNil(t, cmd) return cmd() } -// unwrapCmds returns the component cmds of a tea.Batch or tea.Sequence -// result. sequenceMsg is private to bubbletea; we identify it by the -// underlying []tea.Cmd slice kind and extract via reflect. +// unwrapCmds pulls the cmds out of a tea.Batch/tea.Sequence result. +// sequenceMsg is unexported, so we fall back to reflect on the []tea.Cmd +// underlying type — update if bubbletea renames it. func unwrapCmds(t *testing.T, msg tea.Msg) []tea.Cmd { t.Helper() if bm, ok := msg.(tea.BatchMsg); ok { @@ -56,8 +54,8 @@ func unwrapCmds(t *testing.T, msg tea.Msg) []tea.Cmd { return cmds } -// printedText returns the body of a tea.Println message. printLineMessage -// is private to bubbletea, so we pull the first string field via reflect. +// printedText pulls the body out of a tea.Println result. +// printLineMessage is unexported, so reflect on its only string field. func printedText(t *testing.T, msg tea.Msg) string { t.Helper() rv := reflect.ValueOf(msg) From 00e671c6fe119329dbec7abd35fa1091aca40342 Mon Sep 17 00:00:00 2001 From: simon Date: Thu, 23 Apr 2026 11:48:07 +0200 Subject: [PATCH 6/9] Align pager stream doc and rename firstBatch Two cleanups from codex review: - SupportsPager's doc said "stderr for the prompt", which was true under the old x/term path. With bubbletea, View and Println share one stream, and we route both to stdout so paged and non-paged renders land in the same place. Update the comment to match. - firstBatch flipped to true on every batch, not just the first; rename to hasPrinted to match View()'s actual gate. Co-authored-by: Isaac --- libs/cmdio/capabilities.go | 8 +++++--- libs/cmdio/pager.go | 6 +++--- libs/cmdio/pager_test.go | 14 +++++++------- 3 files changed, 15 insertions(+), 13 deletions(-) diff --git a/libs/cmdio/capabilities.go b/libs/cmdio/capabilities.go index 56a13562d90..5e5f8194ab8 100644 --- a/libs/cmdio/capabilities.go +++ b/libs/cmdio/capabilities.go @@ -48,9 +48,11 @@ func (c Capabilities) SupportsColor(w io.Writer) bool { return isTTY(w) && c.color } -// SupportsPager returns true when all three std streams are TTYs (stdin -// for keystrokes, stdout for content, stderr for the prompt). Git Bash -// is excluded because raw-mode stdin reads are unreliable there. +// SupportsPager returns true when all three std streams are TTYs. The +// pager reads keys from stdin and drives both content and prompt through +// bubbletea on stdout; stderr must also be a TTY so diagnostics or +// spinners that fire mid-drain don't interleave into a redirected log. +// Git Bash is excluded because its raw-mode stdin is unreliable. func (c Capabilities) SupportsPager() bool { return c.stdinIsTTY && c.stdoutIsTTY && c.stderrIsTTY && !c.isGitBash } diff --git a/libs/cmdio/pager.go b/libs/cmdio/pager.go index bb16708d9fc..878d77d8250 100644 --- a/libs/cmdio/pager.go +++ b/libs/cmdio/pager.go @@ -31,7 +31,7 @@ type pagerModel[T any] struct { // the next fetch. fetching bool drainAll bool - firstBatch bool + hasPrinted bool iterDone bool err error } @@ -87,7 +87,7 @@ func (m *pagerModel[T]) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.err = msg.err return m, tea.Quit } - m.firstBatch = true + m.hasPrinted = true // One Println cmd (not N) keeps the batch ordered even though // tea.Sequence dispatches each cmd on its own goroutine. var printCmd tea.Cmd @@ -156,7 +156,7 @@ func (m *pagerModel[T]) startDrain() tea.Cmd { } func (m *pagerModel[T]) View() string { - if m.iterDone || m.drainAll || m.err != nil || !m.firstBatch { + if m.iterDone || m.drainAll || m.err != nil || !m.hasPrinted { return "" } return pagerPromptText diff --git a/libs/cmdio/pager_test.go b/libs/cmdio/pager_test.go index 236fe5e0378..5b0110528a7 100644 --- a/libs/cmdio/pager_test.go +++ b/libs/cmdio/pager_test.go @@ -81,7 +81,7 @@ func TestPagerModelBatchPrintsAndQuitsWhenDone(t *testing.T) { m := newTestPager(t, &numberIterator{n: 3}, 10) _, cmd := m.Update(batchMsg{lines: []string{"1", "2", "3"}, done: true}) assert.True(t, m.iterDone) - assert.True(t, m.firstBatch) + assert.True(t, m.hasPrinted) cmds := unwrapCmds(t, runCmd(t, cmd)) require.Len(t, cmds, 2) assert.Contains(t, printedText(t, runCmd(t, cmds[0])), "1\n2\n3") @@ -101,7 +101,7 @@ func TestPagerModelBatchPromptsWhenMore(t *testing.T) { m := newTestPager(t, &numberIterator{n: 100}, 5) _, cmd := m.Update(batchMsg{lines: []string{"1", "2"}, done: false}) assert.False(t, m.iterDone) - assert.True(t, m.firstBatch) + assert.True(t, m.hasPrinted) assert.False(t, m.drainAll) assert.Equal(t, pagerPromptText, m.View()) assert.Contains(t, printedText(t, runCmd(t, cmd)), "1\n2") @@ -137,7 +137,7 @@ func TestPagerModelSpaceFetchesNext(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { m := newTestPager(t, &numberIterator{n: 100}, 5) - m.firstBatch = true + m.hasPrinted = true _, cmd := m.Update(tc.key) _, ok := runCmd(t, cmd).(batchMsg) assert.True(t, ok, "space should fire a fetch") @@ -147,7 +147,7 @@ func TestPagerModelSpaceFetchesNext(t *testing.T) { func TestPagerModelEnterSetsDrainAll(t *testing.T) { m := newTestPager(t, &numberIterator{n: 100}, 5) - m.firstBatch = true + m.hasPrinted = true _, cmd := m.Update(tea.KeyMsg{Type: tea.KeyEnter}) assert.True(t, m.drainAll) assert.Empty(t, m.View(), "no prompt while draining") @@ -157,7 +157,7 @@ func TestPagerModelEnterSetsDrainAll(t *testing.T) { func TestPagerModelEnterIsNoOpWhileAlreadyDraining(t *testing.T) { m := newTestPager(t, &numberIterator{n: 100}, 5) - m.firstBatch = true + m.hasPrinted = true m.drainAll = true _, cmd := m.Update(tea.KeyMsg{Type: tea.KeyEnter}) assert.Nil(t, cmd, "re-entering drain shouldn't stack another fetch") @@ -195,7 +195,7 @@ func TestPagerModelQuitKeys(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { m := newTestPager(t, &numberIterator{n: 100}, 5) - m.firstBatch = true + m.hasPrinted = true _, cmd := m.Update(tc.key) _, ok := runCmd(t, cmd).(tea.QuitMsg) assert.True(t, ok) @@ -210,7 +210,7 @@ func TestPagerModelQuitKeysInterruptDrain(t *testing.T) { {Type: tea.KeyCtrlC}, } { m := newTestPager(t, &numberIterator{n: 100}, 5) - m.firstBatch = true + m.hasPrinted = true m.drainAll = true _, cmd := m.Update(key) _, ok := runCmd(t, cmd).(tea.QuitMsg) From 030e79c351e122ad616e9c8175ff68f4691761bf Mon Sep 17 00:00:00 2001 From: simon Date: Thu, 23 Apr 2026 11:57:31 +0200 Subject: [PATCH 7/9] Show a loading spinner while the pager fetches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the empty view during a fetch with the same braille spinner the rest of the CLI uses, plus a "loading…" label. Appears on initial load, after SPACE between pages, and while draining via ENTER. Cleared as soon as the batchMsg lands and the next prompt (or more content) is ready. Co-authored-by: Isaac --- libs/cmdio/paged_template.go | 1 + libs/cmdio/pager.go | 36 +++++++++++++++++++++++++++++++++--- libs/cmdio/pager_test.go | 21 ++++++++++++++++++--- 3 files changed, 52 insertions(+), 6 deletions(-) diff --git a/libs/cmdio/paged_template.go b/libs/cmdio/paged_template.go index 098950f4810..bdb05be4c9d 100644 --- a/libs/cmdio/paged_template.go +++ b/libs/cmdio/paged_template.go @@ -106,6 +106,7 @@ func renderIteratorPagedTemplateCore[T any]( ctx: ctx, iter: iter, pager: pager, + spinner: newPagerSpinner(), pageSize: pageSize, limit: limitFromContext(ctx), } diff --git a/libs/cmdio/pager.go b/libs/cmdio/pager.go index 878d77d8250..caf0e6b3f95 100644 --- a/libs/cmdio/pager.go +++ b/libs/cmdio/pager.go @@ -3,8 +3,11 @@ package cmdio import ( "context" "strings" + "time" + bubblespinner "github.com/charmbracelet/bubbles/spinner" tea "github.com/charmbracelet/bubbletea" + "github.com/charmbracelet/lipgloss" "github.com/databricks/databricks-sdk-go/listing" ) @@ -14,6 +17,9 @@ const pagerPageSize = 50 // pagerPromptText is shown between pages. const pagerPromptText = "[space] more [enter] all [q|esc] quit" +// pagerLoadingText is appended to the spinner while a fetch is in flight. +const pagerLoadingText = "loading…" + // pagerModel is the tea.Model that drives the paged render loop: one // fetchCmd produces a batchMsg, Update prints it via tea.Println, and // View shows the prompt between pages. @@ -21,6 +27,7 @@ type pagerModel[T any] struct { ctx context.Context iter listing.Iterator[T] pager *templatePager + spinner bubblespinner.Model pageSize int limit int total int @@ -36,6 +43,18 @@ type pagerModel[T any] struct { err error } +// newPagerSpinner builds a spinner matching the one the cmdio package's +// NewSpinner uses, so interactive feedback looks the same everywhere. +func newPagerSpinner() bubblespinner.Model { + s := bubblespinner.New() + s.Spinner = bubblespinner.Spinner{ + Frames: []string{"⣾", "⣽", "⣻", "⢿", "⡿", "⣟", "⣯", "⣷"}, + FPS: time.Second / 5, + } + s.Style = lipgloss.NewStyle().Foreground(lipgloss.Color("10")) + return s +} + // batchMsg carries the rendered lines from one fetchCmd. done is true // when the iterator is exhausted or the limit is reached. type batchMsg struct { @@ -46,7 +65,7 @@ type batchMsg struct { func (m *pagerModel[T]) Init() tea.Cmd { m.fetching = true - return m.fetchCmd() + return tea.Batch(m.fetchCmd(), m.spinner.Tick) } // fetchCmd runs off the update loop so a slow network fetch doesn't @@ -81,6 +100,11 @@ func (m *pagerModel[T]) fetchCmd() tea.Cmd { func (m *pagerModel[T]) Update(msg tea.Msg) (tea.Model, tea.Cmd) { switch msg := msg.(type) { + case bubblespinner.TickMsg: + var cmd tea.Cmd + m.spinner, cmd = m.spinner.Update(msg) + return m, cmd + case batchMsg: m.fetching = false if msg.err != nil { @@ -156,8 +180,14 @@ func (m *pagerModel[T]) startDrain() tea.Cmd { } func (m *pagerModel[T]) View() string { - if m.iterDone || m.drainAll || m.err != nil || !m.hasPrinted { + switch { + case m.iterDone || m.err != nil: + return "" + case m.fetching: + return m.spinner.View() + " " + pagerLoadingText + case m.drainAll || !m.hasPrinted: return "" + default: + return pagerPromptText } - return pagerPromptText } diff --git a/libs/cmdio/pager_test.go b/libs/cmdio/pager_test.go index 5b0110528a7..0c859d462a1 100644 --- a/libs/cmdio/pager_test.go +++ b/libs/cmdio/pager_test.go @@ -25,6 +25,7 @@ func newTestPager(t *testing.T, iter listing.Iterator[int], pageSize int) *pager headerT: headerT, rowT: rowT, }, + spinner: newPagerSpinner(), pageSize: pageSize, } } @@ -71,10 +72,17 @@ func printedText(t *testing.T, msg tea.Msg) string { func TestPagerModelInitFetchesFirstBatch(t *testing.T) { m := newTestPager(t, &numberIterator{n: 3}, 10) - b, ok := runCmd(t, m.Init()).(batchMsg) - require.True(t, ok, "init should produce a batchMsg") + // Init returns a tea.Batch(fetchCmd, spinner.Tick); find the fetch. + var b batchMsg + for _, c := range unwrapCmds(t, runCmd(t, m.Init())) { + if msg, ok := c().(batchMsg); ok { + b = msg + break + } + } assert.True(t, b.done, "small iterator is drained in one batch") assert.Len(t, b.lines, 3) + assert.True(t, m.fetching, "Init must mark the model as fetching") } func TestPagerModelBatchPrintsAndQuitsWhenDone(t *testing.T) { @@ -150,7 +158,7 @@ func TestPagerModelEnterSetsDrainAll(t *testing.T) { m.hasPrinted = true _, cmd := m.Update(tea.KeyMsg{Type: tea.KeyEnter}) assert.True(t, m.drainAll) - assert.Empty(t, m.View(), "no prompt while draining") + assert.NotContains(t, m.View(), pagerPromptText, "no prompt while draining") _, ok := runCmd(t, cmd).(batchMsg) assert.True(t, ok, "enter should fire a fetch") } @@ -229,3 +237,10 @@ func TestPagerModelViewHiddenUntilFirstBatch(t *testing.T) { m := newTestPager(t, &numberIterator{n: 10}, 5) assert.Empty(t, m.View(), "prompt must not flash before any output is rendered") } + +func TestPagerModelViewShowsSpinnerWhileFetching(t *testing.T) { + m := newTestPager(t, &numberIterator{n: 100}, 5) + m.fetching = true + assert.Contains(t, m.View(), pagerLoadingText) + assert.NotContains(t, m.View(), pagerPromptText) +} From ea9e103fa5978458058fd6a4f2b6efd512108ce1 Mon Sep 17 00:00:00 2001 From: simon Date: Thu, 23 Apr 2026 12:48:17 +0200 Subject: [PATCH 8/9] Address review: ctx, pageSize, plumbing, and loading ellipsis MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Thread c.in through renderIteratorPagedTemplate instead of hardcoding os.Stdin, so the cmdIO override is honored. - SupportsPager = SupportsPrompt() && stdoutIsTTY, so the pager picks up the Git Bash and stdin checks from one place. - Drop ctx from pagerModel. newPagerModel binds the fetch closure with the caller's context so tea.Cmd doesn't need a context parameter and the struct stays ctx-free. - Size pages from tea.WindowSizeMsg (height - overhead, floored at 5). 50 is kept only as the pre-resize fallback. - "loading…" -> "loading..." for portability. - Note near p.Run why the pager doesn't need the acquire/release dance the spinner does. Co-authored-by: Isaac --- libs/cmdio/capabilities.go | 11 ++-- libs/cmdio/paged_template.go | 17 +++--- libs/cmdio/pager.go | 112 +++++++++++++++++++++++------------ libs/cmdio/pager_test.go | 29 +++++---- libs/cmdio/render.go | 2 +- 5 files changed, 104 insertions(+), 67 deletions(-) diff --git a/libs/cmdio/capabilities.go b/libs/cmdio/capabilities.go index 5e5f8194ab8..62ac4b6ae91 100644 --- a/libs/cmdio/capabilities.go +++ b/libs/cmdio/capabilities.go @@ -48,13 +48,12 @@ func (c Capabilities) SupportsColor(w io.Writer) bool { return isTTY(w) && c.color } -// SupportsPager returns true when all three std streams are TTYs. The -// pager reads keys from stdin and drives both content and prompt through -// bubbletea on stdout; stderr must also be a TTY so diagnostics or -// spinners that fire mid-drain don't interleave into a redirected log. -// Git Bash is excluded because its raw-mode stdin is unreliable. +// SupportsPager returns true when we can drive an interactive pager. +// It builds on SupportsPrompt (stderr+stdin TTY, not Git Bash) and +// additionally requires stdout to be a TTY so rendered rows land on +// the terminal rather than a redirected file. func (c Capabilities) SupportsPager() bool { - return c.stdinIsTTY && c.stdoutIsTTY && c.stderrIsTTY && !c.isGitBash + return c.SupportsPrompt() && c.stdoutIsTTY } // detectGitBash returns true if running in Git Bash on Windows (has broken promptui support). diff --git a/libs/cmdio/paged_template.go b/libs/cmdio/paged_template.go index bdb05be4c9d..a579fa48afd 100644 --- a/libs/cmdio/paged_template.go +++ b/libs/cmdio/paged_template.go @@ -4,7 +4,6 @@ import ( "bytes" "context" "io" - "os" "regexp" "strings" "text/template" @@ -25,10 +24,11 @@ var ansiCSIPattern = regexp.MustCompile("\x1b\\[[0-9;]*m") func renderIteratorPagedTemplate[T any]( ctx context.Context, iter listing.Iterator[T], + in io.Reader, out io.Writer, headerTemplate, tmpl string, ) error { - return renderIteratorPagedTemplateCore(ctx, iter, os.Stdin, out, headerTemplate, tmpl, pagerPageSize) + return renderIteratorPagedTemplateCore(ctx, iter, in, out, headerTemplate, tmpl, pagerFallbackPageSize) } // templatePager renders accumulated rows, locking column widths from the @@ -102,14 +102,7 @@ func renderIteratorPagedTemplateCore[T any]( rowT: rowT, headerStr: headerTemplate, } - m := &pagerModel[T]{ - ctx: ctx, - iter: iter, - pager: pager, - spinner: newPagerSpinner(), - pageSize: pageSize, - limit: limitFromContext(ctx), - } + m := newPagerModel(ctx, iter, pager, pageSize, limitFromContext(ctx)) p := tea.NewProgram( m, tea.WithInput(in), @@ -118,6 +111,10 @@ func renderIteratorPagedTemplateCore[T any]( // so Ctrl+C also interrupts a stalled iterator fetch. tea.WithoutSignalHandler(), ) + // Unlike cmdio.NewSpinner, the pager doesn't need to acquire/release + // through cmdIO: p.Run is blocking and tea restores the terminal on + // its own before returning, so there's no other tea.Program that could + // race with ours. if _, err := p.Run(); err != nil { return err } diff --git a/libs/cmdio/pager.go b/libs/cmdio/pager.go index caf0e6b3f95..4149753218a 100644 --- a/libs/cmdio/pager.go +++ b/libs/cmdio/pager.go @@ -11,29 +11,41 @@ import ( "github.com/databricks/databricks-sdk-go/listing" ) -// pagerPageSize is the number of items rendered between prompts. -const pagerPageSize = 50 +// pagerFallbackPageSize is used before the first WindowSizeMsg arrives, +// and when the terminal height is too small to size a page by itself. +const pagerFallbackPageSize = 50 + +// pagerMinPageSize is the floor: one line of header plus a few rows so +// the prompt still has something to sit under. +const pagerMinPageSize = 5 + +// pagerViewOverhead is the number of lines we keep below the printed +// rows so the prompt (or spinner) doesn't force the top row off-screen. +const pagerViewOverhead = 2 // pagerPromptText is shown between pages. const pagerPromptText = "[space] more [enter] all [q|esc] quit" // pagerLoadingText is appended to the spinner while a fetch is in flight. -const pagerLoadingText = "loading…" +const pagerLoadingText = "loading..." // pagerModel is the tea.Model that drives the paged render loop: one -// fetchCmd produces a batchMsg, Update prints it via tea.Println, and +// fetch produces a batchMsg, Update prints it via tea.Println, and // View shows the prompt between pages. type pagerModel[T any] struct { - ctx context.Context - iter listing.Iterator[T] - pager *templatePager - spinner bubblespinner.Model + iter listing.Iterator[T] + pager *templatePager + spinner bubblespinner.Model + // fetch is bound at construction with the caller's context captured + // so we don't have to stash ctx on the struct (tea.Cmd has no ctx + // parameter of its own). + fetch func() tea.Msg pageSize int limit int total int - // Keep only one fetchCmd in flight at a time: the iterator is not - // safe to read from two goroutines. If SPACE or ENTER arrives while + // Keep only one fetch in flight at a time: the iterator is not safe + // to read from two goroutines. If SPACE or ENTER arrives while // fetching, drainAll is recorded and the pending batchMsg chains // the next fetch. fetching bool @@ -43,6 +55,25 @@ type pagerModel[T any] struct { err error } +// newPagerModel wires ctx into the fetch closure so nothing on the +// struct has to hold onto a context. +func newPagerModel[T any]( + ctx context.Context, + iter listing.Iterator[T], + pager *templatePager, + pageSize, limit int, +) *pagerModel[T] { + m := &pagerModel[T]{ + iter: iter, + pager: pager, + spinner: newPagerSpinner(), + pageSize: pageSize, + limit: limit, + } + m.fetch = func() tea.Msg { return m.doFetch(ctx) } + return m +} + // newPagerSpinner builds a spinner matching the one the cmdio package's // NewSpinner uses, so interactive feedback looks the same everywhere. func newPagerSpinner() bubblespinner.Model { @@ -55,8 +86,8 @@ func newPagerSpinner() bubblespinner.Model { return s } -// batchMsg carries the rendered lines from one fetchCmd. done is true -// when the iterator is exhausted or the limit is reached. +// batchMsg carries the rendered lines from one fetch. done is true when +// the iterator is exhausted or the limit is reached. type batchMsg struct { lines []string done bool @@ -65,41 +96,44 @@ type batchMsg struct { func (m *pagerModel[T]) Init() tea.Cmd { m.fetching = true - return tea.Batch(m.fetchCmd(), m.spinner.Tick) + return tea.Batch(m.fetch, m.spinner.Tick) } -// fetchCmd runs off the update loop so a slow network fetch doesn't -// stall key handling. -func (m *pagerModel[T]) fetchCmd() tea.Cmd { - return func() tea.Msg { - buf := make([]any, 0, m.pageSize) - done := false - for len(buf) < m.pageSize { - if m.limit > 0 && m.total+len(buf) >= m.limit { - done = true - break - } - if !m.iter.HasNext(m.ctx) { - done = true - break - } - n, err := m.iter.Next(m.ctx) - if err != nil { - return batchMsg{err: err} - } - buf = append(buf, n) +// doFetch reads one page from the iterator and renders it into lines. +// It runs off the update loop so a slow network fetch doesn't stall +// key handling. +func (m *pagerModel[T]) doFetch(ctx context.Context) tea.Msg { + buf := make([]any, 0, m.pageSize) + done := false + for len(buf) < m.pageSize { + if m.limit > 0 && m.total+len(buf) >= m.limit { + done = true + break + } + if !m.iter.HasNext(ctx) { + done = true + break } - lines, err := m.pager.flushLines(buf) + n, err := m.iter.Next(ctx) if err != nil { return batchMsg{err: err} } - m.total += len(buf) - return batchMsg{lines: lines, done: done} + buf = append(buf, n) } + lines, err := m.pager.flushLines(buf) + if err != nil { + return batchMsg{err: err} + } + m.total += len(buf) + return batchMsg{lines: lines, done: done} } func (m *pagerModel[T]) Update(msg tea.Msg) (tea.Model, tea.Cmd) { switch msg := msg.(type) { + case tea.WindowSizeMsg: + m.pageSize = max(msg.Height-pagerViewOverhead, pagerMinPageSize) + return m, nil + case bubblespinner.TickMsg: var cmd tea.Cmd m.spinner, cmd = m.spinner.Update(msg) @@ -124,7 +158,7 @@ func (m *pagerModel[T]) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, tea.Sequence(printCmd, tea.Quit) case m.drainAll: m.fetching = true - return m, tea.Sequence(printCmd, m.fetchCmd()) + return m, tea.Sequence(printCmd, m.fetch) default: return m, printCmd } @@ -162,7 +196,7 @@ func (m *pagerModel[T]) startAdvance() tea.Cmd { return nil } m.fetching = true - return m.fetchCmd() + return m.fetch } func (m *pagerModel[T]) startDrain() tea.Cmd { @@ -176,7 +210,7 @@ func (m *pagerModel[T]) startDrain() tea.Cmd { return nil } m.fetching = true - return m.fetchCmd() + return m.fetch } func (m *pagerModel[T]) View() string { diff --git a/libs/cmdio/pager_test.go b/libs/cmdio/pager_test.go index 0c859d462a1..0153d5eac06 100644 --- a/libs/cmdio/pager_test.go +++ b/libs/cmdio/pager_test.go @@ -18,16 +18,10 @@ func newTestPager(t *testing.T, iter listing.Iterator[int], pageSize int) *pager require.NoError(t, err) headerT, err := template.New("header").Funcs(renderFuncMap).Parse("") require.NoError(t, err) - return &pagerModel[int]{ - ctx: t.Context(), - iter: iter, - pager: &templatePager{ - headerT: headerT, - rowT: rowT, - }, - spinner: newPagerSpinner(), - pageSize: pageSize, - } + return newPagerModel(t.Context(), iter, &templatePager{ + headerT: headerT, + rowT: rowT, + }, pageSize, 0) } func runCmd(t *testing.T, cmd tea.Cmd) tea.Msg { @@ -72,7 +66,7 @@ func printedText(t *testing.T, msg tea.Msg) string { func TestPagerModelInitFetchesFirstBatch(t *testing.T) { m := newTestPager(t, &numberIterator{n: 3}, 10) - // Init returns a tea.Batch(fetchCmd, spinner.Tick); find the fetch. + // Init returns a tea.Batch(m.fetch, spinner.Tick); find the fetch. var b batchMsg for _, c := range unwrapCmds(t, runCmd(t, m.Init())) { if msg, ok := c().(batchMsg); ok { @@ -244,3 +238,16 @@ func TestPagerModelViewShowsSpinnerWhileFetching(t *testing.T) { assert.Contains(t, m.View(), pagerLoadingText) assert.NotContains(t, m.View(), pagerPromptText) } + +func TestPagerModelWindowSizeResizesPage(t *testing.T) { + m := newTestPager(t, &numberIterator{n: 100}, 50) + _, cmd := m.Update(tea.WindowSizeMsg{Height: 30, Width: 120}) + assert.Nil(t, cmd, "resize should not itself dispatch a command") + assert.Equal(t, 30-pagerViewOverhead, m.pageSize) +} + +func TestPagerModelWindowSizeFloorsAtMin(t *testing.T) { + m := newTestPager(t, &numberIterator{n: 100}, 50) + _, _ = m.Update(tea.WindowSizeMsg{Height: 3, Width: 80}) + assert.Equal(t, pagerMinPageSize, m.pageSize) +} diff --git a/libs/cmdio/render.go b/libs/cmdio/render.go index eb2459d2dfa..fec2f222545 100644 --- a/libs/cmdio/render.go +++ b/libs/cmdio/render.go @@ -282,7 +282,7 @@ func Render(ctx context.Context, v any) error { func RenderIterator[T any](ctx context.Context, i listing.Iterator[T]) error { c := fromContext(ctx) if c.capabilities.SupportsPager() && c.outputFormat == flags.OutputText && c.template != "" { - return renderIteratorPagedTemplate(ctx, i, c.out, c.headerTemplate, c.template) + return renderIteratorPagedTemplate(ctx, i, c.in, c.out, c.headerTemplate, c.template) } return renderWithTemplate(ctx, newIteratorRenderer(i), c.outputFormat, c.out, c.headerTemplate, c.template) } From bf33ceec9fa5bd69a50847e8ffd5056c10052f8a Mon Sep 17 00:00:00 2001 From: simon Date: Thu, 23 Apr 2026 14:52:06 +0200 Subject: [PATCH 9/9] Reserve 1 line for the prompt, not 2 pagerViewOverhead was 2 on the assumption of header + prompt, but the header is part of the already-printed rows, not the reserved region below. Only the prompt (or spinner) line sits under the current page. Co-authored-by: Isaac --- libs/cmdio/pager.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libs/cmdio/pager.go b/libs/cmdio/pager.go index 4149753218a..6070e2f2ff1 100644 --- a/libs/cmdio/pager.go +++ b/libs/cmdio/pager.go @@ -20,8 +20,8 @@ const pagerFallbackPageSize = 50 const pagerMinPageSize = 5 // pagerViewOverhead is the number of lines we keep below the printed -// rows so the prompt (or spinner) doesn't force the top row off-screen. -const pagerViewOverhead = 2 +// rows for the prompt (or spinner). +const pagerViewOverhead = 1 // pagerPromptText is shown between pages. const pagerPromptText = "[space] more [enter] all [q|esc] quit"