fix(plugin-meetings): saving subnet ip details in reachability #4300
Conversation
📝 WalkthroughWalkthroughThis change modifies the subnet reachability tracking in 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. 🔧 ast-grep (0.38.1)packages/@webex/plugin-meetings/test/unit/spec/locus-info/index.js🔧 ESLint
yarn install v1.22.22 (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) ✨ 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: 0
🔭 Outside diff range comments (1)
packages/@webex/plugin-meetings/src/reachability/index.ts (1)
166-179:⚠️ Potential issueFix logging of subnet objects to avoid "[object Object]" output.
The subnet is now an object, but line 174 still logs it directly which will result in "[object Object]" being displayed. Consider logging the subnet details properly.
Apply this diff to fix the logging:
- logMessage += `${subnet}`; + logMessage += `${subnet.serverIp}:${subnet.port || 'default'} (${subnet.protocol})`;
🧹 Nitpick comments (3)
packages/@webex/plugin-meetings/src/reachability/index.ts (1)
463-489: Consider simplifying the null-safe access pattern.The current implementation correctly handles undefined
clusterReachability, but the code could be more concise.Apply this diff for a cleaner implementation:
- const clusterReachability = this.clusterReachability[clusterKey]; - const subnets = clusterReachability - ? Array.from(clusterReachability.reachedSubnets || []) - : []; + const subnets = Array.from( + this.clusterReachability[clusterKey]?.reachedSubnets || [] + );packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts (2)
52-58: Consider removing the redundantreachablefield or making it a boolean.The
reachablefield is always set to'true'(as a string) in the code. Since items are only added toreachedSubnetswhen they are reachable, this field appears redundant.Either remove the field entirely:
public readonly reachedSubnets: Set<{ protocol: string; serverIp: string; - reachable: string; latency: number; port?: number; }> = new Set();Or make it a boolean for type safety:
public readonly reachedSubnets: Set<{ protocol: string; serverIp: string; - reachable: string; + reachable: boolean; latency: number; port?: number; }> = new Set();
300-315: Good implementation of duplicate prevention!The duplicate checking logic correctly prevents adding multiple entries with the same serverIp, protocol, and port combination.
Consider renaming the
latencyfield tolatencyInMillisecondsfor consistency with the rest of the codebase.this.reachedSubnets.add({ protocol, serverIp, reachable: 'true', - latency, + latencyInMilliseconds: latency, port, });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts(4 hunks)packages/@webex/plugin-meetings/src/reachability/index.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: AWS Amplify Console Web Preview
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
…na/webex-js-sdk into subnet_details_nwc
marcin-bazyl
left a comment
marcin-bazyl
left a comment
There was a problem hiding this comment.
This is not the right approach, we should be storing the reached server IP and port in reachability results that we store in boundedStorage as REACHABILITY.localStorageResult and make sure that getReachabilityResults() returns them in the format as described on Orpheus wiki. We will need to also take into account that server IPs might be IPV6.
| public readonly reachedSubnets: Set<{ | ||
| protocol: string; | ||
| serverIp: string; | ||
| reachable: string; |
There was a problem hiding this comment.
why do we need reachable here? the name of this set is reachedSubnets so they're all always reachable.
There was a problem hiding this comment.
This is subnet level reachability result. There might be chance in the cluster, one subnet fails and other(s) success. This reachedSubnets is list of all subnets in the cluster. Subnet level details include individual subnet IP(serverIp), its result, time for this subnet, etc.
| protocol: string; | ||
| serverIp: string; | ||
| reachable: string; | ||
| latency: number; |
There was a problem hiding this comment.
latency is already stored in reachability results, why do we need it here?
| if (serverIp) { | ||
| this.reachedSubnets.add(serverIp); | ||
| // Checking if the serverIp and port combination already exists in the reachedSubnets | ||
| const isDuplicate = Array.from(this.reachedSubnets).some( |
There was a problem hiding this comment.
this feels wrong... we have reachedSubnets defined as a Set so that it's fast and efficient to look up entries in it and check for duplicates, converting it to an array to search for entries doesn't feel right.... I guess you've done this because you only want to check the ip, port and protocol and not latency, best I see no reason why we have latency in this set in the first place...
There was a problem hiding this comment.
This Latency is for individual subnet. And I'll try to avoid using array as Rabbit scan suggesting, I'll use Map.
| results = mapValues(allClusterResults, (clusterResult, clusterKey) => { | ||
| const clusterReachability = this.clusterReachability[clusterKey]; | ||
| const subnets = clusterReachability | ||
| ? Array.from(clusterReachability.reachedSubnets || []) |
There was a problem hiding this comment.
this is not ideal, because reachability results are stored in localStorage, but reachedSubnets is not, so in theory we might want to use getReachabilityResults() without doing reachability and relying on the results from localStorage. We should be storing the reached server ips/ports in reachability results themselves, not using the ones from allClusterResults[..].reachedSubnets - Kacper added them there because he only needed it for sending the metric at the end of reachability, but you guys need to send this to Orpheus together with reachabiltiy results, so it needs to be stored with reachability results.
Also, the returned data from getReachabilityResults() doesn't match now what it was supposed to match - described in this wiki and once you make it match, you need to update the ReachabilityResultsForBackend type definition too.
…required fields for orpheous
…ed merge conflicts
There was a problem hiding this comment.
Actionable comments posted: 22
🔭 Outside diff range comments (1)
packages/@webex/plugin-meetings/src/locus-info/index.ts (1)
366-370:⚠️ Potential issueFix potential null reference and clarify self parameter handling.
There are two issues in this code segment:
The method signature shows
self: anyas a required parameter, but lines 367-368 have a conditional checkif (self && !isEqual(this.self, self)), suggestingselfcould be null/undefined. Ifselfcan be null, update the signature toself?: any.Line 370 uses
this.selfas the previous state reference instead ofthis.parsedLocus.self. This could cause issues ifthis.selfis not properly initialized or synchronized. Consider usingthis.parsedLocus.selffor consistency with other parts of the codebase.- updateSelf(self: any, participants: Array<any>) { + updateSelf(self?: any, participants: Array<any>) { // @ts-ignore - check where this.self come from if (self && !isEqual(this.self, self)) { // @ts-ignore - const parsedSelves = SelfUtils.getSelves(this.self, self, this.webex.internal.device.url); + const parsedSelves = SelfUtils.getSelves(this.parsedLocus.self || this.self, self, this.webex.internal.device.url);
♻️ Duplicate comments (1)
packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts (1)
309-338:⚠️ Potential issueOptimize subnet lookup by using a Map instead of converting Set to Array
Converting a Set to an Array for searching defeats the purpose of using a Set. This operation has O(n) complexity for each lookup.
Consider using a Map with a composite key or a more efficient data structure:
-public reachedSubnets: Set<{ +public reachedSubnets: Map<string, { serverIps: string; port: number; protocol?: string; reachable?: string; 'answered-tx'?: number; 'lost-tx'?: number; latencies: number[]; -}> = new Set(); +}> = new Map(); // In saveResult method: -const subnetIndex = Array.from(this.reachedSubnets).findIndex( - (subnet) => - subnet.serverIps === serverIp && subnet.protocol === protocol && subnet.port === port -); - -if (subnetIndex !== -1) { - const subnet = Array.from(this.reachedSubnets)[subnetIndex]; +const key = `${serverIp}:${port}:${protocol}`; +const subnet = this.reachedSubnets.get(key); + +if (subnet) { subnet.reachable = 'true'; // ... rest of the update logic } else { - this.reachedSubnets.add({ + this.reachedSubnets.set(key, { serverIps: serverIp, // ... rest of the object }); }
🧹 Nitpick comments (17)
cspell.json (1)
227-255: Suggest sorting and deduplicating thewordslist
The new block at the end of thewordsarray is currently unsorted and may include duplicates (e.g., multiple variants of"messageid"). Alphabetical ordering and removal of duplicate entries will make this list easier to manage and review in the future.packages/@webex/plugin-meetings/src/member/util.ts (3)
111-131: Refactor to use optional chaining and enforce boolean.Simplify patterns like
participant && participant.propto!!participant?.prop, improving readability:-MemberUtil.isUser = (participant: any) => participant && participant.type === _USER_; +MemberUtil.isUser = (participant: any): boolean => + participant?.type === _USER_;Apply similar changes to
isModerator,isGuest,isDevice,isModeratorAssignmentProhibited, andisPresenterAssignmentProhibited.🧰 Tools
🪛 Biome (1.9.4)
[error] 113-113: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 119-119: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 128-128: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 131-131: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
150-156: Consider optional chaining for nested checks.You can simplify and guard against missing arrays:
-MemberUtil.isAssociatedSame = (participant: any, id: string) => - participant && - participant.associatedUsers && - participant.associatedUsers.some( - (user) => user.id === id || (user.person && user.person.id === id) - ); +MemberUtil.isAssociatedSame = (participant: any, id: string): boolean => + participant?.associatedUsers?.some( + (user) => user.id === id || user.person?.id === id + ) || false;🧰 Tools
🪛 Biome (1.9.4)
[error] 151-155: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
259-276: EnsureisMutedreturns a boolean.The current implementation may return
undefined. Consider normalizing:MemberUtil.isMuted = (participant: any, statusAccessor: string, controlsAccessor: string): boolean => { - const remoteMute = participant?.controls?.[controlsAccessor]?.muted; + const remoteMute = !!participant?.controls?.[controlsAccessor]?.muted; if (remoteMute) { return true; } - const localStatus = participant?.status?.[statusAccessor]; + const localStatus = participant?.status?.[statusAccessor]; if (localStatus === _RECEIVE_ONLY_) { return true; } if (localStatus === _SEND_RECEIVE_) { return false; } - return remoteMute; + return remoteMute; };This ensures consistency in boolean returns.
packages/@webex/plugin-cc/src/services/core/Utils.ts (1)
51-51: Potential redundant fallback in error handling.The error object creation now includes a fallback (
?? \Error while performing ${methodName}`), butreason` already has the same fallback on line 37. This creates redundant logic that might be unnecessary.Consider simplifying to avoid redundancy:
- error: new Error(reason ?? `Error while performing ${methodName}`), + error: new Error(reason),Since
reasonalready has the fallback logic, the additional nullish coalescing is redundant.packages/@webex/plugin-cc/typedoc.md (1)
78-78: Fix style and grammar issues in documentation.Static analysis identified two issues in this line:
- "Agent based operations" should be hyphenated as "Agent-based operations"
- Consider using stronger wording than "if you want to"
-- [`Contact Center`](./classes/ContactCenter.html) - Click here if you want to learn more about `Agent based operations` such as station login, user state management, outdial, and related functionalities. +- [`Contact Center`](./classes/ContactCenter.html) - Click here to learn more about `Agent-based operations` such as station login, user state management, outdial, and related functionalities.🧰 Tools
🪛 LanguageTool
[style] ~78-~78: Consider using an alternative to strengthen your wording.
Context: ...ContactCenter.html) - Click here if you want to learn more about `Agent based operat...(WANT_KEEN)
[uncategorized] ~78-~78: This expression is usually spelled with a hyphen.
Context: ...ck here if you want to learn more aboutAgent based operationssuch as station login, user...(BASED_HYPHEN)
packages/@webex/plugin-meetings/src/reachability/reachability.types.ts (1)
12-21: Comprehensive subnet details type definition.The
SubnetDetailstype provides thorough tracking of per-subnet reachability metrics including protocol, transmission statistics, latency arrays, and port information. This supports granular analysis of network connectivity.Consider adding JSDoc comments to document the purpose of transmission count properties (
answered-tx,lost-tx) for better developer understanding.+/** + * Detailed reachability information for a specific subnet IP address + */ export type SubnetDetails = { protocol?: 'udp' | 'tcp' | 'xtls'; // Protocol used for the test serverIps: string; // IP address of the subnet being tested reachable: true | false; // Result for this specific address - 'answered-tx'?: number; // 1 if reachable, 0 otherwise - 'lost-tx'?: number; // 1 if unreachable, 0 otherwise + 'answered-tx'?: number; // Number of successful transmissions (1 if reachable, 0 otherwise) + 'lost-tx'?: number; // Number of failed transmissions (1 if unreachable, 0 otherwise) port: number; // Port used for the test latencies?: number[]; };packages/@webex/plugin-meetings/src/locus-info/index.ts (1)
367-370: Consider addressing TypeScript type issues in a follow-up.There are multiple
@ts-ignorecomments in this code that suggest unresolved TypeScript type issues. While not critical for this PR, consider creating a follow-up task to properly type these properties and remove the need for these suppressions.Would you like me to help create proper TypeScript interfaces for the self and participant objects to eliminate these type suppressions?
Also applies to: 1627-1627
packages/@webex/plugin-meetings/test/unit/spec/locus-info/index.js (1)
859-859: Consider using a consistent approach for setting self state in testsThroughout the test file, there's inconsistent usage between direct assignment (
locusInfo.self = ...) and callingupdateSelf. While both approaches work for testing, using a consistent pattern would improve test readability and maintainability.Consider standardizing on one approach, preferably using
updateSelfto better simulate actual usage:-locusInfo.self = selfWithBrbChanged; +locusInfo.updateSelf(selfWithBrbChanged, []);Also applies to: 883-883, 997-997, 1032-1033, 1039-1039, 1050-1050, 1094-1094, 1126-1126, 1148-1148, 1187-1187, 1228-1228, 1253-1253, 1294-1294, 1415-1415, 1434-1434, 1453-1453, 1475-1475, 1497-1497, 1950-1950
packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts (2)
321-324: Document the XTLS domain name replacement logicThe special handling for XTLS where domain names are replaced with resolved IPs could be confusing and prone to bugs. Consider adding a comment explaining why this is necessary.
// Replacing domain name with resolved IP for XTLS if (protocol === 'xtls' && subnet.serverIps.includes('.public.')) { + // XTLS initially uses domain names in serverIps, but we need to store the resolved IP + // for accurate tracking and comparison subnet.serverIps = serverIp; }
536-545: Add null check for edge cases in updateMinLatencyThe method should handle the edge case where
this.result[protocol]might be undefined.private updateMinLatency(protocol: 'udp' | 'tcp' | 'xtls') { - if (this.reachedSubnets) { + if (this.reachedSubnets && this.result[protocol]) { const latencies = Array.from(this.reachedSubnets) .filter((subnet) => subnet.protocol === protocol && subnet.latencies.length > 0) .flatMap((subnet) => subnet.latencies); if (latencies.length > 0) { this.result[protocol].minLatency = Math.min(...latencies); } } }packages/@webex/webex-core/test/unit/spec/services-v2/service-url.js (1)
71-71: Fix typo in test description.- describe('#defautUrl', () => { + describe('#defaultUrl', () => {packages/@webex/webex-core/test/unit/spec/services-v2/service-registry.js (1)
641-641: Fix typo in comment.- it('should reset all hosts when called withour a filter', () => { + it('should reset all hosts when called without a filter', () => {packages/@webex/webex-core/test/unit/spec/services-v2/interceptors/service.js (1)
8-8: Remove unused CONFIG import.The CONFIG import is not used anywhere in this test file.
import {ServiceInterceptor} from '@webex/webex-core'; -import CONFIG from '../../../../../src/config';packages/@webex/webex-core/test/integration/spec/services-v2/services-v2.js (1)
237-237: Remove or fix commented assertionCommented code should be removed or fixed if it's meant to be active.
-// assert.equal(serviceFound.url, catalog.get('exampleValid', true));packages/@webex/webex-core/src/lib/services-v2/service-registry.js (1)
446-446: Inconsistent error message prefixThe error message uses "service-catalogs:" prefix but this is the ServiceRegistry class.
-throw new Error(`service-catalogs: '${catalog}' is not a valid catalog`); +throw new Error(`service-registry: '${catalog}' is not a valid catalog`);packages/@webex/webex-core/src/lib/services-v2/services-v2.js (1)
710-714: Consider more explicit error handling.The try-catch block silently returns from the forEach iteration when URL parsing fails. Consider logging a warning to help with debugging.
let host; try { host = new URL(serviceUrl).host; } catch (e) { + this.logger.warn(`services: invalid service URL for ${serviceName}: ${serviceUrl}`); return; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (83)
cspell.json(2 hunks)docs/assets/css/main.css(0 hunks)docs/index.html(5 hunks)docs/labs/contact-center/README.md(0 hunks)docs/labs/contact-center/auth.js(0 hunks)docs/labs/contact-center/cleanup.js(0 hunks)docs/labs/contact-center/index.html(0 hunks)docs/labs/contact-center/index.js(0 hunks)docs/labs/contact-center/registration.js(0 hunks)docs/labs/contact-center/state-change.js(0 hunks)docs/labs/contact-center/station-login.js(0 hunks)docs/labs/contact-center/styles.css(0 hunks)docs/labs/contact-center/task-consult.js(0 hunks)docs/labs/contact-center/task-manager.js(0 hunks)docs/samples/contact-center/app.js(0 hunks)docs/samples/contact-center/index.html(0 hunks)packages/@webex/http-core/src/interceptors/http-status.js(0 hunks)packages/@webex/http-core/test/unit/spec/interceptors/http-status.js(0 hunks)packages/@webex/internal-plugin-encryption/package.json(1 hunks)packages/@webex/media-helpers/package.json(1 hunks)packages/@webex/plugin-cc/src/cc.ts(14 hunks)packages/@webex/plugin-cc/src/services/core/Utils.ts(2 hunks)packages/@webex/plugin-cc/src/services/task/index.ts(15 hunks)packages/@webex/plugin-cc/src/types.ts(7 hunks)packages/@webex/plugin-cc/test/unit/spec/cc.ts(49 hunks)packages/@webex/plugin-cc/test/unit/spec/services/core/Utils.ts(10 hunks)packages/@webex/plugin-cc/typedoc.md(1 hunks)packages/@webex/plugin-meetings/package.json(1 hunks)packages/@webex/plugin-meetings/src/breakouts/index.ts(0 hunks)packages/@webex/plugin-meetings/src/config.ts(0 hunks)packages/@webex/plugin-meetings/src/constants.ts(0 hunks)packages/@webex/plugin-meetings/src/locus-info/index.ts(7 hunks)packages/@webex/plugin-meetings/src/locus-info/selfUtils.ts(6 hunks)packages/@webex/plugin-meetings/src/media/index.ts(0 hunks)packages/@webex/plugin-meetings/src/meeting-info/meeting-info-v2.ts(2 hunks)packages/@webex/plugin-meetings/src/meeting-info/utilv2.ts(0 hunks)packages/@webex/plugin-meetings/src/meeting/index.ts(1 hunks)packages/@webex/plugin-meetings/src/meetings/index.ts(0 hunks)packages/@webex/plugin-meetings/src/member/index.ts(2 hunks)packages/@webex/plugin-meetings/src/member/util.ts(1 hunks)packages/@webex/plugin-meetings/src/members/index.ts(0 hunks)packages/@webex/plugin-meetings/src/members/request.ts(0 hunks)packages/@webex/plugin-meetings/src/members/util.ts(1 hunks)packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts(9 hunks)packages/@webex/plugin-meetings/src/reachability/index.ts(2 hunks)packages/@webex/plugin-meetings/src/reachability/reachability.types.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/breakouts/index.ts(19 hunks)packages/@webex/plugin-meetings/test/unit/spec/locus-info/index.js(35 hunks)packages/@webex/plugin-meetings/test/unit/spec/locus-info/selfUtils.js(8 hunks)packages/@webex/plugin-meetings/test/unit/spec/media/index.ts(3 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting-info/meetinginfov2.js(0 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting-info/utilv2.js(0 hunks)packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js(0 hunks)packages/@webex/plugin-meetings/test/unit/spec/meetings/index.js(0 hunks)packages/@webex/plugin-meetings/test/unit/spec/members/index.js(0 hunks)packages/@webex/plugin-meetings/test/unit/spec/members/request.js(0 hunks)packages/@webex/plugin-meetings/test/unit/spec/members/utils.js(0 hunks)packages/@webex/webex-core/src/index.js(1 hunks)packages/@webex/webex-core/src/interceptors/redirect.js(0 hunks)packages/@webex/webex-core/src/lib/services-v2/index.js(1 hunks)packages/@webex/webex-core/src/lib/services-v2/service-catalog.js(16 hunks)packages/@webex/webex-core/src/lib/services-v2/service-detail.ts(0 hunks)packages/@webex/webex-core/src/lib/services-v2/service-host.js(1 hunks)packages/@webex/webex-core/src/lib/services-v2/service-registry.js(1 hunks)packages/@webex/webex-core/src/lib/services-v2/service-state.js(1 hunks)packages/@webex/webex-core/src/lib/services-v2/service-url.js(1 hunks)packages/@webex/webex-core/src/lib/services-v2/services-v2.js(10 hunks)packages/@webex/webex-core/src/lib/services-v2/types.ts(0 hunks)packages/@webex/webex-core/test/fixtures/host-catalog-v2.js(1 hunks)packages/@webex/webex-core/test/integration/spec/services-v2/service-catalog.js(1 hunks)packages/@webex/webex-core/test/integration/spec/services-v2/services-v2.js(1 hunks)packages/@webex/webex-core/test/unit/spec/interceptors/redirect.js(0 hunks)packages/@webex/webex-core/test/unit/spec/services-v2/interceptors/hostmap.js(1 hunks)packages/@webex/webex-core/test/unit/spec/services-v2/interceptors/server-error.js(1 hunks)packages/@webex/webex-core/test/unit/spec/services-v2/interceptors/service.js(1 hunks)packages/@webex/webex-core/test/unit/spec/services-v2/service-catalog.js(1 hunks)packages/@webex/webex-core/test/unit/spec/services-v2/service-detail.ts(0 hunks)packages/@webex/webex-core/test/unit/spec/services-v2/service-host.js(1 hunks)packages/@webex/webex-core/test/unit/spec/services-v2/service-registry.js(1 hunks)packages/@webex/webex-core/test/unit/spec/services-v2/service-state.js(1 hunks)packages/@webex/webex-core/test/unit/spec/services-v2/service-url.js(1 hunks)packages/@webex/webex-core/test/unit/spec/services-v2/services-v2.js(3 hunks)packages/calling/package.json(1 hunks)
💤 Files with no reviewable changes (36)
- packages/@webex/http-core/test/unit/spec/interceptors/http-status.js
- packages/@webex/plugin-meetings/test/unit/spec/members/request.js
- packages/@webex/http-core/src/interceptors/http-status.js
- packages/@webex/plugin-meetings/src/config.ts
- packages/@webex/plugin-meetings/test/unit/spec/meeting/index.js
- packages/@webex/plugin-meetings/src/members/index.ts
- packages/@webex/plugin-meetings/src/media/index.ts
- packages/@webex/plugin-meetings/test/unit/spec/meeting-info/utilv2.js
- docs/samples/contact-center/index.html
- packages/@webex/plugin-meetings/test/unit/spec/members/utils.js
- packages/@webex/webex-core/src/interceptors/redirect.js
- packages/@webex/plugin-meetings/src/meeting-info/utilv2.ts
- packages/@webex/plugin-meetings/test/unit/spec/members/index.js
- packages/@webex/plugin-meetings/src/meetings/index.ts
- packages/@webex/webex-core/src/lib/services-v2/types.ts
- docs/labs/contact-center/auth.js
- packages/@webex/plugin-meetings/test/unit/spec/meeting-info/meetinginfov2.js
- packages/@webex/webex-core/test/unit/spec/interceptors/redirect.js
- packages/@webex/plugin-meetings/src/breakouts/index.ts
- docs/labs/contact-center/styles.css
- docs/labs/contact-center/cleanup.js
- packages/@webex/webex-core/src/lib/services-v2/service-detail.ts
- packages/@webex/plugin-meetings/test/unit/spec/meetings/index.js
- packages/@webex/plugin-meetings/src/constants.ts
- docs/assets/css/main.css
- docs/labs/contact-center/station-login.js
- docs/labs/contact-center/state-change.js
- packages/@webex/plugin-meetings/src/members/request.ts
- docs/labs/contact-center/index.js
- docs/labs/contact-center/index.html
- docs/labs/contact-center/task-consult.js
- packages/@webex/webex-core/test/unit/spec/services-v2/service-detail.ts
- docs/labs/contact-center/task-manager.js
- docs/labs/contact-center/README.md
- docs/samples/contact-center/app.js
- docs/labs/contact-center/registration.js
✅ Files skipped from review due to trivial changes (12)
- packages/@webex/plugin-meetings/package.json
- packages/@webex/plugin-meetings/src/member/index.ts
- packages/@webex/internal-plugin-encryption/package.json
- packages/calling/package.json
- packages/@webex/media-helpers/package.json
- packages/@webex/webex-core/src/lib/services-v2/index.js
- packages/@webex/webex-core/test/fixtures/host-catalog-v2.js
- docs/index.html
- packages/@webex/plugin-meetings/test/unit/spec/media/index.ts
- packages/@webex/plugin-cc/src/services/task/index.ts
- packages/@webex/plugin-cc/src/types.ts
- packages/@webex/plugin-meetings/test/unit/spec/breakouts/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/@webex/plugin-meetings/src/reachability/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (11)
packages/@webex/webex-core/test/unit/spec/services-v2/interceptors/hostmap.js (2)
packages/@webex/test-helper-mock-webex/src/index.js (1)
MockWebex(126-222)packages/@webex/webex-core/src/lib/services-v2/interceptors/hostmap.js (1)
HostMapInterceptor(12-36)
packages/@webex/plugin-meetings/src/members/util.ts (1)
packages/@webex/plugin-meetings/src/constants.ts (1)
VALID_EMAIL_ADDRESS(225-226)
packages/@webex/webex-core/test/unit/spec/services-v2/service-state.js (1)
packages/@webex/webex-core/src/lib/services-v2/service-state.js (1)
ServiceState(15-78)
packages/@webex/webex-core/src/lib/services-v2/service-state.js (1)
packages/@webex/webex-core/src/lib/services-v2/constants.js (1)
SERVICE_CATALOGS(2-2)
packages/@webex/webex-core/test/unit/spec/services-v2/service-host.js (1)
packages/@webex/webex-core/src/lib/services-v2/service-host.js (1)
ServiceHost(30-267)
packages/@webex/webex-core/test/unit/spec/services-v2/service-url.js (2)
packages/@webex/test-helper-mock-webex/src/index.js (1)
MockWebex(126-222)packages/@webex/webex-core/src/lib/services-v2/service-url.js (3)
ServiceUrl(9-121)foundHost(97-97)url(25-25)
packages/@webex/webex-core/test/unit/spec/services-v2/interceptors/server-error.js (3)
packages/@webex/webex-core/test/unit/spec/services-v2/interceptors/service.js (1)
chai(10-10)packages/@webex/webex-core/src/lib/services-v2/interceptors/server-error.js (1)
ServerErrorInterceptor(10-48)packages/@webex/webex-core/src/lib/webex-http-error.js (1)
WebexHttpError(10-60)
packages/@webex/webex-core/test/unit/spec/services-v2/service-registry.js (4)
packages/@webex/webex-core/src/lib/services-v2/service-registry.js (1)
ServiceRegistry(35-465)packages/@webex/webex-core/src/lib/services-v2/constants.js (1)
SERVICE_CATALOGS(2-2)packages/@webex/webex-core/src/lib/services-v2/services-v2.js (2)
hostCatalog(672-672)host(678-678)packages/@webex/webex-core/src/lib/services-v2/service-url.js (2)
filteredHosts(59-61)foundHost(97-97)
packages/@webex/webex-core/src/lib/services-v2/service-registry.js (2)
packages/@webex/webex-core/src/lib/services-v2/constants.js (2)
SERVICE_CATALOGS_ENUM_TYPES(4-7)SERVICE_CATALOGS(2-2)packages/@webex/webex-core/src/lib/services-v2/service-host.js (1)
ServiceHost(30-267)
packages/@webex/plugin-meetings/src/meeting/index.ts (1)
packages/@webex/plugin-meetings/src/reachability/reachability.types.ts (1)
ReachabilityMetrics(35-49)
packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts (1)
packages/@webex/plugin-meetings/src/reachability/request.ts (1)
ClusterNode(5-10)
🪛 Biome (1.9.4)
packages/@webex/webex-core/test/unit/spec/services-v2/service-host.js
[error] 198-198: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 203-203: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 208-208: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 213-213: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 218-218: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 223-223: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/@webex/webex-core/test/unit/spec/services-v2/interceptors/server-error.js
[error] 182-182: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/@webex/webex-core/test/integration/spec/services-v2/services-v2.js
[error] 1061-1085: Excessive describe() nesting detected.
Excessive nesting of describe() calls can hinder test readability.
Consider refactoring and reduce the level of nested describe to improve code clarity.
(lint/complexity/noExcessiveNestedTestSuites)
[error] 1087-1109: Excessive describe() nesting detected.
Excessive nesting of describe() calls can hinder test readability.
Consider refactoring and reduce the level of nested describe to improve code clarity.
(lint/complexity/noExcessiveNestedTestSuites)
[error] 1192-1203: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
packages/@webex/webex-core/src/lib/services-v2/service-registry.js
[error] 77-77: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
packages/@webex/plugin-meetings/src/member/util.ts
[error] 113-113: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 119-119: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 128-128: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 131-131: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 151-155: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 164-165: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 166-168: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 287-287: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 302-302: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 345-345: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 403-403: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 LanguageTool
packages/@webex/plugin-cc/typedoc.md
[style] ~78-~78: Consider using an alternative to strengthen your wording.
Context: ...ContactCenter.html) - Click here if you want to learn more about `Agent based operat...
(WANT_KEEN)
[uncategorized] ~78-~78: This expression is usually spelled with a hyphen.
Context: ...ck here if you want to learn more about Agent based operations such as station login, user...
(BASED_HYPHEN)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (61)
cspell.json (1)
271-271:ignoreWordsformatting is concise and clear
Consolidating the ignore list into a single line improves readability without sacrificing clarity.packages/@webex/plugin-meetings/src/meeting-info/meeting-info-v2.ts (2)
636-636: Formatting change looks good.The registrationId parameter declaration appears to be a minor formatting or consistency update.
687-692:✅ Verification successful
Verify the URI handling simplification doesn't break existing functionality.
The logic has been simplified from presumably more complex URI handling (involving
fullSiteUrl) to a basicdirectURIcheck. While this simplification is good for maintainability, ensure that any edge cases or fallback behavior previously handled by the removed logic are still properly addressed.
🏁 Script executed:
#!/bin/bash # Description: Check for any references to fullSiteUrl in the codebase to understand the scope of this change # Expected: Find references that might indicate where fullSiteUrl was used and if there are other places that need updating echo "Searching for fullSiteUrl references in the codebase..." rg -i "fullsiteurl" --type ts --type js -A 3 -B 3 echo "Searching for disableWebRedirect references..." rg -i "disablewebredirect" --type ts --type js -A 3 -B 3 echo "Searching for usage of fetchMeetingInfo method to check parameter compatibility..." rg "fetchMeetingInfo\(" --type ts --type js -A 5Length of output: 66053
✅ No legacy URI fallback logic detected
I searched the codebase for
fullSiteUrlanddisableWebRedirectreferences and found none. All calls tofetchMeetingInfonow correctly branch ondirectURIversus theWBXAPPAPI_SERVICE/meetingInfofallback, and existing unit tests inmeeting-info-v2.jsand related specs cover:
- Direct-URI paths
- Service/resource fallback
- Error and edge-case scenarios
No further action is needed.
packages/@webex/plugin-meetings/src/member/util.ts (17)
59-61: LGTM.The implementation is concise and defaults to
falsewhen BRB status is not present.
76-92: LGTM.The specialized role checks (
hasCohost,hasModerator,hasPresenter) correctly delegate tohasRolewith afalsefallback.
97-105: LGTM.
extractControlRolesneatly aggregates specific role flags into an external shape.
140-142: LGTM.
isSamecorrectly handles both direct and nestedperson.idcomparisons.
180-198: LGTM.Audio/video mute checks correctly guard against missing participants and delegate to
isMuted.
204-210: LGTM.Properly throws when
participantis undefined and safely returns the raised status.
216-222: LGTM.Correctly validates breakout support and covers undefined participant.
228-236: LGTM.Interpretation support flag is handled consistently with error guarding.
242-250: LGTM.Live annotation support logic is clear and safe.
283-292: LGTM.The method safely checks nested properties and returns recording member or
null.🧰 Tools
🪛 Biome (1.9.4)
[error] 287-287: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
298-307: LGTM.Recording detection correctly throws on undefined participants and returns a boolean.
🧰 Tools
🪛 Biome (1.9.4)
[error] 302-302: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
309-321: LGTM.Removal logic is straightforward and covers all edge cases.
323-338: LGTM.Mutability rules are clearly implemented with proper condition ordering.
344-368: LGTM.Status extraction handles all device and state combinations consistently.
🧰 Tools
🪛 Biome (1.9.4)
[error] 345-345: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
374-380: LGTM.Simple and correct ID extraction with null fallback.
387-396: LGTM.Media status extraction is safe and uses JSDoc for type assurance.
402-408: LGTM.Name extraction is correctly guarded and returns
nullwhen absent.🧰 Tools
🪛 Biome (1.9.4)
[error] 403-403: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/@webex/plugin-cc/test/unit/spec/services/core/Utils.ts (3)
4-4: LGTM - Import addition.The addition of
WebexRequestPayloadimport aligns with its usage in the test code.
37-37: LGTM - Formatting improvements.The formatting changes improve code readability by adding consistent spacing in object literals and improving alignment.
Also applies to: 71-71, 123-123, 195-197, 204-205
1-220:❓ Verification inconclusive
Inconsistency between PR objectives and files under review.
The PR objectives mention fixing subnet IP details in
plugin-meetings, but all files under review are fromplugin-cc(Contact Center plugin). Please verify that the correct files are being reviewed for this PR.
🏁 Script executed:
#!/bin/bash # Check if this PR contains any plugin-meetings files echo "=== Searching for plugin-meetings files in the repository ===" fd -t f . | rg "plugin-meetings" | head -20 echo -e "\n=== Searching for ClusterReachability class mentioned in AI summary ===" ast-grep --pattern 'class ClusterReachability { $$$ }' echo -e "\n=== Searching for reachability-related changes ===" rg -i "reachability|subnet" --type ts -A 3 -B 3Length of output: 83335
🏁 Script executed:
#!/bin/bash # Fetch main to compare against PR branch git fetch origin main --quiet echo "=== plugin-meetings files modified in this PR ===" git diff --name-only origin/main..HEAD | grep "packages/@webex/plugin-meetings" || echo "none" echo -e "\n=== plugin-cc files modified in this PR ===" git diff --name-only origin/main..HEAD | grep "packages/@webex/plugin-cc" || echo "none"Length of output: 864
🏁 Script executed:
#!/bin/bash # Fetch master to compare against PR branch git fetch origin master --quiet echo "=== plugin-meetings files modified in this PR ===" git diff --name-only origin/master...HEAD | grep "packages/@webex/plugin-meetings" || echo "none" echo -e "\n=== plugin-cc files modified in this PR ===" git diff --name-only origin/master...HEAD | grep "packages/@webex/plugin-cc" || echo "none"Length of output: 841
Inconsistent PR scope: plugin-meetings vs. plugin-cc
The PR description refers to updating subnet IPs inplugin-meetings, but the only changed files are underpackages/@webex/plugin-cc(e.g.test/unit/spec/services/core/Utils.ts).Please verify that:
- The intended
plugin-meetingsfiles are included in this PR.- You’re reviewing the correct set of changes against the PR objectives.
packages/@webex/plugin-cc/test/unit/spec/cc.ts (1)
33-33: LGTM - Consistent formatting improvements.The formatting changes improve code readability through consistent spacing, proper object literal formatting, and better alignment of multi-line constructs. The test logic and assertions remain unchanged.
Also applies to: 79-79, 281-284, 568-568, 728-732
packages/@webex/plugin-cc/src/services/core/Utils.ts (1)
2-2: LGTM - Import addition.The import of
WebexRequestPayloadtype is appropriate for the function signatures in this file.packages/@webex/plugin-cc/typedoc.md (1)
3-85: LGTM - Excellent documentation improvements.The documentation rewrite significantly improves the developer experience by:
- Providing clear, concise installation and initialization examples
- Adding a comprehensive configuration reference table
- Focusing on practical usage rather than verbose explanations
- Maintaining a professional, reference-style format
🧰 Tools
🪛 LanguageTool
[style] ~78-~78: Consider using an alternative to strengthen your wording.
Context: ...ContactCenter.html) - Click here if you want to learn more about `Agent based operat...(WANT_KEEN)
[uncategorized] ~78-~78: This expression is usually spelled with a hyphen.
Context: ...ck here if you want to learn more aboutAgent based operationssuch as station login, user...(BASED_HYPHEN)
packages/@webex/plugin-cc/src/cc.ts (8)
46-46: LGTM! Import looks correct.The
getErrorDetailsimport is properly maintained and used throughout the file for error handling.
67-78: Excellent documentation simplification!The streamlined class documentation is much cleaner and focuses on the core capabilities without unnecessary verbosity. The bullet point structure makes it easy to understand the main features.
83-86: Well-structured example code.The simplified example clearly demonstrates the basic workflow (register → stationLogin → setAgentState) which is exactly what developers need to understand the core usage pattern.
91-122: Clean and consistent property documentation.The simplified property comments are much more readable and maintain consistent formatting. Each property's purpose is clearly stated without unnecessary verbosity.
197-206: Good documentation streamlining for register method.The documentation maintains all critical information (return type, throws, usage) while being more concise and focused. The example demonstrates proper usage.
259-272: Clear and improved deregister documentation.The simplified documentation clearly explains the method's purpose while preserving the important note about not performing station signout. The example shows the proper sequence of operations.
524-524: Verify consistency with the AI summary.The AI summary mentions removal of
this.webCallingService.setLoginOptioncalls, but I can see this call still exists at line 524 in thestationLoginmethod. Please verify if the summary accurately reflects the changes made to this specific file.Likely an incorrect or invalid review comment.
347-347: Consistent documentation improvements across all methods.The documentation simplification is well-executed across all methods:
- Examples are more focused and demonstrate core functionality
- Parameter descriptions are concise but complete
- Essential information is preserved while removing verbosity
- Consistent formatting and style throughout
These changes significantly improve the API documentation's readability and usability.
Also applies to: 472-478, 566-566, 638-654, 985-998, 1059-1072, 1120-1125, 1133-1151
packages/@webex/plugin-meetings/src/members/util.ts (1)
113-113: LGTM: Email validation simplified correctly.The change removes SIP address format validation and now only validates email addresses using the
VALID_EMAIL_ADDRESSregex pattern, which aligns with the removal of SIP functionality mentioned in the PR objectives.packages/@webex/plugin-meetings/src/reachability/reachability.types.ts (1)
8-10: LGTM: Well-designed type enhancements for detailed reachability tracking.The addition of
minLatencyanddetailsproperties toTransportResulteffectively supports the PR objective of saving subnet IP details. The optional nature of these properties ensures backward compatibility.packages/@webex/plugin-meetings/src/meeting/index.ts (1)
265-265: LGTM! Clean type simplification.The intersection type syntax is correct and the simplification aligns well with the architectural improvements described in the PR summary. Moving detailed subnet handling to the reachability subsystem while keeping the essential boolean flag here makes the code more maintainable.
packages/@webex/plugin-meetings/test/unit/spec/locus-info/selfUtils.js (3)
7-7: Import looks good.The addition of
_IDLE_and_WAIT_constants is appropriate for the updated test cases that check device intent states.
278-349: Verify the inconsistentgetSelvesmethod signatures.Most calls to
SelfUtils.getSelveshave been updated to use 2 parameters (removing the participants parameter), but line 349 uses 3 parameters withself.devices[0].urlas the third parameter. This inconsistency suggests either:
- The method has different behaviors based on the number of parameters
- There's a missing update in other test cases
Please verify that this is intentional and consider adding a comment to clarify why this specific test uses a different signature.
343-391: Test refactoring looks good.The renaming from
hasUserEnteredLobbytoisUserUnadmittedand the corresponding helper function updates improve semantic clarity. The test cases properly cover the different state transitions for user admission status.packages/@webex/plugin-meetings/src/locus-info/index.ts (3)
384-384: Good improvement for data consistency.Using the passed
participantsparameter instead ofthis.participantsensures the method works with the most current data, preventing potential issues with stale participant information.
267-267: Call sites properly updated.The
updateSelfmethod calls have been correctly updated to include theparticipantsparameter, maintaining consistency with the new method signature.Also applies to: 452-452
1530-1540: Event naming improvements look good.The renaming from
hasUserEnteredLobby/hasUserBeenAdmittedtoisUserUnadmitted/isUserAdmittedimproves clarity and follows boolean naming conventions.packages/@webex/plugin-meetings/src/locus-info/selfUtils.ts (2)
29-74: LGTM! Good simplification of the parse methodThe removal of the
participantsparameter and the simplification ofisUserUnadmittedlogic makes the code cleaner and more maintainable. The direct calculation ofisUserUnadmittedbased on state and intent type is more straightforward.
97-140: LGTM! Clean refactoring of getSelves methodThe removal of participant dependency and the simplified parsing logic improves code clarity. The renamed flags (
isUserUnadmittedandisUserAdmitted) are more descriptive than the previous naming.packages/@webex/webex-core/src/index.js (1)
34-40: LGTM! Export additions support the services-v2 refactor.The new exports for
ServiceRegistryV2,ServiceStateV2,ServiceHostV2, andServiceUrlV2are properly structured and follow the established naming conventions. These additions correctly support the architectural shift fromServiceDetailto the new modular service management classes.packages/@webex/webex-core/test/unit/spec/services-v2/interceptors/hostmap.js (1)
1-79: LGTM! Comprehensive test coverage for HostMapInterceptor.The test suite properly validates the
HostMapInterceptorfunctionality:
- URI replacement when
options.uriis defined- No action when
options.uriis undefined- Error handling when
replaceHostFromHostmapthrows exceptionsThe mocking approach using sinon stubs is appropriate and the test structure follows established patterns in the codebase.
packages/@webex/webex-core/test/unit/spec/services-v2/interceptors/server-error.js (1)
1-204: Excellent test coverage for ServerErrorInterceptor.The test suite comprehensively validates the interceptor's behavior across multiple scenarios:
- Feature flag checking and conditional logic
- Metrics submission when web-high-availability is enabled
- URL marking as failed for different HTTP error codes (500, 503)
- Proper error propagation when feature is disabled
- Edge cases like missing URI
The test structure is well-organized and follows established patterns.
🧰 Tools
🪛 Biome (1.9.4)
[error] 182-182: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/@webex/webex-core/src/lib/services-v2/service-state.js (1)
1-78: LGTM! Well-designed ServiceState class implementation.The
ServiceStateclass is cleanly implemented with:
- Comprehensive JSDoc documentation including proper typedef for
CatalogState- Constructor that properly initializes state for all catalog types from
SERVICE_CATALOGS- Validation checks in
setCollectingandsetReadymethods to prevent errors on invalid catalogs- Static factory method for generating default catalog states
- Clear separation of concerns for managing service catalog states
The implementation follows established patterns and best practices for the Webex core SDK.
packages/@webex/webex-core/src/lib/services-v2/service-url.js (4)
1-16: Class structure and props definition look good.The use of AmpState and the props configuration with appropriate types and defaults is well implemented.
24-32: URL generation logic is correctly implemented.Good handling of the port preservation and the explanatory comment about the
url.hostnamelimitation is helpful.
91-104: Clarify the defaultUrl special case behavior.The method returns
truewhen thedefaultUrlis passed without actually marking anything as failed. This behavior is counterintuitive and could lead to confusion.Consider either:
- Documenting why
defaultUrlis treated specially- Returning
falsefordefaultUrlsince no host is actually marked as failed- Throwing an error if
defaultUrlis passedfailHost(url) { if (url === this.defaultUrl) { - return true; + // Default URL doesn't have a specific host to mark as failed + return false; }
114-120: Clean implementation of the get method.Good separation of concerns by delegating complex logic to
_getPriorityHostUrl.packages/@webex/webex-core/test/unit/spec/services-v2/service-host.js (1)
130-177: Comprehensive setStatus method testing.Excellent coverage of all property combinations and edge cases.
packages/@webex/webex-core/test/unit/spec/services-v2/service-url.js (1)
97-258: Excellent test coverage for ServiceUrl class.The tests thoroughly cover all methods including edge cases, error conditions, and the clusterId filtering logic.
packages/@webex/webex-core/test/unit/spec/services-v2/service-registry.js (1)
1-747: Outstanding test coverage for ServiceRegistry.This test suite provides comprehensive coverage of all ServiceRegistry methods including filtering, state management, and static utilities. The test organization and use of fixtures is exemplary.
packages/@webex/webex-core/test/unit/spec/services-v2/interceptors/service.js (1)
15-194: Well-structured interceptor tests with good async handling.Excellent use of stubs and spies to test the asynchronous behavior of the interceptor. The test coverage for all methods is comprehensive.
packages/@webex/webex-core/test/unit/spec/services-v2/services-v2.js (1)
318-335: Test correctly reflects the API change toupdateServiceUrls.The test has been properly updated to use
catalog.updateServiceUrlsinstead of the deprecatedupdateServiceGroups, aligning with the refactored service catalog implementation.packages/@webex/webex-core/src/lib/services-v2/service-catalog.js (2)
115-115: Good addition of method chaining support.Returning
thisenables fluent API usage, which is a nice improvement to the interface.
391-419: Excellent refactoring of the service update logic.The method now properly updates existing services in place (lines 404-405) rather than replacing the entire list, which is more efficient. The addition of method chaining support (line 418) also improves the API usability.
packages/@webex/webex-core/src/lib/services-v2/services-v2.js (1)
27-29: Important notice about v2 implementation.The comment clearly indicates this is for DNSSec enabled users and may not be backwards compatible. Ensure this is well-documented in release notes and migration guides.
| const MemberUtil: any = {}; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer a strongly typed interface over any for MemberUtil.
Using any erases type safety for the entire utility. Define an interface that describes the shape of MemberUtil or use Record<string, Function> to retain type checking.
🤖 Prompt for AI Agents
In packages/@webex/plugin-meetings/src/member/util.ts at lines 30 to 31, the
MemberUtil object is typed as any, which removes type safety. Replace the any
type with a strongly typed interface that defines the expected properties and
methods of MemberUtil, or use Record<string, Function> if the shape is dynamic,
to ensure proper type checking and maintain code safety.
| MemberUtil.canReclaimHost = (participant) => { | ||
| if (!participant) { | ||
| throw new ParameterError( | ||
| 'canReclaimHostRole could not be processed, participant is undefined.' | ||
| ); | ||
| } | ||
|
|
||
| return participant.canReclaimHostRole || false; | ||
| }; |
There was a problem hiding this comment.
Fix error message to match method name.
The thrown ParameterError references canReclaimHostRole but the method is canReclaimHost. Update the message for clarity:
- throw new ParameterError(
- 'canReclaimHostRole could not be processed, participant is undefined.'
- );
+ throw new ParameterError(
+ 'canReclaimHost could not be processed, participant is undefined.'
+ );📝 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.
| MemberUtil.canReclaimHost = (participant) => { | |
| if (!participant) { | |
| throw new ParameterError( | |
| 'canReclaimHostRole could not be processed, participant is undefined.' | |
| ); | |
| } | |
| return participant.canReclaimHostRole || false; | |
| }; | |
| MemberUtil.canReclaimHost = (participant) => { | |
| if (!participant) { | |
| throw new ParameterError( | |
| 'canReclaimHost could not be processed, participant is undefined.' | |
| ); | |
| } | |
| return participant.canReclaimHostRole || false; | |
| }; |
🤖 Prompt for AI Agents
In packages/@webex/plugin-meetings/src/member/util.ts around lines 36 to 44, the
error message thrown in the ParameterError references 'canReclaimHostRole'
instead of the correct method name 'canReclaimHost'. Update the error message
string to accurately reflect the method name 'canReclaimHost' for clarity and
correctness.
| MemberUtil.getControlsRoles = (participant: ParticipantWithRoles): Array<ServerRoleShape> => | ||
| participant?.controls?.role?.roles; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure getControlsRoles always returns an array.
participant?.controls?.role?.roles can be undefined, leading to downstream errors. Provide a default empty array:
-MemberUtil.getControlsRoles = (participant: ParticipantWithRoles): Array<ServerRoleShape> =>
- participant?.controls?.role?.roles;
+MemberUtil.getControlsRoles = (
+ participant: ParticipantWithRoles
+): Array<ServerRoleShape> =>
+ participant?.controls?.role?.roles || [];🤖 Prompt for AI Agents
In packages/@webex/plugin-meetings/src/member/util.ts around lines 50 to 52, the
getControlsRoles method may return undefined if participant.controls.role.roles
is undefined, causing downstream errors. Modify the return statement to provide
a default empty array when roles is undefined, ensuring the method always
returns an array.
| MemberUtil.hasRole = (participant: any, controlRole: ServerRoles): boolean => | ||
| MemberUtil.getControlsRoles(participant)?.some( | ||
| (role) => role.type === controlRole && role.hasRole | ||
| ); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure hasRole always returns a boolean.
Currently .some(...) may return undefined if roles are undefined. Add a fallback:
-MemberUtil.hasRole = (participant: any, controlRole: ServerRoles): boolean =>
- MemberUtil.getControlsRoles(participant)?.some(
- (role) => role.type === controlRole && role.hasRole
- );
+MemberUtil.hasRole = (
+ participant: any,
+ controlRole: ServerRoles
+): boolean =>
+ MemberUtil.getControlsRoles(participant)?.some(
+ (role) => role.type === controlRole && role.hasRole
+ ) || false;📝 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.
| MemberUtil.hasRole = (participant: any, controlRole: ServerRoles): boolean => | |
| MemberUtil.getControlsRoles(participant)?.some( | |
| (role) => role.type === controlRole && role.hasRole | |
| ); | |
| MemberUtil.hasRole = ( | |
| participant: any, | |
| controlRole: ServerRoles | |
| ): boolean => | |
| MemberUtil.getControlsRoles(participant)?.some( | |
| (role) => role.type === controlRole && role.hasRole | |
| ) || false; |
🤖 Prompt for AI Agents
In packages/@webex/plugin-meetings/src/member/util.ts at lines 67 to 71, the
hasRole function uses .some(...) which can return undefined if getControlsRoles
returns undefined. To fix this, ensure hasRole always returns a boolean by
adding a fallback to false when getControlsRoles(participant) is undefined
before calling .some(...).
| MemberUtil.isNotAdmitted = (participant: any, isGuest: boolean, status: string): boolean => | ||
| participant && | ||
| participant.guest && | ||
| ((participant.devices && | ||
| participant.devices[0] && | ||
| participant.devices[0].intent && | ||
| participant.devices[0].intent.type === _WAIT_ && | ||
| // @ts-ignore | ||
| isGuest && | ||
| status === _IN_LOBBY_) || | ||
| // @ts-ignore | ||
| !status === _IN_MEETING_); |
There was a problem hiding this comment.
Fix logical error in admission check.
The expression !status === _IN_MEETING_ is incorrect (negating status instead of comparing inequality). Update to status !== _IN_MEETING_ and use optional chaining:
-MemberUtil.isNotAdmitted = (participant: any, isGuest: boolean, status: string): boolean =>
- participant &&
- participant.guest &&
- ((participant.devices &&
- participant.devices[0] &&
- participant.devices[0].intent &&
- participant.devices[0].intent.type === _WAIT_ &&
- // @ts-ignore
- isGuest &&
- status === _IN_LOBBY_) ||
- // @ts-ignore
- !status === _IN_MEETING_);
+MemberUtil.isNotAdmitted = (
+ participant: any,
+ isGuest: boolean,
+ status: string
+): boolean =>
+ !!participant &&
+ participant.guest &&
+ ((participant.devices?.[0]?.intent?.type === _WAIT_ && isGuest && status === _IN_LOBBY_) ||
+ status !== _IN_MEETING_);📝 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.
| MemberUtil.isNotAdmitted = (participant: any, isGuest: boolean, status: string): boolean => | |
| participant && | |
| participant.guest && | |
| ((participant.devices && | |
| participant.devices[0] && | |
| participant.devices[0].intent && | |
| participant.devices[0].intent.type === _WAIT_ && | |
| // @ts-ignore | |
| isGuest && | |
| status === _IN_LOBBY_) || | |
| // @ts-ignore | |
| !status === _IN_MEETING_); | |
| MemberUtil.isNotAdmitted = ( | |
| participant: any, | |
| isGuest: boolean, | |
| status: string | |
| ): boolean => | |
| !!participant && | |
| participant.guest && | |
| ((participant.devices?.[0]?.intent?.type === _WAIT_ && isGuest && status === _IN_LOBBY_) || | |
| status !== _IN_MEETING_); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 164-165: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 166-168: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🤖 Prompt for AI Agents
In packages/@webex/plugin-meetings/src/member/util.ts lines 163 to 174, the
condition uses '!status === _IN_MEETING_' which incorrectly negates status
instead of checking inequality. Replace '!status === _IN_MEETING_' with 'status
!== _IN_MEETING_' and apply optional chaining to safely access nested properties
like participant.devices[0].intent.type to avoid runtime errors.
| before('collect remote catalogs', () => | ||
| Promise.all([ | ||
| services._fetchNewServiceHostmap(), | ||
| services._fetchNewServiceHostmap({ | ||
| from: 'limited', | ||
| query: {userId: webexUser.id}, | ||
| }), | ||
| ]).then(([fRHM, lRHM]) => { | ||
| fullRemoteHM = fRHM; | ||
| limitedRemoteHM = lRHM; | ||
| }) | ||
| ); |
There was a problem hiding this comment.
Duplicate before hook will cause test failures
Having duplicate before hooks in the same describe block is not allowed and will cause test failures.
Remove the duplicate before hook since the remote catalogs are already collected in the parent describe block's before hook:
-before('collect remote catalogs', () =>
- Promise.all([
- services._fetchNewServiceHostmap(),
- services._fetchNewServiceHostmap({
- from: 'limited',
- query: {userId: webexUser.id},
- }),
- ]).then(([fRHM, lRHM]) => {
- fullRemoteHM = fRHM;
- limitedRemoteHM = lRHM;
- })
-);📝 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.
| before('collect remote catalogs', () => | |
| Promise.all([ | |
| services._fetchNewServiceHostmap(), | |
| services._fetchNewServiceHostmap({ | |
| from: 'limited', | |
| query: {userId: webexUser.id}, | |
| }), | |
| ]).then(([fRHM, lRHM]) => { | |
| fullRemoteHM = fRHM; | |
| limitedRemoteHM = lRHM; | |
| }) | |
| ); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 1192-1203: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
🤖 Prompt for AI Agents
In packages/@webex/webex-core/test/integration/spec/services-v2/services-v2.js
around lines 1192 to 1203, there is a duplicate before hook that collects remote
catalogs, which will cause test failures. Remove this duplicate before hook
entirely since the remote catalogs are already collected in the parent describe
block's before hook to avoid conflicts and test failures.
| new Promise((resolve) => { | ||
| setTimeout(() => { | ||
| webexUser = user; | ||
| webexUserEU = userEU; | ||
| resolve(); | ||
| }, 1000); | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Arbitrary setTimeout could cause test flakiness
Similar to the previous file, the 1-second setTimeout appears arbitrary.
-new Promise((resolve) => {
- setTimeout(() => {
- webexUser = user;
- webexUserEU = userEU;
- resolve();
- }, 1000);
-})
+Promise.resolve().then(() => {
+ webexUser = user;
+ webexUserEU = userEU;
+})📝 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.
| new Promise((resolve) => { | |
| setTimeout(() => { | |
| webexUser = user; | |
| webexUserEU = userEU; | |
| resolve(); | |
| }, 1000); | |
| }) | |
| Promise.resolve().then(() => { | |
| webexUser = user; | |
| webexUserEU = userEU; | |
| }) |
🤖 Prompt for AI Agents
In packages/@webex/webex-core/test/integration/spec/services-v2/services-v2.js
between lines 42 and 48, replace the arbitrary 1-second setTimeout with a more
reliable synchronization method. Instead of waiting a fixed time, modify the
code to wait for a specific event, promise resolution, or condition that
confirms webexUser and webexUserEU are ready. This will prevent test flakiness
caused by timing assumptions.
| foundHostCatalogPriority < currentHostCatalogPriority || | ||
| foundHost.priority < currentHost.priority | ||
| ) { | ||
| filteredHosts.splice(filteredHosts.indexOf(foundHost, 1)); |
There was a problem hiding this comment.
Syntax error in splice call - missing comma between arguments
The splice call has incorrect syntax with the second argument inside the indexOf call.
-filteredHosts.splice(filteredHosts.indexOf(foundHost, 1));
+filteredHosts.splice(filteredHosts.indexOf(foundHost), 1);📝 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.
| filteredHosts.splice(filteredHosts.indexOf(foundHost, 1)); | |
| filteredHosts.splice(filteredHosts.indexOf(foundHost), 1); |
🤖 Prompt for AI Agents
In packages/@webex/webex-core/src/lib/services-v2/service-registry.js at line
245, the splice method call has a syntax error because the second argument is
incorrectly placed inside the indexOf call. Fix this by closing the indexOf call
after foundHost, then add a comma, and pass 1 as the second argument to splice
to correctly remove one element at the found index.
| get map() { | ||
| // Get a list of active, local, and priority-mapped hosts. | ||
| return this.find({ | ||
| active: true, | ||
| local: true, | ||
| priority: true, | ||
| }).reduce((map, host) => { | ||
| // Generate a new object to assign the existing map. | ||
| const hostReference = {}; | ||
|
|
||
| // Assign the key:value pair for the service and url. | ||
| hostReference[host.service] = host.url; | ||
|
|
||
| // Assign the reference to the map and return. | ||
| return {...map, ...hostReference}; | ||
| }, {}); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Performance issue: O(n²) complexity due to spread in reduce
Using spread syntax in a reduce accumulator creates a new object on each iteration, resulting in O(n²) time complexity.
Use Object.assign or build the object after reduction:
get map() {
// Get a list of active, local, and priority-mapped hosts.
- return this.find({
+ const hosts = this.find({
active: true,
local: true,
priority: true,
- }).reduce((map, host) => {
- // Generate a new object to assign the existing map.
- const hostReference = {};
-
- // Assign the key:value pair for the service and url.
- hostReference[host.service] = host.url;
-
- // Assign the reference to the map and return.
- return {...map, ...hostReference};
- }, {});
+ });
+
+ const map = {};
+ hosts.forEach(host => {
+ map[host.service] = host.url;
+ });
+
+ return map;
}📝 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.
| get map() { | |
| // Get a list of active, local, and priority-mapped hosts. | |
| return this.find({ | |
| active: true, | |
| local: true, | |
| priority: true, | |
| }).reduce((map, host) => { | |
| // Generate a new object to assign the existing map. | |
| const hostReference = {}; | |
| // Assign the key:value pair for the service and url. | |
| hostReference[host.service] = host.url; | |
| // Assign the reference to the map and return. | |
| return {...map, ...hostReference}; | |
| }, {}); | |
| } | |
| get map() { | |
| // Get a list of active, local, and priority-mapped hosts. | |
| const hosts = this.find({ | |
| active: true, | |
| local: true, | |
| priority: true, | |
| }); | |
| const map = {}; | |
| hosts.forEach((host) => { | |
| map[host.service] = host.url; | |
| }); | |
| return map; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 77-77: Avoid the use of spread (...) syntax on accumulators.
Spread syntax should be avoided on accumulators (like those in .reduce) because it causes a time complexity of O(n^2).
Consider methods such as .splice or .push instead.
(lint/performance/noAccumulatingSpread)
🤖 Prompt for AI Agents
In packages/@webex/webex-core/src/lib/services-v2/service-registry.js between
lines 63 and 79, the reduce function uses spread syntax to merge objects on each
iteration, causing O(n²) complexity. To fix this, replace the spread syntax with
Object.assign to mutate the accumulator object directly or build the map object
after the reduce completes to avoid creating a new object every iteration,
improving performance.
| this._updateHostCatalog(serviceHostmap.hostCatalog); | ||
|
|
There was a problem hiding this comment.
Redundant host catalog updates.
The _hostCatalog is updated twice with the same data - once at the beginning (line 699) and again at the end (line 773). Consider removing one of these calls to avoid redundancy.
_formatReceivedHostmap(serviceHostmap) {
this._updateHostCatalog(serviceHostmap.hostCatalog);
// ... rest of the method ...
// update all the service urls in the host catalog
this._updateServiceUrls(serviceHostmap.serviceLinks);
- this._updateHostCatalog(serviceHostmap.hostCatalog);
return formattedHostmap;
}Also applies to: 772-773
🤖 Prompt for AI Agents
In packages/@webex/webex-core/src/lib/services-v2/services-v2.js between lines
699 and 700, the method _updateHostCatalog is called redundantly with the same
hostCatalog data again at lines 772-773. Remove the earlier call at line 699 to
avoid updating _hostCatalog twice unnecessarily, keeping only the update at the
end to ensure a single, consolidated update.
2 participants
COMPLETES #SPARK-610518
This pull request addresses
Saving the subnet ip details and sending to web-client
by making the following changes
Saved the subnet ip details in reachability results and sending to web-client
Change Type
The following scenarios were tested
Tested manually whether we are getting the subnets in the response or not in webex-web-client.
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
Make sure to have followed the contributing guidelines before submitting.