feat(oxlint-plugin): add `client-prefer-keybind-library` rule by devin-ai-integration[bot] · Pull Request #475 · millionco/react-doctor

devin-ai-integration · 2026-05-25T08:17:32Z

Summary

Adds client-prefer-keybind-library, a new draft rule that flags manual addEventListener("keydown" | "keyup" | "keypress", …) calls on window/document or inside useEffect/useLayoutEffect, recommending a keybind library like react-hotkeys-hook instead for consistent, declarative, and accessible keyboard shortcut management.

Category: Architecture (not Performance — the benefits are focus scoping, modifier normalization, and cleanup automation)

Detection scope:

window.addEventListener("keydown", …) / document.addEventListener("keydown", …) anywhere
Any .addEventListener("keydown", …) inside useEffect or useLayoutEffect callbacks
All three keyboard event types: keydown, keyup, keypress

Does NOT flag:

Non-keyboard events (click, scroll, resize, mousedown, etc.)
Element-scoped listeners outside effects (e.g. ref.current.addEventListener)
Dynamic event names (variables, template literals)
Calls with fewer than 2 arguments
Test/spec/story files (via test-noise tag)

Flags unconditionally — even if a keybind library is already imported in the file. Prefers false positives over false negatives.

30 tests covering positive cases, false positive avoidance, real-world open-source patterns (Cmd+K search, Escape-to-close, arrow nav, focus trap), and test file skipping.

Review & Testing Checklist for Human

Verify the rule fires correctly on a real codebase with manual keyboard listeners (e.g., run react-doctor against a project that uses window.addEventListener("keydown", …))
Confirm the rule does NOT fire on non-keyboard events in a real codebase
Check the rule message is clear and actionable for developers unfamiliar with keybind libraries

Notes

Constants added to dom.ts: KEYBOARD_EVENT_NAMES
KEYBIND_LIBRARY_PACKAGES was removed per review feedback — the rule flags all manual implementations unconditionally

Link to Devin session: https://app.devin.ai/sessions/0b43ca64311c427394bd367288cc28cb
Requested by: @NisargIO

Note

Low Risk
Adds a new warn-only lint rule and constants with no runtime or security impact; main risk is extra warnings on codebases that intentionally use manual keyboard listeners.

Overview
Adds a new Architecture lint rule client-prefer-keybind-library that warns when apps register manual keyboard shortcuts via addEventListener with literal keydown, keyup, or keypress on window/document (anywhere) or on any target inside useEffect / useLayoutEffect. Diagnostics recommend a library such as react-hotkeys-hook and still fire even if a keybind package is already imported.

Shared DOM constants gain KEYBOARD_EVENT_NAMES. The rule is wired into the generated rule-registry as a global warn-level entry and ships with a large test suite (including real-world patterns and test-noise skipping for test/spec/story files).

^{Reviewed by Cursor Bugbot for commit ed5221b. Bugbot is set up for automated code reviews on this repo. Configure here.}

Flags manual addEventListener('keydown'|'keyup'|'keypress', …) calls on window/document or inside useEffect/useLayoutEffect callbacks, and recommends using a keybind library (react-hotkeys-hook, tinykeys, mousetrap, etc.) instead. The rule: - Detects global keyboard event listeners (window/document) - Detects keyboard listeners inside React effect hooks - Skips files that already import from a known keybind library - Skips non-keyboard events (click, scroll, resize, etc.) - Skips element-scoped listeners outside effects - Tagged test-noise to auto-skip test/story/spec files Includes 35 tests covering: - Positive cases (all keyboard event types, global/document/effect) - False positive avoidance (non-keyboard events, existing imports) - Open-source repo patterns (Cmd+K search, Escape-to-close, arrow nav) - Keybind library detection (react-hotkeys-hook, tinykeys, mousetrap, etc.) - Test file skipping Co-Authored-By: nisarg@million.dev <awesomenisarg@gmail.com>

devin-ai-integration · 2026-05-25T08:17:42Z

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

Disable automatic comment and CI monitoring

Co-Authored-By: nisarg@million.dev <awesomenisarg@gmail.com>

