refactor(ui-voip): split DevicePicker components into separate files #38327
Conversation
Resolves react/no-multi-comp ESLint violation by separating DevicePickerButton and DevicePicker components into individual files. Changes: - Created DevicePicker/ directory structure - Extracted DevicePickerButton to separate file - Extracted getDefaultDeviceItem helper to separate file - Updated component exports in index.ts - Removed eslint-disable comment BREAKING CHANGE: None (same public API)
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughRefactors DevicePicker by removing the single-file implementation and introducing a modular folder with Changes
Sequence Diagram(s)(No sequence diagrams generated — changes are a refactor/splitting of components without new multi-component control flows requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. 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
🤖 Fix all issues with AI agents
In `@packages/ui-voip/src/components/DevicePicker/index.tsx`:
- Around line 84-89: The call to requestPermission in DevicePicker (the promise
currently invoked with void requestPermission(...).then(...)) lacks rejection
handling; add a .catch handler (or use try/catch if converting to async) on
requestPermission to handle user-denied or error cases, call stopTracks if a
stream is returned on success, call setIsOpen(true) on success, and on failure
log the error and/or set an error state or show user feedback so the rejection
isn't swallowed (refer to requestPermission, stopTracks, and setIsOpen to locate
where to attach the error handling).
🧹 Nitpick comments (1)
packages/ui-voip/src/components/DevicePicker/index.tsx (1)
36-36: Consider adding onChange handler to controlled RadioButton.The
RadioButtonis controlled (checkedprop set) but has noonChangehandler. While the selection is handled viaGenericMenu'sonAction, this pattern may trigger React warnings about controlled components without change handlers in some React versions.Proposed fix
- addon: <RadioButton checked={device.id === selectedAudioDevices?.audioInput?.id} />, + addon: <RadioButton checked={device.id === selectedAudioDevices?.audioInput?.id} onChange={() => undefined} />,Apply the same pattern at line 53 for output devices.
Resolves react/no-multi-comp ESLint violation by separating DevicePickerButton and DevicePicker components into individual files. Changes: - Created DevicePicker/ directory structure - Extracted DevicePickerButton to separate file - Extracted getDefaultDeviceItem helper to separate file - Updated component exports in index.ts - Removed eslint-disable comment BREAKING CHANGE: None (same public API)
…hub.com/Agarwalchetan/Rocket.Chat into refactor/split-device-picker-components
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?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/ui-voip/src/components/DevicePicker/index.tsx`:
- Around line 114-129: The parser for deviceId currently uses includes + replace
which can mangle IDs that contain “-input”/“-output” internally; change the
checks in the DevicePicker component to use endsWith('-input') and
endsWith('-output') and derive the base id with slice(0, deviceId.length -
'-input'.length) / slice(0, deviceId.length - '-output'.length) (and guard for
empty id) before calling availableDevices?.audioInput?.find /
availableDevices?.audioOutput?.find and onDeviceChange; update the branches that
reference deviceId, id, availableDevices, and onDeviceChange accordingly so only
suffixes are removed.
🧹 Nitpick comments (1)
packages/ui-voip/src/components/DevicePicker/index.tsx (1)
91-94: Remove inline comment to match TS/TSX implementation guidelines.The log + state update already communicates intent; the comment can be dropped.
♻️ Proposed change
.catch((error) => { - // Permission denied or error occurred, keep menu closed console.warn('DevicePicker: Failed to request device permissions', error); setIsOpen(false); });As per coding guidelines, avoid code comments in implementation.
…hub.com/Agarwalchetan/Rocket.Chat into refactor/split-device-picker-components
…hub.com/Agarwalchetan/Rocket.Chat into refactor/split-device-picker-components
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/ui-voip/src/components/DevicePicker/index.tsx`:
- Around line 23-38: The current mapping for availableInputDevice uses
getDefaultDeviceItem whenever either device.id or device.label is missing,
losing the real id and collapsing devices; change the logic in the
availableInputDevice mapping so that you only call getDefaultDeviceItem when
device.id is falsy, otherwise preserve device.id (use `${device.id}-input` for
the id and RadioButton checked logic using selectedAudioDevices?.audioInput?.id)
and fall back to a default label/string when device.label is missing (e.g., use
t('Default') or similar for the Box content). Apply the same change to the
corresponding output mapping (the block around lines 41-59) so audioOutput keeps
real ids and only uses getDefaultDeviceItem when id is missing.
🧹 Nitpick comments (1)
packages/ui-voip/src/components/DevicePicker/index.tsx (1)
91-95: Prefer suffix constants over inline comments and magic numbers.Inline comments in implementation aren’t needed here; constants convey intent and remove the need for comments and hardcoded lengths.
🧹 Suggested cleanup
+ const INPUT_SUFFIX = '-input'; + const OUTPUT_SUFFIX = '-output'; @@ - .catch((error) => { - // Permission denied or error occurred, keep menu closed + .catch((error) => { console.warn('DevicePicker: Failed to request device permissions', error); setIsOpen(false); }); @@ - // Use endsWith to check suffix and slice to remove it - if (deviceId.endsWith('-input')) { - const id = deviceId.slice(0, -6); // Remove '-input' suffix + if (deviceId.endsWith(INPUT_SUFFIX)) { + const id = deviceId.slice(0, -INPUT_SUFFIX.length); @@ - if (deviceId.endsWith('-output')) { - const id = deviceId.slice(0, -7); // Remove '-output' suffix + if (deviceId.endsWith(OUTPUT_SUFFIX)) { + const id = deviceId.slice(0, -OUTPUT_SUFFIX.length);As per coding guidelines, avoid code comments in implementation.
Also applies to: 114-116, 124-126
…hub.com/Agarwalchetan/Rocket.Chat into refactor/split-device-picker-components
…hub.com/Agarwalchetan/Rocket.Chat into refactor/split-device-picker-components
1 participant
This PR refactors the
DevicePickercomponent to remove thereact/no-multi-compESLint suppression by splitting it into smaller, focused files.Previously, multiple components and helpers were defined in a single file, which required disabling the ESLint rule. This change restructures the code to follow existing patterns used in
Keypad,Widget, andPeerInfo, while keeping behavior and public APIs unchanged.What changed
Split
DevicePicker.tsxinto a dedicated directory:Extracted
DevicePickerButtoninto its own componentExtracted
getDefaultDeviceItemhelper into a separate fileRemoved
react/no-multi-compESLint disableUpdated component exports to preserve the existing public API
Why
eslint-disablecomment)Scope & risk
Verification
react/no-multi-comp)yarn lint/yarn typecheckpassedRelated issue
Closes: fixes #38326 Refactor DevicePicker to remove react/no-multi-comp ESLint disable
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.