Fix duplicate keybinding rule when replacing with an existing rule#3969
Fix duplicate keybinding rule when replacing with an existing rule#3969kridaydave wants to merge 1 commit into
Conversation
In upsertKeybindingRule, the replace path only dropped the entry matching replaceTarget before appending the new rule. If that new rule already existed elsewhere in the config, the result contained a duplicate binding that survived downstream (no dedup in compile/merge) and got persisted. The filter now also excludes any entry equal to the incoming rule, so it is appended exactly once. Added a regression test covering the case where the replacement rule already exists in the config.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
ApprovabilityVerdict: Approved Straightforward bug fix that prevents duplicate keybinding entries when replacing a rule with one that already exists. The change is minimal (one additional filter condition) with a clear test demonstrating the fix. You can customize Macroscope's approvability policy. Learn more. |
In upsertKeybindingRule, the replace path only dropped the entry matching replaceTarget before appending the new rule. If that new rule already existed elsewhere in the config, the result contained a duplicate binding that survived downstream and got persisted.
The filter now also excludes any entry equal to the incoming rule, so it is appended exactly once. Added a regression test covering the case where the replacement rule already exists in the config.
Note
Low Risk
Narrow change to custom keybinding upsert/replace persistence with a targeted regression test; no auth or security impact.
Overview
Fixes duplicate keybinding entries when
upsertKeybindingRuleruns with areplacetarget and the new rule already exists elsewhere in custom config.The pre-append filter used to drop only the
replaceentry, so an identical incoming rule could remain and get appended again and persisted. It now also removes any entry equal to the incoming rule, so the binding is written once.Adds a regression test: replace
mod+rwithmod+alt+rwhenmod+alt+ris already present leaves a singlemod+alt+rrule.Reviewed by Cursor Bugbot for commit 6c419c6. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix duplicate keybinding rule when upserting with an existing replace target
When calling
upsertKeybindingRulewith a replace target, the new rule could be duplicated if it already existed elsewhere in the custom config. The fix extends the filter in keybindings.ts to remove entries matching either the replace target or the new rule before appending, ensuring only one instance appears in the persisted config.Macroscope summarized 6c419c6.