Conversation
📝 WalkthroughWalkthroughIntroduces a capability-based vt backend, a new emulator, and a simplified mock terminal facade. Replaces the old mock TTY implementation with a test-oriented MockBackend, MockTerm, and associated tests; adds vt attribute flags and backend interfaces used by the emulator and mock term. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Test/Client
participant Term as MockTerm
participant Emu as Emulator
participant BE as MockBackend
Note over Test,Term: Test writes escape sequences / data
Test->>Term: Write([]byte)
Term->>Emu: Write([]byte)
Emu->>Emu: parse CSI/OSC/UTF-8 (state machine)
Emu->>BE: PutAbs(coord, rune, Attr) / SetPosition(...)
BE-->>Emu: Ack (state stored)
Emu-->>Term: (Drain/Read results / DA responses)
Term-->>Test: Read(...) / GetCell(coord) / Bells()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #925 +/- ##
==========================================
- Coverage 39.63% 29.72% -9.91%
==========================================
Files 30 32 +2
Lines 2768 3028 +260
==========================================
- Hits 1097 900 -197
- Misses 1569 2069 +500
+ Partials 102 59 -43 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (7)
mock/term.go (2)
55-55: Mark unused parameter with underscore.The
resizqparameter is unused; prefix it with_to indicate intentional non-use.🔎 Proposed fix
-func (mt *mockTerm) NotifyResize(resizq chan<- bool) {} +func (mt *mockTerm) NotifyResize(_ chan<- bool) {}
57-60: Propagate error fromStop().
Close()discards the error returned byStop(). Consider returning it to the caller.🔎 Proposed fix
func (mt *mockTerm) Close() error { - mt.Stop() - return nil + return mt.Stop() }mock/term_test.go (1)
72-73: RedundantStop()call beforeClose().Since
Close()already callsStop()internally (per mock/term.go line 58), the explicitStop()call is redundant.🔎 Proposed fix
- trm.Stop() trm.Close()vt/emulate.go (2)
888-916: Non-standardio.Readerreturn value on error.Returning
-1fornon error deviates from theio.Readercontract which expectsn >= 0. Consider returning0instead.Verify if returning
-1is intentional for signaling to specific callers, or if it should follow standardio.Readersemantics.
864-886: Non-standardio.Writerreturn value on error.Similar to
Read(), returning-1fornon error deviates fromio.Writercontract. Consider returning0or actual bytes written before error.mock/backend.go (1)
145-146: Unnecessary intermediate variable.The
bevariable is assignedmband returned unchanged. Can simplify to returnmbdirectly.🔎 Proposed fix
func NewMockBackend(options ...MockOpt) MockBackend { mb := &mockBackend{ size: vt.Coord{X: 80, Y: 24}, colors: 256, } - var be MockBackend - be = mb for _, opt := range options { opt.SetMockOpt(mb) // TODO: possibly be could be "filtered" for some options (e.g. to hide colorer API, etc.) } mb.cells = make([]Cell, int(mb.size.X)*int(mb.size.Y)) mb.modes = make(map[vt.PrivateMode]vt.ModeStatus) mb.modes[vt.PmShowCursor] = vt.ModeOn - return be + return mb }vt/backend.go (1)
92-94: Minor documentation inconsistency.The comment says "at the given position" but
PutGraphemedoesn't take a position argument—it writes at the current cursor position (consistent withPutChar). Consider clarifying.🔎 Proposed fix
- // Put the grapheme cluster at the given position. This will occupy a single + // Put the grapheme cluster at the current cursor position. This will occupy a single // narrow or wide terminal cell. PutGrapheme([]rune)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
mock/backend.gomock/mock.gomock/mock_test.gomock/term.gomock/term_test.govt/attr.govt/backend.govt/emulate.govt/vt.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-18T02:50:48.847Z
Learnt from: gdamore
Repo: gdamore/tcell PR: 908
File: stdin_unix.go:128-155
Timestamp: 2025-12-18T02:50:48.847Z
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:
mock/term_test.gomock/term.govt/vt.gomock/mock.go
🧬 Code graph analysis (4)
mock/mock_test.go (1)
vt/attr.go (3)
Attr(18-18)Plain(21-21)Bold(22-22)
vt/backend.go (3)
vt/mode.go (2)
PrivateMode(23-23)ModeStatus(63-63)vt/coord.go (1)
Coord(28-31)vt/attr.go (1)
Attr(18-18)
mock/backend.go (4)
vt/attr.go (1)
Attr(18-18)vt/backend.go (1)
Backend(24-47)vt/coord.go (1)
Coord(28-31)vt/mode.go (5)
PrivateMode(23-23)ModeStatus(63-63)ModeOn(67-67)ModeOff(68-68)PmShowCursor(30-30)
mock/mock.go (1)
vt/attr.go (2)
Attr(18-18)Plain(21-21)
🔇 Additional comments (11)
vt/vt.go (1)
18-21: LGTM!The development-status disclaimer is a good practice for signaling API instability to consumers.
mock/mock_test.go (1)
66-68: LGTM!The attribute constant updates correctly migrate from
tcell.AttrMasktovt.Attrvalues, aligning with the new attribute system invt/attr.go.Also applies to: 84-84, 92-94
mock/mock.go (1)
42-42: LGTM!The attribute field type migration to
vt.Attrand initialization withvt.Plaincorrectly align with the new VT attribute system.Also applies to: 647-647
mock/term.go (1)
82-90: LGTM!Constructor correctly wires the backend, channels, and emulator together with appropriate initialization.
mock/term_test.go (2)
128-145: LGTM!The synchronous
Write()implementation ensures bell processing completes beforeBells()is called, making explicitDrain()unnecessary here.
147-211: LGTM!Device attribute tests properly verify primary DA and extended attribute responses, including legacy ESC Z sequence handling.
vt/attr.go (1)
16-38: LGTM!The bit-flag attribute system is well-designed with clear separation of basic attributes (bits 0-7) and underline style variants (bits 13-15). The
UnderlineMaskcorrectly covers all underline-related bits.vt/emulate.go (2)
62-77: LGTM!The constructor correctly initializes the emulator in a stopped state (closed
stopQ) requiringStart()before use. The function pointer assignment for state machine is a good pattern.
820-840: LGTM!The start/stop lifecycle management correctly uses channel signaling for state coordination, preventing double-start and ensuring clean shutdown.
mock/backend.go (1)
138-156: LGTM!The constructor with functional options pattern provides clean extensibility. Default values of 80x24 and 256 colors are sensible for terminal testing.
vt/backend.go (1)
24-47: LGTM!The
Backendinterface is well-designed with clear documentation. The edge-case behavior forSetPosition(clipping to window bounds) andSetPrivateMode(ignoring invalid values) is properly specified.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
vt/backend.go (2)
73-75: This documentation typo was already flagged in a previous review.
78-80: This documentation mismatch was already flagged in a previous review.mock/backend.go (1)
44-59: Missing Colors() method for vt.Colorer interface.The comment states mockBackend implements vt.Colorer, but the required
Colors() intmethod is missing. SetFgColor and SetBgColor are now present, but Colors() is still needed.🔎 Proposed fix
+func (mb *mockBackend) Colors() int { + return mb.colors +} + func (mb *mockBackend) SetFgColor(c color.Color) { mb.setColor(c, &mb.fg, mb.defaultFg) }
🧹 Nitpick comments (1)
mock/term.go (1)
73-76: Consider returning the Stop() error in Close().Close() currently ignores the error returned by Stop(). While this may be acceptable for a mock, explicitly returning the error would be more consistent with Go error-handling conventions.
🔎 Proposed refactor
func (mt *mockTerm) Close() error { - mt.Stop() - return nil + return mt.Stop() }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
mock/backend.gomock/mock.gomock/mock_test.gomock/term.gomock/term_test.govt/attr.govt/backend.govt/emulate.govt/vt.go
✅ Files skipped from review due to trivial changes (1)
- vt/vt.go
🚧 Files skipped from review as they are similar to previous changes (2)
- mock/mock_test.go
- vt/attr.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-18T02:50:48.847Z
Learnt from: gdamore
Repo: gdamore/tcell PR: 908
File: stdin_unix.go:128-155
Timestamp: 2025-12-18T02:50:48.847Z
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:
vt/backend.gomock/term_test.gomock/term.gomock/mock.go
🧬 Code graph analysis (4)
vt/backend.go (3)
vt/mode.go (2)
PrivateMode(23-23)ModeStatus(63-63)vt/coord.go (1)
Coord(28-31)vt/attr.go (1)
Attr(18-18)
mock/term_test.go (6)
mock/term.go (2)
MockTerm(98-109)NewMockTerm(112-118)vt/coord.go (3)
Col(25-25)Row(22-22)Coord(28-31)mock/backend.go (2)
MockOptSize(160-160)MockOptColors(165-165)resize.go (1)
WindowSize(46-51)vt/attr.go (3)
Attr(18-18)Plain(21-21)Bold(22-22)sim_test.go (1)
TestClearScreen(48-64)
mock/term.go (6)
mock/backend.go (2)
MockBackend(34-42)Cell(24-30)vt/emulate.go (2)
Emulator(34-57)NewEmulator(62-75)resize.go (1)
WindowSize(46-51)vt/backend.go (1)
Resizer(98-101)vt/coord.go (1)
Coord(28-31)tty.go (1)
Tty(32-69)
mock/mock.go (1)
vt/attr.go (2)
Attr(18-18)Plain(21-21)
🔇 Additional comments (8)
mock/mock.go (2)
42-42: LGTM! Attr field migrated to vt.Attr.The migration from tcell.AttrMask to vt.Attr is consistent with the new attribute system introduced in vt/attr.go.
647-647: LGTM! Reset correctly uses vt.Plain.The initialization of mt.Attr with vt.Plain aligns with the new vt attribute system.
mock/term_test.go (1)
63-239: LGTM! Comprehensive test coverage.The test suite provides excellent coverage of cursor movement, DECALN rendering, bell counting, and device attribute responses. Proper use of helper functions and vt.Attr constants throughout.
mock/term.go (1)
111-118: LGTM! Clean constructor design.The NewMockTerm constructor properly initializes the mock terminal with a backend and emulator, and sets the terminal identity for device attribute responses.
vt/emulate.go (1)
34-921: LGTM! Emulator implementation is comprehensive.The emulator provides a complete terminal emulation layer with proper escape sequence parsing, state management, and backend integration. Notable implementation details:
- Correct use of
&^=for clearing underline mask (line 387)- Proper CHA implementation setting column directly (line 473)
- Correct erase operations with appropriate bounds (lines 707-733)
- Clean Start/Stop/Drain lifecycle management with channel synchronization
The bugs flagged in previous reviews appear to have been addressed.
mock/backend.go (3)
82-92: LGTM! Width now correctly initialized.The PutAbs method now properly sets the cell Width using uniseg.StringWidth, addressing the previous review comment.
147-153: LGTM! Colorer color setter methods implemented.SetFgColor and SetBgColor are now properly implemented using the setColor helper, addressing part of the previous review comment.
169-188: LGTM! Well-designed constructor with options pattern.NewMockBackend provides a clean options-based initialization pattern with sensible defaults (80x24, 256 colors, cursor enabled).
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
mock/backend.go (1)
44-59: MissingColors()method to fully implementvt.Colorer.The comment claims
mockBackendimplementsvt.Colorer, but theColors() intmethod is not present.SetFgColorandSetBgColorare implemented, butColors()is required by the interface.🔎 Proposed fix
func (mb *mockBackend) GetSize() vt.Coord { return mb.size } func (mb *mockBackend) Beep() { mb.bells++ } +func (mb *mockBackend) Colors() int { return mb.colors }
🧹 Nitpick comments (4)
vt/emulate.go (2)
345-411: SGR handling is incomplete.The current implementation handles basic attributes (0-9) but misses:
- Color setting (30-37, 40-47, 90-97, 100-107)
- Extended colors via 38/48 (256-color and RGB)
- Attribute resets (22-29)
This appears intentional for WIP, but consider adding a TODO comment.
792-800: SendRaw may block if read buffer fills.With a 32-byte buffer on
readQ, large responses (like device attributes) could block if the consumer doesn't read promptly. This is acceptable for testing but worth documenting for real emulator usage.mock/backend.go (2)
128-145: Silent no-op for invalid non-Reset colors may be unexpected.When
c.Valid()is false andc != color.Reset, the color is silently ignored. This handlescolor.Nonebut could surprise callers with other invalid values. Consider documenting this behavior or handling explicitly.
169-188: Consider initializingfg/bgto default colors.The factory sets
defaultFganddefaultBgbut leavesfgandbgat their zero values (color.Color(0)). IfPutAbsis called before any color is set, cells will have the zero color rather than the intended defaults.🔎 Proposed fix
func NewMockBackend(options ...MockOpt) MockBackend { mb := &mockBackend{ size: vt.Coord{X: 80, Y: 24}, colors: 256, defaultFg: color.Silver, defaultBg: color.Black, + fg: color.Silver, + bg: color.Black, }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
mock/backend.gomock/mock.gomock/mock_test.gomock/term.gomock/term_test.govt/attr.govt/backend.govt/emulate.govt/vt.go
💤 Files with no reviewable changes (2)
- mock/mock.go
- mock/mock_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- vt/attr.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-18T02:50:48.847Z
Learnt from: gdamore
Repo: gdamore/tcell PR: 908
File: stdin_unix.go:128-155
Timestamp: 2025-12-18T02:50:48.847Z
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:
mock/term_test.govt/vt.govt/backend.gomock/backend.gomock/term.go
🧬 Code graph analysis (2)
vt/backend.go (3)
vt/mode.go (2)
PrivateMode(23-23)ModeStatus(63-63)vt/coord.go (1)
Coord(28-31)vt/attr.go (1)
Attr(18-18)
mock/backend.go (5)
vt/attr.go (1)
Attr(18-18)vt/backend.go (1)
Backend(24-47)vt/coord.go (1)
Coord(28-31)vt/mode.go (5)
PrivateMode(23-23)ModeStatus(63-63)ModeOn(67-67)ModeOff(68-68)PmShowCursor(30-30)color/color.go (4)
IsRGB(49-49)Reset(860-860)Silver(68-68)Black(61-61)
🔇 Additional comments (14)
vt/vt.go (1)
18-20: Good addition of stability disclaimer.The caveat emptor warning appropriately sets expectations for consumers of this WIP package.
mock/term_test.go (3)
25-40: Well-structured test helper.The
writeFhelper properly handles partial writes by looping until all bytes are consumed, and ensures data is flushed viaDrain().
109-156: Good test coverage for DECALN and attribute handling.The test properly validates both content and attributes after DECALN, and verifies that bold attribute is preserved when combined with DECALN.
176-212: Solid device attribute response testing.Testing both modern CSI and legacy ESC Z forms of Primary DA request provides good coverage.
mock/term.go (4)
22-26: Clean composition pattern.The
mockTermstruct cleanly separates concerns by delegating toMockBackendfor state andvt.Emulatorfor processing.
38-42: Drain is essential for test reliability.Properly flushing pending commands before assertions prevents race conditions in tests.
64-70: Correct optional capability pattern.The type assertion check before calling
NotifyResizefollows the established pattern for optional backend capabilities. Based on learnings, caller-provided synchronization is the expected contract.
110-117: Factory function properly initializes the stack.
NewMockTermcorrectly creates the backend first, then wraps it with the emulator, and sets the terminal identity.vt/emulate.go (2)
62-75: Emulator initialization looks correct.The constructor properly initializes channels, buffers, and sets the initial state function. Closing
stopQimmediately indicates "not running" state.
903-921: Run loop design is sound.The goroutine properly handles both data items and drain synchronization channels, with clean shutdown via
stopQ.vt/backend.go (3)
24-47: Well-designed core Backend interface.The interface captures essential terminal operations with clear documentation. The capability-based design allows backends to implement only what they support.
69-76: Good interface composition for UnderlineColorer.Embedding
Colorerensures underline color support implies full color support.
87-95: Consider adding Attr parameter to PutGrapheme.
PutChar(rune, Attr)takes attributes explicitly, butPutGrapheme([]rune)does not. This inconsistency may be intentional (using current attribute state), but worth verifying the design intent.mock/backend.go (1)
122-126: Correct bounds clamping per Backend contract.Position is properly clamped to
[0, size-1]as specified in thevt.Backendinterface documentation.
This new emulator backend will be able to be used for more than just the mock, but also potentially to create real terminal emulators. Right now its not useful yet.
The MockTerm is better because it uses a common terminal emulator framework that can be used for other things, including creating a real emulator.
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 (9)
mock/backend.gomock/mock.gomock/mock_test.gomock/term.gomock/term_test.govt/attr.govt/backend.govt/emulate.govt/vt.go
💤 Files with no reviewable changes (2)
- mock/mock_test.go
- mock/mock.go
✅ Files skipped from review due to trivial changes (1)
- vt/vt.go
🚧 Files skipped from review as they are similar to previous changes (2)
- mock/term_test.go
- vt/attr.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-18T02:50:48.847Z
Learnt from: gdamore
Repo: gdamore/tcell PR: 908
File: stdin_unix.go:128-155
Timestamp: 2025-12-18T02:50:48.847Z
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:
mock/term.gomock/backend.govt/backend.go
🧬 Code graph analysis (2)
mock/backend.go (5)
vt/attr.go (1)
Attr(18-18)vt/backend.go (1)
Backend(24-47)vt/coord.go (1)
Coord(28-31)vt/mode.go (5)
PrivateMode(23-23)ModeStatus(63-63)ModeOn(67-67)ModeOff(68-68)PmShowCursor(30-30)color/color.go (4)
IsRGB(49-49)Reset(860-860)Silver(68-68)Black(61-61)
vt/backend.go (3)
vt/mode.go (2)
PrivateMode(23-23)ModeStatus(63-63)vt/coord.go (1)
Coord(28-31)vt/attr.go (1)
Attr(18-18)
🔇 Additional comments (7)
mock/term.go (1)
111-117: LGTM - Clean integration of backend and emulator.The constructor properly initializes the mock backend, creates an emulator on top of it, and sets the terminal identity. The delegation pattern throughout the file is clean and appropriate.
vt/emulate.go (3)
147-161: Previous UTF-8 lead byte issue confirmed as resolved.The lead byte is now properly written to the buffer (lines 150, 154, 158) before switching to the UTF-8 continuation state, ensuring the complete sequence is available for decoding.
389-404: Previous underline attribute handling issue confirmed as resolved.Line 390 correctly uses AND-NOT (
&^=) to clear the underline mask before setting the underline attribute, ensuring deterministic behavior on repeated applications.
729-736: Previous eraseLine bug confirmed as resolved.The function now correctly erases only the current line (line 732-733 iterate over columns at pos.Y only) rather than the entire screen.
vt/backend.go (1)
19-101: LGTM - Well-structured capability-based interface design.The interface suite provides a clean, modular backend abstraction with optional capabilities (Beeper, Colorer, Positioner, etc.). Documentation is clear and previous typos have been corrected.
mock/backend.go (2)
147-157: Previous Colorer interface issues confirmed as resolved.The
Colors(),SetFgColor(), andSetBgColor()methods are now properly implemented, completing thevt.Colorerinterface requirements.
82-92: Previous Cell width initialization issue confirmed as resolved.Line 88 now correctly sets the cell width using
uniseg.StringWidth(string(r)), ensuring proper width calculation for single-width, wide, and zero-width characters.
This new emulator backend will be able to be used for more than just the mock, but also potentially to create real terminal emulators. Right now its not useful yet.
Summary by CodeRabbit
New Features
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.