refactor(hotkeys): migrate Hotkeys from Flow to TypeScript - #4769
Open
bonchevskyi wants to merge 1 commit into
Open
refactor(hotkeys): migrate Hotkeys from Flow to TypeScript#4769bonchevskyi wants to merge 1 commit into
bonchevskyi wants to merge 1 commit into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe PR adds a layered hotkey system with Mousetrap integration, React context, registration components, hotkey-aware modal and overlay wrappers, a localized help modal, TypeScript and Flow modules, public exports, and updated tests. ChangesHotkey service foundation
React hotkey integration
Localized hotkey help
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HotkeyLayer
participant HotkeyService
participant Hotkeys
participant HotkeyContext
participant HotkeyHelpModal
participant Modal
HotkeyLayer->>HotkeyService: create and register layer
HotkeyLayer->>HotkeyContext: provide service
Hotkeys->>HotkeyContext: read service
Hotkeys->>HotkeyService: register configured hotkeys
HotkeyHelpModal->>HotkeyContext: read active hotkeys and types
HotkeyHelpModal->>Modal: render localized categories and bindings
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/components/hotkeys/HotkeyLayer.js.flow (1)
1-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value**Inconsistent
//@flowpragma across the new `.js.flow` stubs.** `HotkeyFriendlyModal.js.flow` starts with `// `@flow, but two other new stubs do not. Use one convention for all stubs added in this PR.
src/components/hotkeys/HotkeyLayer.js.flow#L1-L11: add//@flow`` as the first line, or remove it fromHotkeyFriendlyModal.js.flow.src/components/hotkeys/Hotkeys.js.flow#L1-L64: apply the same convention to this stub.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/hotkeys/HotkeyLayer.js.flow` around lines 1 - 11, Use a consistent Flow pragma convention across the new stubs: add `// `@flow`` as the first line of `src/components/hotkeys/HotkeyLayer.js.flow` and `src/components/hotkeys/Hotkeys.js.flow`, matching `HotkeyFriendlyModal.js.flow`.src/components/hotkeys/__tests__/Hotkeys.test.tsx (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNarrow the mock casts instead of disabling the rule for the whole file.
A file-wide
no-explicit-anydisable also hides futureanyusage in this test. The provider value acceptsHotkeyService | null, so a double cast keeps the mock typed at the use site.♻️ Proposed change
-/* eslint-disable `@typescript-eslint/no-explicit-any` */- <HotkeyContext.Provider value={mockHotkeyLayer as any}> + <HotkeyContext.Provider value={mockHotkeyLayer as unknown as HotkeyService}>
wrapper.instance() as anyon Line 107 still needs a cast, because Enzyme returns the baseComponenttype. Cast it to the component type instead:wrapper.instance() as Hotkeys.Also applies to: 27-27
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/hotkeys/__tests__/Hotkeys.test.tsx` at line 1, Remove the file-wide no-explicit-any suppression and narrow the mock casts at the provider value use sites by casting through unknown to HotkeyService | null. Update each wrapper.instance() cast in the Hotkeys tests to the Hotkeys component type instead of any, preserving the existing test behavior.src/components/hotkeys/HotkeyFriendlyModal.tsx (1)
18-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse event-aware callback types.
Modalpasses an event to both callbacks. TypeonBackdropClickas(event: React.MouseEvent<HTMLDivElement>) => voidandonRequestCloseas(event: React.SyntheticEvent) => void. ReplaceObjectwithReact.CSSProperties.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/hotkeys/HotkeyFriendlyModal.tsx` around lines 18 - 28, Update the HotkeyFriendlyModal props: type onBackdropClick as (event: React.MouseEvent<HTMLDivElement>) => void, onRequestClose as (event: React.SyntheticEvent) => void, and replace the style backdrop/dialog Object types with React.CSSProperties.src/components/hotkeys/Hotkeys.ts (1)
18-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeclare the context field as type-only.
The current
es5target does not emit this uninitialized field. Usedeclareto preserve that intent if the compiler target changes. React 18 typings do not infer the instance context type fromstatic contextType.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/hotkeys/Hotkeys.ts` around lines 18 - 20, Update the context field in the Hotkeys class to use a type-only declaration with the existing HotkeyService | null type, while preserving static contextType = HotkeyContext so React receives the runtime context.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/hotkeys/HotkeyHelpModal.tsx`:
- Around line 87-97: Update the reopen logic in both
src/components/hotkeys/HotkeyHelpModal.tsx (lines 87-97) and
src/components/hotkeys/HotkeyHelpModal.js.flow (lines 68-78): after refreshing
hotkeys and types in the isOpen transition, retain currentType only when it
exists in the refreshed types, otherwise select the first type or null. Add a
test covering active types changing while the modal is closed and reopening.
In `@src/components/hotkeys/HotkeyLayer.tsx`:
- Around line 21-24: Update the exported HotkeyLayerProps interface so
enableHelpModal and helpModalShortcut are optional, matching the existing
defaultProps and non-required PropTypes contract; leave their types and default
behavior unchanged.
- Around line 9-10: Restore the import/no-cycle suppression for the Hotkeys and
HotkeyHelpModal imports in HotkeyLayer.tsx, preserving the existing lint
configuration style and suppressing only the reported TypeScript hotkey cycle.
---
Nitpick comments:
In `@src/components/hotkeys/__tests__/Hotkeys.test.tsx`:
- Line 1: Remove the file-wide no-explicit-any suppression and narrow the mock
casts at the provider value use sites by casting through unknown to
HotkeyService | null. Update each wrapper.instance() cast in the Hotkeys tests
to the Hotkeys component type instead of any, preserving the existing test
behavior.
In `@src/components/hotkeys/HotkeyFriendlyModal.tsx`:
- Around line 18-28: Update the HotkeyFriendlyModal props: type onBackdropClick
as (event: React.MouseEvent<HTMLDivElement>) => void, onRequestClose as (event:
React.SyntheticEvent) => void, and replace the style backdrop/dialog Object
types with React.CSSProperties.
In `@src/components/hotkeys/HotkeyLayer.js.flow`:
- Around line 1-11: Use a consistent Flow pragma convention across the new
stubs: add `// `@flow`` as the first line of
`src/components/hotkeys/HotkeyLayer.js.flow` and
`src/components/hotkeys/Hotkeys.js.flow`, matching
`HotkeyFriendlyModal.js.flow`.
In `@src/components/hotkeys/Hotkeys.ts`:
- Around line 18-20: Update the context field in the Hotkeys class to use a
type-only declaration with the existing HotkeyService | null type, while
preserving static contextType = HotkeyContext so React receives the runtime
context.
🪄 Autofix
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 Plus
Run ID: 08265f37-0e24-470d-a8ce-b3343767c347
⛔ Files ignored due to path filters (2)
src/components/hotkeys/__tests__/__snapshots__/HotkeyFriendlyOverlay.test.tsx.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (33)
package.jsonsrc/components/hotkeys/HotkeyContext.js.flowsrc/components/hotkeys/HotkeyContext.tssrc/components/hotkeys/HotkeyFriendlyModal.js.flowsrc/components/hotkeys/HotkeyFriendlyModal.tsxsrc/components/hotkeys/HotkeyFriendlyOverlay.js.flowsrc/components/hotkeys/HotkeyFriendlyOverlay.tsxsrc/components/hotkeys/HotkeyHelpModal.js.flowsrc/components/hotkeys/HotkeyHelpModal.tsxsrc/components/hotkeys/HotkeyLayer.js.flowsrc/components/hotkeys/HotkeyLayer.tsxsrc/components/hotkeys/HotkeyManager.js.flowsrc/components/hotkeys/HotkeyManager.tssrc/components/hotkeys/HotkeyRecord.js.flowsrc/components/hotkeys/HotkeyRecord.tssrc/components/hotkeys/HotkeyService.js.flowsrc/components/hotkeys/HotkeyService.tssrc/components/hotkeys/Hotkeys.js.flowsrc/components/hotkeys/Hotkeys.stories.tsxsrc/components/hotkeys/Hotkeys.tssrc/components/hotkeys/__tests__/HotkeyFriendlyModal.test.tsxsrc/components/hotkeys/__tests__/HotkeyFriendlyOverlay.test.tsxsrc/components/hotkeys/__tests__/HotkeyHelpModal.test.tsxsrc/components/hotkeys/__tests__/HotkeyLayer.test.tsxsrc/components/hotkeys/__tests__/HotkeyManager.test.tssrc/components/hotkeys/__tests__/HotkeyService.test.tssrc/components/hotkeys/__tests__/HotkeyTestWrapper.jssrc/components/hotkeys/__tests__/HotkeyTestWrapper.tsxsrc/components/hotkeys/__tests__/Hotkeys.test.tsxsrc/components/hotkeys/index.js.flowsrc/components/hotkeys/index.tssrc/components/hotkeys/messages.js.flowsrc/components/hotkeys/messages.ts
💤 Files with no reviewable changes (1)
- src/components/hotkeys/tests/HotkeyTestWrapper.js
bonchevskyi
force-pushed
the
refactor/flow-to-ts-hotkeys
branch
from
August 11, 2026 13:22
d563b32 to
e41851e
Compare
bonchevskyi
force-pushed
the
refactor/flow-to-ts-hotkeys
branch
from
August 11, 2026 14:15
e41851e to
d33096e
Compare
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
1 participant
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.
Convert Hotkeys component to TypeScript
This PR converts
src/components/hotkeysfrom JavaScript with Flow to TypeScript.Changes
.ts/.tsxwith exported props interfaces (HotkeysProps,HotkeyLayerProps,HotkeyHelpModalProps,HotkeyFriendlyModalProps,HotkeyFriendlyOverlayProps) andHotkeyConfigindex.jstoindex.ts, re-exporting components and types (runtime exports unchanged; no new parent barrel export)Hotkeys.stories.jstoHotkeys.stories.tsx__tests__/*.test.jsto.test.ts(x).js.flowstubs for backward compatibility@types/mousetrapfor TypeScript supportContract
HotkeyFriendlyModalProps: Flow declared onlychildren+isOpen; TS also includesclassName,onRequestClose, andtitleused by in-repo callers and forwarded via...rest(previously allowed by Flow inexact objects)Testing
src/components/hotkeys; all 50 pass with snapshots matching previous outputyarn lint:tsandflow checkpassComponents/Hotkeys) that behavior is unchangedSummary by CodeRabbit
Summary by CodeRabbit