Repository navigation
fix(input): decode CSI R as F3 - #1133
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis change adds CSI ChangesCSI Key Mapping
Estimated code review effort: 1 (Trivial) | ~2 minutes Possibly related PRs
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.
🧹 Nitpick comments (1)
input.go (1)
225-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding an explicit
CSI-Rtest case.Test snippets show
CSI-P,CSI-Q,CSI-Scases mirroringcsiAllKeysentries for F1/F2/F4, but no bareCSI-Rcase for F3 exists alongside the new mapping (only SS3-F3 and Linux-F3 are covered).✅ Suggested test addition (input_test.go)
{"CSI-P", []byte{'\x1b', '[', 'P'}, KeyF1, ModNone, ""}, {"CSI-Q", []byte{'\x1b', '[', 'Q'}, KeyF2, ModNone, ""}, + {"CSI-R", []byte{'\x1b', '[', 'R'}, KeyF3, ModNone, ""}, {"CSI-S", []byte{'\x1b', '[', 'S'}, KeyF4, ModNone, ""},🤖 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 `@input.go` at line 225, Add an explicit CSI-R test case to cover the new F3 mapping in the key input tests. The current test table in input_test.go mirrors csiAllKeys for F1/F2/F4 via CSI-P, CSI-Q, and CSI-S, but lacks a bare CSI-R entry for KeyF3; update the relevant test fixture and assertions so CSI-R is validated alongside the existing SS3-F3 and Linux-F3 coverage.
🤖 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.
Nitpick comments:
In `@input.go`:
- Line 225: Add an explicit CSI-R test case to cover the new F3 mapping in the
key input tests. The current test table in input_test.go mirrors csiAllKeys for
F1/F2/F4 via CSI-P, CSI-Q, and CSI-S, but lacks a bare CSI-R entry for KeyF3;
update the relevant test fixture and assertions so CSI-R is validated alongside
the existing SS3-F3 and Linux-F3 coverage.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1133 +/- ##
==========================================
- Coverage 89.36% 89.34% -0.02%
==========================================
Files 47 47
Lines 5718 5718
==========================================
- Hits 5110 5109 -1
- Misses 460 461 +1
Partials 148 148 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
gdamore
left a comment
gdamore
left a comment
There was a problem hiding this comment.
I thought I reviewed and merged this change (or one just like it) recently.
Some terminals emit F3 as CSI R instead of SS3 R or CSI 13~. The parser already recognizes neighboring CSI P/Q/S forms for F1, F2, and F4, but omitted R, causing F3 to be swallowed. Only the no-parameter form is mapped, so cursor position reports like CSI row;col R should remain unaffected. Signed-off-by: Kimmo Lehto <kimmo.lehto@gmail.com>
Some terminals emit F3 as CSI R instead of SS3 R or CSI 13~.
The parser already recognizes neighboring CSI P/Q/S forms for F1, F2, and F4, but omitted R, causing F3 to be swallowed.
Only the no-parameter form is mapped, so cursor position reports like
CSI row;col Rshould remain unaffected.Tested to enable F3 using iTerm2 to connect to an Ubuntu 24.04 machine over Mosh+SSH+tmux without any obvious side-effects.
Summary by CodeRabbit