fix(sdk): fixed-join-link-parsing #4114
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a new constant, 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)
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
Documentation and Community
|
|
|
||
| export const JOIN = 'join'; | ||
|
|
||
| export const JOIN_LINK = 'j.php'; |
There was a problem hiding this comment.
Had to add this - no other way to check for the join link...
There was a problem hiding this comment.
Were we able to get to the right team to verify this one?
There was a problem hiding this comment.
yes. we need to confirm we always get j.php? any doc reference?
There was a problem hiding this comment.
| parsedUrl.pathname.includes(`/${JOIN}`)); | ||
| parsedUrl.pathname.includes(`/${JOIN}`) || | ||
| (parsedUrl.pathname.includes(`/${JOIN_LINK}`) && | ||
| parsedUrl.search.includes(JOIN_LINK_MTID))); |
There was a problem hiding this comment.
We will only check if both exists at the same time - hence the &&.
There was a problem hiding this comment.
Again, we need to confirm if MITD is the only type of query param there
There was a problem hiding this comment.
For joinLink this parameter will be there by default. Hence, we only need to check for this, have updated the code and added a comment also.
![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
🧹 Nitpick comments (1)
packages/@webex/plugin-meetings/test/unit/spec/meeting-info/utilv2.js (1)
349-364: Good test coverage, consider adding more edge cases.The test suite effectively validates the core functionality of join link parsing. However, consider adding tests for:
- Links without query parameters
- Links with empty MTID values
- Links with multiple query parameters
- Links with malformed j.php paths
it('should return false for join link without query parameters', () => { const result = MeetingInfoUtil.isMeetingLink('https://cisco.webex.com/cisco/j.php'); expect(result).to.be.false; }); it('should return false for join link with empty MTID', () => { const result = MeetingInfoUtil.isMeetingLink('https://cisco.webex.com/cisco/j.php?MTID='); expect(result).to.be.false; }); it('should return true for join link with additional query params', () => { const result = MeetingInfoUtil.isMeetingLink('https://cisco.webex.com/cisco/j.php?MTID=abc123&other=param'); expect(result).to.be.true; }); it('should return false for malformed j.php path', () => { const result = MeetingInfoUtil.isMeetingLink('https://cisco.webex.com/cisco/j.php.invalid?MTID=abc123'); expect(result).to.be.false; });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/@webex/plugin-meetings/src/constants.ts(1 hunks)packages/@webex/plugin-meetings/src/meeting-info/utilv2.ts(2 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting-info/utilv2.js(1 hunks)
🔇 Additional comments (2)
packages/@webex/plugin-meetings/src/constants.ts (1)
39-40: LGTM! Well-defined constants for join link parsing.The new constants
JOIN_LINKandJOIN_LINK_MTIDare appropriately defined to handle join links with the format 'j.php?MTID=...'.packages/@webex/plugin-meetings/src/meeting-info/utilv2.ts (1)
75-77: LGTM! Robust join link validation logic.The enhanced validation correctly handles both standard meeting links and join links with MTID parameter. The use of
&&ensures that j.php links must have both the correct pathname and MTID parameter.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
|
What was the issue with the |
I am still trying to find the root cause... seems to be something related to the remote media disconnect... |
|
Note |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/@webex/plugin-meetings/src/meeting-info/utilv2.ts (1)
74-75: Consider using optional chaining for better readability and safety.The code change correctly implements the logic to check for both join links in the pathname and MTID in search parameters. However, the boolean check can be improved for better readability and safety.
parsedUrl.pathname.includes(`/${MEET_CISCO}`) || parsedUrl.pathname.includes(`/${MEET_CO}`) || parsedUrl.pathname.includes(`/${JOIN}`) || - (parsedUrl.search && parsedUrl.search.includes(JOIN_LINK_MTID))); + parsedUrl.search?.includes(JOIN_LINK_MTID));Could you add a comment explaining that for
joinLink, the MTID parameter is sufficient for validation? This would help future maintainers understand the rationale behind this check, as indicated in your previous comment.🧰 Tools
🪛 Biome (1.9.4)
[error] 75-75: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/@webex/plugin-meetings/src/meeting-info/utilv2.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/@webex/plugin-meetings/src/meeting-info/utilv2.ts
[error] 75-75: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Packages
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (2)
packages/@webex/plugin-meetings/src/meeting-info/utilv2.ts (2)
22-22: Import addition looks good.The addition of
JOIN_LINK_MTIDto the imports is consistent with the PR objective of enhancing join link parsing.
62-79:✅ Verification successful
Verify that all test cases are covered.
The implementation looks good for validating meeting links. The improved logic now checks for either JOIN in the pathname or MTID in the search parameters, which should handle the various join link formats mentioned in the PR description.
Run the following tests to ensure the changes properly handle all the join link scenarios:
🏁 Script executed:
#!/bin/bash # Look for test cases that verify the updated isMeetingLink functionality echo "Searching for test cases for isMeetingLink..." rg -A 10 "isMeetingLink" --type js packages/@webex/plugin-meetings/test/unit/specLength of output: 2261
Meeting Link Validation Tests Verified
The implementation correctly checks both the hostname and pathname/search conditions, and the existing tests in
packages/@webex/plugin-meetings/test/unit/spec/meeting-info/utilv2.jsadequately cover the key scenarios (valid join meeting links with and without Cisco domain using MTID, as well as invalid links).
- Test cases verify:
- A valid join meeting link with MTID.
- A valid join meeting link without the Cisco-specific domain.
- An invalid meeting link where the query parameter doesn’t match.
No further changes are required.
🧰 Tools
🪛 Biome (1.9.4)
[error] 65-65: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 67-67: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 75-75: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
4 participants
COMPLETES #< SPARK-468090>
This pull request addresses
https://cisco.webex.com/cisco/j.php?MTID=m6dfc73927ea656a2f9b5637c32a2dac5by making the following changes
joinLinkparams in both pathname and search (ie MTID) to validate.Vidcast
https://app.vidcast.io/share/9aa6185b-6d9a-41ee-8e4f-4fa1069d28e3
Change Type
The following scenarios were tested
ciscoand non-cisco domains.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.