Fix VT formatter cursor restoration after margins - #191
Conversation
📝 WalkthroughWalkthrough
ChangesTerminal formatting state
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 |
![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
🤖 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/formatter.zig`:
- Around line 297-325: Move the entire cursor restore and pin-map block guarded
by self.extra.screen.cursor out of the palette-specific path and place it after
all terminal-wide state emission within if (self.opts.emit == .vt). Preserve the
scrolling-region/origin coordinate calculations and pin-map updates, while
ensuring the final CUP sequence is emitted for VT output even when palette
output is disabled and never for .plain or .html output.
🪄 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: 032f256d-3e4d-4376-9546-1920cbb6241b
📒 Files selected for processing (1)
src/terminal/formatter.zig
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e49174beb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Scrolling regions and tabstop restoration move the cursor after | ||
| // ScreenFormatter emits it. Restore the cursor after all terminal | ||
| // state, with CUP coordinates relative to the origin margins. | ||
| if (self.extra.screen.cursor) { |
There was a problem hiding this comment.
This restoration is nested inside the palette block and runs before ScreenFormatter, scrolling-region, and tabstop output. When extra.palette is false—as in the added regression test—it is never emitted; when the palette is enabled, subsequent cursor-moving sequences overwrite it anyway. Consequently, formatting with cursor plus margins or tabstops still produces the wrong final cursor position, so this block needs to execute at the end of VT terminal-state emission.
Useful? React with 👍 / 👎.
54592d0 to
85a4fb9
Compare
85a4fb9 to
d6fdb42
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6fdb42e1b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const origin = self.terminal.modes.get(.origin); | ||
| const row = if (origin) cursor.y - region.top else cursor.y; | ||
| const col = if (origin) cursor.x - region.left else cursor.x; |
There was a problem hiding this comment.
When a caller enables screen.cursor without both modes and scrolling_region, a source terminal with origin mode and nonzero margins produces margin-relative coordinates here even though the output does not recreate that coordinate system. Replaying such a partial snapshot on a fresh terminal therefore places the cursor too far up and left, and this final CUP overrides the earlier absolute CUP emitted by ScreenFormatter; only convert to relative coordinates when the corresponding origin mode and margins are also emitted.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/terminal/formatter.zig (1)
421-425: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve deferred wrap state.
The final CUP restores only coordinates. If the source cursor is pending wrap at the right margin, CUP clears that state. The next printable byte then overwrites the last cell instead of wrapping.
Use one shared cursor-restoration path that preserves wrap-pending behavior. Add a replay test that writes one printable byte after formatting a cursor at the right margin.
Also applies to: 447-492
🤖 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/formatter.zig` around lines 421 - 425, Update formatCursorRestore and the final cursor-restoration flow in ScreenFormatter to preserve a source cursor’s pending-wrap state when restoring right-margin coordinates, rather than relying on CUP alone. Consolidate restoration through one shared path so scrolling-region and tabstop restoration use the same behavior. Add a replay test that formats a cursor at the right margin, writes one printable byte, and verifies it wraps instead of overwriting the last cell.
🧹 Nitpick comments (1)
src/terminal/formatter.zig (1)
5277-5453: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd replay coverage for emitted horizontal margins.
The test at Line 5296 sets only vertical margins. The test at Line 5388 configures horizontal margins, but it omits mode emission, so replay does not enable left-right margin mode.
Add a case that emits
?69h, configures DECSLRM, enables origin mode, and verifies cursor restoration both inside and outside the horizontal margins.🤖 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/formatter.zig` around lines 5277 - 5453, Extend the horizontal-margin replay coverage in the terminal formatter tests, using the existing cursor restoration cases as a guide. Add a scenario that emits DECSLRM mode (?69h), configures left/right margins, enables origin mode, and verifies replay preserves the cursor both within and outside those horizontal margins. Ensure formatter output includes the relevant modes and scrolling-region state.
🤖 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.
Outside diff comments:
In `@src/terminal/formatter.zig`:
- Around line 421-425: Update formatCursorRestore and the final
cursor-restoration flow in ScreenFormatter to preserve a source cursor’s
pending-wrap state when restoring right-margin coordinates, rather than relying
on CUP alone. Consolidate restoration through one shared path so
scrolling-region and tabstop restoration use the same behavior. Add a replay
test that formats a cursor at the right margin, writes one printable byte, and
verifies it wraps instead of overwriting the last cell.
---
Nitpick comments:
In `@src/terminal/formatter.zig`:
- Around line 5277-5453: Extend the horizontal-margin replay coverage in the
terminal formatter tests, using the existing cursor restoration cases as a
guide. Add a scenario that emits DECSLRM mode (?69h), configures left/right
margins, enables origin mode, and verifies replay preserves the cursor both
within and outside those horizontal margins. Ensure formatter output includes
the relevant modes and scrolling-region state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d43e5679-1d74-48b7-b06f-f7641c1a65d1
📒 Files selected for processing (1)
src/terminal/formatter.zig
1 participant
Fix VT formatter cursor restoration after scrolling-region and tabstop state.
The first commit adds a regression test. The second commit restores the cursor after terminal-wide state and converts CUP coordinates for origin mode.
Verification will run through the cmux exact-head hosted workflow on Blacksmith macOS and Linux, plus hosted Windows. No local Zig or Rust command was run.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Restore the cursor after emitting scrolling margins, origin mode, and tabstop state in the VT formatter. Prevents cursor jumps, preserves pending-wrap, and keeps saved cursors intact during replay, including when origin/scrolling region are omitted and when the cursor is outside origin margins.
pin_map.Written for commit 533c27a. Summary will update on new commits.
Summary by CodeRabbit