From 6d9735070cda73b81e757498de19a8e82a476b85 Mon Sep 17 00:00:00 2001 From: fullstackjam Date: Sat, 18 Jul 2026 00:38:05 +0800 Subject: [PATCH] fix(ui): drop scroll-region progress bar that garbled non-DECSTBM terminals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit StickyProgress reserved the bottom rows as a sticky status bar via a DECSTBM scroll region. On terminals that report a normal TERM but don't honour scroll regions, every newline scrolled the whole screen, so the reserved bar was dragged into the log and reprinted on every tick — the install read as repeated, overprinted garbage. Most visible during npm, whose fast per-package installs redraw the bar rapidly. IsScrollRegionSupported() only checked TTY + TERM + height; it could not detect actual scroll-region support, so it enabled the fragile path on the terminals that break it. Replace it with a plain in-place \r\033[K status line that trails top-to-bottom output on every terminal, and remove the scroll-region plumbing (scrollregion.go) and its failure mode entirely. --- docs/HARNESS.md | 15 ++-- internal/ui/AGENTS.md | 3 +- internal/ui/progress.go | 63 +++----------- internal/ui/progress_test.go | 18 ++-- internal/ui/scrollregion.go | 143 ------------------------------- internal/ui/scrollregion_test.go | 90 ------------------- 6 files changed, 35 insertions(+), 297 deletions(-) delete mode 100644 internal/ui/scrollregion.go delete mode 100644 internal/ui/scrollregion_test.go diff --git a/docs/HARNESS.md b/docs/HARNESS.md index 00d240e..b040f85 100644 --- a/docs/HARNESS.md +++ b/docs/HARNESS.md @@ -109,11 +109,16 @@ it survives doc rot. the in-session loop (Step 8), the sensor became dead code and was removed. If `--auto` ever comes back, the sensor needs to come back with it. -- **No archtest rule for scroll-region usage.** `internal/ui/scrollregion.go` - detects support at runtime (`TERM`, TTY, terminal height) and falls back - to the inline `\r\033[K` renderer when unavailable. A static rule can't - see runtime terminal capabilities, so this stays a runtime concern. The - fallback is covered by `TestStickyProgressFallsBackWhenScrollRegionUnsupported`. +- **No terminal scroll-region rendering.** StickyProgress once reserved the + bottom rows via a DECSTBM scroll region (`internal/ui/scrollregion.go`, + since removed) to keep a "sticky" status bar frozen while the log scrolled. + It rendered correctly only on terminals that fully honour scroll regions; + on ones that report a normal `TERM` but don't (e.g. some GPU/block + terminals), the reserved bar was dragged into the log and reprinted on + every tick, so the install read as garbage. Runtime detection couldn't tell + the two apart from `TERM`/TTY/height alone. It now draws a plain in-place + `\r\033[K` status line that trails top-to-bottom output on every terminal. + Don't reintroduce the scroll region. - **L4 runs on GitHub Actions, not a self-hosted runner.** `macos-14` runners are Apple Silicon VMs — each job gets a fresh clean macOS environment, which is exactly what L4 needs. Tart is no longer required. diff --git a/internal/ui/AGENTS.md b/internal/ui/AGENTS.md index 141e093..2fa900f 100644 --- a/internal/ui/AGENTS.md +++ b/internal/ui/AGENTS.md @@ -8,9 +8,8 @@ live in the `tui/` subpackage. | File | Lines | Purpose | |------|-------|---------| | `ui.go` | 247 | Base styles, color helpers, output helpers (Header/Success/Error/Info/Warn/Muted/DryRun*), huh form wrappers (InputGitConfig, SelectPreset, Confirm, SelectOption, Input) | -| `progress.go` | 246 | StickyProgress: per-package timing, succeeded/failed/skipped counters | +| `progress.go` | 245 | StickyProgress: per-package timing, succeeded/failed/skipped counters, in-place status line | | `scanprogress.go` | 221 | ScanProgress: step timing, overall counter `[3/8]` | -| `scrollregion.go` | 143 | ANSI scroll-region plumbing for StickyProgress | ## FILES (internal/ui/tui/) diff --git a/internal/ui/progress.go b/internal/ui/progress.go index 139a63d..f3f13e5 100644 --- a/internal/ui/progress.go +++ b/internal/ui/progress.go @@ -39,9 +39,6 @@ type StickyProgress struct { succeeded int failed int skipped int - - // Scroll region rendering (nil when terminal doesn't support it). - region *ScrollRegion } var spinnerFrames = []string{"⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏"} @@ -58,10 +55,6 @@ func (sp *StickyProgress) Start() { sp.mu.Lock() sp.active = true sp.startTime = time.Now() - if IsScrollRegionSupported() { - sp.region = NewScrollRegion(2) - sp.region.Start() - } sp.mu.Unlock() signal.Stop(sp.sigCh) @@ -90,16 +83,19 @@ func (sp *StickyProgress) Start() { }() } +// render draws the live status line in place: carriage-return to the start of +// the current line, clear it, then write the spinner, progress count, current +// package, and elapsed time. It writes no newline, so PrintLine's completed +// rows scroll above it and this line trails the output. +// +// This deliberately does NOT reserve a "sticky" bottom bar via a terminal +// scroll region (DECSTBM). That looked nicer where it worked, but it silently +// corrupted on terminals that report a normal TERM yet don't honour scroll +// regions — the reserved bar got dragged into the log and reprinted on every +// tick, so the install read as garbage. A plain in-place status line renders +// top-to-bottom on every terminal, which is the property that matters for a +// one-shot install log. Don't reintroduce the scroll region. func (sp *StickyProgress) render() { - if sp.region != nil { - sp.region.DrawBottom(sp.formatLines()) - return - } - sp.renderInline() -} - -// renderInline is the fallback renderer used when scroll region is unavailable. -func (sp *StickyProgress) renderInline() { spinner := spinnerFrames[sp.spinnerIdx] pkg := truncate(sp.currentPkg, 20) elapsed := sp.pkgElapsed() @@ -111,27 +107,6 @@ func (sp *StickyProgress) renderInline() { etaStyle.Render(elapsed)) } -// formatLines returns the two strings to render in the bottom-reserved scroll -// region: a divider and a status line. -func (sp *StickyProgress) formatLines() []string { - cols := 80 - if sp.region != nil { - cols = sp.region.Cols() - } - - spinner := spinnerFrames[sp.spinnerIdx] - pkg := truncate(sp.currentPkg, 20) - elapsed := sp.pkgElapsed() - - divider := strings.Repeat("─", cols) - status := fmt.Sprintf("%s %s %s %s", - spinner, - progressTextStyle.Render(fmt.Sprintf("[%d/%d]", sp.completed+1, sp.total)), - currentPkgStyle.Render(pkg), - etaStyle.Render(elapsed)) - return []string{divider, status} -} - // pkgElapsed returns elapsed seconds for the current package, or "" if not yet started. func (sp *StickyProgress) pkgElapsed() string { if sp.pkgStart.IsZero() { @@ -217,12 +192,7 @@ func (sp *StickyProgress) onInterrupt() (force bool) { } sp.aborting = true sp.active = false - if sp.region != nil { - sp.region.Stop() - sp.region = nil - } else { - fmt.Printf("\r\033[K") - } + fmt.Printf("\r\033[K") fmt.Println(mutedStyle.Render(" aborting — waiting for the current step to stop · ctrl+c again to force quit")) return false } @@ -233,12 +203,7 @@ func (sp *StickyProgress) Finish() { sp.mu.Lock() defer sp.mu.Unlock() sp.active = false - if sp.region != nil { - sp.region.Stop() - sp.region = nil - } else { - fmt.Printf("\r\033[K") - } + fmt.Printf("\r\033[K") elapsed := time.Since(sp.startTime) diff --git a/internal/ui/progress_test.go b/internal/ui/progress_test.go index eeca6fb..438c972 100644 --- a/internal/ui/progress_test.go +++ b/internal/ui/progress_test.go @@ -159,16 +159,18 @@ func TestStickyProgressPkgElapsed(t *testing.T) { assert.Contains(t, elapsed, "s") } -func TestStickyProgressFallsBackWhenScrollRegionUnsupported(t *testing.T) { +func TestStickyProgressStartFinishDoNotPanic(t *testing.T) { + // The renderer no longer reserves a scroll region — it draws a plain + // in-place status line that works on every terminal. Start/Finish must run + // cleanly regardless of TERM (here a dumb terminal that supports nothing). t.Setenv("TERM", "dumb") sp := NewStickyProgress(3) - sp.Start() - defer sp.Finish() - // Region should NOT have been attached. - sp.mu.Lock() - hasRegion := sp.region != nil - sp.mu.Unlock() - assert.False(t, hasRegion, "scroll region should be disabled on dumb TERM") + assert.NotPanics(t, func() { + sp.Start() + sp.SetCurrent("wget") + sp.IncrementWithStatus(true) + sp.Finish() + }) } // Ctrl+C during a linear install must not exit from the progress goroutine: diff --git a/internal/ui/scrollregion.go b/internal/ui/scrollregion.go deleted file mode 100644 index 3a837c6..0000000 --- a/internal/ui/scrollregion.go +++ /dev/null @@ -1,143 +0,0 @@ -package ui - -import ( - "fmt" - "io" - "os" - - "golang.org/x/term" -) - -// minRowsForRegion is the smallest terminal height where reserving 2 sticky -// rows still leaves a usable scroll region. Tiny terminals fall back to the -// non-region path. -const minRowsForRegion = 6 - -// ScrollRegion owns the ANSI plumbing for reserving the bottom N rows as a -// "frozen" status area while output above scrolls. Use IsScrollRegionSupported -// to check before enabling; on unsupported terminals, callers should render -// inline instead. -// -// We chose the bottom over the top because a top-reserved region puts the -// progress bar at the very top of the terminal viewport — far from where the -// user actually typed the command. The bottom mirrors how tmux/screen status -// bars sit and stays closer to the user's eye-line for a one-shot CLI run. -type ScrollRegion struct { - w io.Writer - rows int - cols int - reserved int - active bool -} - -// NewScrollRegion constructs a scroll region for stdout with the given number -// of reserved bottom rows. Caller must invoke Start before drawing and Stop on -// teardown (defer / signal handler). -func NewScrollRegion(reserved int) *ScrollRegion { - w, h := terminalSize() - return &ScrollRegion{ - w: os.Stdout, - rows: h, - cols: w, - reserved: reserved, - } -} - -// Start reserves the region and hides the cursor. -func (r *ScrollRegion) Start() { - r.reserve() - r.active = true -} - -// Stop resets the scroll region and shows the cursor. Idempotent. -func (r *ScrollRegion) Stop() { - if !r.active { - return - } - r.reset() - r.active = false -} - -// DrawBottom writes lines to the reserved region without disturbing the cursor -// in the scrolling area. lines[0] goes to the topmost reserved row, lines[N-1] -// to the bottommost. len(lines) must be <= reserved. -func (r *ScrollRegion) DrawBottom(lines []string) { - r.drawBottom(lines) -} - -// Cols returns the current terminal width. -func (r *ScrollRegion) Cols() int { return r.cols } - -// write is the internal sink for ANSI control sequences. Terminal writes -// are best-effort — a failed write to stdout is not recoverable and -// silently dropping the error matches every other UI helper in this file. -func (r *ScrollRegion) write(format string, args ...interface{}) { - _, _ = fmt.Fprintf(r.w, format, args...) -} - -func (r *ScrollRegion) reserve() { - // Hide cursor, set scroll region from row 1 to (rows-reserved), then - // move the cursor to the last row of the scrollable area so new output - // prints just above the reserved bottom rows. - r.write("\x1b[?25l") - r.write("\x1b[1;%dr", r.rows-r.reserved) - r.write("\x1b[%d;1H", r.rows-r.reserved) -} - -func (r *ScrollRegion) reset() { - // Clear the reserved rows before releasing the region so the bar - // doesn't leave residue at the bottom of the scrollback. Save/restore - // keeps the cursor at whatever position the scrolling content left it, - // so subsequent prompts append naturally. - r.write("\x1b7") - for i := 0; i < r.reserved; i++ { - row := r.rows - r.reserved + 1 + i - r.write("\x1b[%d;1H\x1b[2K", row) - } - r.write("\x1b8") - r.write("\x1b[r") - r.write("\x1b[?25h") -} - -func (r *ScrollRegion) drawBottom(lines []string) { - if len(lines) > r.reserved { - panic(fmt.Sprintf("scrollregion: %d lines exceed reserved %d", len(lines), r.reserved)) - } - // DEC save cursor (caller's position in the scrolling area), draw each - // reserved row from the top of the reserved band downward, then restore. - r.write("\x1b7") - for i, line := range lines { - row := r.rows - r.reserved + 1 + i - r.write("\x1b[%d;1H\x1b[2K%s", row, line) - } - r.write("\x1b8") -} - -// IsScrollRegionSupported reports whether the current terminal can use the -// scroll-region rendering path. Falls back to false if TERM is dumb / empty, -// stdout is not a TTY, or the terminal is too short. -func IsScrollRegionSupported() bool { - _, h := terminalSize() - return isScrollRegionSupportedFor(os.Getenv("TERM"), h, term.IsTerminal(int(os.Stdout.Fd()))) //nolint:gosec // os.Stdout.Fd() returns a valid file descriptor; uintptr fits in int on all supported platforms -} - -func isScrollRegionSupportedFor(termEnv string, rows int, isTTY bool) bool { - if !isTTY { - return false - } - if termEnv == "" || termEnv == "dumb" { - return false - } - if rows < minRowsForRegion { - return false - } - return true -} - -func terminalSize() (cols, rows int) { - w, h, err := term.GetSize(int(os.Stdout.Fd())) //nolint:gosec // os.Stdout.Fd() returns a valid file descriptor; uintptr fits in int on all supported platforms - if err != nil || w <= 0 || h <= 0 { - return 80, 24 - } - return w, h -} diff --git a/internal/ui/scrollregion_test.go b/internal/ui/scrollregion_test.go deleted file mode 100644 index ff264a8..0000000 --- a/internal/ui/scrollregion_test.go +++ /dev/null @@ -1,90 +0,0 @@ -package ui - -import ( - "bytes" - "testing" - - "github.com/stretchr/testify/assert" -) - -func TestIsScrollRegionSupported(t *testing.T) { - tests := []struct { - name string - term string - rows int - expected bool - }{ - {"xterm-ghostty rows OK", "xterm-ghostty", 24, true}, - {"xterm-256color rows OK", "xterm-256color", 24, true}, - {"terminal exactly at threshold", "xterm-256color", 6, true}, - {"dumb terminal", "dumb", 24, false}, - {"empty TERM", "", 24, false}, - {"terminal too short", "xterm-256color", 4, false}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := isScrollRegionSupportedFor(tt.term, tt.rows, true) - assert.Equal(t, tt.expected, got) - }) - } -} - -func TestIsScrollRegionSupportedNonTTY(t *testing.T) { - // Even with good TERM and rows, non-TTY disables. - got := isScrollRegionSupportedFor("xterm-256color", 24, false) - assert.False(t, got) -} - -func TestScrollRegionReserveEmitsSequence(t *testing.T) { - var buf bytes.Buffer - r := &ScrollRegion{w: &buf, rows: 24, cols: 80, reserved: 2} - r.reserve() - out := buf.String() - // Expected: hide cursor, set scroll region rows 1..(24-2)=22, move cursor - // to the last row of the scrollable area (row 22). - assert.Contains(t, out, "\x1b[?25l") - assert.Contains(t, out, "\x1b[1;22r") - assert.Contains(t, out, "\x1b[22;1H") -} - -func TestScrollRegionResetClearsReservedRowsAndReleasesRegion(t *testing.T) { - var buf bytes.Buffer - r := &ScrollRegion{w: &buf, rows: 24, cols: 80, reserved: 2} - r.reset() - out := buf.String() - // Reset must: - // 1. clear each reserved row (rows 23 and 24 with EL2 = \x1b[2K) - // 2. release the scroll region (\x1b[r) - // 3. show the cursor (\x1b[?25h) - // 4. preserve the caller's cursor position via DEC save/restore - assert.Contains(t, out, "\x1b7") - assert.Contains(t, out, "\x1b[23;1H\x1b[2K") - assert.Contains(t, out, "\x1b[24;1H\x1b[2K") - assert.Contains(t, out, "\x1b8") - assert.Contains(t, out, "\x1b[r") - assert.Contains(t, out, "\x1b[?25h") -} - -func TestScrollRegionDrawBottomUsesSaveRestoreCursor(t *testing.T) { - var buf bytes.Buffer - r := &ScrollRegion{w: &buf, rows: 24, cols: 80, reserved: 2} - r.drawBottom([]string{"line one", "line two"}) - out := buf.String() - // DEC save (\x1b7) before, DEC restore (\x1b8) after. - // With rows=24, reserved=2: lines write to rows 23 and 24. - assert.Contains(t, out, "\x1b7") - assert.Contains(t, out, "\x1b8") - assert.Contains(t, out, "\x1b[23;1H") - assert.Contains(t, out, "\x1b[24;1H") - assert.Contains(t, out, "line one") - assert.Contains(t, out, "line two") -} - -func TestScrollRegionDrawBottomTooManyLinesPanics(t *testing.T) { - var buf bytes.Buffer - r := &ScrollRegion{w: &buf, rows: 24, cols: 80, reserved: 2} - assert.Panics(t, func() { - r.drawBottom([]string{"a", "b", "c"}) // reserved=2, given 3 - }) -}