Pull pushed rows back down when a height shrink/grow cycle reverses - #190
Pull pushed rows back down when a height shrink/grow cycle reverses#190ejc3 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughChangesResize row debt recovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2f3ca73 to
29dec42
Compare
|
Rebased onto current main (11aa609); the four tests updated to the new One contract note for review, after an adversarial pass raised it: the carve-out pull moves the active-area origin without touching pins, so a caller tracking the cursor by raw (x, y) across the resize would write at a stale row. The real caller doesn't: |
…reverses A no-reflow height shrink pushes the viewport's top rows into history. The matching grow declines to pull them back when the cursor sits above the bottom row (the intentional no-yank UX), so a full-screen TUI that parks its cursor in an input box -- with a multiplexer above it that fully repaints after every resize -- leaks one viewport-top of duplicated rows into scrollback per resize cycle. Interactive window drags fire many such cycles, and the duplicates read as corrupted scrollback right above the screen. Track the rows a shrink pushes as a debt, and let the next grow pull at most that many rows back down before appending blank rows. Any content scroll clears the debt, so history that has become real is never yanked; ordinary grows with no preceding shrink behave exactly as before. One test walks three shrink/grow cycles and asserts totalRows() is stable (it failed at 13 vs 10 before the fix); a second pins the existing no-pull behavior when output intervenes.
29dec42 to
2d5b4f7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/terminal/PageList.zig (2)
2361-2380: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider migrating the remaining duplicate viewport-pin checks to
viewportPinToActive().
viewportPinToActive()correctly factors out the repeatedswitch (self.viewport) { .pin => if (self.pinIsActive(...)) ... }pattern, and it is used at line 2547. The identical inline pattern still exists, unconverted, inresize()(around lines 1326-1331) andresizeCols()(around lines 1494-1499). Migrating those call sites to the new helper would complete the deduplication this change already started.This is not required for correctness in this PR; it is a follow-up consistency improvement now that the helper exists.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/terminal/PageList.zig` around lines 2361 - 2380, Replace the duplicated viewport-pin switch checks in resize() and resizeCols() with calls to viewportPinToActive(). Preserve the existing call in the nearby flow and keep the helper’s behavior unchanged.
13054-13185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage of the core symmetric/accumulation/content-scroll scenarios.
These four tests correctly exercise the primary contract (symmetric shrink/grow, bottom-cursor consumption, accumulated debt, and content-scroll clearing), and all match the traced implementation behavior.
One documented edge case is not directly exercised here:
resizeWithoutReflow's comment at line ~2497 notes that "pruning can leave the debt stale," referring to a scenario where an operation likeeraseHistoryreduces real surplus below the outstandingresize_row_debtwithout clearing it (sinceeraseHistorydoes not callgrow()). A regression test asserting that a subsequent grow only pulls the reduced, capped amount (not the stale debt value) would lock in this explicitly-called-out safety property.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/terminal/PageList.zig` around lines 13054 - 13185, Extend the PageList resize tests with an eraseHistory scenario that leaves resize_row_debt larger than the remaining history surplus, then grow the viewport and assert only the reduced, capped surplus is pulled back rather than the stale debt amount. Anchor the setup and assertions in resizeWithoutReflow, eraseHistory, and the existing resize growth path, preserving the current debt and content-scroll scenarios.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/terminal/PageList.zig`:
- Around line 414-423: Update reset() to explicitly set resize_row_debt to zero
alongside the other mutable page-list state it clears, ensuring a reset leaves
no stale resize debt while preserving the existing reset behavior.
---
Nitpick comments:
In `@src/terminal/PageList.zig`:
- Around line 2361-2380: Replace the duplicated viewport-pin switch checks in
resize() and resizeCols() with calls to viewportPinToActive(). Preserve the
existing call in the nearby flow and keep the helper’s behavior unchanged.
- Around line 13054-13185: Extend the PageList resize tests with an eraseHistory
scenario that leaves resize_row_debt larger than the remaining history surplus,
then grow the viewport and assert only the reduced, capped surplus is pulled
back rather than the stale debt amount. Anchor the setup and assertions in
resizeWithoutReflow, eraseHistory, and the existing resize growth path,
preserving the current debt and content-scroll scenarios.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c50ebb2-271c-44fb-93b2-b487490a8e8c
📒 Files selected for processing (1)
src/terminal/PageList.zig
| /// Rows that a no-reflow height shrink pushed into history and that the | ||
| /// next height grow may pull back down, so a shrink/grow cycle is | ||
| /// symmetric. Without this, an application that keeps its cursor above | ||
| /// the bottom row (a full-screen TUI on the primary screen) leaks one | ||
| /// viewport-top of duplicated rows into history on every resize cycle, | ||
| /// because the grow path deliberately refuses to pull down scrollback. | ||
| /// Any content scroll (grow()) clears it: after new output, history | ||
| /// above the screen is real again and pulling it down would be wrong. | ||
| resize_row_debt: usize = 0, | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Clear resize_row_debt in reset() for invariant consistency.
reset() rebuilds the page list from scratch and explicitly clears almost every other piece of mutable state (viewport, tracked pins, total_rows, page_serial_epoch), but it does not clear resize_row_debt. After reset(), the field can hold a stale, pre-reset value.
This is currently harmless: every consumer of the debt (takeRowDebt) always caps the amount taken by the freshly computed surplus = self.total_rows - self.rows at the time of use, so a stale value can never cause an over-pull. But it leaves an inconsistent invariant: reset() is documented and designed to bring the list to a pristine state, and this field silently escapes that contract. A future change to the surplus-capping logic could turn this into a real bug.
🧹 Proposed fix
// Move our viewport back to the active area since everything is gone.
self.viewport = .active;
+
+ // Any pending resize-row debt is meaningless once the whole list is
+ // rebuilt from scratch.
+ self.resize_row_debt = 0;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/terminal/PageList.zig` around lines 414 - 423, Update reset() to
explicitly set resize_row_debt to zero alongside the other mutable page-list
state it clears, ensuring a reset leaves no stale resize debt while preserving
the existing reset behavior.
1 participant
What breaks today
A no-reflow height shrink pushes the viewport's top rows into history. The matching grow declines to pull them back whenever the cursor sits above the bottom row — the intentional no-yank UX in
PageList.resizeWithoutReflow. A full-screen TUI that parks its cursor in an input box (Claude Code, and ink apps generally) defeats that heuristic permanently, and when tmux sits above the TUI and fully repaints after every resize, each shrink/grow cycle appends one viewport-top of duplicated rows to scrollback. A window drag fires many cycles; the duplicates read as corrupted scrollback just above the screen. tmux's own history stays clean throughout — the drift is entirely in the terminal's ledger.Repro: run a TUI under tmux with the alternate screen off, put real content on screen, shrink and grow the window height a few times, scroll up.
totalRows()grows by the pushed count on every cycle.The change
Track the rows a shrink pushes as
resize_row_debt. The next cursor-above-bottom grow pulls at most that many rows back down before appending blanks. Nothing else moves:grow()), so history that has become real is never yanked down — the existing no-pull UX is preserved for every ordinary grow.totalRows() - rows), so pruned pages can't leave the active area short.resizeCols): rewrapped history invalidates row-for-row accounting, so those paths keep upstream behavior exactly.The comment at this branch says the no-pull choice was arbitrary and open to change if resize behavior isn't regressed in a user-hostile way; scoping the pull-down to reversing a just-made push is intended to meet that bar — a user who shrank and re-grew gets their screen back, and no one else sees anything new.
Edge cases considered
totalRowsstableTests, red first
Each behavioral piece was watched failing before its fix:
height cycle with cursor above bottom is symmetric— failed on unmodified code at 13 rows vs 10 on the first cycle.bottom-cursor grow consumes the debt— failed against the first version of this change as atotalRows() >= rowsassertion panic; the consumption in the pull-down path is what fixes it.grow after content scroll keeps scrollbackandmultiple shrinks accumulate…pin the preserved behaviors and pass on both sides by design.zig build test -Dtest-filter=PageList: 350/350. Full suite: 3328/3344 passed, 16 skipped, none failed.Verified in a real window
The unit tests drive
PageListdirectly, so they cannot show that a person dragging a window edge is fixed. A host app embedding this branch was built twice, once at this commit and once at its parent, and both were driven through the same script: fill a full screen with 400 numbered lines under tmux with the alternate screen off, park the cursor above the bottom row, then drag the window's bottom edge through 8 shrink/grow cycles and count the lines in scrollback.Both numbers repeated exactly across two runs. The script also checks that the drag reaches the terminal at all — a window resize that changes no rows would make either build look clean — and confirms 31 rows at the short height against 49 at the tall one before it grades anything.
Summary by CodeRabbit