-
Notifications
You must be signed in to change notification settings - Fork 386
fix(samples): fix locus 1-1 incoming calls on the samples page #4182
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
Conversation
📝 WalkthroughWalkthroughThis pull request introduces modifications to the meeting management logic in the browser-plugin sample. Key changes include a minor formatting adjustment in the OAuth initialization section. The Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
yarn install v1.22.22 (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 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
Documentation and Community
|
57f55dd to
3014c89
Compare
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/samples/browser-plugin-meetings/app.js (1)
3983-3995: Handle potential errors in the promise chain.If
joinWithMedia()ormeeting.acknowledge('ANSWER', false)fail, there is no.catch()block to handle or log errors. This can mask issues during acceptance. Wrapping these in error handling will improve debuggability and stability.Example approach for adding error handling within the existing chain:
meeting.joinWithMedia({joinOptions, mediaOptions}) .then(() => { doPostMediaSetup(meeting); return meeting.acknowledge('ANSWER', false); }) + .catch(error => { + console.error('Failed to join with media or acknowledge ANSWER:', error); + // Optionally, display error to the user or revert UI state + }) .then(() => { toggleDisplay('incomingsection', false); updateMeetingInfoSection(meeting); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/samples/browser-plugin-meetings/app.js(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test - Integration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/samples/browser-plugin-meetings/app.js(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (1)
docs/samples/browser-plugin-meetings/app.js (1)
172-172: No issues with the added blank line.This additional blank line appears to be a harmless spacing improvement.
|
Is it necessary for the user to perform that additional syncing step? Can the user directly accept the call? under the hood we can do that step so developer can copy that piece of code? As a new user/developer to this feature, I am not aware of this additional step. |
It's a good suggestion @rsarika . I've changed the behavior to what you've asked. Please see the updated vidcast link in the description. |
COMPLETES # NA
This pull request addresses
Failure to accept incoming 1-1 Locus calls on the samples page. The current behavior didn't add the media once the call was answered.
by making the following changes
Added the functionality to the accept button.
Change Type
The following scenarios were tested
Vidcast: https://app.vidcast.io/share/b7425a69-c7b4-4233-9ed9-3ac9e4ce2d23
Did the above test multiple times.
I certified that
I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly
Make sure to have followed the contributing guidelines before submitting.