WSL port 7/7: bind the quit chord that WSL actually sends - #14
Conversation
|
@coderabbitai review |
|
📝 WalkthroughWalkthroughThe change adds a WSL-only ChangesWSL Ctrl+Shift+Q fallback
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant WindowsTerminal
participant Neovim
participant quit_drill
participant confirm
WindowsTerminal->>Neovim: Send Ctrl+Shift+Q as legacy <C-q>
Neovim->>quit_drill: Invoke mapping in insert or terminal mode
quit_drill->>confirm: Show quit prompt
confirm-->>Neovim: Return quit decision
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
afecef8 to
f3b9045
Compare
46506ab to
ebf2585
Compare
|
@greptileai review |
f3b9045 to
42f6b9f
Compare
ebf2585 to
b1450e2
Compare
42f6b9f to
d6b6c13
Compare
b1450e2 to
1805547
Compare
d6b6c13 to
094501b
Compare
1805547 to
4f8520c
Compare
094501b to
bd300ab
Compare
4f8520c to
43ced03
Compare
bd300ab to
02a6dd1
Compare
43ced03 to
eb12af7
Compare
02a6dd1 to
eebcd48
Compare
eb12af7 to
9fa6e01
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
nvimrc.lua (2)
1274-1280: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winChange
INSERT ONLYto match the terminal-mode mapping.The mapping includes terminal mode
t. Change the heading toINSERT AND TERMINAL ONLYso the comment matches the implementation.Proposed fix
--- INSERT ONLY, and gated on WSL, because both halves cost something: +-- INSERT AND TERMINAL ONLY, and gated on WSL, because both halves cost something:🤖 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 `@nvimrc.lua` around lines 1274 - 1280, Update the comment heading above the Ctrl+Q mapping from “INSERT ONLY” to “INSERT AND TERMINAL ONLY” so it accurately reflects the terminal-mode mapping.
1289-1291: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd PTY coverage for raw legacy 0x11.
The current PTY cases only send
CSQ = "\x1b[113;6u", so they do not cover the WSL<C-q>fallback. Add insert and terminal-mode cases that send raw\x11after disablingixon, and assert the prompt plus Cancel/Quit behavior.🤖 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 `@nvimrc.lua` around lines 1289 - 1291, Add PTY coverage alongside the existing CSQ cases for the WSL mapping in the <C-q> path: in both insert and terminal modes, disable ixon, send raw 0x11, and assert the expected prompt followed by the appropriate Cancel/Quit behavior.
🤖 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.
Inline comments:
In `@docs/wsl.md`:
- Around line 93-109: Update the WSL behavior documentation around the
nvimrc.lua keybinding description to include terminal mode alongside insert and
normal modes, and revise the consequences text so bare Ctrl+Q is documented as
quitting from both insert and terminal modes. Preserve the existing CSI-u and
non-WSL behavior details.
In `@TODO.md`:
- Around line 78-87: Rename the socket-check output and related gate wording
around the Ctrl+Shift+Q case to describe Neovim key-notation mapping or prompt
behavior, not CSI-u delivery. Keep the check unconditional and preserve the
existing test behavior; only remove the misleading “CSI-u” terminology from the
relevant labels.
---
Nitpick comments:
In `@nvimrc.lua`:
- Around line 1274-1280: Update the comment heading above the Ctrl+Q mapping
from “INSERT ONLY” to “INSERT AND TERMINAL ONLY” so it accurately reflects the
terminal-mode mapping.
- Around line 1289-1291: Add PTY coverage alongside the existing CSQ cases for
the WSL mapping in the <C-q> path: in both insert and terminal modes, disable
ixon, send raw 0x11, and assert the expected prompt followed by the appropriate
Cancel/Quit behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c4dd2dc-145b-47e5-a62a-ea4a9c8b6528
📒 Files selected for processing (4)
KEYS.mdTODO.mddocs/wsl.mdnvimrc.lua
ee56816 to
de2192d
Compare
0bcddf8 to
1296659
Compare
de2192d to
d3cf20c
Compare
1296659 to
d3f3b42
Compare
d3cf20c to
716a400
Compare
716a400 to
e1a9c63
Compare
d3f3b42 to
e1a9c63
Compare
Ctrl+Shift+Q did nothing on WSL. Nothing automated could have found it: suite_quit.sh writes ESC[113;6u straight onto the pty, so it passes on a terminal where the chord could never arrive, and demo.sh --check cannot send CSI-u over --remote-send at all. It took pressing the key. Windows Terminal 1.24 is new enough to speak CSI-u and does not negotiate it with nvim. So the chord arrives as the legacy 0x11 -- plain <C-q> -- and the <C-S-q> mapping is never reached. The documented way out of the editor did not exist on WSL, leaving `:qa!`: the one thing that whole section was written to stop being necessary. So bind what actually arrives. This is not a second key to learn -- you press Ctrl+Shift+Q, exactly as documented, and <C-q> is what nvim is handed. Insert only, and gated on WSL, because both halves cost something. Normal mode keeps Ctrl+Q as visual block: it is the only way to ask for one, and drill's premise is that you are in insert anyway. In insert, vanilla <C-q> is literal-insert, the twin of <C-v> -- in a Python scratchpad where <C-v> is already paste that is close to unreachable, and a quit prompt defaulting to Cancel is the better use of the key. But it is a real vim behaviour, so nowhere but WSL loses it. Verified both branches by loading the config with the detection forced each way. On WSL insert <C-q> is quit_drill. Forced non-WSL -- what a Mac sees -- insert <C-q> is unmapped, normal <C-q> is <C-V>, insert <C-v> is still paste and <C-S-q> is still quit: byte-for-byte the old behaviour. The WSL probe is hoisted to one IS_WSL near the top, since the clipboard fallback asks the same question. Suite: 556/556, ALL SUITES PASSED. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WveBqAyhoM49p9gWfwBDBx
Review, and correct. <C-S-q> is bound in n/i/t precisely so you can quit from inside the interpreter without pressing <C-e> first -- KEYS.md advertises it -- and a fallback covering only insert silently drops that on the one platform that needs the fallback at all. Normal mode stays out: there <C-q> is visual block. python has no use for <C-q>, and drill.sh's `stty -ixon` is what frees it from XON to begin with. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WveBqAyhoM49p9gWfwBDBx
Review on the base PR: the gates named one condition each while the checkpoints named several, so "gate passed" did not mean the checkpoint was done. Spell them out -- including the one doing the least work: "CI green" says nothing about whether xvfb and xclip made the clipboard cases real or whether they were silently skipped, so require that evidence from the log instead. Landed at the tip of the stack rather than on the base branch: TODO.md is edited by every checkpoint, so the same change down there collides with all five checkbox ticks above it for no benefit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WveBqAyhoM49p9gWfwBDBx
Review: the checkpoint 4 entry still carried the socket/CSI-u explanation that commit "Retract the socket-mode quit skip" explicitly withdrew, so the final diff documented behaviour the final code does not have -- and documented a false cause for it. Rewritten to say what was actually wrong and what the measurement really showed, because "I got this wrong and here is why" is worth more in a checklist than a tidy sentence that is false. Checkpoint 3 was stale the same way: it credited `ps -o comm=`, which was replaced by the source marker after that check turned out to accept `python3 train.py --tag drill-timer`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WveBqAyhoM49p9gWfwBDBx
Codex review of this branch, three findings, and all three are the docs claiming more than the code does. The P1 is real and I am fixing it in the documentation rather than the binding, because the binding is the lesser evil. On a terminal with no CSI-u, Ctrl+Shift+Q arrives as 0x11 from every mode; the fallback covers insert and terminal but not normal, so from normal mode you get a visual block and no prompt -- verified on a pty, mode goes n -> \x16 and the screen says VISUAL BLOCK. KEYS.md guaranteed normal mode anyway. Binding normal-mode <C-q> to the prompt would fix that and cost more than it buys: <C-q> is the ONLY route to a visual block, because <C-v> in normal mode is paste. Quitting from normal mode still has :qa!; a block would have nothing. So the trade stands and the docs now say so plainly, including what to press instead. Second: docs/wsl.md said a WSL session that does speak CSI-u is unaffected. It is not -- the gate is IS_WSL, not the protocol, because there is no runtime signal for CSI-u (vim.g.termfeatures is nil even in a real TUI on 0.12). Such a session gets the insert binding too, and loses literal-insert with it. Said outright instead of implied away. Third: the page still carried the "--remote-send collapses <C-S-q>" claim that the checkpoint-4 commit in this same stack retracted. Corrected, and the retraction is named so the next reader does not have to rediscover it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WveBqAyhoM49p9gWfwBDBx
Review: this entry still said demo.sh --check "cannot send CSI-u over --remote-send at all", which is the claim the checkpoint-4 commit in this same stack retracted -- and it contradicted docs/wsl.md two files away. Fourth place that retraction had to be chased down, after the code, the workflow docs and this file's own checkpoint 4. It also said "you press the documented chord and it works" without qualifying it. That is true in insert and terminal mode and false in normal mode, where <C-q> is visual block and stays that way on purpose -- it is the only route to a block, while quitting still has :qa!. Recorded, with what to press instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WveBqAyhoM49p9gWfwBDBx
Review: the WSL page listed insert and normal mode and left out terminal,
though nvimrc.lua binds <C-q> in {i,t} and KEYS.md advertises quitting from
inside the REPL. Quitting without pressing Ctrl+E first is the reason terminal
mode is in that mode list at all, so leaving it out of the page undersold the
fix. The "two consequences" line had the same omission.
TODO.md also quoted the old "(CSI-u)" check label, which the previous commit
renamed for overstating what --remote-send can prove.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WveBqAyhoM49p9gWfwBDBx
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Two review findings on the last branch in the stack. The fallback had no coverage at all: the 556-case gate never launched the config with WSL detection either way, so binding <C-q> in insert and terminal could have regressed silently. Two assertions now check it from whichever side the machine is on -- a WSL box proves the bindings exist, and CI on ubuntu-latest, where /proc/version has no "microsoft", proves they do not leak onto a normal Linux desktop. Neither machine can prove both halves; between the two runs both are covered. And the comment claimed "python has no use for <C-q>". It does: readline in emacs mode binds ^Q to quoted-insert, so the rebind takes a live feature away. It stays -- ^V is bound to the same command in readline, so quoted-insert survives -- but it is a fair trade, not a free one, and the comment said free. suite_options 86/86, up from 84. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WveBqAyhoM49p9gWfwBDBx
There was a problem hiding this comment.
No application code in the PR — skipped Code Health checks.
See analysis details in CodeScene
Quality Gate Profile: Clean Code Collective
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
Stacked on #12. Found by pressing the key — which is exactly what checkpoint 5
said would be needed, because nothing automated can reach this.
What was wrong
Ctrl+Shift+Qdid nothing on WSL.Windows Terminal 1.24 is new enough to speak CSI-u and does not negotiate it
with nvim. So the chord arrives as the legacy
0x11— plain<C-q>— and the<C-S-q>mapping is never reached. The documented way out of the editor did notexist on WSL, leaving
:qa!— the one thing that whole section of the configwas written to stop being necessary.
Why no test caught it
suite_quit.shwritesESC[113;6ustraight onto the pty, so it passes ona terminal where the chord could never arrive.
demo.sh --checkcannot send CSI-u over--remote-sendat all (checkpoint 4).vim.g.termfeaturesisnileven in a real TUI on nvim 0.12, so there is noruntime signal to detect CSI-u from Lua either.
This one genuinely required a human to press it.
The fix
Bind what actually arrives. This is not a second key to learn — you press
Ctrl+Shift+Q, exactly as documented, and<C-q>is what nvim is handed.Insert only, and gated on WSL, because both halves cost something:
Ctrl+Qas visual block — it is the only way to ask forone, and drill's premise is that you are in insert anyway
<C-q>is literal-insert, the twin of<C-v>. In aPython scratchpad where
<C-v>is already paste that is close to unreachable,and a quit prompt defaulting to Cancel is the better use of the key. But it is
a real vim behaviour, so nowhere but WSL loses it.
Verification — both branches, forced each way
On WSL:
Forced non-WSL — what a Mac sees:
Byte-for-byte the old behaviour off WSL.
Suite: 556/556, ALL SUITES PASSED. Every
<C-q>case in the suite isnormal-mode (
suite_options.shasserts it maps to<C-V>;suite_comment.sh'svisual-block case is
--start normal), so none of them touch the new binding.The WSL probe is hoisted to one
IS_WSLnear the top, since the clipboardfallback asks the same question.
🤖 Generated with Claude Code
https://claude.ai/code/session_01WveBqAyhoM49p9gWfwBDBx
Summary by cubic
Make the quit chord work on WSL by binding what Windows Terminal actually sends. On WSL, insert- and terminal‑mode
<C-q>opens the quit prompt; normal‑mode<C-q>stays visual block. Off WSL, behavior is unchanged.Bug Fixes
<C-q>to the quit prompt because Windows Terminal 1.24 doesn’t negotiate CSI‑u withnvimand sends 0x11; lets you quit from inside the REPL while keeping normal‑mode<C-q>for visual block.IS_WSLgate (env, not protocol), the normal‑mode gap on non‑CSI‑u terminals, terminal‑mode quit, the insert‑mode literal‑insert trade‑off, and retract the earlier--remote-sendcollapse claim.Refactors
IS_WSLprobe and applied the fallback in insert and terminal modes.suite_options.shchecks to ensure the<C-q>fallback binds only on WSL (i/t) and does not leak to Linux CI.readlinebinds^Qto quoted‑insert; the fallback trades it for quit, with^Vstill providing quoted‑insert.Written for commit 69b5867. Summary will update on new commits.
Summary by CodeRabbit
Ctrl+Shift+QasCtrl+Q.Ctrl+Qretains visual-block selection.Ctrl+Shift+Qbehavior.Greptile Summary
The PR makes the documented quit chord work when Windows Terminal sends its legacy
<C-q>encoding under WSL.<C-q>fallback in insert and terminal modes while preserving normal-mode visual block.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains; the current WSL-gated mapping includes both insert and terminal modes, so the previously reported missing REPL fallback is fixed.
Important Files Changed
<C-q>.Reviews (25): Last reviewed commit: "Test the WSL quit fallback, and stop cal..." | Re-trigger Greptile