Fix Camera Capture on Retake and Close #9180
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning Rate limit exceeded@JavidSumra has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 33 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📜 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/components/Files/CameraCaptureDialog.tsx (2)
174-176: Consider reordering operations in close handlerThe current order of operations is:
- Reset preview
- Hide dialog
- Reset capture
Consider calling
onResetCapturebefore hiding the dialog to ensure cleanup happens before the component potentially unmounts:onClick={() => { setPreviewImage(null); + onResetCapture(); onHide(); - onResetCapture(); }}
235-237: Consider reordering operations in laptop close handlerSimilar to the mobile close handler, consider reordering operations:
onClick={() => { setPreviewImage(null); + onResetCapture(); onHide(); - onResetCapture(); }}src/hooks/useFileUpload.tsx (1)
264-264: Consider adding reset capability to AudioCaptureDialog for consistency.While the camera capture reset is now handled properly, the audio capture dialog lacks similar reset functionality. This could lead to inconsistent behavior between the two capture methods.
Consider adding the
onResetCaptureprop to AudioCaptureDialog:<AudioCaptureDialog show={audioModalOpen} onHide={() => setAudioModalOpen(false)} onCapture={(file) => { setFiles((prev) => [...prev, file]); }} + onResetCapture={clearFiles} autoRecord />Also applies to: 265-265, 266-266, 267-267, 268-268, 269-269, 270-270, 271-271, 272-272
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/components/Files/CameraCaptureDialog.tsx(5 hunks)src/hooks/useFileUpload.tsx(3 hunks)
🔇 Additional comments (6)
src/components/Files/CameraCaptureDialog.tsx (4)
18-18: LGTM: Appropriate prop addition for reset functionality
The new onResetCapture prop is correctly typed and mandatory, which aligns with its essential role in fixing the unnecessary file saving issue.
22-22: LGTM: Clean props destructuring
The onResetCapture prop is correctly destructured alongside other props.
149-151: LGTM: Correct implementation of reset on retake
The reset functionality is properly implemented in the retake button handler.
212-214: LGTM: Consistent implementation with mobile retake
The reset functionality is properly implemented and consistent with the mobile version.
src/hooks/useFileUpload.tsx (2)
252-255: LGTM! Implementation addresses the core issue.
The clearFiles function correctly resets both the files and file names states, effectively preventing unnecessary file saves when retaking photos.
317-317: Verify usage of clearFiles across the codebase.
The clearFiles function is now properly exposed in the hook's return object. Let's verify its usage across the codebase to ensure it's being used appropriately in all necessary places.
✅ Verification successful
The clearFiles function is properly implemented and exposed
The verification shows that:
- The
clearFilesfunction is correctly implemented inuseFileUploadhook, resetting both files and upload file names - It's properly exposed in the hook's return interface
- It's being used appropriately in the hook's implementation for resetting files (e.g., with
onResetCapture) - The hook is imported and used in two components:
PatientConsentRecordsandFileUpload - No other manual file reset patterns were found that should be replaced with
clearFiles
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of useFileUpload hook and verify clearFiles implementation
# Find files that import useFileUpload
echo "Files importing useFileUpload:"
rg -l "useFileUpload"
# Find potential usage of clearFiles from the hook
echo "\nPotential clearFiles usage:"
rg "clearFiles" --type tsx --type ts
# Find other potential file reset patterns that might need to use clearFiles
echo "\nOther potential reset patterns:"
rg -A 2 "setFiles\(\[\]\)" --type tsx --type ts
Length of output: 494
Script:
#!/bin/bash
# Let's try without file type flags since they're not recognized
# First, let's check the implementation and interface of clearFiles
echo "Implementation and interface of clearFiles:"
rg -A 5 "clearFiles" src/hooks/useFileUpload.tsx
# Find all usages of useFileUpload and surrounding context
echo "\nUseFileUpload hook usages:"
rg -B 2 -A 5 "useFileUpload" src/components/Patient/PatientConsentRecords.tsx src/components/Files/FileUpload.tsx
# Check for file reset patterns without type flags
echo "\nFile reset patterns:"
rg -A 2 "setFiles\(\[\]\)"
Length of output: 3299
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/components/Files/AudioCaptureDialog.tsx (2)
62-65: Consider releasing microphone resources regardless of statusWhile the implementation works, consider moving the
releaseMic()call outside the status check to ensure microphone resources are always released, even in edge cases.const handleStopRecording = () => { + releaseMic(); if (status !== "RECORDING") return; - releaseMic(); setStatus("RECORDED"); stopRecording(); timer.stop(); };
107-115: Optimize the microphone release implementationThe implementation can be simplified while maintaining the same cleanup effectiveness.
const releaseMic = () => { if (mediaStreamRef.current) { - mediaStreamRef.current.getTracks().forEach((track) => { - track.stop(); - mediaStreamRef.current?.removeTrack(track); - }); + mediaStreamRef.current.getTracks().forEach(track => track.stop()); mediaStreamRef.current = null; } };The
removeTrackcall is unnecessary since we're stopping the tracks and nullifying the reference. This change makes the code more concise while maintaining the same cleanup effectiveness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/components/Files/AudioCaptureDialog.tsx(5 hunks)src/components/Files/CameraCaptureDialog.tsx(5 hunks)src/hooks/useFileUpload.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Files/CameraCaptureDialog.tsx
- src/hooks/useFileUpload.tsx
🔇 Additional comments (2)
src/components/Files/AudioCaptureDialog.tsx (2)
29-29: LGTM: Good practice for managing MediaStream lifecycle
The addition of mediaStreamRef using useRef is a proper way to maintain the MediaStream reference across component renders.
46-47: LGTM: Proper stream management implementation
Storing the MediaStream in the ref immediately after obtaining it ensures proper resource management.
|
LGTM |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/components/Files/AudioCaptureDialog.tsx (2)
111-116: Consider separating cleanup into its own effect.While the cleanup logic is correct, it might be better to separate it into a dedicated useEffect for better separation of concerns. The current effect handles both autoRecord initialization and cleanup.
Consider this refactor:
useEffect(() => { if (autoRecord && show && status === "RECORDING") { handleStartRecording(); } - - return () => { - if (mediaStreamRef.current) { - mediaStreamRef.current.getTracks().forEach((track) => track.stop()); - mediaStreamRef.current = null; - } - }; }, [autoRecord, status, show]); + + useEffect(() => { + return () => { + if (mediaStreamRef.current) { + mediaStreamRef.current.getTracks().forEach((track) => track.stop()); + mediaStreamRef.current = null; + } + }; + }, []);
Line range hint
134-134: Address the TODO comment about browser support.The comment indicates a need to find a better link that supports all browsers. This should be addressed to ensure proper cross-browser compatibility.
Would you like me to help research and suggest a more universal browser settings URL pattern?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Files/AudioCaptureDialog.tsx(4 hunks)
🔇 Additional comments (2)
src/components/Files/AudioCaptureDialog.tsx (2)
29-29: LGTM: Good practice for managing media stream resources.
The addition of mediaStreamRef using useRef is a proper way to maintain a reference to the MediaStream across renders.
46-47: LGTM: Proper stream management implementation.
The media stream is correctly stored in the ref before starting the recording, enabling proper cleanup later.
|
👋 Hi, @JavidSumra, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
rithviknishad
removed
changes required
merge conflict
|
@JavidSumra Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
Proposed Changes
clearFilesfunction to reset file states (setFiles([])andsetUploadFileNames([])) when "Retake or Close" is clicked.@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit