Conversation
📝 WalkthroughWalkthroughAdds key-repeat management: new emulator helpers ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant KeyboardState as "KeyboardState\n(vt/layout.go)"
participant Emulator as "emulator\n(vt/emulate.go)"
participant MockTerm as "mockTerm\n(vt/mock.go)"
participant Output as "Terminal Output"
Client->>MockTerm: Inject KeyPress(KeyEvent)
MockTerm->>KeyboardState: Pressed(ev)
alt first press or repeating enabled
KeyboardState->>MockTerm: schedule repeat ticks (delay/interval)
end
MockTerm->>Emulator: handle keyLegacy(ev)
alt ALT/CTRL or non-repeatable
Emulator->>Emulator: noRepeatRaw(ev, data)
Emulator->>Output: SendRaw(data) [single]
else repeatable
Emulator->>Emulator: repeatRaw(ev, data)
loop up to configured repeats
Emulator->>Output: SendRaw(data)
end
end
Note over KeyboardState,Emulator: On repeat tick -> MockTerm invokes Pressed/Repeat -> Emulator re-emits via repeatRaw/noRepeatRaw
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1021 +/- ##
==========================================
+ Coverage 85.77% 85.95% +0.17%
==========================================
Files 46 46
Lines 4817 4834 +17
==========================================
+ Hits 4132 4155 +23
+ Misses 534 528 -6
Partials 151 151 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@vt/layout.go`:
- Around line 132-136: The timing uses ks.repeatInterval for the initial delay
when a new key is pressed; change the logic so that when lastKey != k and
ks.repeatInterval != 0 you call ks.clearRepeat(), then set ks.repeatStart =
time.Now().Add(ks.repeatDelay) (not repeatInterval) and set ks.repeatTime =
ks.repeatStart so the first repeat honors the configured initial delay; update
the block around lastKey, k, ks.clearRepeat(), ks.repeatStart and ks.repeatTime
accordingly.
In `@vt/tests/key_test.go`:
- Around line 339-355: TestKeyRepeatCursor contains a duplicate consecutive call
to term.KeyRelease(vt.KeyRight); remove the extra KeyRelease invocation so there
is only a single KeyRelease(vt.KeyRight) where the key should be released;
locate this in the TestKeyRepeatCursor function and delete the redundant
term.KeyRelease(vt.KeyRight) call to clean up the test.
🧹 Nitpick comments (1)
vt/tests/key_test.go (1)
240-257: Consider increasing timing margins for CI stability.Time-based tests with tight tolerances (50ms delay, 25ms interval, 100ms sleep expecting exactly 4 outputs) may be flaky in CI environments where scheduling delays can cause timing variations.
The expected output "xxxx" assumes repeats at 0ms, 50ms, 75ms, 100ms. However, if the system is under load, timing could drift causing fewer or more repeats.
Consider either:
- Using more generous timing margins (e.g., 200ms delay, 100ms interval, 500ms sleep)
- Accepting a range of valid outputs rather than exact counts
- Adding a comment acknowledging potential CI flakiness
💡 Example with more generous timing
- // these are unreasonable repeat rates, but its somewhere to start - term.SetRepeat(time.Millisecond*50, time.Millisecond*25) + // Use generous timing to reduce CI flakiness + term.SetRepeat(time.Millisecond*100, time.Millisecond*50) term.KeyPress(vt.KeyX) - time.Sleep(100 * time.Millisecond) + time.Sleep(250 * time.Millisecond) term.KeyPress(vt.KeyX) term.KeyRelease(vt.KeyX) term.KeyRelease(vt.KeyCapsLock) - CheckRead(t, term, "xxxx") // 0 ms, 50 ms, 75 ms, 100 ms + CheckRead(t, term, "xxxx") // 0 ms, 100 ms, 150 ms, 200 ms
| func TestKeyRepeatCursor(t *testing.T) { | ||
| term := vt.NewMockTerm() | ||
| defer MustClose(t, term) | ||
|
|
||
| MustStart(t, term) | ||
|
|
||
| // these are unreasonable repeat rates, but its somewhere to start | ||
| term.SetRepeat(time.Millisecond*50, time.Millisecond*25) | ||
|
|
||
| term.KeyPress(vt.KeyRight) | ||
| time.Sleep(100 * time.Millisecond) | ||
| term.KeyPress(vt.KeyRight) | ||
| term.KeyRelease(vt.KeyRight) | ||
| term.KeyRelease(vt.KeyRight) | ||
|
|
||
| CheckRead(t, term, "\x1b[C\x1b[C\x1b[C\x1b[C") // 0 ms, 50 ms, 75 ms, 100 ms | ||
| } |
There was a problem hiding this comment.
Minor: Duplicate KeyRelease call.
Line 352 calls term.KeyRelease(vt.KeyRight) twice in succession. While harmless (second release on an already-released key), this appears to be a copy-paste artifact.
🧹 Suggested fix
term.KeyPress(vt.KeyRight)
time.Sleep(100 * time.Millisecond)
term.KeyPress(vt.KeyRight)
term.KeyRelease(vt.KeyRight)
- term.KeyRelease(vt.KeyRight)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestKeyRepeatCursor(t *testing.T) { | |
| term := vt.NewMockTerm() | |
| defer MustClose(t, term) | |
| MustStart(t, term) | |
| // these are unreasonable repeat rates, but its somewhere to start | |
| term.SetRepeat(time.Millisecond*50, time.Millisecond*25) | |
| term.KeyPress(vt.KeyRight) | |
| time.Sleep(100 * time.Millisecond) | |
| term.KeyPress(vt.KeyRight) | |
| term.KeyRelease(vt.KeyRight) | |
| term.KeyRelease(vt.KeyRight) | |
| CheckRead(t, term, "\x1b[C\x1b[C\x1b[C\x1b[C") // 0 ms, 50 ms, 75 ms, 100 ms | |
| } | |
| func TestKeyRepeatCursor(t *testing.T) { | |
| term := vt.NewMockTerm() | |
| defer MustClose(t, term) | |
| MustStart(t, term) | |
| // these are unreasonable repeat rates, but its somewhere to start | |
| term.SetRepeat(time.Millisecond*50, time.Millisecond*25) | |
| term.KeyPress(vt.KeyRight) | |
| time.Sleep(100 * time.Millisecond) | |
| term.KeyPress(vt.KeyRight) | |
| term.KeyRelease(vt.KeyRight) | |
| CheckRead(t, term, "\x1b[C\x1b[C\x1b[C\x1b[C") // 0 ms, 50 ms, 75 ms, 100 ms | |
| } |
🤖 Prompt for AI Agents
In `@vt/tests/key_test.go` around lines 339 - 355, TestKeyRepeatCursor contains a
duplicate consecutive call to term.KeyRelease(vt.KeyRight); remove the extra
KeyRelease invocation so there is only a single KeyRelease(vt.KeyRight) where
the key should be released; locate this in the TestKeyRepeatCursor function and
delete the redundant term.KeyRelease(vt.KeyRight) call to clean up the test.
There was a problem hiding this comment.
It is, but its benign, and it actually has a benefit of testing a code path that was not previously tested (release without matching press), so I am inclined to leave it there for now.
There was a problem hiding this comment.
@gdamore, that makes perfect sense! Testing the release-without-matching-press code path is valuable, and I appreciate you clarifying the intent. I'll leave it as-is.
✏️ Learnings added
Learnt from: gdamore
Repo: gdamore/tcell PR: 1021
File: vt/tests/key_test.go:339-355
Timestamp: 2026-01-27T17:18:21.368Z
Learning: In vt/tests/key_test.go, the duplicate `term.KeyRelease(vt.KeyRight)` call in TestKeyRepeatCursor (line 352) is intentional to test the code path for releasing a key without a matching press, not a copy-paste error.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: gdamore
Repo: gdamore/tcell PR: 959
File: vt/emulate.go:0-0
Timestamp: 2025-12-31T22:01:49.486Z
Learning: In vt/emulate.go, the autoWrap (pending wrap) flag behavior is directional: backward/leftward cursor movements always clear autoWrap because they move away from the right column, while forward movements may or may not clear it depending on the specific action. CHT (cursor forward tab) intentionally does not clear autoWrap, aligning with HT (horizontal tab) behavior.
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.