Fix/progress tty line overflow 13595#13840
Open
glours wants to merge 2 commits into
Open
Conversation
In narrow terminals (e.g. tmux panes), the TTY progress UI emitted lines wider than terminalWidth because adjustLineWidth could shrink details and taskID but never the progress field. When progress carried the "X.XMB / Y.YMB" size suffix, the truncation loop exited with overflow > 0 and applyPadding's max(timerPad, 1) floor pushed the line one char over. tmux then wrapped the line visually while print() kept counting logical lines, desyncing aec.Up() on the next render and producing the mangled "[+] pull X/Y" header overwriting prior task lines. Track the size suffix byte length on lineData and let adjustLineWidth drop it as an intermediate truncation step before abbreviating the taskID. Fixes docker#13595 Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
maxBeforeStatusWidth used len(l.taskID) (bytes) while applyPadding used utf8.RuneCountInString (runes). For ASCII task IDs the two agree and no symptom surfaces, but a taskID containing multi-byte UTF-8 chars (CJK, emoji, accented Latin) reported a width larger than its visual columns. computeOverflow then triggered truncation where none was needed, and truncateLongestTaskID's byte-indexed slice could land mid-multibyte sequence, corrupting the displayed string. Align the two measurements on rune count. Signed-off-by: Guillaume Lours <glours@users.noreply.github.com>
![docker-agent[bot]](https://avatars.githubusercontent.com/in/2787757?s=60&v=4){kind=small}
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The fix correctly addresses two bugs in the TTY progress display:
- Progress size suffix stripping —
truncateProgressSizenow drops trailingX.XMB / Y.YMBsize info before truncating task IDs, closing the overflow path that caused tmux line-wrapping and desync'daec.Up()cursor movement. - Rune-count alignment —
maxBeforeStatusWidthnow usesutf8.RuneCountInString(l.taskID)to matchapplyPadding, preventing spurious truncation and mid-multibyte slicing for CJK/emoji/accented task IDs.
The logic is sound, new regression tests cover both scenarios, and no CONFIRMED/LIKELY bugs were found in the changed code.
Note (pre-existing, out of scope): truncateLongestTaskID (not changed by this PR) still uses byte-length len() for taskID selection and slicing while overflow is now measured in runes — this inconsistency could cause incorrect truncation for multi-byte task IDs in a future interaction, but it is not introduced by this PR.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes Compose’s TTY progress renderer so lines don’t overflow/wrap in narrow terminals (notably tmux panes), preventing cursor desync and mangled multi-line progress output (Fixes #13595).
Changes:
- Track the byte length of the trailing progress size suffix and allow
adjustLineWidthto drop that suffix as an intermediate truncation step. - Align
maxBeforeStatusWidthwithapplyPaddingby measuringtaskIDwidth in runes (UTF-8 aware). - Add regression tests covering multi-byte task IDs and multi-render narrow-terminal scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/display/tty.go | Adds progress-size suffix tracking and truncation step; fixes taskID width computation to be rune-based. |
| cmd/display/tty_test.go | Adds unit + multi-render regression tests for overflow behavior and UTF-8 taskID width handling. |
Comment on lines
+483
to
+493
| func truncateProgressSize(lines []lineData) bool { | ||
| for i := range lines { | ||
| l := &lines[i] | ||
| if l.progressSizeBytes > 0 { | ||
| l.progress = l.progress[:len(l.progress)-l.progressSizeBytes] | ||
| l.progressSizeBytes = 0 | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
Comment on lines
448
to
453
| func maxBeforeStatusWidth(lines []lineData) int { | ||
| var maxWidth int | ||
| for i := range lines { | ||
| l := &lines[i] | ||
| width := 3 + lenAnsi(l.prefix) + len(l.taskID) + lenAnsi(l.progress) | ||
| width := 3 + lenAnsi(l.prefix) + utf8.RuneCountInString(l.taskID) + lenAnsi(l.progress) | ||
| if width > maxWidth { |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What I did
Fixes the TTY progress UI mangled output in narrow terminals (most visibly tmux panes) reported in #13595.
adjustLineWidthcould shrinkdetailsandtaskIDbut never theprogressfield. Whenprogresscarried theX.XMB / Y.YMBsize suffix, the truncation loop exited withoverflow > 0andapplyPadding'smax(timerPad, 1)floor pushed the line one char overterminalWidth. tmux thenwrapped it visually while
print()kept counting logical lines, desyncingaec.Up()on the next render and producing the[+] pull X/Yheader overwriting prior task lines.Commits
lineDataand letadjustLineWidthdrop it as an intermediate truncation step before abbreviating thetaskID. Adds two regression tests (unit + multi-render).maxBeforeStatusWidthusedlen(l.taskID)whileapplyPaddingusedutf8.RuneCountInString. For multi-byte UTF-8 task IDs (CJK, emoji, accented Latin) this overestimated width, triggering spurious truncation that sliced mid-multibyte andcorrupted the rendered string. Aligns the two measurements.
Related issue
Fixes #13595
(not mandatory) A picture of a cute animal, if possible in relation to what you did
