Skip to content

fix(commands): ignore allowed shortcut conflicts#2272

Merged
arnestrickmann merged 2 commits into
mainfrom
jan/eng-1449-cmdw-closes-multiple-conversation-tabs
May 28, 2026
Merged

fix(commands): ignore allowed shortcut conflicts#2272
arnestrickmann merged 2 commits into
mainfrom
jan/eng-1449-cmdw-closes-multiple-conversation-tabs

Conversation

@janburzinski
Copy link
Copy Markdown
Collaborator

summary

  • fixes that cmd+w would get processed twice

demo of it working now:
https://streamable.com/fdx3s7

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 28, 2026

Greptile Summary

This PR fixes double-processing of cmd+w (and other conflictBehavior: 'allow' shortcuts) by filtering them out of the keys list that CommandShortcutBinder uses to mount SingleKeyBinder components. Previously, both AppKeyboardShortcuts and CommandShortcutBinder were registering handlers for these shortcuts, causing each keypress to be dispatched twice.

  • Adds a one-line filter in CommandShortcutBinder to exclude shortcuts whose APP_SHORTCUTS entry has conflictBehavior === 'allow', so no SingleKeyBinder is mounted for them — dispatch for those shortcuts is left entirely to the existing AppKeyboardShortcuts handler.
  • As a side effect, the isAllow branch inside SingleKeyBinder is now dead code, since allow shortcuts are never passed to it anymore.

Confidence Score: 4/5

The change is minimal and targeted — one filter line that prevents double-dispatch for allow shortcuts. Safe to merge with the dead-code cleanup as a nice-to-have follow-up.

The fix is correct and well-scoped. The only leftover is the now-unreachable isAllow logic in SingleKeyBinder, which doesn't affect behavior but drifts from the new invariant.

The isAllow branches in SingleKeyBinder (lines 20–31) are dead code after this change and could be tidied up.

Important Files Changed

Filename Overview
src/renderer/lib/commands/command-shortcut-binder.tsx Filters out conflictBehavior === 'allow' shortcuts from the keys list in CommandShortcutBinder, preventing double-dispatch for shortcuts like cmd+w. Leaves isAllow dead-code in SingleKeyBinder.

Sequence Diagram

sequenceDiagram
    participant User
    participant AppKeyboardShortcuts
    participant CommandShortcutBinder
    participant SingleKeyBinder
    participant commandRegistry

    Note over CommandShortcutBinder: PR fix: 'allow' shortcuts filtered out
    User->>AppKeyboardShortcuts: press cmd+w
    AppKeyboardShortcuts->>commandRegistry: dispatch(shortcutKey)

    Note over CommandShortcutBinder: Before fix: SingleKeyBinder was ALSO mounted for 'allow' shortcuts
    Note over CommandShortcutBinder: After fix: 'allow' shortcuts have no SingleKeyBinder

    User->>AppKeyboardShortcuts: press non-allow shortcut (e.g. cmd+k)
    AppKeyboardShortcuts->>SingleKeyBinder: hotkey fires
    SingleKeyBinder->>commandRegistry: dispatch(shortcutKey)
Loading

Comments Outside Diff (1)

  1. src/renderer/lib/commands/command-shortcut-binder.tsx, line 20-32 (link)

    P2 Dead code in SingleKeyBinder — since CommandShortcutBinder now filters out every conflictBehavior === 'allow' key before mounting SingleKeyBinder, isAllow will always be false at runtime. The e.preventDefault() branch and the conditional spread of { conflictBehavior: 'allow' } are unreachable. Consider removing this logic to keep the component in sync with the new invariant.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: src/renderer/lib/commands/command-shortcut-binder.tsx
    Line: 20-32
    
    Comment:
    Dead code in `SingleKeyBinder` — since `CommandShortcutBinder` now filters out every `conflictBehavior === 'allow'` key before mounting `SingleKeyBinder`, `isAllow` will always be `false` at runtime. The `e.preventDefault()` branch and the conditional spread of `{ conflictBehavior: 'allow' }` are unreachable. Consider removing this logic to keep the component in sync with the new invariant.
    
    
    
    How can I resolve this? If you propose a fix, please make it concise.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
src/renderer/lib/commands/command-shortcut-binder.tsx:20-32
Dead code in `SingleKeyBinder` — since `CommandShortcutBinder` now filters out every `conflictBehavior === 'allow'` key before mounting `SingleKeyBinder`, `isAllow` will always be `false` at runtime. The `e.preventDefault()` branch and the conditional spread of `{ conflictBehavior: 'allow' }` are unreachable. Consider removing this logic to keep the component in sync with the new invariant.

```suggestion
  useHotkey(
    getHotkeyRegistration(shortcutKey, keyboard),
    () => {
      commandRegistry.dispatch(shortcutKey);
    },
    {
      enabled: getEffectiveHotkey(shortcutKey, keyboard) !== null,
    }
  );
```

Reviews (1): Last reviewed commit: "fix(commands): ignore allowed shortcut c..." | Re-trigger Greptile

Copy link
Copy Markdown
Contributor

@arnestrickmann arnestrickmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for catching this and for the fix

@arnestrickmann arnestrickmann merged commit 6fbd8bd into main May 28, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants