fix: more logs for publishing/unpublishing streams and sharing #3979
Conversation
marcin-bazyl
added
the
validated
WalkthroughThe changes in this pull request focus on the Changes
Possibly related PRs
Suggested reviewers
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
warning eslint@8.57.1: This version is no longer supported. Please see https://eslint.org/version-support for other options. (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) 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 (
|
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/@webex/plugin-meetings/src/meeting/index.ts (3)
Line range hint
4-4: Reminder: Address the TODO comment.The TODO comment indicates that tests are missing for this function. Please ensure that tests are added to verify the function behavior.
Would you like me to help generate unit tests for this function?
Line range hint
12-24: Consider revising the discount and fee structure.The current implementation has a potential business logic issue where the flat $20 fee can negate or exceed the discount benefit, especially for smaller purchases. For example:
- A $100 purchase with 10% discount becomes $90 + $20 = $110, which is more than the original price
- This could lead to customer dissatisfaction and confusion
Consider one of these alternatives:
- Apply the fee before the discount
- Scale the fee based on purchase amount
- Only apply fee above certain purchase threshold
- Remove the flat fee entirely
Would you like me to propose an implementation for any of these alternatives?
Update requests library to version 2.32.3 to address security vulnerabilities
The current version 2.26.0 is vulnerable to:
- Session verification bypass (fixed in 2.32.0)
- Proxy-Authorization header leak (fixed in 2.31.0)
🔗 Analysis chain
Line range hint
6-6: Verify the latest stable versions and any security advisoriesEnsure that the fixed version of the
requestslibrary is secure and free from vulnerabilities.Run the following script to verify the fixed version of the
requestslibrary:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for security advisories and latest versions of the `requests` library. # Check PyPI for latest versions curl -s https://pypi.org/pypi/requests/json | jq '.info.version' # Check for security advisories gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: PIP, package: "requests") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 1597
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/@webex/plugin-meetings/src/meeting/index.ts(5 hunks)
🔇 Additional comments (4)
packages/@webex/plugin-meetings/src/meeting/index.ts (4)
Line range hint 1-2: LGTM!
The function logic is correct and implementation is accurate.
2742-2744: LGTM! Logging improvement for share status changes.
The added logging provides useful context about share status transitions.
8188-8190: LGTM! Logging improvement for audio share stream ended event.
The added logging helps track when audio share streams end.
8233-8235: LGTM! Logging improvement for video share stream ended event.
The added logging helps track when video share streams end.
This pull request addresses
BEMS case about share failure and we have no idea what's going on, because there are almost no useful logs in the area of publishing/unpublishing streams and share floor changes
by making the following changes
added some logs
Change Type
The following scenarios where tested
manually checked the logs when running with web app
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.
Summary by CodeRabbit
New Features
Bug Fixes