From 1fe3b02af1aae3b90cbc827f6df774272ab19d0a Mon Sep 17 00:00:00 2001 From: Dennison Date: Tue, 8 Sep 2026 13:41:03 -0400 Subject: [PATCH] fix(tui): truncate the spinner label by display width, not rune count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit shortenLabel guarantees the cancel hint survives at narrow widths. For double-width characters it did the opposite: the hint was cut off while the label stayed long. The function budgeted in display columns via lipgloss.Width but truncated by rune count, and CJK and emoji occupy two columns per rune. The label overran its allowance, and the final MaxWidth clamp trimmed from the right — taking the hint. The rendered width was always correct, which is why nothing caught it, and every existing test passed because they were all ASCII, where columns and runes are the same number. truncateToWidth now accumulates display width per rune, counting the ellipsis. It is deliberately not grapheme-aware; splitting a combining mark or ZWJ sequence remains possible, which is still strictly better than rune counting and much smaller than full segmentation. Found by the untrusted external reviewer (gpt-6-astra via the Surplus proxy) reading the packed source with no tools, then confirmed locally by rendering the cases rather than taking the report at face value. Closes #1432 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01WJGxhoFhA8JjkwZFcLGdS5 --- .../tui/components/spinner/model.go | 48 +++++++++++++++++-- .../components/spinner/truthful_label_test.go | 37 ++++++++++++++ docs/logs/engineering-log.md | 39 +++++++++++++++ 3 files changed, 120 insertions(+), 4 deletions(-) diff --git a/cmd/harnesscli/tui/components/spinner/model.go b/cmd/harnesscli/tui/components/spinner/model.go index c8377dea..95618265 100644 --- a/cmd/harnesscli/tui/components/spinner/model.go +++ b/cmd/harnesscli/tui/components/spinner/model.go @@ -277,13 +277,53 @@ func shortenLabel(glyph, label, full string, width int) string { // Not even a stub of a label fits; the hint alone is more useful. return glyph + " " + CancelHint } - runes := []rune(label) - if len(runes) > budget { - label = string(runes[:budget-1]) + "\u2026" - } + label = truncateToWidth(label, budget) return glyph + " " + label + " " + CancelHint } +// truncateToWidth shortens label to at most budget terminal columns, appending +// an ellipsis when it cuts. +// +// It measures display columns rather than runes. Counting runes was the bug in +// issue #1432: CJK and emoji occupy two columns each, so a rune-budgeted label +// could be twice its allowance in columns, overrun the line, and get clipped +// from the right — taking the cancel hint with it. ASCII hid this completely, +// because there columns and runes are the same number. +// +// Rune-by-rune accumulation is deliberate and imperfect: it does not segment +// grapheme clusters, so a combining mark or ZWJ emoji sequence can still be +// split. That is a strict improvement over rune counting and a much smaller +// change than full segmentation, which is out of scope until a real case +// appears. +func truncateToWidth(label string, budget int) string { + if budget <= 0 { + return "" + } + if lipgloss.Width(label) <= budget { + return label + } + + const ellipsis = "\u2026" + room := budget - lipgloss.Width(ellipsis) + if room <= 0 { + return ellipsis + } + + var ( + out []rune + width int + ) + for _, r := range label { + w := lipgloss.Width(string(r)) + if width+w > room { + break + } + out = append(out, r) + width += w + } + return string(out) + ellipsis +} + // CompletionLine returns the one-line completion summary shown after the spinner stops. // // Format: "✻ Worked for 5s" or "✻ Worked for 1m 30s" diff --git a/cmd/harnesscli/tui/components/spinner/truthful_label_test.go b/cmd/harnesscli/tui/components/spinner/truthful_label_test.go index b8b211d6..e453bf6f 100644 --- a/cmd/harnesscli/tui/components/spinner/truthful_label_test.go +++ b/cmd/harnesscli/tui/components/spinner/truthful_label_test.go @@ -116,3 +116,40 @@ func TestSpinnerHintSurvivesEvenWhenLabelCannotFit(t *testing.T) { t.Errorf("cancel hint should outrank the label when space is scarce, got %q", view) } } + +// TestSpinnerKeepsCancelHintWithWideRunes pins issue #1432: the hint survives +// regardless of the label's script. +// +// shortenLabel budgeted in display columns but truncated by rune count. For +// CJK and emoji, which occupy two columns per rune, the label overran its +// budget and the final MaxWidth clamp ate the hint — the exact outcome +// shortenLabel exists to prevent. Every existing test passed throughout, +// because they were all ASCII, where columns and runes coincide. +// +// The assertion is on the hint's survival, not on the line's width: the width +// was always correct, which is why this went unnoticed. +func TestSpinnerKeepsCancelHintWithWideRunes(t *testing.T) { + for _, tc := range []struct { + name string + label string + }{ + {name: "cjk", label: "Waiting for 模型模型模型模型模型模型模型模型"}, + {name: "emoji", label: "Running 🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀"}, + // ASCII control: a fix that over-truncates everything to satisfy the + // hint assertion would show up here as a needlessly stunted label. + {name: "ascii control", label: "Waiting for some-long-model-name-v2"}, + } { + t.Run(tc.name, func(t *testing.T) { + m := New(0).Start().SetAction(tc.label) + for _, width := range []int{40, 50} { + view := m.View(width) + if !strings.Contains(view, CancelHint) { + t.Errorf("width %d: cancel hint truncated, got %q", width, view) + } + if got := lipgloss.Width(view); got > width { + t.Errorf("width %d: line is %d columns wide: %q", width, got, view) + } + } + }) + } +} diff --git a/docs/logs/engineering-log.md b/docs/logs/engineering-log.md index 7dba2d52..2aa892d2 100644 --- a/docs/logs/engineering-log.md +++ b/docs/logs/engineering-log.md @@ -1,5 +1,44 @@ # Engineering Log +## 2026-09-08 — Issue #1432 wide labels destroyed the cancel hint + +- Symptom: `shortenLabel` guarantees the `(esc to interrupt)` hint survives at + narrow widths, sacrificing the label instead. For double-width characters it + did the opposite: + ``` + width=40 "· Waiting for 模型模型模型… (esc to inte" + width=40 "· Running 🚀🚀🚀🚀🚀🚀🚀🚀🚀🚀… (esc to " + ``` + The line width was always correct — the final `MaxWidth` clamp saw to that — + which is exactly why nothing caught it. The clamp was cutting the hint off + because the label had not been shortened enough. +- Cause: the function mixed two units. The budget was computed in display + columns (`lipgloss.Width`) but the truncation was applied in runes + (`runes[:budget-1]`). CJK and emoji occupy two columns per rune, so a + rune-budgeted label could be twice its allowance in columns. For ASCII the two + units coincide, so every existing test passed. +- Fix: `truncateToWidth` accumulates `lipgloss.Width` per rune until the budget + is reached, counting the ellipsis's own width. Deliberately not + grapheme-cluster aware — combining marks and ZWJ sequences can still be split. + That is a strict improvement over rune counting and a far smaller change than + full segmentation, which stays out of scope until a real case appears. +- Tests: `TestSpinnerKeepsCancelHintWithWideRunes` over CJK, emoji and an ASCII + control at widths 40 and 50, asserting the hint's *survival* rather than the + line's width — the width was never wrong, and asserting it is what let the bug + hide. The ASCII control guards against a fix that satisfies the hint + assertion by over-truncating every label. +- **Provenance, and the reason this entry matters more than the bug.** Found by + `gpt-6-astra` through the Surplus proxy — an untrusted external reviewer given + the packed source and no tools — then confirmed locally by rendering the cases + before believing it. Our own tests were green throughout and stayed green, + because they were ASCII-only; the code asserted a guarantee in a comment that + it did not keep. An outside reader with no ability to run anything caught what + the test suite structurally could not. + - Signal-to-noise from that review, recorded honestly: three findings, one + confirmed (this), one unconfirmed and overlapping with it (a claim that + `MaxWidth` may wrap rather than clip — not observed), and one false positive + (elapsed-time nondeterminism in snapshots, which set `startTime` explicitly). + ## 2026-09-08 — Issue #1428 CLAUDE.md: tickets, real-run proof, delegation - Three practices this session kept proving necessary were absent from