Add keyboard shortcuts to Motor Direction wizard #4641
Conversation
WalkthroughAdds a keyboard shortcuts UI block to the ESC DShot Direction wizard, implements keyboard handling (Space for spin/stop, keys 1–8 to toggle motor direction) in EscDshotDirectionComponent, adds tooltip styling, and calls component.open() before showing the dialog. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant M as motors.js
participant C as EscDshotDirectionComponent
participant D as Dialog
U->>M: Click "ESC DShot direction"
M->>C: open()
note right of C #e6f2ff: Enable global keyboard handling
M->>D: showModal()
D-->>U: Dialog visible
sequenceDiagram
autonumber
actor K as User (Keyboard)
participant C as EscDshotDirectionComponent
participant W as Wizard State
participant MD as Motor Driver
rect rgb(240,248,255)
note over C,W: Wizard active
K->>C: KeyDown: Space
C->>C: _handleGlobalKeyDown / _handleWizardKeyDown
C->>MD: Spin motors
end
K-->>C: KeyDown: 1..8
C->>W: _toggleMotorDirection(index)
W-->>C: direction state updated
K->>C: KeyUp: Space
C->>MD: Stop motors
note over C: Dialog close or wizard stop
C->>MD: Ensure stop, deactivate driver
C->>C: Disable keyboard handlers
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/EscDshotDirection/EscDshotDirectionComponent.js (1)
463-486: Carefully ordered cleanup sequence with good explanatory comments.The close() method correctly sequences cleanup operations to ensure pending motor direction commands complete before the component fully resets. The comments clearly explain the reasoning behind each step.
Consider adding an idempotency guard at the start of the method to prevent issues if
close()is called multiple times rapidly (e.g., by both ESC key handler and close button).Apply this diff to add an idempotency guard:
close() { + // Guard against multiple close calls + if (!this._globalKeyboardActive && !this._keyboardEventHandlerBound) { + return; + } + // Disable keyboard handlers first to prevent any new input this._disableKeyboardControl(); this._disableGlobalKeyboard();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/EscDshotDirection/Body.html(1 hunks)src/components/EscDshotDirection/EscDshotDirectionComponent.js(3 hunks)src/css/tabs/motors.less(1 hunks)src/js/tabs/motors.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/EscDshotDirection/EscDshotDirectionComponent.js (1)
test/tabs/cli.test.js (2)
event(30-30)event(37-37)
🔇 Additional comments (8)
src/css/tabs/motors.less (1)
195-237: LGTM! Clean CSS implementation for the keyboard shortcuts tooltip.The styling is well-structured with consistent use of CSS variables for theming, proper flexbox layout, and appropriate visual hierarchy. The kbd element styling clearly distinguishes keyboard keys from regular text.
src/js/tabs/motors.js (1)
1356-1356: Good addition! Ensures proper initialization order.Calling
escDshotDirectionComponent.open()beforeshowModal()ensures the global keyboard handler is enabled when the dialog becomes visible, which is necessary for the spacebar workflow to function correctly.src/components/EscDshotDirection/Body.html (1)
30-36: LGTM! Clear and well-structured keyboard shortcuts tooltip.The HTML structure is semantic and the placement within the wizard spinning section is appropriate. The tooltip provides clear, concise instructions for keyboard shortcuts with proper use of
<kbd>elements.src/components/EscDshotDirection/EscDshotDirectionComponent.js (5)
29-38: LGTM! Proper event handler binding and state initialization.The constructor correctly initializes keyboard state flags and binds event handlers to preserve the correct
thiscontext. The comment on line 35 correctly emphasizes the critical nature of binding for event handlers.
298-359: Solid implementation of the progressive spacebar workflow.The global keyboard handler correctly implements the four-step progression: checkbox → start wizard → spin motors → wizard control. The visibility check (lines 318-324) ensures keyboard input is only processed when the dialog is actually visible, preventing unexpected behavior.
The special handling on line 352 (
this._spacebarPressed = true) correctly marks the spacebar as pressed during the transition to wizard control, ensuring consistent state when the wizard handler takes over.
390-456: Well-implemented wizard keyboard controls with proper safeguards.The keyboard handlers correctly implement spacebar hold-to-spin and number key direction toggling. Good use of:
- State flags to gate behavior (
_isKeyboardControlEnabled&&_isWizardActive)!event.repeatto prevent repeated triggers when holding keys- Bounds checking for motor indices (line 414)
The
_toggleMotorDirectionmethod correctly synchronizes both UI state (button class) and motor driver state.
458-461: LGTM! Clean initialization hook.The
open()method provides a clean initialization point for enabling global keyboard handling when the dialog is opened. This is correctly called frommotors.jsbeforeshowModal().
545-574: Proper keyboard lifecycle integration with wizard flow.The keyboard control is correctly enabled when the wizard starts spinning (lines 562-563) and disabled when it stops (lines 573-574). The
_isWizardActiveflag properly gates wizard keyboard functionality and ensures the handlers only process input during the appropriate phase.
- Progressive spacebar workflow: Check checkbox → Start wizard → Spin motors - Hold spacebar to spin motors, release to stop - Number keys 1-8 toggle individual motor directions - Added visual tooltip showing keyboard shortcuts - Properly scoped keyboard handlers to dialog visibility - Fixed settings persistence when closing dialog via any method Improves user workflow by reducing mouse interaction and enabling faster motor direction configuration.
protocol113
force-pushed
the
feature/keyboard-shortcuts-motor-wizard
branch
from
e842a51 to
494c252
Compare
|
|
Preview URL: https://pr4641.betaflight-app.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/EscDshotDirection/Body.html(1 hunks)src/components/EscDshotDirection/EscDshotDirectionComponent.js(3 hunks)src/css/tabs/motors.less(1 hunks)src/js/tabs/motors.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/js/tabs/motors.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/EscDshotDirection/EscDshotDirectionComponent.js (1)
test/tabs/cli.test.js (2)
event(30-30)event(37-37)
🔇 Additional comments (11)
src/components/EscDshotDirection/Body.html (1)
30-36: LGTM! Clean UI addition for keyboard shortcuts.The tooltip is well-structured and appropriately placed within the spinning wizard section. The use of
<kbd>tags for keyboard keys follows HTML best practices.src/components/EscDshotDirection/EscDshotDirectionComponent.js (9)
29-39: LGTM! Proper method binding for event handlers.All keyboard event handlers are correctly bound in the constructor, including the
_handleWindowBlurfix from the previous review. This ensures properthiscontext preservation.
299-360: LGTM! Well-structured progressive workflow.The global keyboard handler implements a clean state machine:
- Check safety checkbox if unchecked
- Start wizard if checked
- Spin motors if wizard ready
- Hand off to wizard keyboard handler when active
The dialog visibility checks prevent input when dialog is hidden, and the spacebar state management at line 353 correctly handles the transition case.
362-384: LGTM! Proper event listener lifecycle management.The keyboard control enable/disable methods correctly:
- Use capture phase (
true) for reliable event handling- Clean up all listeners including the blur handler (fix from previous review)
- Reset spacebar state on disable
386-431: LGTM! Robust wizard keyboard handling.The wizard keyboard handlers properly:
- Guard with state checks (
_isKeyboardControlEnabled,_isWizardActive)- Prevent default and stop propagation
- Use
event.repeatchecks to prevent key-hold spamming- Handle spacebar press/release cycle correctly
- Support number keys 1-8 for motor direction toggles with bounds checking
433-447: LGTM! Clean separation of concerns.The helper methods provide clear, single-responsibility functions for spacebar actions and window blur safety. The blur handler correctly stops motors if spacebar is held when the window loses focus.
449-460: LGTM! Motor direction toggle implementation.The
_toggleMotorDirectionmethod correctly mirrors the button click behavior from_wizardMotorButtonClick(lines 272-285), maintaining consistency between mouse and keyboard interactions.
462-465: LGTM! Dialog initialization hook.The
open()method provides a clean entry point for enabling global keyboard handling when the dialog opens.
549-568: LGTM! Wizard start enables keyboard control.The
_onSpinWizardButtonClickedmethod correctly:
- Resets motor button states
- Sets all motors to normal direction
- Shows spinning wizard UI
- Spins all motors
- Activates wizard motor buttons
- Enables keyboard control and sets wizard active flag (lines 566-567)
570-579: LGTM! Wizard stop cleanly disables keyboard control.The
_onStopWizardButtonClickedmethod correctly disables keyboard control and clears the wizard active flag, ensuring keyboard handlers stop processing input.src/css/tabs/motors.less (1)
196-237: LGTM! Well-structured CSS for keyboard shortcuts tooltip.The styling:
- Uses CSS variables consistently with the codebase theme system
- Implements proper visual hierarchy (border accent, contrasting background)
- Includes responsive flex wrapping
- Provides appropriate spacing and visual polish (box-shadow on
kbdelements)- Follows LESS nesting conventions used throughout the file
| close() { | ||
| // Disable keyboard handlers first to prevent any new input | ||
| this._disableKeyboardControl(); | ||
| this._disableGlobalKeyboard(); | ||
|
|
||
| // If wizard is active, deactivate buttons but DON'T clear the flag yet | ||
| // This ensures pending motor direction commands complete | ||
| if (this._isWizardActive) { | ||
| this._deactivateWizardMotorButtons(); | ||
| } | ||
|
|
||
| // Stop motors (this adds stop commands to the queue) | ||
| this._motorDriver.stopAllMotorsNow(); | ||
|
|
||
| // Deactivate motor driver - this tells queue to stop AFTER processing current commands | ||
| // This is critical - it allows direction change + save commands to complete | ||
| this._motorDriver.deactivate(); | ||
|
|
||
| // Clear wizard flag after motor driver deactivation | ||
| this._isWizardActive = false; | ||
|
|
||
| // Reset GUI last | ||
| this._resetGui(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify cleanup sequence ensures motor commands complete.
The cleanup sequence is carefully ordered:
- Disable keyboard handlers (lines 469-470)
- Deactivate wizard buttons (line 475)
- Stop motors (line 479)
- Deactivate motor driver (line 483)
- Clear wizard flag (line 486)
- Reset GUI (line 489)
This ordering allows pending direction change commands to complete before shutdown. However, the effectiveness depends on _motorDriver.deactivate() and stopAllMotorsNow() implementation details.
Verify that the motor driver's command queue processes all pending direction changes before fully stopping:
🏁 Script executed:
#!/bin/bash
# Check motor driver implementation for queue processing during deactivation
# Find the motor driver file
motor_driver_file=$(fd -t f "EscDshotDirectionMotorDriver" --extension js)
if [ -n "$motor_driver_file" ]; then
echo "=== Motor Driver deactivate() method ==="
ast-grep --pattern $'deactivate() {
$$$
}'
echo ""
echo "=== Motor Driver stopAllMotorsNow() method ==="
ast-grep --pattern $'stopAllMotorsNow() {
$$$
}'
else
echo "Motor driver file not found"
fiLength of output: 88
🏁 Script executed:
#!/bin/bash
# Locate motor driver file(s)
echo "=== Files matching 'MotorDriver' ==="
fd -i MotorDriver -e js
# Inspect implementations of stopAllMotorsNow() and deactivate()
echo -e "\n=== stopAllMotorsNow() definitions ==="
rg -n --type js 'stopAllMotorsNow\s*\(' -C3
echo -e "\n=== deactivate() definitions ==="
rg -n --type js 'deactivate\s*\(' -C3
echo -e "\n=== Class definitions for MotorDriver ==="
rg -n --type js 'class\s+.*MotorDriver' -C3Length of output: 5202
🏁 Script executed:
#!/bin/bash
# Show the first 200 lines of the motor driver file to locate queue initialization
sed -n '1,200p' src/components/EscDshotDirection/EscDshotDirectionMotorDriver.jsLength of output: 4560
Critical: stopAllMotorsNow() drops pending commands
In EscDshotDirectionMotorDriver.js (lines 48–51), stopAllMotorsNow() calls this._EscDshotCommandQueue.clear(), which discards any queued direction-change commands before issuing the stop. If you need to let those commands complete, remove or defer the clear() call (or replace it with stopWhenEmpty()) so pending operations aren’t lost.
🤖 Prompt for AI Agents
In src/components/EscDshotDirection/EscDshotDirectionComponent.js lines 467-490:
the current sequence calls this._motorDriver.stopAllMotorsNow(), but
stopAllMotorsNow() in EscDshotDirectionMotorDriver.js clears the command queue
and drops pending direction-change commands; change the integration so we do NOT
call a method that clears the queue while we still need queued commands to
complete. Fix by either (A) removing the clear() call inside stopAllMotorsNow()
and instead implement a stopWhenEmpty() behavior in the motor driver that waits
for the queue to drain then issues stops, or (B) add an optional parameter to
stopAllMotorsNow({force:false}) and call stopWhenEmpty() /
stopAllMotorsNow({force:false}) here. Ensure the motorDriver.deactivate()
behavior still prevents new commands but allows queued commands to finish, and
update tests/comments accordingly.
Improves user workflow by reducing mouse interaction and enabling faster motor direction configuration.
Summary by CodeRabbit
New Features
Style
Bug Fixes