devin-ai-integration

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 3 additional findings.

devin-ai-integration · 2026-05-27T06:04:45Z

+export const clientPreferKeybindLibrary = defineRule<Rule>({
+  id: "client-prefer-keybind-library",
+  tags: ["test-noise"],
+  severity: "warn",
+  recommendation:
+    'Use a keybind library like react-hotkeys-hook (`useHotkeys("mod+k", handler)`) instead of manual addEventListener("keydown", …) — it handles focus scoping, modifier normalization, and cleanup automatically',


🚩 Rule categorized as "Performance" but has no performance implications

The client bucket defaults to category: "Performance" (see codegen script at packages/oxlint-plugin-react-doctor/scripts/generate-rule-registry.mjs:69). The existing client-localstorage-no-version rule overrides this to "Correctness" in its defineRule call (client-localstorage-no-version.ts:36) because localStorage versioning is about data correctness, not performance. This new rule similarly doesn't fit "Performance" — its recommendation message explicitly lists benefits as focus scoping, modifier normalization, and cleanup automation, which are correctness/architecture concerns. Adding category: "Correctness" or category: "Architecture" to the defineRule call would make the categorization accurate and follow the precedent set by the other client rule.

Was this helpful? React with 👍 or 👎 to provide feedback.

should be in architecture imo, feel free to push back if you think otherwise but i don't find it inherently in performance

Good call — changed the category to "Architecture" in the defineRule call. The codegen now emits category: "Architecture" for this rule.

Already addressed in ed5221b — category is now "Architecture".

devin-ai-integration · 2026-05-27T06:06:05Z

+export const KEYBIND_LIBRARY_PACKAGES: ReadonlyArray<string> = [
+  "react-hotkeys-hook",
+  "react-hotkeys",
+  "@mantine/hooks",
+  "tinykeys",
+  "hotkeys-js",
+  "mousetrap",
+  "@react-hook/hotkey",
+  "use-hotkeys",
+  "@invoke-ai/ui-library",
+];


📝 Info: Broad package names in KEYBIND_LIBRARY_PACKAGES may cause per-file false negatives

@mantine/hooks and @invoke-ai/ui-library are general-purpose packages that happen to include keybind functionality. If a file imports any hook/component from these packages (e.g., import { useDebouncedValue } from '@mantine/hooks'), the rule suppresses all keyboard listener warnings for that file, even if the import is unrelated to keybinds. This is a per-file check (not per-project), so the impact is limited to files that both import from one of these broad packages AND have manual keyboard listeners. The design choice is defensible — the user already has the keybind tool available — but it does mean some real cases will be silently skipped.

Was this helpful? React with 👍 or 👎 to provide feedback.

this will produce edge cases we should detect. imo this list isn''t even needed if the purpose is to tell people not use manual keyboard listneerr,

just flag all manual impls, better to have false positives than false negatives

Agreed — removed KEYBIND_LIBRARY_PACKAGES entirely and the library-detection skip. The rule now flags all manual keyboard listener implementations unconditionally. Better to have false positives than false negatives here.

Already addressed in ed5221b — KEYBIND_LIBRARY_PACKAGES and the library-detection skip were removed entirely. The rule now flags all manual keyboard listeners unconditionally, so this false-negative scenario no longer applies.

rayhanadev

see comments

rayhanadev · 2026-05-27T06:05:16Z

+export const clientPreferKeybindLibrary = defineRule<Rule>({
+  id: "client-prefer-keybind-library",
+  tags: ["test-noise"],
+  severity: "warn",
+  recommendation:
+    'Use a keybind library like react-hotkeys-hook (`useHotkeys("mod+k", handler)`) instead of manual addEventListener("keydown", …) — it handles focus scoping, modifier normalization, and cleanup automatically',


should be in architecture imo, feel free to push back if you think otherwise but i don't find it inherently in performance

rayhanadev · 2026-05-27T06:09:19Z

+export const KEYBIND_LIBRARY_PACKAGES: ReadonlyArray<string> = [
+  "react-hotkeys-hook",
+  "react-hotkeys",
+  "@mantine/hooks",
+  "tinykeys",
+  "hotkeys-js",
+  "mousetrap",
+  "@react-hook/hotkey",
+  "use-hotkeys",
+  "@invoke-ai/ui-library",
+];


this will produce edge cases we should detect. imo this list isn''t even needed if the purpose is to tell people not use manual keyboard listneerr,

rayhanadev · 2026-05-27T06:09:46Z

+export const KEYBIND_LIBRARY_PACKAGES: ReadonlyArray<string> = [
+  "react-hotkeys-hook",
+  "react-hotkeys",
+  "@mantine/hooks",
+  "tinykeys",
+  "hotkeys-js",
+  "mousetrap",
+  "@react-hook/hotkey",
+  "use-hotkeys",
+  "@invoke-ai/ui-library",
+];


just flag all manual impls, better to have false positives than false negatives

- Change category from Performance to Architecture per reviewer feedback - Remove KEYBIND_LIBRARY_PACKAGES constant and library-detection logic — flag all manual keyboard listeners unconditionally to prefer false positives over false negatives - Update tests accordingly (30 tests, all pass) Co-Authored-By: nisarg@million.dev <awesomenisarg@gmail.com>

rayhanadev · 2026-05-27T06:16:26Z

bugbot run

cursor

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit ed5221b. Configure here.}

cursor · 2026-05-27T06:21:45Z

+      (current.callee.name === "useEffect" || current.callee.name === "useLayoutEffect")
+    ) {
+      return true;
+    }


Effect detection misses React.useEffect(...) member expression calls

Medium Severity

isInsideUseEffectCallback only matches callees that are Identifier nodes (e.g. useEffect(…)), so it silently misses React.useEffect(…) or any namespaced call where the callee is a MemberExpression. The codebase already provides getCalleeName (which extracts the method name from both patterns) and the EFFECT_HOOK_NAMES constant — other rules like nextjs-no-client-fetch-for-server-data use isHookCall(node, EFFECT_HOOK_NAMES) for exactly this purpose. This creates a false negative that contradicts the PR's stated preference for false positives over false negatives.

^{Reviewed by Cursor Bugbot for commit ed5221b. Configure here.}

style: apply formatter to rule file

5a9c8ae

Co-Authored-By: nisarg@million.dev <awesomenisarg@gmail.com>

NisargIO requested a review from aidenybai May 25, 2026 08:22

NisargIO marked this pull request as ready for review May 25, 2026 08:22

devin-ai-integration Bot commented May 27, 2026

View reviewed changes

rayhanadev self-requested a review May 27, 2026 06:10

rayhanadev requested changes May 27, 2026

View reviewed changes

cursor Bot reviewed May 27, 2026

View reviewed changes

Conversation

devin-ai-integration Bot commented May 25, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Review & Testing Checklist for Human

Notes

Uh oh!

devin-ai-integration Bot commented May 25, 2026

🤖 Devin AI Engineer

Uh oh!

devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

✅ Devin Review: No Issues Found

Uh oh!

devin-ai-integration Bot May 27, 2026

Choose a reason for hiding this comment

Uh oh!

rayhanadev May 27, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot May 27, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot May 27, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot May 27, 2026

Choose a reason for hiding this comment

Uh oh!

rayhanadev May 27, 2026

Choose a reason for hiding this comment

Uh oh!

rayhanadev May 27, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot May 27, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration Bot May 27, 2026

Choose a reason for hiding this comment

Uh oh!

rayhanadev left a comment

Choose a reason for hiding this comment

Uh oh!

rayhanadev May 27, 2026

Choose a reason for hiding this comment

Uh oh!

rayhanadev May 27, 2026

Choose a reason for hiding this comment

Uh oh!

rayhanadev May 27, 2026

Choose a reason for hiding this comment

Uh oh!

rayhanadev commented May 27, 2026

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

cursor Bot May 27, 2026

Choose a reason for hiding this comment

Effect detection misses React.useEffect(...) member expression calls

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

devin-ai-integration Bot commented May 25, 2026 •

edited by cursor Bot

Loading

Effect detection misses `React.useEffect(...)` member expression calls