Added privacy toggle for a consultation bed and lock camera controls when used by a user #8666
Conversation
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
CARE
|
||||||||||||||||||||||||
| Project |
CARE
|
| Run status |
|
| Run duration | 03m 22s |
| Commit |
|
| Committer | Khavin Shankar |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
125
|
nihal467
added
waiting for related PR
|
👋 Hi, @khavinshankar, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
Deploying care-fe with
|
| Latest commit: |
89c4bb0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://23a0994d.care-fe.pages.dev |
| Branch Preview URL: | https://privacy-consultation-bed.care-fe.pages.dev |
|
|
|
@khavinshankar the basic functionality is failing across, can you fix everything and attach a QA video |
|
@nihal467 I have added a demo video explaining the feature https://drive.google.com/file/d/1hPv1wdme2n87dFL95KwoDwAuOSQPt0pd/view?usp=sharing
This is an expected notification but the content was misleading, so I updated the wording.
I was not able to replicate this, this could have happened due to the backend having migrations from other PRs, please removing the existing database and creating a new one with fresh migrations.
Can we make this as a new good-first issue, so that the basic version of patient privacy and camera control locking can be merged. |
|
|
|
@khavinshankar each time when i open the camera and viewing this notification kind of weird, but we can think of a enhancement once the PR is merged, open an issue and look for a better UX |
|
except the previous comment, rest LGTM |
|
👋 Hi, @khavinshankar, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
👋 Hi, @khavinshankar, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
@khavinshankar lets get this merged. |
|
Blocking this PR till the camera plug is merged. |
WalkthroughThe changes in this pull request enhance the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (10)
src/Components/CameraFeed/routes.ts (1)
28-35: Add JSDoc documentation for the new typesWhile the types are well-structured, adding JSDoc documentation would improve code maintainability and help other developers understand the purpose of these types, especially their role in the privacy control system.
Consider adding documentation like this:
+/** + * Response type for camera locking operations + * @property result.message - Status message about the lock operation + * @property result.camera_user - Information about the user currently using the camera + */ export type GetLockCameraResponse = { result: { message: string; camera_user: UserBareMinimum; }; }; +/** + * Response type for requesting camera access when locked + * Aliases GetLockCameraResponse as the structure is identical + */ export type GetRequestAccessResponse = GetLockCameraResponse;src/Components/CameraFeed/useOperateCamera.ts (2)
44-54: Consider adding payload data for privacy control operations.The new interfaces might benefit from additional payload data to support the privacy control requirements mentioned in the PR objectives. For example:
- Reason for locking/unlocking
- Staff contact information for access requests
- Timestamp for audit purposes
interface LockCameraOperation { type: "lock_camera"; + data?: { + reason?: string; + lockedBy?: string; + timestamp?: number; + }; } interface UnlockCameraOperation { type: "unlock_camera"; + data?: { + unlockedBy?: string; + timestamp?: number; + }; } interface RequestAccessOperation { type: "request_access"; + data: { + requestedBy: string; + reason?: string; + contactDetails?: string; + }; }
Line range hint
71-95: Enhance error handling and state management for camera operations.The current implementation might not fully address the requirements from Issue #5892 regarding concurrent viewer notifications. Consider these improvements:
- Add local state to track camera lock status
- Implement error handling for concurrent access attempts
- Add retry logic for failed operations
export default function useOperateCamera(id: string, silent = false) { const [key, setKey] = useState(0); + const [isLocked, setIsLocked] = useState(false); + const [lockError, setLockError] = useState<string | null>(null); return { key, + isLocked, + lockError, operate: async (action: OperationAction) => { if (action.type === "reset") { setKey((prev) => prev + 1); return request(FeedRoutes.operateAsset, { pathParams: { id }, body: { action: { type: "get_status", }, }, silent, }); } + try { + const response = await request(FeedRoutes.operateAsset, { + pathParams: { id }, + body: { action }, + silent, + }); + + if (action.type === "lock_camera") { + setIsLocked(true); + } else if (action.type === "unlock_camera") { + setIsLocked(false); + } + + return response; + } catch (error) { + if (error.response?.status === 423) { + setLockError("Camera is currently in use by another user"); + } + throw error; + } - return request(FeedRoutes.operateAsset, { - pathParams: { id }, - body: { action }, - silent, - }); }, }; }src/Components/CameraFeed/PrivacyToggle.tsx (1)
63-75: Enhance accessibility and maintainabilityConsider these improvements for better accessibility and maintainability:
<ButtonV2 size="small" variant={isPrivacyEnabled ? "primary" : "secondary"} border + aria-label={isPrivacyEnabled ? t("privacy_enabled_tooltip") : t("privacy_disabled_tooltip")} tooltip={ !iconOnly ? undefined : isPrivacyEnabled ? t("privacy_enabled_tooltip") : t("privacy_disabled_tooltip") } - tooltipClassName="left-0 top-full translate-y-2 text-xs" - className="mr-2" + tooltipClassName={styles.tooltip} + className={styles.button} onClick={async () => {Consider creating a CSS module (
PrivacyToggle.module.css) for better style management:.tooltip { left: 0; top: 100%; transform: translateY(0.5rem); font-size: 0.75rem; } .button { margin-right: 0.5rem; }src/Components/Facility/models.tsx (2)
302-302: Add JSDoc documentation for the new privacy property.The
is_privacy_enabledproperty is correctly typed, but would benefit from documentation explaining its purpose and usage.Add documentation above the property:
+ /** Indicates whether privacy mode is enabled for this bed, preventing unauthorized camera access */ is_privacy_enabled: boolean;
302-303: Consider constraining the meta field type.Since this interface is used for privacy-related features, consider constraining the
metafield to a specific type instead of usingRecord<string, any>. This would provide better type safety and documentation.Example improvement:
- meta: Record<string, any>; + /** Additional metadata for the bed's status */ + meta: { + /** Timestamp when privacy was last enabled */ + privacy_enabled_at?: string; + /** User who last modified privacy settings */ + privacy_modified_by?: string; + };src/Redux/api.tsx (1)
566-571: Well-structured privacy toggle endpoint implementation.The new endpoint follows RESTful conventions and properly implements the privacy control feature requested in Issue #6020. The use of PATCH method is appropriate for toggling a single attribute.
Consider implementing the following enhancements in future iterations:
- Add rate limiting to prevent rapid toggling
- Include audit logging for privacy state changes
- Consider adding a reason field for enabling/disabling privacy
src/Locale/en.json (1)
1038-1043: Consider adding more context to preset-related messages.While the preset management translations are functional, some messages could benefit from additional context to improve user understanding.
Consider these improvements:
- "update_preset": "Update Preset", + "update_preset": "Update Camera Preset", - "update_preset_confirmation": "Are you sure you want to update this preset to the current location?", + "update_preset_confirmation": "Are you sure you want to update this camera preset to the current position?", - "loading_preset": "Loading Preset", + "loading_preset": "Loading Camera Preset",src/Components/CameraFeed/CameraFeed.tsx (2)
84-86: Add error handling inunlockCamerafunctionCurrently, the
unlockCamerafunction does not handle errors that may occur during the camera unlock operation. Adding error handling will ensure that any issues are appropriately managed and communicated to the user.You can apply this diff to add error handling:
const unlockCamera = useCallback(async () => { - await props.operate({ type: "unlock_camera" }); + const { res, error } = await props.operate({ type: "unlock_camera" }); + if (res?.status !== 200) { + Notification.Error({ + msg: t("camera_unlocking_error"), + }); + } }, [props.operate, t]);
292-293: Safely accesscameraUser.usernameIn the
MenuButton, you access the first character ofcameraUser.username. To prevent potential runtime errors ifcameraUser.usernameis undefined or empty, ensure that you check for its existence before accessing it.You can update the code as follows:
<MenuButton className="flex h-8 w-8 items-center justify-center rounded-full bg-primary-500 text-sm uppercase text-white shadow"> - <span>{cameraUser.username[0]}</span> + <span>{cameraUser.username?.[0] ?? ""}</span> </MenuButton>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
src/Components/CameraFeed/CameraFeed.tsx(9 hunks)src/Components/CameraFeed/NoFeedAvailable.tsx(4 hunks)src/Components/CameraFeed/PrivacyToggle.tsx(1 hunks)src/Components/CameraFeed/routes.ts(2 hunks)src/Components/CameraFeed/useOperateCamera.ts(2 hunks)src/Components/Facility/ConsultationDetails/ConsultationFeedTab.tsx(7 hunks)src/Components/Facility/models.tsx(1 hunks)src/Locale/en.json(1 hunks)src/Redux/api.tsx(1 hunks)
🔇 Additional comments (17)
src/Components/CameraFeed/routes.ts (2)
3-4: LGTM: Imports are appropriate
The new imports support the type system for camera locking functionality, with UserBareMinimum being particularly relevant for tracking camera users.
41-46: Verify response type handling in the component
The union type correctly includes the new camera locking responses. However, we should ensure that the component properly handles these new response types.
Let's verify the response handling:
✅ Verification successful
Response types are properly handled in the component
The search results show that the new response types (GetLockCameraResponse and GetRequestAccessResponse) are properly handled in src/Components/CameraFeed/CameraFeed.tsx. The component:
- Correctly type-casts the responses
- Handles success cases (status 200) with appropriate notifications
- Processes the
camera_userfield from the responses - Handles error cases (status 409) with proper error notifications
- Sets the camera user state appropriately in both success and error scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for type narrowing or response handling of the new types
ast-grep --pattern 'if ($result is GetLockCameraResponse) { $$$ }' || \
ast-grep --pattern 'if ($result.camera_user) { $$$ }' || \
rg -l "GetLockCameraResponse|GetRequestAccessResponse" --type ts
Length of output: 68
Script:
#!/bin/bash
# Let's try a broader search for response handling patterns
rg -A 5 "GetLockCameraResponse|GetRequestAccessResponse" --type ts
# Also search for any type guards or switch/case statements that might handle these responses
ast-grep --pattern 'switch ($result.$_) { $$$ }'
# Search for any conditional checks that might handle these responses
rg -A 5 "camera_user|lock_status" --type ts
Length of output: 5416
src/Components/CameraFeed/useOperateCamera.ts (2)
63-66: LGTM! Type definition is correctly updated.
The new operations are properly integrated into the OperationAction type union.
1-2: Verify API route implementation for camera operations.
Let's verify that the API routes support the new camera operations and handle concurrent access properly.
✅ Verification successful
API routes and response handling are properly implemented
The verification shows that:
- The
operateAssetAPI route is properly defined in both Redux and local routes - It supports various camera operations including status checks, stream tokens, and PTZ controls
- Response handling is implemented with appropriate success/error notifications
- The
toggleConsultationBedPrivacyendpoint is available for privacy controls - The implementation aligns with concurrent viewer notification requirements through status checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check API route implementation for camera operations
# Check for API route definitions
echo "Checking API route definitions..."
rg -A 5 "operateAsset|toggleConsultationBedPrivacy" src/
# Check for response handling
echo "Checking response handling..."
rg -A 5 "response\.(status|data)" src/Components/CameraFeed/
Length of output: 6238
src/Components/CameraFeed/NoFeedAvailable.tsx (3)
1-1: LGTM: Translation setup follows best practices
The implementation of the translation hook follows React and i18n best practices.
Also applies to: 17-17
Line range hint 19-23: Excellent security practice: URL redaction implementation
The URL redaction logic effectively masks sensitive information (UUIDs and numeric IDs) while maintaining debugging capability. This aligns well with security best practices.
46-46: Consider namespace prefixing for translation keys
While the implementation works, consider these improvements:
- Prefix translation keys with component or feature namespace to avoid conflicts (e.g., "camera_feed.retry")
- Add fallback text for missing translations
Let's verify the translation keys exist:
Also applies to: 56-56
✅ Verification successful
Translation keys are properly defined in locale files
The translation keys "retry" and "configure" are properly defined in all locale files (en.json, ta.json, kn.json, ml.json, hi.json) with appropriate translations. While namespacing could improve organization, there's no immediate risk of key conflicts as these are common action words that are consistently translated across the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for translation key definitions in locale files
fd -e json -e yaml . | xargs rg '"retry"|"configure"' -A 1 -B 1
Length of output: 1112
src/Components/CameraFeed/PrivacyToggle.tsx (1)
1-103: Verify integration with camera feed component
The privacy toggle implementation looks good, but let's verify its integration with the camera feed component to ensure it properly blocks access when privacy is enabled.
src/Components/Facility/ConsultationDetails/ConsultationFeedTab.tsx (6)
25-27: LGTM: Privacy-related component imports are properly structured
35-37: LGTM: Privacy state management is well-implemented
The state initialization properly handles undefined cases using optional chaining and provides a sensible default value.
45-45: LGTM: Proper type definition for divRef
The explicit type definition improves type safety and code maintainability.
157-170: LGTM: Privacy UI implementation is well-structured
The implementation:
- Properly handles conditional rendering
- Uses semantic HTML and accessible text
- Incorporates translations correctly
- Has appropriate styling for visibility
152-152: Verify translation keys existence
Let's ensure all new translation keys are properly defined in the localization files.
Also applies to: 175-176, 272-273, 284-284
✅ Verification successful
All translation keys are properly defined
All the required translation keys are present in the English locale file (src/Locale/en.json):
- "no_bed_or_asset_linked"
- "update_preset"
- "update_preset_confirmation"
- "save_current_position_to_preset"
- "change_camera_position_and_update_preset"
- "loading_preset"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all required translation keys exist in the English locale file
# Define the translation keys to check
KEYS=(
"no_bed_or_asset_linked"
"update_preset"
"update_preset_confirmation"
"save_current_position_to_preset"
"change_camera_position_and_update_preset"
"loading_preset"
)
# Find and check the English locale file
for key in "${KEYS[@]}"; do
echo "Checking key: $key"
rg "\"$key\":" public/locales/en/
done
Length of output: 1534
Script:
#!/bin/bash
# Let's first find where the translation files are located
fd -t f "translation|locale|i18n|lang" -E "node_modules"
# Also search for JSON files that might contain translations
fd -t f ".json$" -E "node_modules" -E "package" -E "tsconfig" -E ".vscode"
Length of output: 684
Script:
#!/bin/bash
# Now that we found the translation files, let's check for the keys in the English locale file
KEYS=(
"no_bed_or_asset_linked"
"update_preset"
"update_preset_confirmation"
"save_current_position_to_preset"
"change_camera_position_and_update_preset"
"loading_preset"
)
for key in "${KEYS[@]}"; do
echo "Checking key: $key"
rg "\"$key\":" src/Locale/en.json
done
Length of output: 1589
Line range hint 1-290: Verify integration with hospital staff notification system
The implementation handles privacy controls well, but let's verify the integration with the hospital staff notification system as mentioned in Issue #6020.
src/Components/Facility/models.tsx (1)
302-303: Verify handling of the privacy field throughout the codebase.
Let's ensure that all code creating or consuming CurrentBed objects properly handles the new privacy field.
✅ Verification successful
Privacy field implementation appears to be properly handled across the codebase
The verification shows that the privacy feature is well-integrated:
- Privacy field is properly defined in
CurrentBedinterface - API endpoint exists for privacy toggle (
/api/v1/consultationbed/{externalId}/set_privacy/) - UI components handle privacy state:
CameraFeed/PrivacyToggle.tsximplements the toggle functionalityConsultationFeedTab.tsxrespects privacy settings for camera feeds- Proper translations exist for privacy-related messages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all TypeScript/JavaScript files that use CurrentBed
# and verify proper handling of the privacy field
# Find files importing or using CurrentBed
echo "Files potentially affected by CurrentBed changes:"
rg -l "CurrentBed" --type=typescript --type=javascript
# Check for existing privacy-related code
echo -e "\nExisting privacy-related code:"
rg "privacy" --type=typescript --type=javascript
# Look for bed creation/update operations
echo -e "\nBed creation/update operations:"
rg -A 5 "new CurrentBed|createBed|updateBed" --type=typescript --type=javascript
Length of output: 556
Script:
#!/bin/bash
# Let's try with more generic file extensions and without type specifications
echo "Files potentially affected by CurrentBed changes:"
rg -l "CurrentBed" . --glob "*.{ts,tsx,js,jsx}"
echo -e "\nExisting privacy-related code:"
rg "privacy" . --glob "*.{ts,tsx,js,jsx}"
echo -e "\nBed creation/update operations:"
rg -A 5 "new CurrentBed|createBed|updateBed" . --glob "*.{ts,tsx,js,jsx}"
# Let's also check for bed-related interfaces and types
echo -e "\nBed-related type definitions:"
rg "interface.*Bed|type.*Bed" . --glob "*.{ts,tsx}"
Length of output: 3591
src/Locale/en.json (2)
1028-1031: LGTM: Camera access control messages are clear and user-friendly.
The error messages and access request prompts are well-worded and align with the PR's objective of implementing camera access controls.
1032-1037: LGTM: Privacy feature translations effectively communicate privacy states.
The translations for privacy controls are clear and comprehensive:
- Toggle actions are straightforward ("Enable Privacy"/"Disable Privacy")
- Tooltips provide good context about current state
- Error message clearly explains why camera feed is disabled
| interface PrivacyToggleProps { | ||
| consultationBedId: string; | ||
| initalValue?: boolean; | ||
| onChange?: (value: boolean) => void; | ||
| } |
There was a problem hiding this comment.
Fix typo in prop name 'initalValue'
The prop name contains a typo: 'initalValue' should be 'initialValue'.
Apply this fix:
interface PrivacyToggleProps {
consultationBedId: string;
- initalValue?: boolean;
+ initialValue?: boolean;
onChange?: (value: boolean) => void;
}📝 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.
| interface PrivacyToggleProps { | |
| consultationBedId: string; | |
| initalValue?: boolean; | |
| onChange?: (value: boolean) => void; | |
| } | |
| interface PrivacyToggleProps { | |
| consultationBedId: string; | |
| initialValue?: boolean; | |
| onChange?: (value: boolean) => void; | |
| } |
| export default function PrivacyToggle({ | ||
| consultationBedId, | ||
| initalValue, | ||
|
|
||
| onChange, | ||
| }: PrivacyToggleProps) { | ||
| const [isPrivacyEnabled, setIsPrivacyEnabled] = useState( | ||
| initalValue ?? false, | ||
| ); | ||
|
|
||
| const updatePrivacyChange = (value: boolean) => { | ||
| setIsPrivacyEnabled(value); | ||
| onChange?.(value); | ||
| }; | ||
|
|
||
| useQuery(routes.getConsultationBed, { | ||
| pathParams: { externalId: consultationBedId }, | ||
| onResponse(res) { | ||
| updatePrivacyChange(res.data?.is_privacy_enabled ?? false); | ||
| }, | ||
| }); | ||
|
|
||
| return ( | ||
| <TogglePrivacyButton | ||
| value={isPrivacyEnabled} | ||
| consultationBedId={consultationBedId} | ||
| onChange={updatePrivacyChange} | ||
| iconOnly | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for API failures
The useQuery hook should handle API failures to provide better user feedback and maintain a consistent state.
Consider adding error handling:
useQuery(routes.getConsultationBed, {
pathParams: { externalId: consultationBedId },
onResponse(res) {
updatePrivacyChange(res.data?.is_privacy_enabled ?? false);
},
+ onError(error) {
+ console.error('Failed to fetch privacy status:', error);
+ // Consider showing a notification to the user
+ },
});Also, update the prop name to match the interface fix:
export default function PrivacyToggle({
consultationBedId,
- initalValue,
+ initialValue,
onChange,
}: PrivacyToggleProps) {
const [isPrivacyEnabled, setIsPrivacyEnabled] = useState(
- initalValue ?? false,
+ initialValue ?? false,
);📝 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 default function PrivacyToggle({ | |
| consultationBedId, | |
| initalValue, | |
| onChange, | |
| }: PrivacyToggleProps) { | |
| const [isPrivacyEnabled, setIsPrivacyEnabled] = useState( | |
| initalValue ?? false, | |
| ); | |
| const updatePrivacyChange = (value: boolean) => { | |
| setIsPrivacyEnabled(value); | |
| onChange?.(value); | |
| }; | |
| useQuery(routes.getConsultationBed, { | |
| pathParams: { externalId: consultationBedId }, | |
| onResponse(res) { | |
| updatePrivacyChange(res.data?.is_privacy_enabled ?? false); | |
| }, | |
| }); | |
| return ( | |
| <TogglePrivacyButton | |
| value={isPrivacyEnabled} | |
| consultationBedId={consultationBedId} | |
| onChange={updatePrivacyChange} | |
| iconOnly | |
| /> | |
| ); | |
| } | |
| export default function PrivacyToggle({ | |
| consultationBedId, | |
| initialValue, | |
| onChange, | |
| }: PrivacyToggleProps) { | |
| const [isPrivacyEnabled, setIsPrivacyEnabled] = useState( | |
| initialValue ?? false, | |
| ); | |
| const updatePrivacyChange = (value: boolean) => { | |
| setIsPrivacyEnabled(value); | |
| onChange?.(value); | |
| }; | |
| useQuery(routes.getConsultationBed, { | |
| pathParams: { externalId: consultationBedId }, | |
| onResponse(res) { | |
| updatePrivacyChange(res.data?.is_privacy_enabled ?? false); | |
| }, | |
| onError(error) { | |
| console.error('Failed to fetch privacy status:', error); | |
| // Consider showing a notification to the user | |
| }, | |
| }); | |
| return ( | |
| <TogglePrivacyButton | |
| value={isPrivacyEnabled} | |
| consultationBedId={consultationBedId} | |
| onChange={updatePrivacyChange} | |
| iconOnly | |
| /> | |
| ); | |
| } |
| onClick={async () => { | ||
| const { res, data } = await request( | ||
| routes.toggleConsultationBedPrivacy, | ||
| { | ||
| pathParams: { externalId: consultationBedId }, | ||
| body: { | ||
| is_privacy_enabled: !isPrivacyEnabled, | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| if (res?.ok && data) { | ||
| updatePrivacyChange(data.is_privacy_enabled); | ||
| } | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve button interaction handling during API calls
The current implementation could benefit from better user feedback and protection against multiple clicks during API calls.
Consider these improvements:
+ const [isLoading, setIsLoading] = useState(false);
// ...
<ButtonV2
// ...
+ disabled={isLoading}
onClick={async () => {
+ if (isLoading) return;
+ setIsLoading(true);
+ try {
const { res, data } = await request(
routes.toggleConsultationBedPrivacy,
{
pathParams: { externalId: consultationBedId },
body: {
is_privacy_enabled: !isPrivacyEnabled,
},
},
);
if (res?.ok && data) {
updatePrivacyChange(data.is_privacy_enabled);
}
+ } catch (error) {
+ console.error('Failed to toggle privacy:', error);
+ // Consider showing a notification to the user
+ } finally {
+ setIsLoading(false);
+ }
}}
>Committable suggestion skipped: line range outside the PR's diff.
| {props.consultationData.current_bed && ( | ||
| <PrivacyToggle | ||
| initalValue={isPrivacyEnabled} | ||
| onChange={setIsPrivacyEnabled} | ||
| consultationBedId={props.consultationData.current_bed.id} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
Fix typo in initialValue prop name
There's a typo in the initalValue prop name.
- initalValue={isPrivacyEnabled}
+ initialValue={isPrivacyEnabled}📝 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.
| {props.consultationData.current_bed && ( | |
| <PrivacyToggle | |
| initalValue={isPrivacyEnabled} | |
| onChange={setIsPrivacyEnabled} | |
| consultationBedId={props.consultationData.current_bed.id} | |
| /> | |
| )} | |
| {props.consultationData.current_bed && ( | |
| <PrivacyToggle | |
| initialValue={isPrivacyEnabled} | |
| onChange={setIsPrivacyEnabled} | |
| consultationBedId={props.consultationData.current_bed.id} | |
| /> | |
| )} |
| @@ -124,7 +130,7 @@ | |||
| if (divRef.current) { | |||
| divRef.current.scrollIntoView({ behavior: "smooth" }); | |||
| } | |||
| }, [!!bed, loading, !!asset, divRef.current]); | |||
| }, [!!bed, loading, !!asset, divRef.current]); // eslint-disable-line react-hooks/exhaustive-deps | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring the useEffect dependencies
Including divRef.current in the dependencies array might cause unnecessary re-renders as the ref object itself is stable, but its current property changes on every render.
- }, [!!bed, loading, !!asset, divRef.current]); // eslint-disable-line react-hooks/exhaustive-deps
+ }, [!!bed, loading, !!asset]); // The ref itself is stable📝 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.
| }, [!!bed, loading, !!asset, divRef.current]); // eslint-disable-line react-hooks/exhaustive-deps | |
| }, [!!bed, loading, !!asset]); // The ref itself is stable |
| path: "/api/v1/consultationbed/{externalId}/", | ||
| method: "GET", | ||
| TRes: Type<CurrentBed>(), |
There was a problem hiding this comment.
💡 Codebase verification
Parameter naming inconsistency found in consultation bed API routes
The codebase has inconsistent parameter naming in the consultation bed API routes:
getConsultationBeduses{externalId}updateConsultationBeduses{external_id}toggleConsultationBedPrivacyuses{externalId}
All API calls in CameraFeed/PrivacyToggle.tsx use externalId, but the inconsistency in route definitions could cause issues. The parameter should use the same format across all routes.
🔗 Analysis chain
Verify consistent usage of the new parameter name.
The path parameter has been renamed from external_id to externalId. While this improves naming consistency, we should ensure all API calls are updated accordingly.
Based on the search results, I can see that there are inconsistencies in the parameter naming across the codebase:
- The API route definitions in
src/Redux/api.tsxuse both{externalId}and{external_id}for consultation bed endpoints:
path: "/api/v1/consultationbed/{externalId}/"
path: "/api/v1/consultationbed/{external_id}/"
path: "/api/v1/consultationbed/{externalId}/set_privacy/"
Let's verify if there are any actual API calls using these endpoints to ensure we catch all inconsistencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining usage of the old parameter name
# and verify that all calls use the new parameter name
# Search for old parameter usage in API calls
echo "Checking for old parameter usage..."
rg -l "consultationbed/[^/]+/\{external_id\}"
# Search for new parameter usage to confirm proper updates
echo "Checking for new parameter usage..."
rg -l "consultationbed/[^/]+/\{externalId\}"
Length of output: 256
Script:
#!/bin/bash
# Let's try a different approach by:
# 1. Finding all files that mention consultationbed in the API path
# 2. Looking for any external_id or externalId usage in those contexts
# Search for consultationbed API paths with more flexible pattern
echo "Searching for consultation bed API paths..."
rg -l "/api.*consultationbed"
# Search for external_id usage in API contexts
echo "Searching for external_id usage..."
rg -B2 -A2 "external_id"
# Search for externalId usage in API contexts
echo "Searching for externalId usage..."
rg -B2 -A2 "externalId"
# Additionally, let's check the API definition file specifically
echo "Checking API definition file content..."
rg "consultationbed.*/" src/Redux/api.tsx
Length of output: 77796
Script:
#!/bin/bash
# Search for API calls to consultationbed endpoints
echo "Searching for consultationbed API calls..."
rg -B2 -A2 "routes\.(get|update|toggle)ConsultationBed"
# Search for any direct URL construction with consultationbed
echo "Searching for direct URL references..."
rg -B2 -A2 "consultationbed/[^\"']*"
Length of output: 2133
| const lockCamera = useCallback(async () => { | ||
| const { res, data, error } = await props.operate({ type: "lock_camera" }); | ||
|
|
||
| const successData = data as GetLockCameraResponse; | ||
| const errorData = error as GetLockCameraResponse["result"]; | ||
|
|
||
| if (res?.status === 200 && successData?.result) { | ||
| Notification.Success({ | ||
| msg: successData.result.message, | ||
| }); | ||
| setCameraUser(successData.result.camera_user); | ||
| } else if (res?.status === 409 && errorData) { | ||
| Notification.Warn({ | ||
| msg: errorData.message, | ||
| }); | ||
| setCameraUser(errorData.camera_user); | ||
| } else { | ||
| Notification.Error({ | ||
| msg: t("camera_locking_error"), | ||
| }); | ||
| } | ||
| }, []); // eslint-disable-line react-hooks/exhaustive-deps | ||
|
|
||
| const unlockCamera = useCallback(async () => { | ||
| await props.operate({ type: "unlock_camera" }); | ||
| }, []); // eslint-disable-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle dependencies correctly in hooks and avoid disabling ESLint rules
The use of empty dependency arrays and suppression of ESLint warnings in useCallback and useEffect hooks can lead to stale references and unexpected behaviors. To ensure consistent behavior, include all necessary dependencies in the dependency arrays and avoid disabling the react-hooks/exhaustive-deps rule.
Apply the following changes to specify dependencies properly:
const lockCamera = useCallback(async () => {
// Function body
-}, []); // eslint-disable-line react-hooks/exhaustive-deps
+}, [props.operate, t, setCameraUser]);
const unlockCamera = useCallback(async () => {
// Function body
-}, []); // eslint-disable-line react-hooks/exhaustive-deps
+}, [props.operate]);
useEffect(() => {
lockCamera();
return () => {
unlockCamera();
};
-}, [lockCamera, unlockCamera]); // eslint-disable-line react-hooks/exhaustive-deps
+}, [lockCamera, unlockCamera]);Ensure that any variables or functions used inside the hooks are included in their dependency arrays.
Committable suggestion skipped: line range outside the PR's diff.
@ohcnetwork/care-fe-code-reviewers
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
These changes enhance user experience by improving functionality, interaction, and accessibility across the application.