Fix keybinding dispatch for immediate/local commands#639
Fix keybinding dispatch for immediate/local commands#639VitalyOstanin wants to merge 4 commits intoPiebald-AI:mainfrom
Conversation
Fix Claude Code bug: command: keybindings for local-jsx commands (e.g. command:copy) went through the model instead of executing instantly. Root cause: queryGuard.isActive gate blocked the fast path at idle. Patch removes this check for fromKeybinding.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new patch module Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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🧪 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)
src/patches/keybindingImmediateCommands.ts (1)
22-23: Regex should start with a literal delimiter for performance.The pattern starts with an identifier capture group
([$\w]+)=...rather than a literal character like,,;,}, or{. Per the learnings, this can significantly impact matching performance in V8 (e.g., 1.5s vs 30ms for large files).Consider anchoring the pattern with a preceding delimiter:
♻️ Proposed fix
const pattern = - /([$\w]+)=([$\w]+)\.isActive&&\(([$\w]+)\?\.immediate\|\|([$\w]+)\?\.fromKeybinding\)/; + /([,;{}])([$\w]+)=([$\w]+)\.isActive&&\(([$\w]+)\?\.immediate\|\|([$\w]+)\?\.fromKeybinding\)/;Then update the destructuring and replacement to preserve the delimiter:
- const [fullMatch, resultVar, , commandVar, optionsVar] = match; + const [fullMatch, delimiter, resultVar, , commandVar, optionsVar] = match; // ... - const replacement = `${resultVar}=${commandVar}?.immediate||${optionsVar}?.fromKeybinding`; + const replacement = `${delimiter}${resultVar}=${commandVar}?.immediate||${optionsVar}?.fromKeybinding`;Based on learnings: "Use
,;}{literal characters at regex start instead of\\bfor performance in patch patterns"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patches/keybindingImmediateCommands.ts` around lines 22 - 23, The current regex stored in the const pattern (pattern variable) begins with a capture group for an identifier which harms V8 performance; change the regex to start with a literal delimiter (e.g., one of ",", ";", "{", "}") before the identifier and update any capture group indices accordingly, then modify the destructuring and string replacement that use pattern (where you extract the identifier and immediate/fromKeybinding parts) so they preserve and re-insert that leading delimiter in the replacement; ensure the variable name pattern and its usages (the regex literal and the replacement logic) are updated consistently so the preserved delimiter is included in the rebuilt string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/patches/keybindingImmediateCommands.ts`:
- Around line 22-23: The current regex stored in the const pattern (pattern
variable) begins with a capture group for an identifier which harms V8
performance; change the regex to start with a literal delimiter (e.g., one of
",", ";", "{", "}") before the identifier and update any capture group indices
accordingly, then modify the destructuring and string replacement that use
pattern (where you extract the identifier and immediate/fromKeybinding parts) so
they preserve and re-insert that leading delimiter in the replacement; ensure
the variable name pattern and its usages (the regex literal and the replacement
logic) are updated consistently so the preserved delimiter is included in the
rebuilt string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 80da1736-9eb3-41c2-8496-a2da1939582d
📒 Files selected for processing (2)
src/patches/index.tssrc/patches/keybindingImmediateCommands.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/patches/keybindingImmediateCommands.ts (1)
1-15: Remove the new header comment block in this TS patch file.This adds non-requested comments in a logic file.
As per coding guidelines,
**/*.{js,jsx,ts,tsx}: "Do not add comments
unless explicitly requested".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/patches/keybindingImmediateCommands.ts` around lines 1 - 15, Remove the new header comment block at the top of keybindingImmediateCommands.ts (the multi-line comment starting with "// Please see the note about writing patches..." and the following explanatory lines about fixing keybinding dispatch), leaving only the functional code and any existing file-level comments that were present before; ensure the file contains no added non-requested header comments per the repository rule for *.{js,jsx,ts,tsx}.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/patches/keybindingImmediateCommands.ts`:
- Around line 25-40: The current flow returns null before verifying whether the
matched location is already patched and also checks patchedPattern against the
whole oldFile (causing false positives); fix by first performing the original
regex match (pattern) and if no match return null, then inspect the matched
substring (use match[0] / fullMatch) for the patchedPattern (or test
patchedPattern against fullMatch) and if that specific match is already patched
(patchedPattern.test(fullMatch) and ensure it includes '.isActive' where
expected) return oldFile; update the logic around match, fullMatch,
patchedPattern and alreadyPatched to perform the localized check instead of
scanning oldFile.
---
Nitpick comments:
In `@src/patches/keybindingImmediateCommands.ts`:
- Around line 1-15: Remove the new header comment block at the top of
keybindingImmediateCommands.ts (the multi-line comment starting with "// Please
see the note about writing patches..." and the following explanatory lines about
fixing keybinding dispatch), leaving only the functional code and any existing
file-level comments that were present before; ensure the file contains no added
non-requested header comments per the repository rule for *.{js,jsx,ts,tsx}.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 11b1abde-1798-4b6d-932b-1c59f6dd680e
📒 Files selected for processing (1)
src/patches/keybindingImmediateCommands.ts
| const match = oldFile.match(pattern); | ||
| if (!match || match.index === undefined) { | ||
| debug( | ||
| 'patch: keybindingImmediateCommands: failed to find queryGuard.isActive && immediate/fromKeybinding pattern' | ||
| ); | ||
| return null; | ||
| } | ||
|
|
||
| const [fullMatch, delimiter, resultVar, , commandVar, optionsVar] = match; | ||
|
|
||
| const patchedPattern = | ||
| /[,;{}][$\w]+=[$\w]+\?\.immediate\|\|[$\w]+\?\.fromKeybinding/; | ||
| const alreadyPatched = oldFile.match(patchedPattern); | ||
| if (alreadyPatched && !alreadyPatched[0].includes('.isActive')) { | ||
| return oldFile; | ||
| } |
There was a problem hiding this comment.
Fix already-patched detection flow to avoid false failures/no-ops.
At Line 26, null is returned before checking patched form, so an
already-fixed file is marked as failed. Also, Lines 35-40 can
short-circuit on an unrelated occurrence elsewhere in oldFile.
Proposed fix
const match = oldFile.match(pattern);
- if (!match || match.index === undefined) {
- debug(
- 'patch: keybindingImmediateCommands: failed to find queryGuard.isActive && immediate/fromKeybinding pattern'
- );
- return null;
- }
-
- const [fullMatch, delimiter, resultVar, , commandVar, optionsVar] = match;
-
- const patchedPattern =
- /[,;{}][$\w]+=[$\w]+\?\.immediate\|\|[$\w]+\?\.fromKeybinding/;
- const alreadyPatched = oldFile.match(patchedPattern);
- if (alreadyPatched && !alreadyPatched[0].includes('.isActive')) {
- return oldFile;
- }
+ const alreadyPatchedPattern =
+ /[,;{}][$\w]+=[$\w]+\?\.immediate\|\|[$\w]+\?\.fromKeybinding/;
+ if (!match || match.index === undefined) {
+ if (alreadyPatchedPattern.test(oldFile)) {
+ return oldFile;
+ }
+ debug(
+ 'patch: keybindingImmediateCommands: failed to find queryGuard.isActive && immediate/fromKeybinding pattern'
+ );
+ return null;
+ }
+
+ const [fullMatch, delimiter, resultVar, , commandVar, optionsVar] = match;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/patches/keybindingImmediateCommands.ts` around lines 25 - 40, The current
flow returns null before verifying whether the matched location is already
patched and also checks patchedPattern against the whole oldFile (causing false
positives); fix by first performing the original regex match (pattern) and if no
match return null, then inspect the matched substring (use match[0] / fullMatch)
for the patchedPattern (or test patchedPattern against fullMatch) and if that
specific match is already patched (patchedPattern.test(fullMatch) and ensure it
includes '.isActive' where expected) return oldFile; update the logic around
match, fullMatch, patchedPattern and alreadyPatched to perform the localized
check instead of scanning oldFile.
There was a problem hiding this comment.
@coderabbitai Fixed. The already-patched detection now runs before returning null:
const match = oldFile.match(pattern);
if (!match || match.index === undefined) {
if (alreadyPatchedPattern.test(oldFile)) {
return oldFile; // already patched — return unchanged
}
debug("...");
return null; // genuinely missing — report failure
}This avoids both the false-failure (marking an already-patched file as failed) and the false-positive (matching an unrelated occurrence) issues you identified.
There was a problem hiding this comment.
@VitalyOstanin, thanks for addressing this! The fix looks correct — both concerns are now resolved:
- False-failure ✅ — The already-patched check now runs inside the
!matchbranch before returningnull, so a previously patched file correctly returnsoldFileinstead of being treated as a failure. - False-positive ✅ — Because
alreadyPatchedPattern.test(oldFile)only executes when the primarypattern(which includes.isActive&&) fails to match, any hit onalreadyPatchedPatternin that branch unambiguously identifies the already-patched form. The whole-file scan is safe in this conditional context.
This matches the intent of my proposed fix. Well done!
🧠 Learnings used
Learnt from: CR
Repo: Piebald-AI/tweakcc PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-06T21:30:11.641Z
Learning: Applies to **/patches/**/*.{ts,tsx} : Use `,` `;` `}` `{` literal characters at regex start instead of `\\b` for performance in patch patterns
Learnt from: CR
Repo: Piebald-AI/tweakcc PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-06T21:30:11.641Z
Learning: Applies to **/patches/**/*.{ts,tsx} : Avoid `\\b` in regex patterns due to V8 performance issues; use literal character alternatives
Learnt from: CR
Repo: Piebald-AI/tweakcc PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-06T21:30:11.641Z
Learning: Applies to **/patches/**/*.{ts,tsx} : Use `[\$\\w]+` instead of `\\w+` for identifier matching in regex patterns to include `$` for React refs
Learnt from: ljepson
Repo: Piebald-AI/tweakcc PR: 437
File: src/patches/toolsets.ts:811-833
Timestamp: 2026-01-29T21:19:04.770Z
Learning: In src/patches/toolsets.ts, when matching mode change patterns like `let X=Y(Z,{type:"setMode",mode:vv(TA),destination:"session"});`, the regex should capture the inner variable name (TA) not the entire expression (vv(TA)), because the variable holds the raw mode string value (e.g., "plan", "default", "acceptEdits") and is used in direct string comparisons like `if(${modeVar}==="plan")`. The wrapping function vv() is only for dispatch transformation.
Learnt from: bl-ue
Repo: Piebald-AI/tweakcc PR: 419
File: src/patches/thinkingVerbs.ts:35-35
Timestamp: 2026-01-27T18:56:52.902Z
Learning: In src/patches/thinkingVerbs.ts, the regex pattern for matching thinking verbs arrays uses `{50,}` quantifier to require at least 50 items, making it specific enough without needing test coverage since it only matches capitalized verb lists with that many entries.
Learnt from: bl-ue
Repo: Piebald-AI/tweakcc PR: 421
File: src/patches/thinkerSymbolSpeed.ts:46-58
Timestamp: 2026-01-27T21:16:01.318Z
Learning: In patches under src/patches/, assume code works with minified, original Claude Code installations. Patch regexes must be strict and match the minified format exactly (ignore formatting differences like whitespace/newlines). This guidance applies to all .ts files in src/patches and should be followed when adding or reviewing patches.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai All review comments have been addressed in commits
Could you please re-review the latest changes? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 32780
One very minor observation (no action needed): The implementation is solid. Happy to approve. 🐇 [approve] 🧠 Learnings used |
…atches The patch definition and implementation were at the bottom of their respective arrays despite having group: PatchGroup.ALWAYS_APPLIED. Relocate both to sit with the other always-applied entries for consistency.
|
@coderabbitai Addressed the minor observation in |
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings used |
|
Closing: the bug is fixed upstream starting from Claude Code 2.1.88, so this patch is no longer needed. |
1 participant
Summary
command:copy, etc.) executing instantly instead of falling through to the full query pipeline ("Sprouting..." delay)queryGuard.isActive, which is onlytrueduring an active model query — at idle, the command falls through to the slow pathqueryGuard.isActiveguard soimmediateandfromKeybindingcommands always take the fast pathChanges
src/patches/keybindingImmediateCommands.ts— new patch implementationsrc/patches/index.ts— register the patch inALWAYS_APPLIEDgroupSummary by CodeRabbit
Bug Fixes
Chores