resizeY grow path bloats Height via clear/ensureHeight feedback#3
Open
th3godfather wants to merge 1 commit into
Open
resizeY grow path bloats Height via clear/ensureHeight feedback#3th3godfather wants to merge 1 commit into
th3godfather wants to merge 1 commit into
Conversation
The grow branch of Screen.resizeY read v.Height fresh on every inner iteration as the offset for the next row to clear, but the very clear() call inside that loop goes through paint → ensureHeight, which mutates v.Height between iterations. Each new "row" therefore compounded its offset by row+1 per column, so growing a 24×40 terminal to 29×50 ended with v.Height=624 and len(v.Content)=1219. A trailing v.Height = h reset the field, but only after resizeX had already iterated the bloated Content and re-grown v.Height to match. Embedders that wrap midterm as a pane (terminal multiplexers, agent dashboards) saw the side-effect as their host renderer clipping content off-screen — the pane itself drew fine, only the chrome below it disappeared once the bloated Render output overflowed the host viewport. ensureHeight already does exactly what the loop intended: append a row to Content with all cells initialised to ' ', paint EmptyFormat on every cell via the canvas, tick Changes, increment v.Height — once per new row, no feedback loop. Letting it do the work directly keeps Content / Changes / Height / Canvas in lockstep with no per-cell redundancy. TestResizeGrowingHeightDoesNotBloatCanvas pins the size invariants (Height, Width, len(Content), len(Changes), per-row width, and the rendered line count) across four growth shapes, including the 24×40 → 29×50 original repro and a width-only-grow control case that was never affected. Pre-fix the height-growth cases failed with v.Height in the hundreds or hundreds of thousands; post-fix all four pass.
vito
requested changes
May 13, 2026
Owner
vito
left a comment
There was a problem hiding this comment.
Thanks! No need for the big AI slop comment though, referencing how it "previously" worked is confusing, and the new code is simple + obvious enough to not need a comment at all.
Comment on lines
+96
to
+106
| // Grow: ensureHeight already appends empty rows, paints | ||
| // EmptyFormat on each new cell, and ticks Changes — exactly | ||
| // the per-row initialization this branch used to do | ||
| // cell-by-cell via clear(). The previous loop read v.Height | ||
| // fresh on every iteration as the row offset, and clear()'s | ||
| // internal ensureHeight call mutated v.Height between | ||
| // iterations, so each new row compounded the offset by | ||
| // row+1 per column (Height grew from 24 to 624 on a | ||
| // 24×40 → 29×50 resize). Letting ensureHeight do the work | ||
| // directly keeps the canvas, Content, Changes, and Height | ||
| // in lockstep. |
Owner
There was a problem hiding this comment.
Suggested change
| // Grow: ensureHeight already appends empty rows, paints | |
| // EmptyFormat on each new cell, and ticks Changes — exactly | |
| // the per-row initialization this branch used to do | |
| // cell-by-cell via clear(). The previous loop read v.Height | |
| // fresh on every iteration as the row offset, and clear()'s | |
| // internal ensureHeight call mutated v.Height between | |
| // iterations, so each new row compounded the offset by | |
| // row+1 per column (Height grew from 24 to 624 on a | |
| // 24×40 → 29×50 resize). Letting ensureHeight do the work | |
| // directly keeps the canvas, Content, Changes, and Height | |
| // in lockstep. |
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.
The
growbranch ofScreen.resizeYreadv.Heightfresh on every inner iteration as the offset for the next row to clear, but theclear()call inside that loop goes throughpaint→ensureHeight, which mutatesv.Heightbetween iterations. Each new row therefore compounded its offset byrow+1per column, so growing a 24×40 terminal to 29×50 ended withv.Height = 624andlen(v.Content) = 1219. A trailingv.Height = hreset the field, but only afterresizeXhad already iterated the bloatedContentand re-grownv.Heightto match.Fix
ensureHeightalready does exactly what the loop intended: append a row toContentwith all cells initialised to' ', paintEmptyFormaton every cell via the canvas, tickChanges, incrementv.Height— once per new row, no feedback loop. Letting it do the work directly keepsContent/Changes/Height/Canvasin lockstep with no per-cell redundancy.Test
TestResizeGrowingHeightDoesNotBloatCanvaspins the size invariants (Height,Width,len(Content),len(Changes), per-row width, and the rendered line count) across four growth shapes, including the 24×40 → 29×50 original repro and a width-only-grow control case that was never affected. Pre-fix the height-growth cases failed withv.Heightin the hundreds or hundreds of thousands; post-fix all four pass.