feat: added 'forced' param to IpNetworkDetector.detect() #4001
Conversation
WalkthroughThe changes involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant IpNetworkDetector
Client->>IpNetworkDetector: detect(force)
IpNetworkDetector->>IpNetworkDetector: Check current state
alt State is 'in-progress'
IpNetworkDetector->>IpNetworkDetector: Set pendingDetection with force
IpNetworkDetector-->>Client: Return early
else State is not 'initial' and force is false
IpNetworkDetector-->>Client: Skip detection
else
IpNetworkDetector->>IpNetworkDetector: Set state to 'in-progress'
IpNetworkDetector->>IpNetworkDetector: Execute detection logic
IpNetworkDetector->>IpNetworkDetector: Set state to 'idle'
alt Pending detection exists
IpNetworkDetector->>IpNetworkDetector: Call detect(pendingDetection.force)
end
IpNetworkDetector-->>Client: Complete detection
end
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint (1.23.1)
warning eslint@8.57.1: This version is no longer supported. Please see https://eslint.org/version-support for other options. (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/@webex/internal-plugin-device/src/ipNetworkDetector.ts (2)
20-21: Consider using constants or an enumeration for 'state' valuesUsing string literals like
'initial','in-progress', and'idle'for thestateproperty may lead to typos or inconsistencies. Defining these states as constants or an enumeration can improve maintainability and reduce errors.
189-194: Consider handling the Promise returned by 'detect' methodWhen invoking
this.detect(forceParam);, sincedetectis an async function, any unhandled exceptions might be missed. Consider adding error handling or at least acknowledging that this call is intentionally not awaited.packages/@webex/internal-plugin-device/test/unit/spec/ipNetworkDetector.js (1)
6-7: EnsuresetImmediateis compatible with all test environmentsThe use of
setImmediateinflushPromisesmay not be universally supported across all JavaScript environments. For better compatibility, consider usingPromise.resolve()orprocess.nextTick().Suggested change:
-const flushPromises = () => new Promise(setImmediate); +const flushPromises = () => Promise.resolve();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/@webex/internal-plugin-device/src/ipNetworkDetector.ts(2 hunks)packages/@webex/internal-plugin-device/test/unit/spec/ipNetworkDetector.js(3 hunks)
🔇 Additional comments (3)
packages/@webex/internal-plugin-device/src/ipNetworkDetector.ts (1)
167-177: State management and pending detection logic look good
The implementation for handling concurrent calls to detect() and managing the state transitions is well-structured and should prevent race conditions.
packages/@webex/internal-plugin-device/test/unit/spec/ipNetworkDetector.js (2)
345-387: Test correctly validates queuing behavior of detect() calls
The added test case effectively verifies that calling detect() while a detection is already in progress queues the new detection and that the peer connection methods are invoked the expected number of times.
389-425: Verify compatibility of it.each with the testing framework
Ensure that the use of it.each for parameterized tests is supported by your testing framework. In Mocha, it.each is not available by default and may require additional libraries like mocha-each or mocha-param.
To confirm whether it.each is available, you can search for its definition in your dependencies:
✅ Verification successful
The test file is using Jest's test framework
The test file is using Jest's built-in it.each functionality, which is natively supported by Jest. This is confirmed by:
- The package.json shows Jest as the test runner:
"test:unit": "webex-legacy-tools test --unit --runner jest" - The package uses
@webex/jest-config-legacyin its dependencies it.eachis a standard feature in Jest for parameterized testing and doesn't require additional libraries
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if `it.each` is available in the test environment.
# Search for `it.each` in node_modules
rg 'it\.each' node_modules/
# Alternatively, check for installed packages that provide `it.each`
npm list --depth=0 mocha-each mocha-param
Length of output: 173
Script:
#!/bin/bash
# Let's check the package.json and test setup files for testing framework and dependencies
fd "package.json|karma.conf.js|test.*config.*" --type f --exec cat {}
# Also search for any it.each usage in the codebase
rg "it\.each" --type js
Length of output: 101503
Script:
#!/bin/bash
# Let's check the test framework setup and dependencies in the specific package
fd "package.json" --type f --base-directory packages/@webex/internal-plugin-device --exec cat {}
# Also check for any jest setup files in the package
fd "jest.*" --type f --base-directory packages/@webex/internal-plugin-device --exec cat {}
Length of output: 2106
Script:
#!/bin/bash
# Let's check the jest-config-legacy package for test configuration
fd "index.js|jest.config.js" --type f --base-directory packages/@webex/jest-config-legacy/static --exec cat {}
# Also check if jest-each is available in dependencies
rg "jest-each" packages/@webex/jest-config-legacy/package.json
Length of output: 369
packages/@webex/internal-plugin-device/src/ipNetworkDetector.ts
Outdated
Show resolved
Hide resolved
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
marcin-bazyl
added
the
validated
marcin-bazyl
force-pushed
the
spark-580033
branch
from
ac8238d
to
7e7c3ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (10)
packages/@webex/internal-plugin-device/src/ipNetworkDetector.ts (4)
7-11: LGTM! Consider using TypeScript enums for better type safety.The state management implementation looks good. For better type safety, consider using TypeScript enums instead of string constants.
-const STATE = { - INITIAL: 'initial', - IN_PROGRESS: 'in-progress', - IDLE: 'idle', -}; +enum STATE { + INITIAL = 'initial', + IN_PROGRESS = 'in-progress', + IDLE = 'idle', +} props: { - state: ['string', true, STATE.INITIAL], + state: [STATE, true, STATE.INITIAL], pendingDetection: ['object', false, undefined], }Also applies to: 26-27
166-169: Enhance JSDoc documentation for clarity.The JSDoc should be updated to reflect the method's complete behavior, including state transitions and the possibility of queued detections.
/** * Detects if we are on IPv4 and/or IPv6 network. Once it resolves, read the * supportsIpV4 and supportsIpV6 props to find out the result. + * * @param {boolean} force - if false, the detection will only be done if we haven't managed to get any meaningful results yet + * @param {boolean} [force=false] - Whether to force a new detection regardless of previous results + * @returns {Promise<void>} Resolves when detection completes or queues detection if one is in progress + * @throws {Error} When RTCPeerConnection creation or offer setting fails */
185-192: Consider adding error logging for state transitions.While the state transitions are handled correctly, adding debug logs would help with troubleshooting.
try { + this.webex.logger.debug('IpNetworkDetector: Starting detection'); this.state = STATE.IN_PROGRESS; pc = new RTCPeerConnection(); results = await this.gatherLocalCandidates(pc); } finally { pc.close(); + this.webex.logger.debug(`IpNetworkDetector: Detection completed, state: ${this.state}`); this.state = STATE.IDLE; }
195-200: Add type safety for pendingDetection.The pendingDetection property would benefit from a proper TypeScript interface.
+interface PendingDetection { + force: boolean; +} props: { - pendingDetection: ['object', false, undefined], + pendingDetection: [PendingDetection, false, undefined], }packages/@webex/internal-plugin-device/test/unit/spec/ipNetworkDetector.js (3)
331-331: Consider adding test coverage forforce=falsescenario.The test verifies reset behavior with
force=true, but it would be valuable to also verify the behavior whenforce=false.// now call detect() again -const promise2 = ipNetworkDetector.detect(true); +const promise2 = ipNetworkDetector.detect(false); // everything should have been reset assert.equal(ipNetworkDetector.supportsIpV4, undefined);
345-387: Consider verifying state transitions during queued detection.The test effectively verifies the queuing behavior, but it would be valuable to also assert the state transitions during the process.
Add assertions to verify state transitions:
// now call detect() again ipNetworkDetector.detect(true); +assert.equal(ipNetworkDetector.state, 'detecting'); // simulate the end of the detection -> another one should be started simulateEndOfCandidateGathering(10); +assert.equal(ipNetworkDetector.state, 'idle');
389-424: Enhance test readability and coverage.The parametrized test effectively covers various scenarios, but consider these improvements:
- Use more descriptive test titles that explain the expected behavior.
- Verify state transitions during the detection process.
it.each` force | state | receivedOnlyMDnsCandidates | expectedToRunDetection ${true} | ${'initial'} | ${false} | ${true} ${true} | ${'idle'} | ${false} | ${true} ${true} | ${'initial'} | ${true} | ${true} ${true} | ${'idle'} | ${true} | ${true} ${false} | ${'initial'} | ${false} | ${true} ${false} | ${'initial'} | ${true} | ${true} ${false} | ${'idle'} | ${true} | ${true} ${false} | ${'idle'} | ${false} | ${false} `( - 'force=$force, state=$state, receivedOnlyMDnsCandidates=$receivedOnlyMDnsCandidates => expectedToRunDetection=$expectedToRunDetection', + '$state state with force=$force should ${expectedToRunDetection ? "" : "not "}run detection when ${receivedOnlyMDnsCandidates ? "only mDNS candidates were received" : "real candidates were received"}', async ({force, state, receivedOnlyMDnsCandidates, expectedToRunDetection}) => { ipNetworkDetector.state = state; sinon .stub(ipNetworkDetector, 'receivedOnlyMDnsCandidates') .returns(receivedOnlyMDnsCandidates); const result = ipNetworkDetector.detect(force); + assert.equal(ipNetworkDetector.state, expectedToRunDetection ? 'detecting' : state); if (expectedToRunDetection) { simulateEndOfCandidateGathering(10); } await result; + assert.equal(ipNetworkDetector.state, 'idle');packages/@webex/plugin-meetings/src/reachability/index.ts (3)
157-160: LGTM! Non-blocking IP detection implementation looks good.The implementation correctly addresses the PR objectives by:
- Making IP detection non-blocking
- Using the new
forceparameter- Gracefully handling potential failures
Consider adding a more detailed comment explaining why forcing IP detection here is necessary, e.g.:
- // kick off ip version detection. We don't await it, as we don't want to waste time - // and if it fails, that's ok we can still carry on + // Force IP version detection to ensure accurate network information after user media acquisition. + // This is not awaited to avoid blocking the reachability checks, as they can proceed without + // the IP version information if the detection fails.
160-160: Consider improving type safety by removing @ts-ignore.The
@ts-ignorecomment suggests incomplete type definitions for the webex internal API.Consider adding proper type definitions for the webex internal API:
interface IpNetworkDetector { detect(force?: boolean): Promise<void>; } interface WebexInternalDevice { ipNetworkDetector: IpNetworkDetector; } interface WebexInternal { device: WebexInternalDevice; } interface Webex { internal: WebexInternal; // ... other properties }
Line range hint
589-594: Consider consolidating type safety improvements in metrics collection.The metrics collection accesses several IpNetworkDetector properties, all marked with @ts-ignore.
Consider extending the earlier suggested type definitions to include these properties:
interface IpNetworkDetector { detect(force?: boolean): Promise<void>; firstIpV4: boolean; firstIpV6: boolean; firstMdns: boolean; totalTime: number; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/@webex/internal-plugin-device/src/ipNetworkDetector.ts(3 hunks)packages/@webex/internal-plugin-device/test/unit/spec/ipNetworkDetector.js(3 hunks)packages/@webex/plugin-meetings/src/reachability/index.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/reachability/index.ts(1 hunks)
🔇 Additional comments (5)
packages/@webex/plugin-meetings/test/unit/spec/reachability/index.ts (1)
1237-1237: LGTM!
The test correctly asserts that the detect method is called with the force parameter set to true, aligning with the changes introduced in the PR.
packages/@webex/internal-plugin-device/src/ipNetworkDetector.ts (2)
173-182: LGTM! State management logic is robust.
The state checks and early returns are well-implemented, preventing concurrent detections and unnecessary re-runs.
179-182: Verify behavior with mDNS-only candidates.
The PR objectives mention handling scenarios where only mDNS candidates are available. Let's verify this implementation.
✅ Verification successful
mDNS-only scenario is properly handled in the implementation
The implementation correctly handles mDNS-only scenarios:
- The
receivedOnlyMDnsCandidates()method specifically checks for mDNS-only cases - The condition
!force && this.state !== STATE.INITIAL && !this.receivedOnlyMDnsCandidates()ensures re-detection when only mDNS candidates are received - Test coverage confirms the behavior: "works correctly when we get only mDNS candidates" verifies that
supportsIpV4andsupportsIpV6are set toundefinedin mDNS-only cases - The implementation aligns with the PR objectives for handling mDNS-only scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests covering mDNS-only scenarios
rg -l "mDNS" --type ts --type js
# Search for related test cases
ast-grep --pattern 'describe($_, function() {
$$$
it($_, function() {
$$$
firstMdns
$$$
})
$$$
})'
Length of output: 293
Script:
#!/bin/bash
# Let's examine the test file content for mDNS scenarios
cat packages/@webex/internal-plugin-device/test/unit/spec/ipNetworkDetector.js
# Also check the implementation details
cat packages/@webex/internal-plugin-device/src/ipNetworkDetector.ts
Length of output: 22759
packages/@webex/internal-plugin-device/test/unit/spec/ipNetworkDetector.js (2)
6-7: LGTM! Well-implemented utility function.
The flushPromises utility function is correctly implemented using setImmediate to handle Promise microtasks in tests.
Line range hint 1-516: Overall test implementation is robust and comprehensive.
The test suite effectively covers the new functionality for IP network detection, including:
- Handling of concurrent detect calls
- State management
- Force parameter behavior
- Various network scenarios (IPv4, IPv6, mDNS)
The tests are well-structured, maintainable, and align with the PR objectives.
COMPLETES #SPARK-580033
This pull request addresses
When we do ip network version detection, we are not able to tell if we're on ipv4 or ipv6 network if the app hasn't acquired any local user media. Therefore, we need the app to trigger new ip network detection once the app has successfully acquired user media.
by making the following changes
Added
forceparameter toIpNetworkDetector.detect()method, so the app can use it when calling SDK after a stream is acquired. When force is false, SDK can decide if it's worth doing another detection or not - basically we only need to do it if previous one failed, because we only received mDNS candidates (so we couldn't detect ipv4/ipv6).As the app now calls detect(), we also need to make sure to handle cases when detect() is called while detection is already in progress, so I had to add
stateandpendingDetectiontoIpNetworkDetector.Initially I tried to just have a listener on mic/camera permissions in the SDK, but that doesn't work. In Firefox, even when we have the permissions, we don't get real candidates (ipv4 or ipv6) and we only get mDNS candidates until app acquires a user media stream (mic or camera).
related web app PR: https://sqbu-github.cisco.com/WebExSquared/webex-web-client/pull/7002
Change Type
The following scenarios where tested
unit tests, manual test with the web app
I certified that
I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly
Make sure to have followed the contributing guidelines before submitting.
Summary by CodeRabbit
New Features
forceparameter for conditional execution.Bug Fixes
Tests
forceparameter and ensure proper state transitions.