fix(react): make disabled a registration gate and warn on duplicate s… - #9
Merged
Merged
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Multiple moderate issues remain in duplicate detection, warning behavior, lifecycle cleanup, hook tracking, and the sample gate.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR gates React shortcut registration when disabled and adds duplicate-binding warnings across React and core.
Changes:
- Disabled hooks skip registration and scope management.
- Added duplicate-registration tracking and warnings.
- Updated tests, documentation, examples, changelogs, and package versions.
File summaries
| File | Summary |
|---|---|
tests/sample-react/src/App.jsx |
Demonstrates duplicate-instance behavior. |
packages/react/src/useShortcuts.ts |
Implements disabled gating and duplicate tracking. |
packages/react/src/Keybindy.test.tsx |
Updates registration tests. |
packages/react/src/disabled-gating.test.tsx |
Adds gating and warning coverage. |
packages/react/README.md |
Documents registration gating. |
packages/react/package.json |
Bumps the React package version. |
packages/react/CHANGELOG.md |
Records React changes. |
packages/core/src/ShortcutManager.ts |
Adds core duplicate warnings. |
packages/core/src/ShortcutManager.test.ts |
Tests duplicate registration behavior. |
packages/core/package.json |
Bumps the core package version. |
packages/core/CHANGELOG.md |
Records core changes. |
Review details
Suppressed comments (4)
packages/core/src/ShortcutManager.ts:544
- This warning is inside the alias-expansion loop, so one logical duplicate such as
Enter(which expands to Enter and Numpad Enter) emits multiple console warnings. Deduplicate at the logical registration level or emit once after collecting the replaced combinations.
console.warn(
packages/core/src/ShortcutManager.ts:541
warnedDuplicatesis only cleared indestroy(), whileunregister()removes the shortcut without clearing this signature. After a duplicate is unmounted, a later registration of the same scope/key pair will overwrite again but never warn, so the warning does not re-arm with the active registrations; clear or recompute this state when no matching binding remains.
const duplicateSignature = `${targetScope}::${JSON.stringify(normalized)}`;
if (
replaced.length > 0 &&
!this.warnedDuplicates.has(duplicateSignature) &&
!isProductionBuild
packages/react/README.md:172
- This example calls
onSelectbelow, butonSelectis not destructured from the component props, so the copied snippet fails to type-check (and is undefined at runtime). Include it in the parameter list.
function MediaPicker({ isOpen, onClose }) {
packages/react/src/useShortcuts.ts:43
- This hook-level diagnostic runs immediately before
manager.register, whose new core duplicate diagnostic also callsconsole.warn. A duplicateuseShortcutsregistration therefore emits both messages (and alias expansions can emit more), contradicting the sample's “one” warning; keep the diagnostic in one layer or coordinate suppression.
if (count > 1 && !duplicateWarnings.has(signature) && !isProductionBuild) {
duplicateWarnings.add(signature);
console.warn(
`[Keybindy] Duplicate shortcut detected: ${JSON.stringify(keys)} is registered by ${count} component instances in scope "${scope}". Only the most recent instance will fire. ` +
`If these are parallel instances of the same component (dialogs, pickers), pass { disabled: !isOpen } so only the active instance registers.`
- Files reviewed: 11/11 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+25
to
+29
| const isProductionBuild = (() => { | ||
| try { | ||
| return process.env.NODE_ENV === 'production'; | ||
| } catch { | ||
| return false; |
Comment on lines
+532
to
+535
| JSON.stringify(s.keys) === JSON.stringify(normalized) && | ||
| (s.options?.scope || 'global') === targetScope && | ||
| s.id !== id | ||
| ); |
Comment on lines
+19
to
+23
| const isProductionBuild = (() => { | ||
| try { | ||
| return process.env.NODE_ENV === 'production'; | ||
| } catch { | ||
| return false; |
| ); | ||
| } | ||
|
|
||
| const stopTracking = stableShortcuts.map(({ keys }) => trackDuplicate(scope, keys)); |
| }, | ||
| ], | ||
| { | ||
| // disabled: !isOpen, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…hortcuts