LAB-1803: Trim styled blanks in vt scrollback

[cweill]

Motivation

Amux LAB-1803 heap profiles showed vt.cloneLineInto retaining the majority of server heap when scrollback contains full-width styled blank padding. The retained bytes were under the vt scrollback ring push path, so trimming scrollback rows before cloning reduces retained cells without changing the public API.

Summary

• Treat trailing non-continuation blank cells as scrollback-trimmable even when they carry style attributes.
• Preserve width-0 continuation cells so wide-character layout state is not discarded.
• Add a regression test for styled blank scrollback rows.

Testing

• go test -run 'TestScrollback/trims_trailing_styled_blanks' -count=100
• go test -run TestScrollback -count=1
• go test ./...

Review focus

• The scrollback-only blank definition for trailing cells with Content == "" or Content == " ".
• Confirming wide-character continuation cells remain preserved.

Refs LAB-1803.

[github-actions]

Claude finished @cweill's task in 2m 10s —— View job

---

PR Review: LAB-1803: Trim styled blanks in vt scrollback

• Gather context (read changed files)
• Review logic correctness and edge cases
• Post review findings

---

Summary

The change extracts isTrailingScrollbackBlank from the inline condition in trimTrailingEmptyCells and extends it to treat cells with blank content ("" or " ") as trimmable even when they carry style attributes, while explicitly preserving width-0 continuation cells.

The logic is correct. A few observations below, none blocking.

---

Observations

1. Continuation cell test is missing (non-blocking)

The PR description calls out "Preserve width-0 continuation cells" as a stated invariant, but there is no test exercising it. A line like [wide_char(w=2), cont(w=0), styled_blank(w=1), styled_blank(w=1)] should trim to [wide_char(w=2), cont(w=0)]. Given the PR explicitly mentions this guarantee, a test covering it would match the stated scope.

Add this test →

2. Nil check in isTrailingScrollbackBlank is unreachable from trimTrailingEmptyCells (non-blocking)

trimTrailingEmptyCells always calls isTrailingScrollbackBlank(&line[i]) where line[i] is a value, so the pointer is never nil. The nil guard is harmless and defensive — no action needed, just noting it isn't exercised.

3. Styled-space trimming is broader than background-only (intentional, non-blocking)

The check cell.Content == "" || cell.Content == " " trims any styled trailing space — including those with foreground color, bold, etc. The PR description frames the motivating case as background-fill padding, but the trim is actually style-agnostic. This is almost certainly the right call (foreground-only trailing spaces have no practical use in terminal output), and it makes the function simpler. Worth being aware of if this is ever revisited.

4. Width > 1 trailing cells with blank content are trimmed (correct behavior)

A full-width space (e.g., Width=2, Content=" ") at the end of a line without a following continuation cell would be trimmed. This is intentional and correct.

---

LGTM

[greptile-apps]

Greptile Summary

This PR narrows the definition of a "trimmable" trailing cell in the scrollback ring: previously only zero-value cells or EmptyCell were stripped; now any cell with Width > 0 and Content == \"\" or Content == \" \" (regardless of style) is also trimmed, while Width == 0 continuation cells are explicitly preserved.

• isTrailingScrollbackBlank replaces the inline two-condition check in trimTrailingEmptyCells, adding content-based detection for styled blank cells and a guard for wide-character continuation cells.
• A regression test is added that pushes a line of background-styled space cells with a "prompt" prefix and asserts both the trimmed length and the textual content.

Confidence Score: 4/5

Safe to merge; the change is tightly scoped to the scrollback trim path and does not touch the live screen buffer or any public API.

The core logic is correct and the regression test exercises the new path directly. The dead nil guard and the narrower-than-described Width==0 protection do not affect correctness or introduce regressions.

vt/scrollback.go — the ordering of guards in isTrailingScrollbackBlank is worth a second look to confirm zero-value continuation cells at trailing positions are handled as intended.

Important Files Changed

Filename	Overview
vt/scrollback.go	Refactors trimTrailingEmptyCells to delegate to a new isTrailingScrollbackBlank helper; adds content-based blank detection (Content=="" or ==" ") while protecting Width==0 continuation cells. The nil guard in the helper is unreachable dead code; the IsZero() check short-circuits before the Width==0 guard for zero-value continuation cells, matching pre-existing behaviour, while the Width==0 path correctly protects non-zero (e.g. styled) continuation cells.
vt/scrollback_test.go	Adds a targeted regression test for the new styled-blank trimming path; correctly constructs a 12-cell line with background-coloured blanks, writes "prompt" into the leading cells, and asserts both the trimmed length and textual content. No issues found.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["trimTrailingEmptyCells(line)"] --> B["Scan from right to left"]
    B --> C{"isTrailingScrollbackBlank(cell)"}
    C --> D{"cell == nil?"}
    D -- yes --> E["return true (blank)"]
    D -- no --> F{"IsZero() or Equal(EmptyCell)?"}
    F -- yes --> G["return true (blank)"]
    F -- no --> H{"cell.Width == 0?"}
    H -- yes --> I["return false (preserve continuation)"]
    H -- no --> J{"Content == '' or ' '?"}
    J -- yes --> K["return true (styled blank — NEW)"]
    J -- no --> L["return false (non-blank content)"]
    C -- blank --> M["continue scan leftward"]
    C -- non-blank --> N["set lastNonEmpty = i, break"]
    N --> O["return line[:lastNonEmpty+1]"]

Loading

_{Reviews (1): Last reviewed commit: "LAB-1803: Trim trailing styled blanks in..." | Re-trigger Greptile}

[greptile-apps]

The nil check here is dead code. isTrailingScrollbackBlank is only ever called with &line[i] inside trimTrailingEmptyCells, where i is a valid slice index, so the pointer is always non-nil. The branch can be removed without any behavioural change.

Suggested change

	func isTrailingScrollbackBlank(cell *uv.Cell) bool {
	if cell == nil {
	return true
	}
	if cell.IsZero() || cell.Equal(&uv.EmptyCell) {
	func isTrailingScrollbackBlank(cell *uv.Cell) bool {
	if cell.IsZero() || cell.Equal(&uv.EmptyCell) {

[greptile-apps]

Width == 0 guard silently unreachable for zero-value continuation cells

The PR description states "Preserve width-0 continuation cells so wide-character layout state is not discarded," but a zero-value continuation cell (Width: 0, Content: "", Style: {}) satisfies IsZero() and returns true before the Width == 0 guard is ever reached. Only a non-zero continuation cell (e.g. one carrying inherited style) is actually protected. A trailing zero-value continuation cell was already trimmed before this PR, so there is no regression, but the protection is narrower than the PR description implies. Worth a clarifying code comment if zero-value continuations at trailing positions are expected to be preserved too.