feat: Add XBox controller support with take button to prompter screen #1598
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR adds comprehensive Xbox controller support to the prompter UI, including documentation, configuration handling, and a new XboxController implementation using the HTML5 Gamepad API for button mapping, trigger-based scrolling, and speed interpolation. Changes
Sequence DiagramsequenceDiagram
participant User
participant PrompterView as PrompterView<br/>(Config)
participant Manager as ControlManager
participant XboxCtrl as XboxController
participant GamepadAPI as Gamepad API
User->>PrompterView: Load with ?mode=xbox params
PrompterView->>PrompterView: Parse xbox_speedMap,<br/>xbox_reverseSpeedMap,<br/>xbox_triggerDeadZone
PrompterView->>Manager: Initialize with config
Manager->>XboxCtrl: Create XboxController instance
XboxCtrl->>GamepadAPI: Listen for gamepad connect/disconnect
GamepadAPI-->>XboxCtrl: Gamepad connected
XboxCtrl->>XboxCtrl: startPolling() via requestAnimationFrame
loop Polling Loop
XboxCtrl->>GamepadAPI: Poll gamepad state
GamepadAPI-->>XboxCtrl: Button/trigger values
XboxCtrl->>XboxCtrl: handleButtons() & check triggers
alt Button Pressed (A, B, X, Y, etc.)
XboxCtrl->>PrompterView: Trigger action<br/>(take, scroll, segment control)
end
alt Trigger Engaged (LT/RT)
XboxCtrl->>XboxCtrl: Calculate speed via<br/>spline mapping
XboxCtrl->>PrompterView: Update scroll position
end
end
User->>XboxCtrl: Disconnect gamepad
GamepadAPI-->>XboxCtrl: Gamepad disconnected
XboxCtrl->>XboxCtrl: Stop polling (destroy)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
🤖 Fix all issues with AI agents
In `@packages/webui/src/client/ui/Prompter/controller/xbox-controller-device.ts`:
- Around line 65-66: Constructor registers window event listeners for
'gamepadconnected' and 'gamepaddisconnected' using bound callbacks but destroy()
never removes them, causing a leak; store the bound handlers (e.g.,
this.onGamepadConnectedBound = this.onGamepadConnected.bind(this) and
this.onGamepadDisconnectedBound = this.onGamepadDisconnected.bind(this)) when
you attach them, use those stored references in window.addEventListener, and
call window.removeEventListener for both events with the stored bound references
inside destroy() (remove both this.onGamepadConnectedBound and
this.onGamepadDisconnectedBound).
🧹 Nitpick comments (3)
packages/webui/src/client/ui/Prompter/PrompterView.tsx (1)
182-190: Minor:|| undefinedwill ignore explicit0value forxbox_triggerDeadZone.If a user sets
xbox_triggerDeadZone=0, the expressionNumber.parseFloat("0") || undefinedevaluates toundefinedbecause0is falsy, causing the user's explicit value to be discarded. While a dead zone of exactly 0 is rarely practical, consider using nullish coalescing or explicit NaN check for correctness:Suggested fix
- xbox_triggerDeadZone: Number.parseFloat(firstIfArray(queryParams['xbox_triggerDeadZone']) as string) || undefined, + xbox_triggerDeadZone: (() => { + const val = Number.parseFloat(firstIfArray(queryParams['xbox_triggerDeadZone']) as string) + return Number.isNaN(val) ? undefined : val + })(),packages/webui/src/client/ui/Prompter/controller/xbox-controller-device.ts (2)
295-300: Minor race condition window in polling loop.Setting
updateSpeedHandle = nullinside therequestAnimationFramecallback (line 297) before callingupdateScrollPosition()creates a brief window wherestartPolling()could be called (e.g., fromgamepadconnectedevent) and schedule a duplicate polling loop.Consider restructuring to avoid this:
Suggested fix
private updateScrollPosition(): void { // ... existing logic ... // Continue polling - this.updateSpeedHandle = window.requestAnimationFrame(() => { - this.updateSpeedHandle = null - this.updateScrollPosition() - }) + this.updateSpeedHandle = window.requestAnimationFrame(this.updateScrollPosition.bind(this)) }Or keep the handle assigned during the callback execution.
278-285: Minor: Redundant undefined checks.The checks
this.currentPosition !== undefinedandscrollPosition !== undefinedare always true sincecurrentPositionis initialized to0andwindow.scrollYalways returns a number. These can be simplified.Suggested simplification
const scrollPosition = window.scrollY - if (this.currentPosition !== undefined && scrollPosition !== undefined) { - if (speed !== 0 && this.currentPosition === scrollPosition) { - // We tried to move but couldn't (reached end) - // Could add haptic feedback here if supported - } + if (speed !== 0 && this.currentPosition === scrollPosition) { + // We tried to move but couldn't (reached end) + // Could add haptic feedback here if supported } this.currentPosition = scrollPosition
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/documentation/docs/user-guide/features/prompter.mdpackages/webui/src/client/ui/Prompter/PrompterView.tsxpackages/webui/src/client/ui/Prompter/controller/manager.tspackages/webui/src/client/ui/Prompter/controller/xbox-controller-device.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-17T14:49:55.109Z
Learnt from: Julusian
Repo: Sofie-Automation/sofie-core PR: 1585
File: packages/documentation/docs/for-developers/device-integrations/tsr-plugins.md:88-122
Timestamp: 2025-12-17T14:49:55.109Z
Learning: In the sofie-core repository, ensure that code blocks within Markdown files are indented with tabs rather than spaces. This convention is enforced by Prettier in the repository. During reviews, verify that Markdown code blocks use a leading tab for each line of the block (not spaces) and that Prettier's formatting rules reflect this preference.
Applied to files:
packages/documentation/docs/user-guide/features/prompter.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Lint Package (webui)
- GitHub Check: Test Package (main) (22.x, job-worker, true)
- GitHub Check: SonarQube
🔇 Additional comments (10)
packages/webui/src/client/ui/Prompter/PrompterView.tsx (2)
65-67: LGTM!The new Xbox configuration fields are properly typed as optional and align well with existing controller config patterns (joycon, pedal).
83-83: LGTM!The new enum member follows the established naming convention.
packages/webui/src/client/ui/Prompter/controller/xbox-controller-device.ts (3)
112-124: Consider: Broad fallback detection may match non-Xbox controllers.The fallback condition on line 122 (
gamepad.mapping === 'standard' && gamepad.axes.length >= 4 && gamepad.buttons.length >= 17) could match PlayStation controllers and other standard gamepads. This may be intentional for broader compatibility, but could cause unexpected behavior if multiple controller types are used simultaneously.If strict Xbox-only detection is desired, you could remove the fallback or make it opt-in via a config parameter.
175-228: LGTM!The button handling implementation is well-structured:
- Proper edge detection for button presses
- Appropriate debounce for the Take action to prevent accidental rapid-fire
- Clean separation of button state tracking per controller index
230-260: LGTM!Speed calculation correctly handles trigger inputs with dead zones and spline interpolation. The additive behavior when both triggers are pressed (forward speed minus backward speed) provides intuitive control.
packages/webui/src/client/ui/Prompter/controller/manager.ts (2)
9-9: LGTM!Import addition follows the established pattern.
43-45: LGTM!The XboxController integration follows the same pattern as existing controllers (JoyConController, MidiPedalController, etc.), maintaining consistency in the codebase.
packages/documentation/docs/user-guide/features/prompter.md (3)
49-49: LGTM!The new Xbox mode table entry is properly formatted and links to the correct anchor.
165-172: LGTM!The Joycon parameter documentation is accurate and aligns with the code implementation.
203-245: LGTM!Excellent documentation covering:
- Platform compatibility notes (macOS USB limitation)
- Clear scroll control explanations
- Complete button mapping table matching the code
- Configuration parameters with accurate defaults
- Practical calibration guide
The documentation is thorough and will help users get started quickly.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| window.addEventListener('gamepadconnected', this.onGamepadConnected.bind(this)) | ||
| window.addEventListener('gamepaddisconnected', this.onGamepadDisconnected.bind(this)) |
There was a problem hiding this comment.
Event listeners are not removed in destroy(), causing potential memory leak.
The gamepadconnected and gamepaddisconnected event listeners are added in the constructor but never removed in the destroy() method. This could cause memory leaks if the controller is destroyed and recreated.
Suggested fix
Store bound references and remove them in destroy:
+ private boundOnGamepadConnected: (e: GamepadEvent) => void
+ private boundOnGamepadDisconnected: (e: GamepadEvent) => void
+
constructor(view: PrompterViewContent) {
super()
this.prompterView = view
// ... existing code ...
- window.addEventListener('gamepadconnected', this.onGamepadConnected.bind(this))
- window.addEventListener('gamepaddisconnected', this.onGamepadDisconnected.bind(this))
+ this.boundOnGamepadConnected = this.onGamepadConnected.bind(this)
+ this.boundOnGamepadDisconnected = this.onGamepadDisconnected.bind(this)
+ window.addEventListener('gamepadconnected', this.boundOnGamepadConnected)
+ window.addEventListener('gamepaddisconnected', this.boundOnGamepadDisconnected)
// Start polling if a controller is already connected
this.startPolling()
}
public destroy(): void {
+ window.removeEventListener('gamepadconnected', this.boundOnGamepadConnected)
+ window.removeEventListener('gamepaddisconnected', this.boundOnGamepadDisconnected)
if (this.updateSpeedHandle !== null) {
window.cancelAnimationFrame(this.updateSpeedHandle)
this.updateSpeedHandle = null
}
}🤖 Prompt for AI Agents
In `@packages/webui/src/client/ui/Prompter/controller/xbox-controller-device.ts`
around lines 65 - 66, Constructor registers window event listeners for
'gamepadconnected' and 'gamepaddisconnected' using bound callbacks but destroy()
never removes them, causing a leak; store the bound handlers (e.g.,
this.onGamepadConnectedBound = this.onGamepadConnected.bind(this) and
this.onGamepadDisconnectedBound = this.onGamepadDisconnected.bind(this)) when
you attach them, use those stored references in window.addEventListener, and
call window.removeEventListener for both events with the stored bound references
inside destroy() (remove both this.onGamepadConnectedBound and
this.onGamepadDisconnectedBound).
nytamin
left a comment
nytamin
left a comment
There was a problem hiding this comment.
This feature looks well implemented to me.
While the feature is pretty specific the implementation is self-contained and should have minimal impact on future maintenance.
rjmunro
changed the title
manager.ts: - Replace .indexOf() > -1 with .includes() for mode checks xbox-controller-device.ts and joycon-device.ts - Add readonly modifier to members that are never reassigned:
- Store bound handlers for gamepadconnected/gamepaddisconnected events - Remove event listeners in destroy() to prevent memory leaks - For joycon, also cancel any pending requestAnimationFrame in destroy()
rjmunro
force-pushed
the
rjmunro/xbox-controller
branch
from
b15bb17 to
177faef
Compare
|
About the Contributor
This pull request is posted on behalf of me.
Type of Contribution
This is a Feature
Current Behavior
You can control scrolling on the prompter screen with many devices, including a nintendo switch joycon, but not an XBox controller.
New Behavior
You can now use an XBox controller to control the prompter screen, and also to take, enabling presenter self-operation.
Scroll control:
Button map:
Testing
Affected areas
Time Frame
Other Information
In my testing, the way the two buttons work for scrolling worked really well.
Status
XBox Controller Support for Prompter Screen
This PR adds XBox controller support to the prompter screen, enabling presenters to control prompter scrolling and navigation using an Xbox GamePad controller.
Changes
Core Implementation
packages/webui/src/client/ui/Prompter/controller/xbox-controller-device.ts)Button Mappings
Configuration Updates
PrompterView.tsx: Added Xbox configuration support with new fields:
xbox_speedMap: Speed mapping array for RT triggerxbox_reverseSpeedMap: Speed mapping array for LT triggerxbox_triggerDeadZone: Trigger deadzone thresholdPrompterConfigMode.XBOXenum memberController Manager: Integrated XboxController initialization when Xbox mode is enabled
Documentation
joycon_invertJoystickparameter, corrected typo and description)Testing
No unit tests have been added or updated in this PR. The contributor reports having tested the functionality manually.
Files Modified
packages/webui/src/client/ui/Prompter/PrompterView.tsx(+13/-0)packages/webui/src/client/ui/Prompter/controller/manager.ts(+4/-0)packages/webui/src/client/ui/Prompter/controller/xbox-controller-device.ts(+301/-0)packages/documentation/docs/user-guide/features/prompter.md(+47/-1)