feat: add allowedWebHIDDevices#32
Merged
Merged
Conversation
this allows specified webhid devices to be automatically approved in specific windows, in order to work without asking the user for permission
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
ianshade
changed the title
![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 (2)
src/helpers/WindowHelper.ts (1)
473-485: Usage filter behavior whencollectionsis empty.If a device is reported with an empty
collectionsarray while the config specifies ausagePage/usagefilter,.some()returnsfalseand the device is denied. This is the safe default, but for robustness consider logging this case — it's the most common reason a configured device silently fails to auto-approve (hot-plug timing, driver quirks).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/helpers/WindowHelper.ts` around lines 473 - 485, When a device has an empty collections array but the config specifies a usagePage/usage filter, _matchesUsageFilter currently returns false silently; add a guarded log when device.collections.length === 0 and (allowedDevice.usagePage !== undefined || allowedDevice.usage !== undefined) to record why the device was denied. Inside _matchesUsageFilter, before calling device.collections.some(...), emit a debug/warn log including identifying fields from device (e.g., vendorId, productId, productName) and the allowedDevice usagePage/usage so hot-plug/driver timing issues are visible, but keep the existing boolean return behavior unchanged.src/helpers/AllWindowsManager.ts (1)
60-62: Origin-based window resolution is ambiguous when multiple windows share an origin.
getWindowForOriginreturns the first matching window by iteration order. This is fine in the common case, but if two configured windows both load the same origin, the wrong window's allowlist may be consulted insetDevicePermissionHandler.Note: The
detailsobject insetDevicePermissionHandlerdoes not include aframeproperty—onlydeviceType,origin, anddeviceare available. Frame-based lookup is possible in theselect-hid-deviceevent handler (which does receivedetails.frame), but not insetDevicePermissionHandler. Any fix would require alternative approaches beyond frame-based lookup.Not urgent for the current single-page-per-window usage pattern, but worth documenting the assumption.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/helpers/AllWindowsManager.ts` around lines 60 - 62, getWindowForOrigin currently returns the first WindowHelper that matches an origin which is ambiguous when multiple windows share the same origin; change the API to return all matching windows and update the permission logic to consult each candidate's allowlist: add a new method getWindowsForOrigin(origin: string): WindowHelper[] (or change the existing getWindowForOrigin to return an array) and in setDevicePermissionHandler iterate the array of WindowHelper instances (from windowsHandlers) and check each WindowHelper's allowlist/permission logic to decide which window grants permission; also update the select-hid-device handler callers to use the new method if needed and preserve backward compatibility or update all call sites accordingly (refer to getWindowForOrigin, windowsHandlers, WindowHelper, setDevicePermissionHandler, and select-hid-device).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ChefManager.ts`:
- Around line 108-114: The permission handlers currently deny every non-HID
permission/device by returning false; change both
defaultSession.setPermissionCheckHandler and
defaultSession.setDevicePermissionHandler so they only explicitly handle 'hid'
and otherwise return undefined (or no value) to let Electron/Chromium apply its
default behavior; keep the existing webContents null guard and the window lookup
via this.windowsHelper.getWindowForWebContents and continue to return
window?.hasAllowedWebHIDDevices() for HID checks while returning undefined for
all other permission or device types.
---
Nitpick comments:
In `@src/helpers/AllWindowsManager.ts`:
- Around line 60-62: getWindowForOrigin currently returns the first WindowHelper
that matches an origin which is ambiguous when multiple windows share the same
origin; change the API to return all matching windows and update the permission
logic to consult each candidate's allowlist: add a new method
getWindowsForOrigin(origin: string): WindowHelper[] (or change the existing
getWindowForOrigin to return an array) and in setDevicePermissionHandler iterate
the array of WindowHelper instances (from windowsHandlers) and check each
WindowHelper's allowlist/permission logic to decide which window grants
permission; also update the select-hid-device handler callers to use the new
method if needed and preserve backward compatibility or update all call sites
accordingly (refer to getWindowForOrigin, windowsHandlers, WindowHelper,
setDevicePermissionHandler, and select-hid-device).
In `@src/helpers/WindowHelper.ts`:
- Around line 473-485: When a device has an empty collections array but the
config specifies a usagePage/usage filter, _matchesUsageFilter currently returns
false silently; add a guarded log when device.collections.length === 0 and
(allowedDevice.usagePage !== undefined || allowedDevice.usage !== undefined) to
record why the device was denied. Inside _matchesUsageFilter, before calling
device.collections.some(...), emit a debug/warn log including identifying fields
from device (e.g., vendorId, productId, productName) and the allowedDevice
usagePage/usage so hot-plug/driver timing issues are visible, but keep the
existing boolean return behavior unchanged.
🪄 Autofix (Beta)
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
Run ID: 0f6bd2f7-d86e-4b96-a076-c076f2ed15d0
📒 Files selected for processing (7)
README.mddocs/WebHID-Configuration.mdexamples/config-with-webhid.jsonsrc/ChefManager.tssrc/helpers/AllWindowsManager.tssrc/helpers/WindowHelper.tssrc/lib/config.ts
nytamin
approved these changes
Apr 27, 2026
in Electron by default most permissions are pre-approved in the absence of this callback; most still need explicit approvals in other subsequent callbacks
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/ChefManager.ts (1)
141-161:select-hid-devicealways picks the first allowed match — document or revisit if multiple matching devices may be connected.
details.deviceList.find(...)at Line 150 deterministically returns the first allowed device. If a kiosk has two physically connected devices that both satisfy a singleallowedWebHIDDevicesentry (e.g. two identical control surfaces), the page can never reach the second one — andnavigator.hid.requestDevicewill not show a chooser sinceevent.preventDefault()was called at Line 142. This is probably intentional for the typical single-device deployment, but worth either:
- Adding a brief comment stating the auto-select-first policy, or
- Returning the list filtered (callback isn't a multi-select API in Electron, so a comment is the pragmatic choice).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ChefManager.ts` around lines 141 - 161, The select-hid-device handler currently auto-selects the first matching device (details.deviceList.find(...) inside the 'select-hid-device' listener) which prevents the user chooser from appearing because event.preventDefault() is called; update the code by adding a concise comment above the 'select-hid-device' handler documenting this auto-select-first policy and the rationale (e.g., kiosk/single-device use), referencing the use of details.deviceList.find, window.isAllowedWebHIDDevice, and the callback(device.deviceId) behavior so future reviewers know this is intentional (or where to change it if multi-device support is later required).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ChefManager.ts`:
- Around line 108-112: The inline comment misstates intent: update the comment
for defaultSession.setPermissionCheckHandler to state that this handler
intentionally denies the 'deprecated-sync-clipboard-read' permission (to enforce
use of async clipboard APIs) while granting all other non-'hid' permissions,
i.e., it deliberately diverges from Electron's default permissive behavior;
alternatively, if the original goal was to match Electron's default, remove the
custom handler for non-HID permissions and let Electron handle permission checks
instead.
---
Nitpick comments:
In `@src/ChefManager.ts`:
- Around line 141-161: The select-hid-device handler currently auto-selects the
first matching device (details.deviceList.find(...) inside the
'select-hid-device' listener) which prevents the user chooser from appearing
because event.preventDefault() is called; update the code by adding a concise
comment above the 'select-hid-device' handler documenting this auto-select-first
policy and the rationale (e.g., kiosk/single-device use), referencing the use of
details.deviceList.find, window.isAllowedWebHIDDevice, and the
callback(device.deviceId) behavior so future reviewers know this is intentional
(or where to change it if multi-device support is later required).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
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
2 participants
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.
About the Contributor
This pull request is posted on behalf of TV 2 Norge.
Type of Contribution
This is a:
Feature
Current Behavior
No way to use webhid devices with Sofie Chef
New Behavior
New
ConfigWindowproperty calledallowedWebHIDDevicesis added. It allows to define webhid devices to be automatically approved in specific windows. A device in this array will not require explicit user approval when the page displayed in the window tries to access it.Testing Instructions
allowedWebHIDDevicesas documentedOther Information
Status