feat: support inline resize and refactor private mode support#912
Conversation
WalkthroughParser now posts eventPrivateMode for DEC private-mode responses and expanded CSI handling for resize/window reports. tScreen adds a privateMode abstraction with enable/disable/query, tracks inlineResize, and replaces many hard-coded private-mode sequences with privateMode methods. Changes
Sequence DiagramsequenceDiagram
participant Term as Terminal
participant Parser as input.go (parser)
participant Screen as tscreen.go (screen)
Term->>Parser: CSI response (private-mode / resize / window size)
Parser->>Parser: validate intermediate field / branch selection
alt private-mode response
Parser->>Parser: handlePrivateModeResponse(params)
Parser->>Screen: post eventPrivateMode{Mode,Supported,Enabled,Permanent}
else window-size or resize report
Parser->>Screen: post EventResize / EventWindowSize
end
Screen->>Screen: receive eventPrivateMode
Screen->>Screen: ev.usable() (Supported && !Permanent)
alt usable
Screen->>Screen: set inlineResize = true
Screen->>Term: pmResizeReports.query()/enable() (ANSI)
else not usable
Screen->>Screen: fallback to tty.NotifyResize
end
Note right of Screen: Subsequent flows emit pmX.enable()/disable()/query() for cursor, mouse, paste, focus, buffering, resize
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tscreen.go (1)
361-388:eventPrivateModeis not routed toinitQ, soprocessInitQwill never receive it.The
filterEventsfunction only forwards*eventTermNameand*eventPrimaryAttributestoinitQ, butprocessInitQ(lines 348-354) expects*eventPrivateModeevents. SinceeventPrivateModefalls through toeventQ, the inline resize feature will never be detected during initialization.🔎 Proposed fix
func (t *tScreen) filterEvents() chan Event { inQ := make(chan Event, 128) go func() { for { var ev Event select { case ev = <-inQ: case <-t.quit: return } switch ev.(type) { case *eventTermName: select { case t.initQ <- ev: default: } case *eventPrimaryAttributes: select { case t.initQ <- ev: default: } + case *eventPrivateMode: + select { + case t.initQ <- ev: + default: + } default: t.eventQ <- ev } } }() return inQ }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
input.go(6 hunks)input_test.go(1 hunks)tscreen.go(12 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: gdamore
Repo: gdamore/tcell PR: 908
File: stdin_unix.go:128-155
Timestamp: 2025-12-18T02:50:37.669Z
Learning: In the tcell Tty interface implementations (stdin_unix.go, tty_unix.go, tty_win.go, tty_plan9.go), the caller is responsible for providing synchronization/mutual exclusion guarantees for method calls like NotifyResize. The Tty implementations themselves do not need internal locking for these methods, as documented in tty.go. The tscreen.go layer handles the synchronization.
📚 Learning: 2025-12-18T02:50:37.669Z
Learnt from: gdamore
Repo: gdamore/tcell PR: 908
File: stdin_unix.go:128-155
Timestamp: 2025-12-18T02:50:37.669Z
Learning: In the tcell Tty interface implementations (stdin_unix.go, tty_unix.go, tty_win.go, tty_plan9.go), the caller is responsible for providing synchronization/mutual exclusion guarantees for method calls like NotifyResize. The Tty implementations themselves do not need internal locking for these methods, as documented in tty.go. The tscreen.go layer handles the synchronization.
Applied to files:
input.goinput_test.gotscreen.go
🧬 Code graph analysis (2)
input.go (2)
resize.go (1)
NewEventResize(25-33)event.go (1)
EventTime(30-32)
tscreen.go (1)
screen.go (3)
MouseButtonEvents(263-263)MouseDragEvents(264-264)MouseMotionEvents(265-265)
🔇 Additional comments (10)
input_test.go (1)
15-16: LGTM! Build constraints correctly exclude tests from JS/Wasm builds.The constraints align with the corresponding changes in
input.go, ensuring tests only run on platforms where the input parser is available.input.go (4)
25-26: LGTM! Build constraints correctly added.The constraints match the test file and exclude this input parser from JS/Wasm builds where a different implementation is presumably used.
913-937: LGTM! CSI handling correctly extended for DECRPM responses.The guard for empty intermediate bytes and the new
'y'case with"$"intermediate correctly implements DECRPM (DEC Private Mode Report) parsing per ANSI/DEC standards.
986-991: LGTM! Inline resize report handling correctly implemented.The case 48 handling properly posts a resize event with the correct width/height parameter order, consistent with the existing case 8 handling.
1086-1097: LGTM! Clean event type for private mode responses.The
eventPrivateModestruct follows the established pattern (embeddingEventTime) and theusable()helper correctly identifies controllable modes.tscreen.go (5)
101-130: LGTM! Clean abstraction for DEC private mode handling.The
privateModetype withenable(),disable(), andquery()methods provides a clean, type-safe interface that replaces scattered escape sequence strings. The escape sequence formats are correct per DEC/ANSI standards.
348-354: Logic is correct, but depends on thefilterEventsfix above.Once
eventPrivateModeis properly routed toinitQ, this handling will correctly detect inline resize support by checking if the mode is usable (supported and not permanently set).
727-727: LGTM! Consistent refactor of escape sequences to useprivateModemethods.The replacements maintain identical behavior while improving code maintainability. The generated escape sequences match the previous hardcoded strings.
Also applies to: 781-793, 864-880, 907-913, 931-935
1176-1216: LGTM! Initialization correctly queries resize capability and conditionally enables it.The sequence of querying for resize reports during initialization, then conditionally enabling either inline resize or TTY-based resize notifications, is correct. Based on learnings, the
tty.NotifyResizecall is properly synchronized by the tscreen layer.
1233-1258: LGTM! Cleanup correctly mirrors the engagement logic.The disengage properly disables inline resize (or unregisters TTY notifications), restores the cursor, and re-enables auto margin to return the terminal to its original state.
99233f5 to
5208295
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
input.go(6 hunks)input_test.go(1 hunks)tscreen.go(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- input_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-18T02:50:37.669Z
Learnt from: gdamore
Repo: gdamore/tcell PR: 908
File: stdin_unix.go:128-155
Timestamp: 2025-12-18T02:50:37.669Z
Learning: In the tcell Tty interface implementations (stdin_unix.go, tty_unix.go, tty_win.go, tty_plan9.go), the caller is responsible for providing synchronization/mutual exclusion guarantees for method calls like NotifyResize. The Tty implementations themselves do not need internal locking for these methods, as documented in tty.go. The tscreen.go layer handles the synchronization.
Applied to files:
input.gotscreen.go
🧬 Code graph analysis (2)
input.go (2)
resize.go (1)
NewEventResize(25-33)event.go (1)
EventTime(30-32)
tscreen.go (1)
screen.go (3)
MouseButtonEvents(263-263)MouseDragEvents(264-264)MouseMotionEvents(265-265)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build (oldstable)
🔇 Additional comments (10)
input.go (4)
25-27: Build constraints are correct.The
//go:build !js || !wasmcorrectly excludes js+wasm builds while including all other platforms. The legacy// +buildtag is equivalent for backward compatibility.
864-890: DECRPM response handling now correctly implemented.The switch statement properly handles all DECRPM response values: 1 (set), 2 (reset), 3 (permanently set), 4 (permanently reset). Values 0 or >4 correctly result in
Supported=false. This addresses the previous review concern about incorrectPermanentdetection.
928-952: Improved CSI parsing with proper intermediate byte validation.The added guards correctly ensure:
- Mouse events only process when no intermediate bytes are present
- Primary DA only when intermediate is empty
- Private mode response (DECRPM) only when intermediate is
$This prevents misinterpreting sequences with unexpected intermediate bytes.
1101-1112: Well-designed event type for private mode responses.The
eventPrivateModestruct follows the established pattern withEventTimeembedding. Theusable()method correctly identifies modes that can be enabled/disabled (supported and not permanent).tscreen.go (6)
101-130: Clean abstraction for DEC private modes.The
privateModetype withenable(),disable(), andquery()methods provides a consistent interface for managing terminal modes. This replaces scattered escape sequence strings with a maintainable, type-safe approach. The escape sequence formats follow DEC/ANSI standards.
348-354: Inline resize capability detection looks correct.The code properly checks
ev.usable()before enablinginlineResize, ensuring the terminal supports the feature and hasn't permanently disabled it. This integrates well with theprocessInitQflow.
859-875: Mouse tracking control refactored to use privateMode.The mouse enable/disable logic now uses
pmMouseButton,pmMouseDrag,pmMouseMotion, andpmMouseSgrwith theirenable()/disable()methods. The functionality is preserved while gaining consistency with the new abstraction.
776-788: Buffering and cursor visibility use privateMode consistently.
startBuffering(),endBuffering(),showCursor(), andhideCursor()now use thepmSyncOutputandpmShowCursormode methods, maintaining consistency with the new abstraction.
1207-1211: Inline resize fallback logic is correctly implemented.The code queries terminal capabilities during initialization, then chooses between inline resize reports (
pmResizeReports.enable()) or signal-based notifications (tty.NotifyResize) based on terminal support. This gracefully handles terminals that don't support mode 2048.
1228-1253: Disengage properly reverses inline resize setup.The cleanup correctly mirrors the engage logic: disabling
pmResizeReportsif inline resize was active, or clearingNotifyResizeotherwise. Terminal state (cursor, auto-margin) is properly restored.
This adds a number new private mode values that we can query for, and a cleaner internal API for setting and querying them. The first use of this is to query for the ability to do inline resize notifications, and if we have it, to skip use of signals. This may allow us to work with resize events in circumstances where the OS does not provide resize indications (for example if the file descriptor in use not actually a tty!) Only a few terminals support this resize notifcation (ghostty, iTerm2, kitty, foot), btu they are not niche.
5208295 to
2849e10
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
input.go (1)
25-26: Build constraint mismatch between new and legacy syntax.The new build constraint
//go:build !js || !wasmand legacy// +build !js !wasmhave different semantics. This should match tscreen.go's approach of//go:build !(js && wasm).🔎 Proposed fix
-//go:build !js || !wasm -// +build !js !wasm +//go:build !(js && wasm) +// +build !js !wasm
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
input.go(6 hunks)input_test.go(1 hunks)tscreen.go(13 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-18T02:50:37.669Z
Learnt from: gdamore
Repo: gdamore/tcell PR: 908
File: stdin_unix.go:128-155
Timestamp: 2025-12-18T02:50:37.669Z
Learning: In the tcell Tty interface implementations (stdin_unix.go, tty_unix.go, tty_win.go, tty_plan9.go), the caller is responsible for providing synchronization/mutual exclusion guarantees for method calls like NotifyResize. The Tty implementations themselves do not need internal locking for these methods, as documented in tty.go. The tscreen.go layer handles the synchronization.
Applied to files:
tscreen.goinput.goinput_test.go
🧬 Code graph analysis (1)
input.go (2)
resize.go (1)
NewEventResize(25-33)event.go (1)
EventTime(30-32)
🔇 Additional comments (10)
input.go (4)
864-890: LGTM: DECRPM response values correctly handled.The switch-based logic correctly interprets all DECRPM response values (0=not recognized, 1=set, 2=reset, 3=permanently set, 4=permanently reset) with proper field assignments.
928-952: LGTM: Defensive intermediate byte validation strengthens CSI parsing.The guards ensure CSI sequences are only processed when intermediate bytes match expected patterns, preventing misinterpretation of malformed or ambiguous sequences.
989-1009: LGTM: Bounds checks added for both resize report paths.Both case 8 (window size report) and case 48 (resize report) now properly guard parameter array access with
len(P) > 2checks, preventing index out-of-bounds panics on malformed input.
1103-1115: LGTM: Clean abstraction for private mode event handling.The
eventPrivateModestruct andusable()helper provide a clear interface for determining whether a terminal private mode can be toggled, correctly excluding hardwired modes.tscreen.go (6)
101-130: LGTM: Clean privateMode abstraction simplifies terminal control.The
privateModetype withenable(),disable(), andquery()methods provides a unified, maintainable interface for DEC private mode sequences, replacing scattered hard-coded escape sequences throughout the codebase.
348-354: LGTM: Inline resize capability detection integrated into initialization.The initialization flow correctly queries
pmResizeReportsand setsinlineResizewhen the terminal confirms support, enabling the optimization to skip signal-based resize notifications.
372-372: LGTM: Event filtering correctly routes private mode responses to initialization.Adding
*eventPrivateModeto the filter ensures these responses are processed during terminal initialization, consistent witheventTermNameandeventPrimaryAttributes.
722-722: LGTM: Consistent replacement of hard-coded sequences with privateMode API.The refactoring systematically replaces hard-coded escape sequences with
privateModemethod calls across cursor visibility, buffering, mouse tracking, pasting, and focus reporting. This improves maintainability and consistency.Also applies to: 776-776, 780-780, 788-788, 859-875, 902-904, 926-926, 930-930
1171-1172: LGTM: Inline resize optimization correctly integrated into engagement flow.The engagement sequence queries resize capability (line 1172) and conditionally enables inline resize reports (line 1208) or falls back to signal-based
NotifyResize(line 1210), implementing the optimization described in the PR objectives.Also applies to: 1207-1211
1228-1232: LGTM: Disengage correctly reverses engagement and restores terminal state.The disengagement logic symmetrically reverses the inline resize setup and restores cursor visibility and auto-margin using the
privateModeAPI, ensuring clean terminal restoration.Also applies to: 1243-1243, 1253-1253
| //go:build !js || !wasm | ||
| // +build !js !wasm |
There was a problem hiding this comment.
Build constraint mismatch between new and legacy syntax.
The new build constraint //go:build !js || !wasm (line 15) and the legacy constraint // +build !js !wasm (line 16) have different semantics:
//go:build !js || !wasmmeans "exclude only when GOOS=js AND GOARCH=wasm together"// +build !js !wasmmeans "exclude when GOOS=js OR GOARCH=wasm"
These should match. Based on tscreen.go line 15 using //go:build !(js && wasm), the intent appears to be excluding the js+wasm combination specifically.
🔎 Proposed fix
-//go:build !js || !wasm
-// +build !js !wasm
+//go:build !(js && wasm)
+// +build !js !wasm🤖 Prompt for AI Agents
input_test.go around lines 15-16: the two build constraints have different
semantics; update them so they match the intent of excluding only the combined
js+wasm target by changing the new-style to the negation of the conjunction
(i.e. not (js and wasm)) and making the legacy-style the equivalent
comma-separated negation (not js OR not wasm) so both express the same
condition.
This adds a number new private mode values that we can query for, and a cleaner internal API for setting and querying them.
The first use of this is to query for the ability to do inline resize notifications, and if we have it, to skip use of signals. This may allow us to work with resize events in circumstances where the OS does not provide resize indications (for example if the file descriptor in use not actually a tty!) Only a few terminals support this resize notifcation (ghostty, iTerm2, kitty, foot), btu they are not niche.
Summary by CodeRabbit
Refactor
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.