test: implement more of the mock screen#917
Conversation
WalkthroughMockTty gains an input state machine (ESC/CSI/OSC), UTF‑8 decoding/buffering, cursor and erase handlers, and goroutine lifecycle coordination. Public changes: Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-12-18T02:50:37.669ZApplied to files:
🧬 Code graph analysis (1)mock/mock.go (1)
🔇 Additional comments (18)
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 #917 +/- ##
==========================================
+ Coverage 27.95% 29.68% +1.73%
==========================================
Files 40 40
Lines 3456 3743 +287
==========================================
+ Hits 966 1111 +145
- Misses 2399 2534 +135
- Partials 91 98 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
mock/mock_test.go (1)
43-43: Fixed sleep may be fragile in tests.Fixed-duration sleeps can cause test flakiness on slow CI runners. Consider whether a synchronization mechanism (e.g., waiting for a specific state or using a channel) would be more robust. However, for a mock test helper, this 100ms delay may be acceptable if the goal is simply to allow initialization sequences to complete.
mock/mock.go (1)
85-100: Minor: redundantstrings.Splitcall.The string is split twice (lines 86 and 94). Consider reusing the
wordsslice:🔎 Proposed fix
func intParams(str string, minNum int, defVal int) []int { words := strings.Split(str, ";") if len(words) > minNum { minNum = len(words) } result := make([]int, minNum) for i := range len(result) { result[i] = defVal } - for i, word := range strings.Split(str, ";") { + for i, word := range words { if v, e := strconv.Atoi(word); e == nil { result[i] = v } } return result }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mock/mock.go(5 hunks)mock/mock_test.go(2 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:
mock/mock.go
🧬 Code graph analysis (1)
mock/mock.go (1)
attr.go (2)
AttrMask(22-22)AttrNone(35-35)
🔇 Additional comments (4)
mock/mock_test.go (1)
15-16: Build constraints look correct.The dual format (
//go:buildand// +build) ensures compatibility with both Go 1.17+ and earlier versions. Excludingjsandwasmtargets makes sense since these platform targets don't support the terminal operations being tested.mock/mock.go (3)
439-456: Good goroutine lifecycle management.The
WaitGroupcorrectly synchronizes therun()goroutine, and thestopQclose is guarded against double-close. This aligns with the Tty interface pattern where the caller provides synchronization. Based on learnings, this design is consistent with other tcell Tty implementations.
517-533: Initialization looks correct.The
Reset()function properly initializes the state machine, escape buffer, and default terminal attributes on first call.
102-128: Movement helper functions look correct.The bounds checking in
moveUp,moveDown,moveForward, andmoveBackwardcorrectly constrains the cursor to valid positions.
41d833e to
1cea08c
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
mock/mock.go (2)
73-83: StatesstateDCS,stateSOS,statePM,stateAPCare defined but not handled.These states are entered (lines 313-321, 382-396) but the main
run()switch has no cases for them. The mock will remain stuck in these states, ignoring all subsequent input until stopped.
332-357: UTF-8 validation fixed, but wide character bounds issue remains.Line 333 now correctly validates UTF-8 continuation bytes (fixed from previous review). However, the wide character handling at lines 351-356 still has a potential out-of-bounds access.
When a wide character is placed at the last column of the last row (
mt.X == mt.Cols-1andmt.Y == mt.Rows-1), line 352 accessesmt.Cells[ix+1], which equalsmt.Rows * mt.Colsand is out of bounds.🔎 Proposed fix
if uniseg.StringWidth(string(r)) > 1 && mt.X < mt.Cols { - mt.Cells[ix+1] = Cell{C: nil, Fg: mt.Fg, Bg: mt.Bg, Attr: mt.Attr} - if mt.X < mt.Cols-1 { - mt.X++ + if ix+1 < len(mt.Cells) { + mt.Cells[ix+1] = Cell{C: nil, Fg: mt.Fg, Bg: mt.Bg, Attr: mt.Attr} + if mt.X < mt.Cols-1 { + mt.X++ + } } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
mock/mock.go(4 hunks)mock/mock_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- mock/mock_test.go
🧰 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:
mock/mock.go
🧬 Code graph analysis (1)
mock/mock.go (1)
attr.go (2)
AttrMask(22-22)AttrNone(35-35)
🔇 Additional comments (9)
mock/mock.go (9)
85-100: LGTM!The
intParamshelper correctly parses CSI parameters with sensible defaults and handles edge cases.
102-135: LGTM!The movement helpers correctly clamp cursor positions, and
eraseCellnow uses the correct index calculation(y * mt.Cols) + x(fixed from previous review).
153-177: Good fixes from previous review.The CHA (line 175) now correctly validates against
mt.Colsinstead ofmt.Rows. The cursor movement commands are implemented correctly.
224-245: LGTM!The device attribute and status report handlers correctly respond with the configured attributes.
358-413: Escape sequence handling improved.Lines 366-374 now correctly compare
mt.Yagainstmt.Rows-1instead ofmt.Cols-1(fixed from previous review). The ESC sequence parsing logic looks correct.
414-435: LGTM!The CSI and OSC state handlers correctly accumulate sequence parameters and detect terminators.
453-480: LGTM!The
Read()implementation correctly follows VMIN 1, VTIME 0 semantics: blocking on the first byte, then reading available bytes without blocking, with proper stop signal handling.
495-505: Good fix from previous review.The
Drain()method now uses a select-with-default pattern to safely closestopQwithout panicking if it's already closed (fixed from previous review).
515-533: LGTM!The
Reset()method properly initializes all state, including the new fields (Attr,PrimaryAttributes,SecondaryAttributes,escBuf).
| case "J": // erase in display (ED) | ||
| switch intParams(str, 1, 0)[0] { | ||
| case 0: // erase below | ||
| for y := mt.Y + 1; y < mt.Rows; y++ { | ||
| for x := 0; x < mt.Cols; x++ { | ||
| mt.eraseCell(x, y) | ||
| } | ||
| } | ||
| case 1: // erase above | ||
| for y := 0; y < mt.Y; y++ { | ||
| for x := 0; x < mt.Cols; x++ { | ||
| mt.eraseCell(x, y) | ||
| } | ||
| } | ||
| case 2: // erase all | ||
| for y := range mt.Rows { | ||
| for x := range mt.Cols { | ||
| mt.eraseCell(x, y) | ||
| } | ||
| } | ||
| // others not supported (3 is erase saved lines) | ||
| } |
There was a problem hiding this comment.
ED case 1 (erase above) is incomplete.
Lines 198-204 correctly fix the "erase all" case from the previous review. However, ED case 1 (erase above) at lines 192-197 only erases complete rows above the cursor line. According to the VT specification, it should also erase the current row from column 0 to the cursor position (inclusive).
🔎 Proposed fix
case 1: // erase above
for y := 0; y < mt.Y; y++ {
for x := 0; x < mt.Cols; x++ {
mt.eraseCell(x, y)
}
}
+ for x := 0; x <= mt.X; x++ {
+ mt.eraseCell(x, mt.Y)
+ }🤖 Prompt for AI Agents
In mock/mock.go around lines 184 to 205, the ED case for parameter 1 (erase
above) currently only erases full rows above the cursor and omits clearing the
portion of the current row from column 0 through the cursor column; update that
branch to first erase columns 0..mt.X on the current row (inclusive of the
cursor position) by calling mt.eraseCell for x from 0 to mt.X, then continue to
erase all full rows y from 0 up to mt.Y-1 as already implemented.
1cea08c to
8c22982
Compare
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.