fix(plugin-meetings): added saving subnet ip details in reachability results #4334
Conversation
…na/webex-js-sdk into subnet_details_nwc
…required fields for orpheous
📝 WalkthroughWalkthroughIntroduces per-protocol, per-subnet reachability tracking by adding SubnetDetails arrays for udp/tcp/xtls and seeding them from cluster URLs. ClusterReachability now exposes a public clusterInfo property, a new resultDetailsUpdated event, and uses a dynamic checkIP helper. ICE candidate parsing was changed to extract server IP and port (from candidate url or fields) and pass port into saveResult; saveResult now updates/creates per-subnet detail entries. Types and persisted result shapes were extended (SubnetDetails, ClusterNode, TransportResult.details and TransportResultForBackend.details/minLatency). Adds STUN_SERVER_URL_REGEX, PROTOCOLS_LIST, and an is-ip dependency. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing Touches🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
venky-mediboina
changed the title
![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/@webex/plugin-meetings/src/reachability/index.ts (1)
168-178:⚠️ Potential issueFix subnet logging to display meaningful information
The subnet is now an object with properties, but line 174 logs it directly which will output
[object Object]. Consider logging specific subnet details for better debugging.- logMessage += `${subnet}`; + logMessage += `${subnet.serverIps}:${subnet.port}/${subnet.protocol}`;
🧹 Nitpick comments (2)
packages/@webex/plugin-meetings/src/reachability/reachability.types.ts (1)
13-21: Consider refining the SubnetDetails type definitionA few suggestions for the type definition:
- The
protocolproperty appears to always be set based on usage - consider making it required- Use
booleaninstead oftrue | falsefor consistency- Consider renaming
serverIpstoserverIpsince it contains a single IP addressexport 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 + protocol: 'udp' | 'tcp' | 'xtls'; // Protocol used for the test + serverIp: string; // IP address of the subnet being tested + reachable: boolean; // Result for this specific address 'answered-tx'?: number; // 1 if reachable, 0 otherwise 'lost-tx'?: number; // 1 if unreachable, 0 otherwise port: number; // Port used for the test latencies?: number[]; };packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts (1)
183-183: Remove commented codeThe commented line should be removed to maintain code cleanliness.
- // this.pc = null;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
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)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: AWS Amplify Console Web Preview
packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts
Outdated
Show resolved
Hide resolved
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
…sed review comments
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
packages/@webex/plugin-meetings/src/reachability/index.ts (1)
541-602: Missing null-safety & quadratic uniqueness loop
cleanUpDomainNamesrepeatedly does
protocolResult.details.filter(...)
inside nested loops without optional chaining; if any cluster lacksdetails, an exception will bubble.Additionally, uniqueness is enforced with
findIndexper element ‑ an O(n²) op. ASetkeyed byserverIps:portwould be linear and simpler.Not blocking, but worth tightening.
🧰 Tools
🪛 Biome (1.9.4)
[error] 558-558: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts (2)
53-66: Inline type deserves a dedicated interfaceThe anonymous
{serverIps, port, …}shape is used throughout the file and inindex.ts. Declare aSubnetDetailsinterface once and reuse to avoid drift and improve IntelliSense.
466-498: Pre-population inflateslost-txand hides duplicatesEvery subnet is seeded with
lost-tx: 1. Before any traffic, loss is already 100 %. Initialise to0.Additionally, using
${serverIps}:${port}:${protocol}as key means dual-stack (v4/v6) or multiple DNS A-records at the same host/port will overwrite each other. Consider including the resolved IP in the key or keeping a per-protocolMap<serverIps, Map<port, SubnetDetails>>.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts(10 hunks)packages/@webex/plugin-meetings/src/reachability/index.ts(5 hunks)packages/@webex/plugin-meetings/src/reachability/reachability.types.ts(2 hunks)packages/@webex/plugin-meetings/src/reachability/util.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/@webex/plugin-meetings/src/reachability/reachability.types.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/@webex/plugin-meetings/src/reachability/index.ts
[error] 558-558: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: AWS Amplify Console Web Preview
packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts
Outdated
Show resolved
Hide resolved
marcin-bazyl
left a comment
marcin-bazyl
left a comment
There was a problem hiding this comment.
I haven't tested this PR, but based on the comments I have serious doubts if this code works at all. Has this been tested and proven to work?
Also there are multiple AI comments, please go through all of them and either address them or mark as resolved if you think they're wrong.
packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/src/reachability/reachability.types.ts
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/src/reachability/reachability.types.ts
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/src/reachability/reachability.types.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/@webex/plugin-meetings/src/reachability/index.ts (1)
941-943: Remove hardcoded domain name assumptions.The hardcoded
.public.check was previously flagged as problematic since:
- Domain names can appear in any protocol, not just xtls
- We shouldn't make assumptions about what FQDNs contain
Consider using the
isDomainNameutility function consistently instead of substring matching.- .filter((subnet) => subnet.protocol === 'xtls' && subnet.serverIps.includes('.public.')) + .filter((subnet) => subnet.protocol === 'xtls' && isDomainName(subnet.serverIps))packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts (1)
345-361: Fix critical logic bug in new subnet creation.When adding a new subnet that was just successfully reached, the code incorrectly marks it as unreachable. This creates contradictory data since we just proved the subnet is reachable.
this.subnets.set(subnetKey, { serverIps: serverIp, port, protocol, - reachable: false, // Retain domain name for failures - 'answered-tx': 0, - 'lost-tx': 1, - latencies: [], + reachable: true, + 'answered-tx': 1, + 'lost-tx': 0, + latencies: [latency], });
🧹 Nitpick comments (2)
packages/@webex/plugin-meetings/src/reachability/util.ts (1)
47-58: Remove duplicate JSDoc comments.There are duplicate JSDoc comment blocks for the
isDomainNamefunction. Keep only one set of comments to improve code cleanliness.-/** - * Checks if the given server IP is a domain name. - * - * @param {string} serverIps - The server IP or domain name to check. - * @returns {boolean} true if the server IP is a domain name, false otherwise. - */ /** * Checks if the given server IP is a domain name. * * @param {string} serverIps - The server IP or domain name to check. * @returns {boolean} true if the server IP is a domain name, false otherwise. */packages/@webex/plugin-meetings/src/reachability/index.ts (1)
530-530: Apply optional chaining suggestion from static analysis.- if (protocolResult && protocolResult.details) { + if (protocolResult?.details) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts(9 hunks)packages/@webex/plugin-meetings/src/reachability/index.ts(6 hunks)packages/@webex/plugin-meetings/src/reachability/reachability.types.ts(2 hunks)packages/@webex/plugin-meetings/src/reachability/util.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/@webex/plugin-meetings/src/reachability/reachability.types.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/@webex/plugin-meetings/src/reachability/index.ts
[error] 530-530: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (9)
packages/@webex/plugin-meetings/src/reachability/util.ts (1)
75-85: LGTM - Clean utility function.The
constructSubnetKeyfunction provides a consistent way to generate unique keys for subnet identification.packages/@webex/plugin-meetings/src/reachability/index.ts (3)
164-194: LGTM - Proper Map usage in subnet reachability check.The method correctly uses the new
subnetsMap structure and properly iterates over values.
464-501: Enhanced result processing with proper domain name segregation.The method now correctly separates domain names from IP subnet details and processes them appropriately for each protocol.
922-961: Enhanced storage with detailed subnet information.The method now properly stores subnet details and domain names per protocol, enabling richer reachability reporting.
packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts (5)
58-80: Excellent refactoring to use Map for subnet tracking.The change from Set to Map with composite keys provides efficient lookups and proper data structure for subnet management.
92-113: Comprehensive result object initialization.All new fields are properly initialized with appropriate default values.
421-447: Improved ICE candidate processing with port extraction.The enhanced regex properly handles IPv6 and domain names, and port information is correctly extracted and passed to
saveResult.
473-507: Valuable subnet prepopulation for comprehensive tracking.The prepopulation ensures all subnets are tracked from the start, providing complete visibility into reachability status across all protocols.
197-203: Simplified peer connection cleanup.The removal of property nullification after closing is a reasonable simplification.
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)
450-511: Method has become overly complex with multiple responsibilities.The
getReachabilityResultsmethod now handles data transformation, domain name segregation, storage operations, and result formatting. This violates the single responsibility principle and makes the code harder to maintain and test.Consider breaking this method into smaller, focused functions:
transformReachabilityResults()- handles data transformationsegregateDomainNames()- separates domains from IPssaveProcessedResults()- handles storage operations
♻️ Duplicate comments (1)
packages/@webex/plugin-meetings/src/reachability/index.ts (1)
464-491: Backend schema violation remains unresolved.The
detailsarrays still contain subnet objects with boolean fields that are not converted to the string format expected by the backend schema. This is the same issue flagged in previous reviews.The
mapTransportResultToBackendDataFormatfunction is only applied to the top-level protocol result, but thedetailsarray objects bypass this conversion, leaving boolean values likereachable: trueinstead ofreachable: "true".Apply the backend format conversion to each subnet object in the details arrays:
details: details.filter((subnet) => !isDomainName(subnet.serverIps)), + details: details + .filter((subnet) => !isDomainName(subnet.serverIps)) + .map((subnet) => this.mapTransportResultToBackendDataFormat(subnet)),
🧹 Nitpick comments (2)
packages/@webex/plugin-meetings/src/reachability/index.ts (2)
530-530: Use optional chaining for safer property access.The condition
protocolResult && protocolResult.detailscan be simplified using optional chaining for better readability and safety.- if (protocolResult && protocolResult.details) { + if (protocolResult?.details) {
557-565: Improve minLatency calculation logic.The nested array operations and filtering could be simplified for better readability and performance.
- const latencies = filteredDetails - .map((subnet) => Math.min(...(subnet.latencies || []))) - .filter((latency) => !Number.isNaN(latency)); // Use Number.isNaN instead of isNaN - - if (latencies.length > 0) { - protocolResult.minLatency = Math.min(...latencies); - } else { - protocolResult.minLatency = undefined; // No valid latencies - } + const validLatencies = filteredDetails + .flatMap((subnet) => subnet.latencies || []) + .filter((latency) => !Number.isNaN(latency)); + + protocolResult.minLatency = validLatencies.length > 0 + ? Math.min(...validLatencies) + : undefined;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/@webex/plugin-meetings/src/reachability/index.ts(6 hunks)packages/@webex/plugin-meetings/src/reachability/util.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/@webex/plugin-meetings/src/reachability/util.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/@webex/plugin-meetings/src/reachability/index.ts
[error] 530-530: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (3)
packages/@webex/plugin-meetings/src/reachability/index.ts (3)
15-15: LGTM!The import of utility functions is correctly implemented and aligns with their usage throughout the file.
164-193: LGTM!The method correctly adapts to the new subnet data structure using
cluster.subnets.values()and properly extracts the server IP fromsubnet.serverIps. The JSON logging provides better debugging information.
922-948: ```shell
#!/bin/bashLocate definition and usage of clusterReachability in reachability/index.ts
rg "clusterReachability" -n packages/@webex/plugin-meetings/src/reachability/index.ts
</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-meetings/src/reachability/clusterReachability.ts
Outdated
Show resolved
Hide resolved
…eachability check
| this.pc = this.createPeerConnection(clusterInfo); | ||
| this.clusterInfo = clusterInfo; | ||
| this.isVideoMesh = !!clusterInfo.isVideoMesh; | ||
| this.numUdpUrls = Array.isArray(clusterInfo.udp) ? clusterInfo.udp.length : 0; |
There was a problem hiding this comment.
why do we need the isArray() checks? the type of clusterInfo is defined saying that these will always be arrays
| } | ||
|
|
||
| // Ensure upsert uses raw host string even if not a literal IP. | ||
| private upsertDetailByHost(protocol: 'udp' | 'tcp' | 'xtls', host: string, port: number) { |
There was a problem hiding this comment.
the name is a bit misleading - "upsert" means insert or update, but this function is not doing any update, I can only see it doing an insert if the entry doesn't exist.
| const {details} = this.result[protocol]; | ||
| let existing = details.find((subnet) => subnet.serverIp === host && subnet.port === port); | ||
| if (!existing) { | ||
| existing = { |
There was a problem hiding this comment.
would be better for existing to be a const and not re-use it here as we're adding a new entry, so it should be const newEntry here, not existing as it's not an existing entry, or you could just call push({serverIpd: host, ....}) directly without having any variable
| if (match) { | ||
| return {host: match[1], port: Number(match[2])}; | ||
| } | ||
| const stripped = rawUrl.replace(/^stun:/i, '').split(';')[0]; |
There was a problem hiding this comment.
what's the purpose of the code block lines 151-157?
| }; | ||
|
|
||
| // Regular expression to match STUN and TURN server URLs | ||
| export const STUN_GENERIC_URL_REGEX = /^stun:([^:;]+):(\d+)(?:;.*)?$/i; |
There was a problem hiding this comment.
why do we have semicolons in these regexes?
|
|
||
| let subnet = result.details.find((s) => s.serverIp === serverIp && s.port === port); | ||
| if (!subnet) { | ||
| if (protocol === 'udp' && this.perUrlProtocols.size === 0) { |
There was a problem hiding this comment.
why do we need any special handling for "per url" mode in saveResult()? Shouldn't it just work fine with same code always? if per url mode creates more results, then we will save more results, each with different serverIp and port, so they shouldn't conflict with anything. We only need to make sure that latencyInMilliseconds stores the first latency (the smallest one) and this was already being done in saveResult() anyway, so why do you need any more changes?
|
|
||
| private perUrlProtocols: Set<'udp' | 'tcp' | 'xtls'> = new Set(); | ||
|
|
||
| public enablePerUrlMode(protocols: ('udp' | 'tcp' | 'xtls')[] = ['udp']) { |
There was a problem hiding this comment.
there is a lot of hardcoded logic that makes the "per url" mode work in a very specific way only for UDP, so I don't think it makes much sense to try to be generic here with a set of protocols as only "udp" matters here and if someone called enablePerUrlMode() with other protocols, then nothing would happen, so maybe better to have a simple flag or even nothing at all and just use the config.meetings.reachabilityEnablePerUrlForUdp
| const perUrlUdpOnly = this.perUrlProtocols.size === 1 && this.perUrlProtocols.has('udp'); | ||
|
|
||
| // Per‑URL UDP multi‑PC mode | ||
| if (perUrlUdpOnly) { |
There was a problem hiding this comment.
there is awful lot of code duplication introduced in this PR, the whole flow of what we do with an RTCPeerConnection for a single UDP url or for a list of TCP, xTLS urls or for a combination of both (like in "legacy" mode) should be really the same and should be managed by a single piece of code, preferably all contained within a class that would then be instantiated multiple times from ClusterReachability: in legacy mode it would be instantiated and run only once, in the new mode it would be instantiated and run for each UDP url and once for xtls and tcp urls together.
| isFirstResult[protocol] = false; | ||
| const clusterChecker = new ClusterReachability(key, cluster); | ||
| if (perUrlUdpMode) { | ||
| clusterChecker.enablePerUrlMode(['udp']); |
There was a problem hiding this comment.
would be better if this was passed in as a ClusterReachability constructor parameter
| file: 'reachability', | ||
| function: 'resultReady event handler', | ||
| }, | ||
| 'reachability:firstResultAvailable', |
There was a problem hiding this comment.
what happened to this code? why is it deleted? why are we not sending "reachability:firstResultAvailable" event anymore?
5 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
< 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.