Added centralized function to handle camera permission error #10989
Conversation
WalkthroughThis pull request introduces a new custom hook, Changes
Sequence Diagram(s)sequenceDiagram
participant AEM as AvatarEditModal
participant UC as useCamera Hook
participant ND as Navigator.mediaDevices
AEM->>UC: Call requestPermission("user")
UC->>ND: Request media stream
ND-->>UC: Return stream or error
UC-->>AEM: Return permission result
alt Permission Denied
AEM->>AEM: Set camera state closed & show warning toast
end
sequenceDiagram
participant CCD as CameraCaptureDialog
participant UC as useCamera Hook
participant ND as Navigator.mediaDevices
CCD->>UC: Invoke requestPermission("user")
UC->>ND: Request media stream
ND-->>UC: Return stream or error
UC-->>CCD: Return permission result
alt Permission Granted
CCD->>CCD: Set stream state and display video
else Permission Denied
CCD->>CCD: Call onOpenChange(false) to close dialog
end
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)
🪧 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 |
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)
r.mp4Video of changed working |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/Utils/cameraPermissionHandler.ts (1)
12-17: 🛠️ Refactor suggestionReset flag at the start creates potential race conditions
Resetting
toastShownRef.current = falseat the beginning of each function call can create race conditions if the function is called multiple times in quick succession.Consider using a timestamp-based approach instead:
- toastShownRef.current = false; + // Remove this line, let the flag persist between callsAnd implement a timestamp-based approach to limit toast frequency:
- const toastShownRef = useRef(false); + const lastToastTimeRef = useRef(0); + const TOAST_COOLDOWN_MS = 3000; // Only show toast once every 3 seconds
🧹 Nitpick comments (1)
src/Utils/cameraPermissionHandler.ts (1)
9-10: Ensure function parameter types match the implementationThe function accepts a device parameter of type "camera", but based on line 14-16, it seems intended to handle both camera and audio. Consider expanding the type to include "audio" or other media types you'll support.
- async (device: "camera", cameraFacingMode: string = "user") => { + async (device: "camera" | "audio", cameraFacingMode: string = "user") => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(1 hunks)src/Utils/cameraPermissionHandler.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- public/locale/en.json
🔇 Additional comments (2)
src/Utils/cameraPermissionHandler.ts (2)
1-8: Good job implementing this as a React hookThe implementation correctly uses React hooks for state management. Good choice of using
useRefto track toast state across renders.
20-26: Enhance error handling with specific error typesThe current implementation doesn't distinguish between different types of errors, but there are various reasons why
getUserMediamight fail (permission denied, device not found, etc.).Consider implementing more specific error handling:
- catch (_error) { - if (!toastShownRef.current) { - toastShownRef.current = true; - toast.warning(t("camera_permission_denied")); - } + catch (error) { + const now = Date.now(); + if (now - lastToastTimeRef.current > TOAST_COOLDOWN_MS) { + lastToastTimeRef.current = now; + + // Different message based on error type + if (error instanceof DOMException && + (error.name === "NotAllowedError" || error.name === "PermissionDeniedError")) { + toast.warning(t("camera_permission_denied")); + } else if (error instanceof DOMException && error.name === "NotFoundError") { + toast.error(t("camera_not_found")); + } else { + toast.error(t("camera_error_occurred")); + } + }Note: You'll need to add the appropriate translation keys to your i18n resources.
src/Utils/cameraPermissionHandler.ts
Outdated
| [], | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include necessary dependencies in the useCallback dependency array
The useCallback dependency array is empty, but the function uses t from useTranslation which might change when the app language changes.
- [],
+ [t],📝 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.
| [], | |
| ); | |
| [t], | |
| ); |
src/Utils/cameraPermissionHandler.ts
Outdated
| export const handleCameraPermission = () => { | ||
| const toastShownRef = useRef(false); | ||
| const { t } = useTranslation(); | ||
|
|
||
| const requestPermission = useCallback( | ||
| async (device: "camera", cameraFacingMode: string = "user") => { | ||
| try { | ||
| toastShownRef.current = false; | ||
| const constraints = | ||
| device === "camera" | ||
| ? { video: { facingMode: cameraFacingMode } } | ||
| : { audio: true }; | ||
|
|
||
| await navigator.mediaDevices.getUserMedia(constraints); | ||
| return true; | ||
| } catch (_error) { | ||
| if (!toastShownRef.current) { | ||
| toastShownRef.current = true; | ||
| toast.warning(t("camera_permission_denied")); | ||
| } | ||
| return false; | ||
| } | ||
| }, | ||
| [], | ||
| ); | ||
|
|
||
| return { requestPermission }; | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm this utility is used consistently across the codebase
Based on PR objectives and past reviews, ensure this centralized function is applied in all areas where camera functionality is utilized, particularly in CameraCaptureDialog.tsx.
🏁 Script executed:
#!/bin/bash
echo "Finding places still using direct camera access that should use the centralized handler:"
rg -g "*.{ts,tsx}" -A 3 -B 3 "navigator\.mediaDevices\.getUserMedia" | grep -v "cameraPermissionHandler"
echo -e "\nPlaces already using the centralized handler:"
rg -g "*.{ts,tsx}" -A 1 "handleCameraPermission"Length of output: 2244
Ensure consistent use of the centralized camera permission utility
Our search confirms that while components like AvatarEditModal.tsx and parts of CameraCaptureDialog.tsx are using the handleCameraPermission function, there remain instances with direct calls to navigator.mediaDevices.getUserMedia. Specifically:
- In
src/components/Files/CameraCaptureDialog.tsx, there's a direct call alongside the centralized handler usage. - In
src/Utils/useVoiceRecorder.ts, the direct use ofnavigator.mediaDevices.getUserMediapersists.
Please update these sections to uniformly leverage handleCameraPermission to guarantee consistent handling of camera permissions across the codebase.
src/Utils/cameraPermissionHandler.ts
Outdated
| import { useTranslation } from "react-i18next"; | ||
| import { toast } from "sonner"; | ||
|
|
||
| export const handleCameraPermission = () => { |
| } | ||
|
|
||
| try { | ||
| const mediaStream = await navigator.mediaDevices.getUserMedia({ |
There was a problem hiding this comment.
you can use the hook, it can return mediaStream and hasPermission.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/components/Common/AvatarEditModal.tsx (1)
397-402: Good implementation of camera permission error handling.The modification of the
onUserMediaErrorcallback to be asynchronous allows proper handling of the permission request before determining whether to close the camera interface.However, consider adding error logging when permission is denied to help with debugging:
onUserMediaError={async () => { const hasPermission = await requestPermission("user"); if (!hasPermission) { + console.error("Camera permission denied"); setIsCameraOpen(false); } }}src/Utils/useCamera.ts (2)
1-38: Well-structured camera permission handling hook.The implementation follows React best practices and provides a centralized approach to camera permission handling as required by the PR objective.
Here are a few suggestions to make this hook even more robust:
- Use a more specific type for the
cameraFacingModeparameter:- async (cameraFacingMode: string = "user") => { + async (cameraFacingMode: "user" | "environment" = "user") => {
- Add proper cleanup of the media stream if an error occurs after obtaining it:
try { toastShownRef.current = false; const constraints: MediaStreamConstraints = { video: { facingMode: cameraFacingMode }, }; const mediaStream = await navigator.mediaDevices.getUserMedia(constraints); if (mediaStream == null) { return { hasPermission: false, mediaStream: null }; } return { hasPermission: true, mediaStream: mediaStream }; } catch (_error) { + // Ensure any partial media stream is properly cleaned up + if (_error instanceof MediaStreamError && _error.message) { + console.error(`Camera error: ${_error.message}`); + } if (!toastShownRef.current) { toastShownRef.current = true; toast.warning(t("camera_permission_denied")); } return { hasPermission: false, mediaStream: null }; }
- The null check on
mediaStreammight be redundant asgetUserMediatypically throws errors rather than returning null.
5-37: Consider adding documentation for the hook.Adding JSDoc comments would enhance code maintainability and make it easier for other developers to understand how to use this hook properly.
+/** + * Hook to manage camera permissions and access. + * @returns {Object} Object containing requestPermission function + */ export const useCamera = () => { const toastShownRef = useRef(false); const { t } = useTranslation(); + /** + * Request camera permission with specified facing mode + * @param {string} cameraFacingMode - Camera facing mode ("user" for front, "environment" for back) + * @returns {Promise<{hasPermission: boolean, mediaStream: MediaStream | null}>} Object containing permission status and media stream if granted + */ const requestPermission = useCallback( async (cameraFacingMode: string = "user") => { // ...existing implementation... }, [t], ); return { requestPermission }; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Utils/useCamera.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/Files/CameraCaptureDialog.tsx
🔇 Additional comments (2)
src/components/Common/AvatarEditModal.tsx (2)
26-26: Good addition of the useCamera hook import.This import aligns with the PR objective of centralizing camera permission handling.
82-82: Correctly destructured the requestPermission function.Properly extracting the function from the hook for use in the component.
| } catch (_error) { | ||
| if (!toastShownRef.current) { | ||
| toastShownRef.current = true; | ||
| toast.warning(t("camera_permission_denied")); | ||
| } | ||
| return { hasPermission: false, mediaStream: null }; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify this hook in other contexts across the application.
As mentioned in PR comments by rithviknishad, this hook should be applied in other areas of the codebase where camera functionality is utilized.
🏁 Script executed:
#!/bin/bash
# Find other places in the codebase that might benefit from using this hook
echo "Searching for getUserMedia usage that could be replaced with useCamera hook:"
rg "getUserMedia" --type ts --type tsx -C 3 | grep -v "useCamera.ts"
echo "\nSearching for potential camera error handling locations:"
rg "camera.*error|media.*error" --type ts --type tsx -C 3Length of output: 472
Action: Verify Consistent Use of useCamera Hook
The modifications in src/Utils/useCamera.ts (lines 25–31) properly handle camera permission errors by showing a toast notification and returning a consistent error structure. However, per rithviknishad’s previous PR comments, please ensure that all areas in the application using camera functionality—specifically direct getUserMedia calls and camera error handling code in both .ts and .tsx files—are updated to use this hook for better consistency and maintainability.
- Confirm that any
getUserMediacalls outside theuseCamerahook are refactored to utilize this hook. - Verify that error handling logic related to camera or media issues in other components aligns with the behavior defined in this hook.
---
🏁 Script executed:
```shell
#!/bin/bash
# Re-run the search for getUserMedia usage in both .ts and .tsx files using glob patterns
echo "Searching for getUserMedia usage in .ts and .tsx files that could be replaced with useCamera hook:"
rg "getUserMedia" --glob "*.ts" --glob "*.tsx" -C 3 | grep -v "useCamera.ts"
echo "\nSearching for potential camera error handling locations in .ts and .tsx files:"
rg "camera.*error|media.*error" --glob "*.ts" --glob "*.tsx" -C 3
Length of output: 2094
Action Required: Refactor Camera-Related Error Handling
The implementation in src/Utils/useCamera.ts (lines 25–31) correctly handles camera permission failures by displaying a toast and returning a consistent error object. However, our search indicates that at least one camera-related component—specifically, src/components/Files/CameraCaptureDialog.tsx—handles camera errors manually (via a try–catch block that logs the error) instead of leveraging the hook’s logic.
- src/components/Files/CameraCaptureDialog.tsx: Refactor the manual error handling (e.g., the
console.error("Error accessing camera:", error);block) to use the standardized logic from theuseCamerahook. - Review Other Camera Usages: Although the search found direct calls to
getUserMediain files such asuseVoiceRecorder.tsandAudioCaptureDialog.tsx, verify that these components are correctly distinguishing between audio and camera responsibilities. Only camera functionalities should adopt theuseCamerahook, while audio-specific implementations may remain unchanged if intentional.
Please review all camera functionality areas to ensure consistency and centralized error handling.
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes