fix(tui): wrap status line instead of truncating when too long#3054
Conversation
|
Good direction — now that the chat view is alt-screen ( Before it can land,
Fix: count both lines through the same |
SivanCola
left a comment
There was a problem hiding this comment.
Thanks for working on this. This is related to #3278 because both changes affect the CLI TUI bottom rail height budget (bottomRows() / View() / composer/status rows). I tested the integration order locally as main-v2 + #3278 + #3054; #3278 applies cleanly, but #3054 conflicts in internal/cli/chat_tui.go, so this needs a rebase after #3278.
One behavioral issue also needs fixing before this can land: View() now wraps the working line, the first status line, and the data/statusline row via wrapStatusLine(...), but bottomRows() only reserves one row for working and computeStatusLineCount() assumes the first status line is always one row. On narrow terminals, long mode/status text, or a running turn with queued feedback, the rendered bottom region can become taller than transcriptHeight() reserved, so the viewport will overlap or clip the bottom rail.
Please rebase on top of the current main-v2 / #3278 shape and make the height accounting use the same wrapped strings that View() renders, including:
- wrapped working line height when
m.state == tuiRunning - wrapped first status line height
- wrapped data/custom statusline height
- dynamic composer height from #3278 (
m.input.Height())
A focused regression test with a narrow width and long status/custom statusline would make this much safer.
|
Thanks for this! Since this PR was opened, main-v2 has changed the status line to clamp each row to width independently (a fixed-height approach specifically to avoid wrap-induced resize ghosting). That overlaps with the goal here but takes the opposite approach (wrapping vs clamping), so rebasing surfaces semantic conflicts in chat_tui.go between the two designs. Could you rebase onto the latest main-v2 and reconcile whether wrapping is still wanted over the now-landed clamp approach, and if so how the two should coexist? Thank you! |
|
@CnsMaple gentle ping — the wrap-instead-of-truncate direction was accepted in review, but the branch has gone stale and now conflicts with main-v2. Could you rebase and address the earlier review notes? Happy to merge once it's green. |
The bottom status/data line (model · git · effort · context · cache · jobs ·
balance, or custom statusline) was truncated with an ellipsis when its content
exceeded the terminal width. This lost information with no way to reveal it.
Replace clampStatusLine (ansi.Truncate) with wrapStatusLine (ansi.Hardwrap)
so the data line wraps to additional rows instead of being silently cut.
Add statusLineCount field + computeStatusLineCount() method so bottomRows()
reserves the correct dynamic height, including the working/spinner line when
running and the custom statusline output.
computeStatusLineCount mirrors View()'s construction exactly: same width,
both the mode/state line and the data line are wrapped for counting, and the
mode tag width includes the Padding(0,1) that View() applies — without this
the count drifts from the rendered height by 1 row at certain widths, hiding
the bottom transcript line.
Also fix gitTag() to never truncate itself — it previously took a maxWidth
argument and silently truncated with ansi.Truncate, causing the git tag to
be cut off ('项...字@项目分支') or disappear entirely before wrapStatusLine
had a chance to wrap the whole line. Now gitTag() renders the full identity
and lets wrapStatusLine handle any needed wrapping at word boundaries.
c37a7f3 to
959195a
Compare
esengine
left a comment
There was a problem hiding this comment.
Re-reviewed the rework — this lands the invariant the first review asked for: computeStatusLineCount mirrors View()'s exact construction, bottomRows() reserves the real wrapped height, and the tests pin transcriptHeight + bottomRows == height across widths including the CJK boundary sweep. The wrap-vs-clamp reconciliation is also sound: the ghosting that motivated clamping was a scrollback artifact, and the status block now lives in the alt-screen view where there is no scrollback to strand — the comment says exactly that.
Two notes for a follow-up, neither blocking:
Update()now recomputesstatusLineCounttwice back to back (once withcontentW, immediately overwritten withcm.width) — merge artifact; the first call is a dead store and should go.computeStatusLineCountmirrors ~80 lines ofView(); longer term I'd like the line builders extracted and shared so the two can't drift. Your width-sweep tests are the guard until then.
Thanks for sticking with this through three rounds — merging.
Problem
The bottom status/data line (model · git · effort · context · cache · jobs · balance, or custom statusline command output) was truncated with
…when its content exceeded the terminal width on narrow terminals or with many active tags. Information was silently lost with no way to reveal it.Solution
Replace truncation with ANSI-aware hard-wrapping so the data line flows onto additional rows instead of being cut off.
Changes
wrapStatusLine(replacesclampStatusLine): usesansi.Hardwrapto wrap text at word boundaries to the given width instead of truncating withansi.Truncate(…).computeStatusLineCount: new method that replicates the data-tag construction from View() and returns the number of terminal rows the status block will occupy after wrapping. Called fromUpdate()so the viewport height is correctly reserved.statusLineCountfield: stored on the model and used bybottomRows()instead of the hardcoded+ 2. Falls back to 2 when uninitialized (e.g. in tests).View(): updated to usewrapStatusLinefor both status rows and the working spinner line.Tests
All existing tests pass. No behavioral change on wide terminals — wrapping only kicks in when the content exceeds
widthcolumns.