Australia au-nswrtm1 cluster mobius url hardcoding for validation #4394
Conversation
📝 WalkthroughWalkthroughThe changes update the handling of the Mobius server URL in the calling and registration logic. In 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 7275 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: 2
🔭 Outside diff range comments (1)
packages/calling/src/CallingClient/calling/call.ts (1)
237-244: Hard-coding cluster URL breaks multi-region support and leavesactiveUrlunusedThe previous implementation allowed
mobiusUrlto be supplied through theactiveUrlconstructor parameter. By forcing a single literal:this.mobiusUrl = "https://mobius.au-nswrtm1.a4.prod.infra.webex.com/api/v1/calling/web/";you now:
- Ignore the
activeUrlargument (still present in the ctor signature) – this will raise unused-parameter / no-unused-vars lint errors and mislead future readers.- Lock every call/registration flow to a single NSW cluster, silently breaking all other environments (prod-US/EU, FedRAMP, staging, on-prem, tests, etc.).
- Expose an internal service hostname in OSS code — a potential information-leak and maintenance hazard when the host or path changes.
At minimum, keep the dynamic behaviour and fall back to the hard-coded value only when
activeUrlis falsy:- this.mobiusUrl = "https://mobius.au-nswrtm1.a4.prod.infra.webex.com/api/v1/calling/web/"; + const DEFAULT_MOBIUS_URL = + 'https://mobius.au-nswrtm1.a4.prod.infra.webex.com/api/v1/calling/web/'; + + this.mobiusUrl = activeUrl?.trim() || DEFAULT_MOBIUS_URL;Ideally, lift the constant to
packages/calling/src/CallingClient/constants.ts(or a runtime config/env variable) and remove theactiveUrlparameter if the SDK is no longer supposed to be dynamic.
🧹 Nitpick comments (1)
packages/calling/src/CallingClient/registration/register.ts (1)
136-136: Architectural concern: Validation approach introduces technical debtBoth changes follow the same anti-pattern of hardcoding production URLs for validation purposes. While this might work for immediate validation needs, it:
- Creates technical debt: Will require future code changes to revert
- Bypasses existing architecture: Ignores the configuration and failover systems already in place
- Introduces deployment risk: Easy to accidentally deploy validation code to production
Recommended approach for cluster validation:
- Use environment variables or feature flags
- Implement a validation mode that can be toggled without code changes
- Consider using configuration files that can be updated without redeployment
- Ensure validation code is clearly marked and easily removable
This approach would maintain the flexibility of the existing system while enabling targeted validation.
Also applies to: 641-641
📜 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/registration/register.ts (2)
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: 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.
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.
⏰ 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
| @@ -637,6 +638,7 @@ export class Registration implements IRegistration { | |||
|
|
|||
| return abort; | |||
| } | |||
| servers = ["https://mobius.au-nswrtm1.a4.prod.infra.webex.com/api/v1/calling/web/"]; | |||
There was a problem hiding this comment.
Critical: Hardcoded server override breaks failover logic
This change completely undermines the registration resilience system by forcing a single hardcoded server, which:
- Breaks failover capability: Ignores backup servers and primary server lists
- Defeats load balancing: Forces all traffic to one server
- Reduces reliability: Eliminates redundancy that prevents service outages
- Makes testing impossible: Can't test different server configurations
This change effectively disables the entire multi-server registration strategy. If this is for validation purposes, consider a configuration-based approach:
- servers = ["https://mobius.au-nswrtm1.a4.prod.infra.webex.com/api/v1/calling/web/"];
+ // For au-nswrtm1 cluster validation, consider using environment override
+ // if (process.env.MOBIUS_VALIDATION_MODE === 'au-nswrtm1') {
+ // servers = [process.env.MOBIUS_VALIDATION_URL];
+ // }Alternative: Use feature flags or configuration parameters instead of hardcoding production URLs directly in source code.
📝 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-nswrtm1.a4.prod.infra.webex.com/api/v1/calling/web/"]; | |
| // For au-nswrtm1 cluster validation, consider using environment override | |
| // if (process.env.MOBIUS_VALIDATION_MODE === 'au-nswrtm1') { | |
| // servers = [process.env.MOBIUS_VALIDATION_URL]; | |
| // } |
🤖 Prompt for AI Agents
In packages/calling/src/CallingClient/registration/register.ts at line 641, the
code hardcodes the servers array to a single URL, which breaks the failover,
load balancing, and redundancy mechanisms. To fix this, remove the hardcoded
assignment and instead implement a configuration-based approach or feature flag
that allows selecting the server list dynamically. Ensure the original
multi-server registration logic remains intact and configurable for testing and
production environments.
| @@ -133,6 +133,7 @@ | |||
| } | |||
|
|
|||
| public setActiveMobiusUrl(url: string) { | |||
| url = "https://mobius.au-nswrtm1.a4.prod.infra.webex.com/api/v1/calling/web/" | |||
There was a problem hiding this comment.
Critical: Remove hardcoded URL override - breaks method contract
This change completely ignores the input parameter and hardcodes a production URL, which breaks the method's intended functionality and creates several issues:
- Breaks method contract: Callers expect their
urlparameter to be used - Security concern: Hardcoding production URLs in source code
- Maintainability: Makes the code inflexible and hard to test
- Configuration management: Should use environment variables or configuration files
Consider these alternatives for validation purposes:
public setActiveMobiusUrl(url: string) {
- url = "https://mobius.au-nswrtm1.a4.prod.infra.webex.com/api/v1/calling/web/"
+ // For au-nswrtm1 cluster validation, consider using environment variable
+ // url = process.env.MOBIUS_OVERRIDE_URL || url;
log.info(`${METHOD_START_MESSAGE} with ${url}`, {Or use a configuration-based approach to avoid hardcoding production URLs.
📝 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-nswrtm1.a4.prod.infra.webex.com/api/v1/calling/web/" | |
| public setActiveMobiusUrl(url: string) { | |
| - url = "https://mobius.au-nswrtm1.a4.prod.infra.webex.com/api/v1/calling/web/" | |
| + // For au-nswrtm1 cluster validation, consider using an environment variable | |
| + // url = process.env.MOBIUS_OVERRIDE_URL || url; | |
| log.info(`${METHOD_START_MESSAGE} with ${url}`, { | |
| // … | |
| }); | |
| } |
🤖 Prompt for AI Agents
In packages/calling/src/CallingClient/registration/register.ts at line 136,
remove the hardcoded production URL assignment that overrides the input
parameter. Instead, ensure the method uses the passed-in url parameter as
intended. If validation or defaulting is needed, implement it using environment
variables or configuration files rather than hardcoding, preserving method
contract, security, and maintainability.
|
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.