FCE-2802: DOMExeption when Android permissions missing#480
FCE-2802: DOMExeption when Android permissions missing#480MiloszFilimowski merged 3 commits intomainfrom
Conversation
MiloszFilimowski
changed the title
There was a problem hiding this comment.
Pull request overview
This PR fixes React Native media error handling by replacing instanceof DOMException checks with instanceof Error checks. Since DOMException is a browser-only API unavailable in React Native, all media errors were previously misclassified as UNHANDLED_ERROR on mobile. The change preserves browser behavior (where DOMException extends Error) while enabling proper error classification on React Native (where WebRTC errors are regular Error objects).
Changes:
- Replace
instanceof DOMExceptionwithinstanceof Errorin media error handling logic - Affects three error check locations across two files handling
getUserMediaerrors
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/react-client/src/utils/errors.ts | Updated error type check in parseUserMediaError function |
| packages/react-client/src/devices/mediaInitializer.ts | Updated error type checks in getSingleMedia and getAvailableMedia functions |
Comments suppressed due to low confidence (1)
packages/react-client/src/utils/errors.ts:13
- The PR title says "add DOMException polyfill" but the actual change removes the dependency on DOMException by using Error instead. Consider updating the PR title to accurately reflect the change, such as "Replace DOMException with Error for React Native compatibility" or similar.
if (!(error instanceof Error)) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!(err instanceof Error)) return [null, UNHANDLED_ERROR]; | ||
|
|
There was a problem hiding this comment.
question: isn't that condition meaningless? why are we checking against a base Error class? wouldn't it be better to just return [null, errorMap[err.name] ?? UNHANDLED_ERROR]?
There was a problem hiding this comment.
Good point.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
packages/react-client/src/utils/errors.ts:13
- The PR description mentions "switching to instanceof Error" but the code uses type assertion
(error as Error).nameinstead of actually checking withinstanceof Error.
The type assertion provides no runtime safety, unlike the instanceof check mentioned in the PR description. This is inconsistent with the stated approach and with error handling patterns elsewhere in the codebase (see useDataChannel.ts for examples of proper instanceof Error checks).
switch ((error as Error).name) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Description
Replace
instanceof DOMExceptionwithinstanceof Errorinreact-clienterror handling forgetUserMediaerrors.DOMExceptionis a browser-only API unavailable in React Native, so these checks always failed on mobile — all media errors were silently classified asUNHANDLED_ERROR. ADOMExceptionpolyfill wouldn't help either, since RN WebRTC errors aren't instances of the polyfilled class.Since
DOMExceptionextendsErrorin browsers, switching toinstanceof Errorpreserves existing behavior on web while fixing error classification on mobile.Motivation and Context
Media error handling was broken on mobile due to reliance on a browser-specific global.
Documentation impact
Types of changes
not work as expected)