Australia au-vicrtm1 cluster mobius url hardcoding for validation #4395
Conversation
📝 WalkthroughWalkthroughThe changes update how the Mobius URL is handled in both the 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 ... [truncated 7274 characters] ... [email protected]: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-optional-chaining instead. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/calling/src/CallingClient/calling/call.ts(1 hunks)packages/calling/src/CallingClient/registration/register.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: robstax
PR: webex/webex-js-sdk#3948
File: packages/@webex/webex-core/test/unit/spec/webex-core.js:178-254
Timestamp: 2024-11-01T00:30:53.442Z
Learning: In the webex-js-sdk project, when reviewing changes in `packages/@webex/webex-core/test/unit/spec/webex-core.js`, if the user declines to add negative test cases for invalid interceptor configurations, accept their decision and do not insist.
packages/calling/src/CallingClient/calling/call.ts (1)
Learnt from: sreenara
PR: webex/webex-js-sdk#3904
File: packages/calling/src/CallingClient/constants.ts:59-59
Timestamp: 2024-10-24T10:00:26.858Z
Learning: In `packages/calling/src/CallingClient/constants.ts`, when validating phone numbers using `VALID_PHONE_REGEX`, prefer to keep the permissive regex `/[\d\s()*#+.-]+/`, as the call processing agent will handle any invalid phone numbers. Do not enforce stricter phone number validation in the client code.
packages/calling/src/CallingClient/registration/register.ts (3)
Learnt from: sreenara
PR: webex/webex-js-sdk#3904
File: packages/calling/src/CallingClient/constants.ts:59-59
Timestamp: 2024-10-24T10:00:26.858Z
Learning: In `packages/calling/src/CallingClient/constants.ts`, when validating phone numbers using `VALID_PHONE_REGEX`, prefer to keep the permissive regex `/[\d\s()*#+.-]+/`, as the call processing agent will handle any invalid phone numbers. Do not enforce stricter phone number validation in the client code.
Learnt from: robstax
PR: webex/webex-js-sdk#3948
File: packages/@webex/webex-core/test/unit/spec/webex-core.js:178-254
Timestamp: 2024-11-01T00:30:53.442Z
Learning: In the webex-js-sdk project, when reviewing changes in `packages/@webex/webex-core/test/unit/spec/webex-core.js`, if the user declines to add negative test cases for invalid interceptor configurations, accept their decision and do not insist.
Learnt from: mmulcair1981
PR: webex/webex-js-sdk#3936
File: packages/@webex/internal-plugin-usersub/src/usersub.ts:65-66
Timestamp: 2024-10-24T08:50:23.626Z
Learning: In the `updateAnswerCallsCrossClient` method in the `Usersub` class (`packages/@webex/internal-plugin-usersub/src/usersub.ts`), the `deviceId` can be empty, and the request should not be rejected in this case.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: AWS Amplify Console Web Preview
|
|
||
| return abort; | ||
| } | ||
| servers = ["https://mobius.au-vicrtm1.a5.prod.infra.webex.com/api/v1/calling/web/"]; |
There was a problem hiding this comment.
Remove hardcoded servers override that defeats the method's purpose.
This hardcoded array assignment completely ignores the servers parameter and makes the method's failover logic meaningless since only one server is ever used. This breaks the method's contract and makes testing impossible.
The method is designed to handle multiple servers for failover scenarios, but this change eliminates that capability entirely. If this is for validation testing, consider:
- Using environment-specific configuration
- Adding a validation mode flag
- Creating a dedicated testing endpoint
- servers = ["https://mobius.au-vicrtm1.a5.prod.infra.webex.com/api/v1/calling/web/"];
+ // TODO: Remove hardcoded servers override before production merge
+ // servers = ["https://mobius.au-vicrtm1.a5.prod.infra.webex.com/api/v1/calling/web/"];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| servers = ["https://mobius.au-vicrtm1.a5.prod.infra.webex.com/api/v1/calling/web/"]; | |
| // TODO: Remove hardcoded servers override before production merge | |
| // servers = ["https://mobius.au-vicrtm1.a5.prod.infra.webex.com/api/v1/calling/web/"]; |
🤖 Prompt for AI Agents
In packages/calling/src/CallingClient/registration/register.ts at line 641,
remove the hardcoded assignment of the servers array that overrides the input
parameter. This override disables the method's failover logic by forcing a
single server, breaking its intended functionality. Instead, rely on the
passed-in servers parameter or implement environment-specific configuration or a
validation mode flag if testing with fixed servers is needed.
| } | ||
|
|
||
| public setActiveMobiusUrl(url: string) { | ||
| url = "https://mobius.au-vicrtm1.a5.prod.infra.webex.com/api/v1/calling/web/" |
There was a problem hiding this comment.
Remove hardcoded URL override that breaks method contract.
This hardcoded URL assignment completely ignores the url parameter, making the method signature misleading and breaking its contract. Callers would expect their provided URL to be used.
If this is for temporary validation testing, consider these alternatives:
- Use environment variables or configuration files
- Add a feature flag or debug mode
- Create a separate validation-specific method
- url = "https://mobius.au-vicrtm1.a5.prod.infra.webex.com/api/v1/calling/web/"
+ // TODO: Remove hardcoded URL override before production merge
+ // url = "https://mobius.au-vicrtm1.a5.prod.infra.webex.com/api/v1/calling/web/"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| url = "https://mobius.au-vicrtm1.a5.prod.infra.webex.com/api/v1/calling/web/" | |
| // TODO: Remove hardcoded URL override before production merge | |
| // url = "https://mobius.au-vicrtm1.a5.prod.infra.webex.com/api/v1/calling/web/" |
🤖 Prompt for AI Agents
In packages/calling/src/CallingClient/registration/register.ts at line 136,
remove the hardcoded URL assignment that overrides the method's url parameter.
Instead, ensure the method uses the url parameter passed to it, preserving the
method contract. If a specific URL is needed for testing, implement it via
environment variables, configuration, feature flags, or a separate validation
method rather than hardcoding it here.
| this.localRoapMessage = {} as RoapMessage; | ||
|
|
||
| this.mobiusUrl = activeUrl; | ||
| this.mobiusUrl = "https://mobius.au-vicrtm1.a5.prod.infra.webex.com/api/v1/calling/web/"; |
There was a problem hiding this comment.
Avoid hardcoding URLs - use configuration-based approach instead.
Hardcoding the Mobius URL to a specific cluster endpoint violates configuration management principles and creates several issues:
- Makes the code inflexible across different environments
- Ignores the
activeUrlparameter passed to the constructor, causing confusion - Couples the code to a specific infrastructure endpoint (
au-vicrtm1) - Could lead to deployment issues if this code reaches other environments
For validation testing, consider:
- Using environment variables or configuration files
- Adding a feature flag to conditionally override the URL
- Implementing this change at the deployment/configuration level rather than in the code
- this.mobiusUrl = "https://mobius.au-vicrtm1.a5.prod.infra.webex.com/api/v1/calling/web/";
+ this.mobiusUrl = process.env.MOBIUS_URL_OVERRIDE || activeUrl;Alternatively, if this is specifically for validation purposes, consider implementing a feature flag mechanism or moving this logic to a configuration layer.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.mobiusUrl = "https://mobius.au-vicrtm1.a5.prod.infra.webex.com/api/v1/calling/web/"; | |
| this.mobiusUrl = process.env.MOBIUS_URL_OVERRIDE || activeUrl; |
🤖 Prompt for AI Agents
In packages/calling/src/CallingClient/calling/call.ts at line 237, avoid
hardcoding the Mobius URL directly in the code. Instead, refactor to use a
configuration-based approach by retrieving the URL from environment variables,
configuration files, or constructor parameters like the existing activeUrl.
Optionally, implement a feature flag to override the URL for validation
purposes. This will make the code flexible across environments and decouple it
from a specific infrastructure endpoint.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
COMPLETES #< INSERT LINK TO ISSUE >
This pull request addresses
< DESCRIBE THE CONTEXT OF THE ISSUE >
by making the following changes
< DESCRIBE YOUR CHANGES >
Change Type
The following scenarios were tested
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
Make sure to have followed the contributing guidelines before submitting.