Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add error handler to video element #368

Merged
merged 6 commits into from
Dec 12, 2024
Merged

Add error handler to video element #368

merged 6 commits into from
Dec 12, 2024

Conversation

ayinloya
Copy link
Collaborator

@ayinloya ayinloya commented Dec 3, 2024

Handle video abort error and not report to sentry

@prfectionist
Copy link

prfectionist bot commented Dec 3, 2024

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Code Duplication
The error handling pattern is duplicated across multiple components. Consider extracting the video error handling logic into a shared utility function.

Resource Cleanup
Verify that video and canvas elements are properly cleaned up when errors occur to prevent memory leaks and resource issues

Error Handling
The try-catch block may catch errors that should be handled differently than stream errors. Consider more specific error handling.

@prfectionist
Copy link

prfectionist bot commented Dec 3, 2024

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Best practice
Release media resources when errors occur to prevent camera from staying active

Add cleanup logic to release media resources when error occurs. Call
stream.getTracks().forEach(track => track.stop()) to stop camera access if an error
happens.

packages/web-components/components/document/src/document-capture/DocumentCapture.js [410-414]

 } catch (error) {
   if (error.name !== 'AbortError') {
     console.error(error);
   }
+  if (stream) {
+    stream.getTracks().forEach(track => track.stop());
+  }
 }
Suggestion importance[1-10]: 9

Why: This is a critical improvement that prevents memory leaks and ensures proper cleanup of camera resources when errors occur. Not releasing media tracks can lead to the camera staying active unnecessarily.

9
Enhancement
Simplify asynchronous code flow using async/await pattern

Consider using async/await pattern instead of promise chaining for better error
handling and code readability.

packages/smart-camera-web/smart-camera-web.js [1602-1613]

-navigator.mediaDevices
-  .getUserMedia({ audio: false, video: true })
-  .then((stream) => {
-    try {
-      this.handleSuccess(stream);
-    } catch (error) {
-      this.handleError(error);
-    }
-  })
-  .catch((e) => {
-    this.handleError(e);
-  });
+try {
+  const stream = await navigator.mediaDevices.getUserMedia({ audio: false, video: true });
+  await this.handleSuccess(stream);
+} catch (error) {
+  this.handleError(error);
+}
Suggestion importance[1-10]: 5

Why: While the suggestion would improve code readability, the current promise-based implementation is functionally correct and the change is primarily stylistic. The error handling is already properly implemented in both versions.

5

@ayinloya ayinloya requested a review from beaglebets December 10, 2024 15:57
Copy link
Contributor

@solnsubuga solnsubuga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!

@ayinloya ayinloya merged commit 94b1bac into main Dec 12, 2024
11 checks passed
@ayinloya ayinloya deleted the catch-video-error branch December 12, 2024 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants