Added centralized function to handle camera permission error #10989
Conversation
WalkthroughThis pull request introduces a new asynchronous function Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
src/Utils/cameraPermissionHandler.ts
Outdated
| import { useTranslation } from "react-i18next"; | ||
| import { toast } from "sonner"; | ||
|
|
||
| const { t } = useTranslation(); |
There was a problem hiding this comment.
hooks are supposed to be used only inside react components
src/Utils/cameraPermissionHandler.ts
Outdated
|
|
||
| const { t } = useTranslation(); | ||
|
|
||
| let toastShown = false; |
There was a problem hiding this comment.
keeping it global will cause undesired behaviours
src/Utils/cameraPermissionHandler.ts
Outdated
| export const handleCameraPermission = async ( | ||
| cameraFacingMode: string, | ||
| onPermissionDenied: () => void, | ||
| ) => { | ||
| toastShown = false; | ||
| try { | ||
| await navigator.mediaDevices.getUserMedia({ | ||
| video: { facingMode: cameraFacingMode }, | ||
| }); | ||
| } catch (_error) { | ||
| if (!toastShown) { | ||
| toastShown = true; | ||
| toast.warning(t("camera_permission_denied")); | ||
| } | ||
| onPermissionDenied(); |
There was a problem hiding this comment.
I'd say make a hook instead called useMediaDevicePermissions instead
|
let's use it on other places as well where camera is used |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/Utils/cameraPermissionHandler.ts (2)
1-4:⚠️ Potential issueCritical issue: Hook usage outside of a React component
React hooks like
useTranslation()can only be used inside React function components or custom hooks. Using them at the module level will cause runtime errors.Refactor this to properly handle translations without using hooks at the module level:
-import { useTranslation } from "react-i18next"; +import i18next from "i18next"; import { toast } from "sonner"; -const { t } = useTranslation(); +const t = (key: string) => i18next.t(key);
6-6:⚠️ Potential issueAvoid using global state variables
Using a global
toastShownvariable can cause unexpected behavior when multiple components use this utility simultaneously.This should be encapsulated within the function or potentially use a more sophisticated approach like a debounce mechanism.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Utils/cameraPermissionHandler.ts(1 hunks)src/components/Common/AvatarEditModal.tsx(2 hunks)
🔇 Additional comments (3)
src/components/Common/AvatarEditModal.tsx (2)
26-26: Import looks good.Clean addition of the import for the centralized camera permission handler.
396-399: Great refactoring of camera error handling.The previously implicit error handling has been improved by delegating to a centralized function. This approach will make it easier to maintain consistent camera permission handling across the application.
src/Utils/cameraPermissionHandler.ts (1)
8-24: Consider creating a React hook insteadAs suggested in previous comments, this utility might be better implemented as a React hook for more idiomatic React code.
A hook would allow proper use of the
useTranslationhook and manage state properly within React's lifecycle. Consider refactoring to a hook like:import { useCallback } from "react"; import { useTranslation } from "react-i18next"; import { toast } from "sonner"; export function useMediaDevicePermissions() { const { t } = useTranslation(); const requestCameraPermission = useCallback(async ( cameraFacingMode: string, onPermissionDenied: () => void, ) => { try { await navigator.mediaDevices.getUserMedia({ video: { facingMode: cameraFacingMode }, }); // Permission granted return true; } catch (error) { if (error instanceof DOMException && (error.name === "NotAllowedError" || error.name === "PermissionDeniedError")) { toast.warning(t("camera_permission_denied")); } else { toast.error(t("camera_error_occurred")); } onPermissionDenied(); return false; } }, [t]); return { requestCameraPermission }; }This would be used in components like:
const { requestCameraPermission } = useMediaDevicePermissions(); // Then in your component: onUserMediaError={async () => { await requestCameraPermission("user", () => setIsCameraOpen(false)); }}
src/Utils/cameraPermissionHandler.ts
Outdated
| cameraFacingMode: string, | ||
| onPermissionDenied: () => void, | ||
| ) => { | ||
| toastShown = false; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reset state inside the function creates race conditions
Resetting toastShown at the beginning of each function call can create race conditions if the function is called multiple times in quick succession.
This line should be removed in favor of the timestamp-based approach suggested in the earlier comment.
src/Utils/cameraPermissionHandler.ts
Outdated
| export const handleCameraPermission = async ( | ||
| cameraFacingMode: string, | ||
| onPermissionDenied: () => void, | ||
| ) => { | ||
| toastShown = false; | ||
| try { | ||
| await navigator.mediaDevices.getUserMedia({ | ||
| video: { facingMode: cameraFacingMode }, | ||
| }); | ||
| } catch (_error) { | ||
| if (!toastShown) { | ||
| toastShown = true; | ||
| toast.warning(t("camera_permission_denied")); | ||
| } | ||
| onPermissionDenied(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement more specific error handling and fix global state issues
The current implementation has several issues:
- It uses a global variable for tracking toast state
- It doesn't distinguish between different types of errors (permission denied vs. device not found)
- It resets the global state at the beginning of each call
Here's a more robust implementation:
-export const handleCameraPermission = async (
- cameraFacingMode: string,
- onPermissionDenied: () => void,
-) => {
- toastShown = false;
- try {
- await navigator.mediaDevices.getUserMedia({
- video: { facingMode: cameraFacingMode },
- });
- } catch (_error) {
- if (!toastShown) {
- toastShown = true;
- toast.warning(t("camera_permission_denied"));
- }
- onPermissionDenied();
- }
-};
+// Track toast display with a timestamp-based approach instead of a boolean
+let lastToastTime = 0;
+const TOAST_COOLDOWN_MS = 3000; // Only show toast once every 3 seconds
+
+export const handleCameraPermission = async (
+ cameraFacingMode: string,
+ onPermissionDenied: () => void,
+) => {
+ try {
+ await navigator.mediaDevices.getUserMedia({
+ video: { facingMode: cameraFacingMode },
+ });
+ // Permission granted, do nothing
+ } catch (error) {
+ // Show toast only if we haven't shown one recently
+ const now = Date.now();
+ if (now - lastToastTime > TOAST_COOLDOWN_MS) {
+ lastToastTime = now;
+
+ // Different message based on error type
+ if (error instanceof DOMException &&
+ (error.name === "NotAllowedError" || error.name === "PermissionDeniedError")) {
+ toast.warning(t("camera_permission_denied"));
+ } else {
+ toast.error(t("camera_error_occurred"));
+ }
+ }
+
+ // Always call the callback
+ onPermissionDenied();
+ }
+};Note: You'll need to add the camera_error_occurred translation key to your i18n resources.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const handleCameraPermission = async ( | |
| cameraFacingMode: string, | |
| onPermissionDenied: () => void, | |
| ) => { | |
| toastShown = false; | |
| try { | |
| await navigator.mediaDevices.getUserMedia({ | |
| video: { facingMode: cameraFacingMode }, | |
| }); | |
| } catch (_error) { | |
| if (!toastShown) { | |
| toastShown = true; | |
| toast.warning(t("camera_permission_denied")); | |
| } | |
| onPermissionDenied(); | |
| } | |
| }; | |
| // Track toast display with a timestamp-based approach instead of a boolean | |
| let lastToastTime = 0; | |
| const TOAST_COOLDOWN_MS = 3000; // Only show toast once every 3 seconds | |
| export const handleCameraPermission = async ( | |
| cameraFacingMode: string, | |
| onPermissionDenied: () => void, | |
| ) => { | |
| try { | |
| await navigator.mediaDevices.getUserMedia({ | |
| video: { facingMode: cameraFacingMode }, | |
| }); | |
| // Permission granted, do nothing | |
| } catch (error) { | |
| // Show toast only if we haven't shown one recently | |
| const now = Date.now(); | |
| if (now - lastToastTime > TOAST_COOLDOWN_MS) { | |
| lastToastTime = now; | |
| // Different message based on error type | |
| if ( | |
| error instanceof DOMException && | |
| (error.name === "NotAllowedError" || error.name === "PermissionDeniedError") | |
| ) { | |
| toast.warning(t("camera_permission_denied")); | |
| } else { | |
| toast.error(t("camera_error_occurred")); | |
| } | |
| } | |
| // Always call the callback | |
| onPermissionDenied(); | |
| } | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Utils/cameraPermissionHandler.ts (2)
7-27: 🛠️ Refactor suggestionFix potential race conditions and improve error handling.
The current implementation has several issues:
- Resetting
toastShownat the beginning of each call can create race conditions- The error handling doesn't distinguish between different types of errors
- The dependency array includes
toastShownwhich could cause unnecessary recreationconst requestPermission = useCallback( async (device: "camera", cameraFacingMode: string = "user") => { try { - setToastShown(false); const constraints = device === "camera" ? { video: { facingMode: cameraFacingMode } } : { audio: true }; await navigator.mediaDevices.getUserMedia(constraints); return true; } catch (_error) { if (!toastShown) { setToastShown(true); - toast.warning(`${device} permission denied`); + // Different message based on error type + if (_error instanceof DOMException && + (_error.name === "NotAllowedError" || _error.name === "PermissionDeniedError")) { + toast.warning(`${device} permission denied`); + } else { + toast.error(`Error accessing ${device}`); + } } return false; // Permission denied } }, - [toastShown], + [], );🧰 Tools
🪛 ESLint
[error] 10-10: Delete
·(prettier/prettier)
4-6: 🛠️ Refactor suggestionConvert to proper React custom hook.
React hooks should be named with the 'use' prefix according to React conventions. This was also mentioned in previous review comments.
-export const handleCameraPermission = () => { +export const useMediaDevicePermissions = () => { const [toastShown, setToastShown] = useState(false);
🧹 Nitpick comments (3)
src/Utils/cameraPermissionHandler.ts (3)
10-10: Remove trailing whitespace.There's an extra space at the end of this line.
- setToastShown(false); + setToastShown(false);🧰 Tools
🪛 ESLint
[error] 10-10: Delete
·(prettier/prettier)
1-30: Enhance internationalization support for error messages.The current implementation uses string interpolation for error messages, which might not work well with translation systems. Consider using translation keys instead.
import { useCallback, useState } from "react"; import { toast } from "sonner"; +import { t } from "i18next"; // [...rest of the code...] - toast.warning(`${device} permission denied`); + toast.warning(device === "camera" ? t("camera_permission_denied") : t("audio_permission_denied"));🧰 Tools
🪛 ESLint
[error] 10-10: Delete
·(prettier/prettier)
8-25: Expand device type support with proper TypeScript typing.The current implementation accepts only "camera" but has logic for audio devices too. Consider improving the type definition.
- async (device: "camera", cameraFacingMode: string = "user") => { + async (device: "camera" | "audio" | "microphone", cameraFacingMode: string = "user") => { try { setToastShown(false); const constraints = - device === "camera" + device === "camera" ? { video: { facingMode: cameraFacingMode } } - : { audio: true }; + : { audio: true };🧰 Tools
🪛 ESLint
[error] 10-10: Delete
·(prettier/prettier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Utils/cameraPermissionHandler.ts(1 hunks)src/components/Common/AvatarEditModal.tsx(3 hunks)src/components/Files/CameraCaptureDialog.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Common/AvatarEditModal.tsx
🧰 Additional context used
🪛 ESLint
src/Utils/cameraPermissionHandler.ts
[error] 10-10: Delete ·
(prettier/prettier)
🔇 Additional comments (7)
src/components/Files/CameraCaptureDialog.tsx (4)
18-18: Good addition: Importing centralized camera permission handler.This is a good step toward centralizing camera permission handling across the application.
31-32: Good implementation: Using centralized permission handler and stream state management.Converting the local variable to state and using the centralized permission handler improves code maintainability and error handling.
49-67: Robust implementation for camera permissions and stream acquisition.The new implementation correctly:
- Checks for camera permission first
- Handles permission denial gracefully by closing the dialog
- Only attempts to access the camera if permission is granted
- Properly sets the media stream state
- Includes appropriate error handling
This is a significant improvement over direct media access.
68-75: Clean stream cleanup in the useEffect.The cleanup function properly stops all tracks in the stream when the component unmounts or the dialog closes.
src/Utils/cameraPermissionHandler.ts (3)
1-3: Import React hooks and toast library for notification handling.The imports look good and are necessary for the implemented functionality.
29-30: Return object with the permission request function.The return statement correctly makes the requestPermission function available to consumers of this hook.
1-30: Consider implementing a timestamp-based approach for toast throttling.The current boolean-based approach to prevent multiple toasts isn't ideal for handling multiple calls in a short time period. A timestamp-based approach would be more robust.
-export const handleCameraPermission = () => { - const [toastShown, setToastShown] = useState(false); +export const useMediaDevicePermissions = () => { + const [lastToastTime, setLastToastTime] = useState(0); + const TOAST_COOLDOWN_MS = 3000; // Only show toast once every 3 seconds const requestPermission = useCallback( async (device: "camera", cameraFacingMode: string = "user") => { try { - setToastShown(false); const constraints = device === "camera" ? { video: { facingMode: cameraFacingMode } } : { audio: true }; await navigator.mediaDevices.getUserMedia(constraints); return true; } catch (_error) { - if (!toastShown) { - setToastShown(true); + const now = Date.now(); + if (now - lastToastTime > TOAST_COOLDOWN_MS) { + setLastToastTime(now); toast.warning(`${device} permission denied`); } return false; // Permission denied } }, - [toastShown], + [lastToastTime], ); return { requestPermission }; };🧰 Tools
🪛 ESLint
[error] 10-10: Delete
·(prettier/prettier)
src/Utils/cameraPermissionHandler.ts
Outdated
| import { useCallback, useState } from "react"; | ||
| import { toast } from "sonner"; | ||
|
|
||
| export const handleCameraPermission = () => { | ||
| const [toastShown, setToastShown] = useState(false); | ||
|
|
||
| const requestPermission = useCallback( | ||
| async (device: "camera", cameraFacingMode: string = "user") => { | ||
| try { | ||
| setToastShown(false); | ||
| const constraints = | ||
| device === "camera" | ||
| ? { video: { facingMode: cameraFacingMode } } | ||
| : { audio: true }; | ||
|
|
||
| await navigator.mediaDevices.getUserMedia(constraints); | ||
| return true; | ||
| } catch (_error) { | ||
| if (!toastShown) { | ||
| setToastShown(true); | ||
| toast.warning(`${device} permission denied`); | ||
| } | ||
| return false; // Permission denied | ||
| } | ||
| }, | ||
| [toastShown], | ||
| ); | ||
|
|
||
| return { requestPermission }; | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify this utility is used consistently across the codebase.
Based on PR objectives, this centralized function should be applied in other areas of the codebase where camera functionality is utilized.
🏁 Script executed:
#!/bin/bash
# Search for any direct calls to navigator.mediaDevices.getUserMedia in the codebase
# that could be replaced with this centralized handler
echo "Finding potential locations for applying centralized camera permission handling:"
rg -A 3 -B 3 "navigator\.mediaDevices\.getUserMedia" --type tsx --type ts | grep -v "cameraPermissionHandler"
echo -e "\nPlaces where the camera permission handler is already used:"
rg -A 1 "handleCameraPermission" --type tsx --type tsLength of output: 525
Action: Re-run the Search for Camera Permission Usages
It appears the previous search command failed due to the unrecognized file type “tsx.” To verify if the centralized camera permission utility is applied consistently across the codebase, please re-run the updated search using a glob pattern for both .ts and .tsx files.
Once you have the output from this script, please verify that:
- There are no remaining direct calls to
navigator.mediaDevices.getUserMediathat bypass the centralized handler. - The
handleCameraPermissionfunction is being used wherever camera functionality is expected.
🏁 Script executed:
#!/bin/bash
echo "Finding potential locations for applying centralized camera permission handling in .ts and .tsx files:"
rg -g "*.{ts,tsx}" -A 3 -B 3 "navigator\.mediaDevices\.getUserMedia" | grep -v "cameraPermissionHandler"
echo -e "\nFinding usage of handleCameraPermission in .ts and .tsx files:"
rg -g "*.{ts,tsx}" -A 1 "handleCameraPermission"Length of output: 2322
Action Required: Ensure Consistent Usage of Centralized Camera Permission Handler
It has been detected that while the centralized handler is correctly used in parts of the codebase (e.g., in AvatarEditModal.tsx), there are still direct calls to navigator.mediaDevices.getUserMedia in at least one location. For example, in src/components/Files/CameraCaptureDialog.tsx, a direct call is made instead of utilizing the centralized handleCameraPermission utility. Please update the camera access logic in this file (and any other similar cases) so that all camera-related getUserMedia calls are funneled through the centralized handler.
- src/components/Files/CameraCaptureDialog.tsx
• Replace the direct call tonavigator.mediaDevices.getUserMedia({ video: { facingMode: cameraFacingMode } })with an invocation of the handler’srequestPermissionfunction.
Review and apply this refactoring consistently across the codebase wherever camera functionality is implemented.
🧰 Tools
🪛 ESLint
[error] 10-10: Delete ·
(prettier/prettier)
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit