refactor(hotkeys): migrate Hotkeys from Flow to TypeScript - #4769
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 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
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to Hotkey components now compile from TypeScript while preserving the existing runtime behavior; no current merge-readiness risk is identified. 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
🚥 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. I tap my paws on keys so bright, 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}
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
e41851e to
d33096e
Compare
9c3b40c
d33096e to
9c3b40c
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Merge Queue Status
This pull request spent 16 minutes 10 seconds in the queue, including 15 minutes 46 seconds running CI. Required conditions to merge
|
3 participants
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