Add keyboard shortcuts to Motor Direction wizard #4641
Conversation
WalkthroughAdds keyboard-driven controls and UI hints to the ESC DShot direction wizard: Space starts/stops spinning, keys 1–8 toggle motor directions; introduces keyboard lifecycle and safety (window blur); adds a keyboard shortcuts tooltip; exposes Changes
Sequence DiagramsequenceDiagram
actor User
participant Dialog as EscDshotDirection Dialog
participant Component as EscDshotDirection Component
participant Motors as Motor Driver
User->>Dialog: Open modal
Dialog->>Component: open()
Component->>Component: _enableGlobalKeyboard()
Note right of Component: Wizard active -> capture-phase handlers enabled
User->>Component: Press Space
Component->>Component: _handleWizardKeyDown (Space)
Component->>Motors: Spin all motors
User->>Component: Press key 1-8
Component->>Component: _toggleMotorDirection()
Component->>Motors: Toggle selected motor direction
User->>Component: Release Space
Component->>Component: _handleWizardKeyUp (Space)
Component->>Motors: Stop all motors
User->>Dialog: Close modal
Dialog->>Component: close()
Component->>Component: _disableKeyboardControl()
Component->>Component: _disableGlobalKeyboard()
Component->>Motors: Stop motors and deactivate
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-09-19T20:42:20.332ZApplied to files:
📚 Learning: 2025-09-19T20:41:44.286ZApplied to files:
🧬 Code graph analysis (1)src/components/EscDshotDirection/EscDshotDirectionComponent.js (1)
🔇 Additional comments (10)
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 |
![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
|
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
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/EscDshotDirection/EscDshotDirectionComponent.js (2)
377-384: Consider consistent cleanup in_disableKeyboardControl().Line 383 sets
_spacebarPressed = falseduring cleanup, but unlike_handleWindowBlur()(lines 442-448), it does not call_handleSpacebarRelease()to stop the motors. While this is safe becauseclose()explicitly callsstopAllMotorsNow()at line 480, the inconsistency makes the cleanup path less obvious.For clarity and maintainability, consider adding the same safety pattern from
_handleWindowBlur:_disableKeyboardControl() { document.removeEventListener("keydown", this._handleWizardKeyDown, true); document.removeEventListener("keyup", this._handleWizardKeyUp, true); window.removeEventListener("blur", this._handleWindowBlur); + + // Stop motors if spacebar was held during cleanup + if (this._spacebarPressed) { + this._spacebarPressed = false; + this._handleSpacebarRelease(); + } + this._keyboardEventHandlerBound = false; this._isKeyboardControlEnabled = false; - this._spacebarPressed = false; }This ensures motors are stopped via a consistent code path regardless of how keyboard control is disabled.
Note: This suggestion was previously discussed in past review comments and remains relevant for code clarity.
450-461: Consider extracting shared direction-toggle logic.The motor direction toggle logic in
_toggleMotorDirection(lines 450-461) duplicates logic from_wizardMotorButtonClick(lines 272-285). Both methods:
- Check if the button has the
PUSHED_BUTTON_CLASS- Toggle the class
- Call
setEscSpinDirectionwith the appropriate directionExtract the shared logic to reduce duplication:
_toggleMotorDirection(motorIndex) { const button = this._wizardMotorButtons[motorIndex]; this._setMotorDirectionFromButton(button, motorIndex); } _wizardMotorButtonClick(button, motorIndex) { this._deactivateWizardMotorButtons(); this._setMotorDirectionFromButton(button, motorIndex); this._activateWizardMotorButtons(this._buttonTimeoutMs); } _setMotorDirectionFromButton(button, motorIndex) { const currentlyDown = button.hasClass(EscDshotDirectionComponent.PUSHED_BUTTON_CLASS); if (currentlyDown) { button.removeClass(EscDshotDirectionComponent.PUSHED_BUTTON_CLASS); this._motorDriver.setEscSpinDirection(motorIndex, DshotCommand.dshotCommands_e.DSHOT_CMD_SPIN_DIRECTION_1); } else { button.addClass(EscDshotDirectionComponent.PUSHED_BUTTON_CLASS); this._motorDriver.setEscSpinDirection(motorIndex, DshotCommand.dshotCommands_e.DSHOT_CMD_SPIN_DIRECTION_2); } }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/EscDshotDirection/EscDshotDirectionComponent.js(3 hunks)src/components/EscDshotDirection/EscDshotDirectionMotorDriver.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 (6)
src/components/EscDshotDirection/EscDshotDirectionComponent.js (5)
29-39: LGTM: Constructor keyboard state initialization.The keyboard control state flags and handler bindings are properly initialized. Binding methods in the constructor ensures correct
thiscontext in event handlers, which is critical for the keyboard control lifecycle.
311-360: Review spacebar state transition at line 353.At line 353,
_spacebarPressedis set totruewhen transitioning to wizard control while the spacebar key is physically down. This creates a potential edge case:Scenario: User presses spacebar on Step 3 (line 354 calls
_onSpinWizardButtonClicked()) → wizard activates and enables keyboard control (line 568) → but the user is still holding spacebar → when they release it,_handleWizardKeyUp(line 424) will process the release.Issue: The global handler sets
_spacebarPressed = true, but_handleSpacebarPress()(line 435) isn't called, sospinAllMotors()is only called once on line 562 via_onSpinWizardButtonClicked(). This appears correct for the intended flow, but the state management is subtle.Verify this sequence works as expected:
- User presses Space on "Spin Wizard" button visibility
_handleGlobalKeyDownsets_spacebarPressed = trueand calls_onSpinWizardButtonClicked()_onSpinWizardButtonClicked()enables keyboard control and callsspinAllMotors()- User releases Space →
_handleWizardKeyUpdetects_spacebarPressed = trueand calls_handleSpacebarRelease()→ motors stopTest this flow manually to ensure the state transition is clean and motors behave as expected (spin when pressed, stop when released).
386-432: LGTM: Wizard keyboard event handlers.The keyboard event handlers are well-implemented with appropriate safeguards:
- Proper guard conditions checking both
_isKeyboardControlEnabledand_isWizardActiveevent.repeatchecks prevent multiple triggers when holding keys (lines 397, 405)- Bounds checking for motor indices (line 410)
- Proper use of
preventDefault()andstopPropagation()to prevent conflicts- Clear inline documentation of the stopAllMotorsNow() behavior (lines 422-423)
463-491: LGTM: Dialog lifecycle methods with proper cleanup ordering.The
open()andclose()methods implement a careful cleanup sequence:
- Line 469-471: Disables keyboard handlers first to prevent new input
- Line 475-477: Deactivates wizard buttons (if active)
- Line 480: Stops motors (now preserves pending direction changes)
- Line 484: Deactivates motor driver (drains queue when empty)
- Line 487: Clears wizard flag
- Line 490: Resets GUI
This ordering ensures pending direction-change commands complete before shutdown, addressing the critical issue raised in past reviews. The inline comments (lines 473-474, 479, 482-483, 486) clearly document the rationale.
Note: This implementation relies on the modified behavior of
stopAllMotorsNow()in EscDshotDirectionMotorDriver.js to preserve queued commands.
550-580: LGTM: Wizard button handlers with keyboard lifecycle integration.The wizard start and stop handlers correctly integrate the keyboard control lifecycle:
- Lines 566-568: When the wizard starts spinning,
_isWizardActiveis set and keyboard control is enabled, allowing spacebar and number key shortcuts.- Lines 577-579: When the wizard stops, keyboard control is disabled and
_isWizardActiveis cleared.This ties the keyboard shortcut functionality directly to the wizard's active state, ensuring shortcuts are only available when appropriate.
src/components/EscDshotDirection/EscDshotDirectionMotorDriver.js (1)
48-61: BothstopAllMotors()andstopAllMotorsNow()are now identical—verify this is intentional.The methods on lines 44-45 and 48-61 both call
_spinAllMotors(this._motorStopValue)with no distinguishing logic. The comment onstopAllMotorsNow()states the method preserves the queue "without clearing pending commands," which meansclear()was removed. However, if both methods now preserve the queue identically, then:
stopAllMotors()(lines 176, 223 in normal button handlers) will now also delay up to ~100ms during direction changes, not just spacebar release.- No emergency-stop mechanism exists. All stop paths queue-preserve; there's no way to force immediate motor halt if needed.
The ~100ms spin trade-off for direction-change preservation is documented and reasonable, but the identical implementation suggests either:
stopAllMotors()should callclear()then_spinAllMotors()for immediate stops in normal workflows, OR- One method is redundant and should be removed
Confirm the intended behavior difference between these two methods.
Use stopAllMotors() instead of stopAllMotorsNow() to avoid clearing the command queue. This ensures direction-change and SAVE_SETTINGS commands complete even when spacebar is released or dialog is closed. stopAllMotorsNow() retains its original behavior (clears queue) for emergency stop scenarios where immediate halt is required.
protocol113
force-pushed
the
feature/keyboard-shortcuts-motor-wizard
branch
from
50d56ea to
58e983f
Compare
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 (1)
src/components/EscDshotDirection/EscDshotDirectionComponent.js(3 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)
- Allow single spacebar press to cascade through safety check -> start wizard -> spin motors - Make spacebar release fully stop wizard (same as Stop Motors button) - Move keyboard shortcuts tooltip outside SpinningWizard div so it's visible before motors start - Refactor tooltip to use i18n instead of hardcoded text This improves discoverability and provides a more intuitive toggle behavior for the spacebar shortcut.
|
|
Preview URL: https://pr4641.betaflight-app-preview.pages.dev |
4 participants
Improves user workflow by reducing mouse interaction and enabling faster motor direction configuration.
Summary by CodeRabbit