feat(u2cv2): handle update active cluster mercury message #4386
Conversation
…dk into deregister-plugin
📝 WalkthroughWalkthroughA new event listener for the Estimated code review effort2 (~20 minutes) 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. 🔧 ESLint
yarn install v1.22.22 ... [truncated 7273 characters] ... [email protected]: This proposal has been merged to the ECMAScript standard and thus this plugin is no longer maintained. Please use @babel/plugin-transform-optional-chaining instead. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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
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: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
packages/@webex/internal-plugin-mercury/src/mercury.js(1 hunks)packages/@webex/internal-plugin-mercury/test/unit/spec/mercury.js(3 hunks)packages/@webex/webex-core/src/index.js(2 hunks)packages/@webex/webex-core/src/lib/constants.js(1 hunks)packages/@webex/webex-core/src/lib/interceptors/server-error.js(1 hunks)packages/@webex/webex-core/src/lib/services-v2/constants.ts(0 hunks)packages/@webex/webex-core/src/lib/services-v2/index.ts(0 hunks)packages/@webex/webex-core/src/lib/services-v2/interceptors/server-error.js(0 hunks)packages/@webex/webex-core/src/lib/services-v2/service-catalog.ts(1 hunks)packages/@webex/webex-core/src/lib/services-v2/services-v2.ts(4 hunks)packages/@webex/webex-core/src/lib/services-v2/types.ts(1 hunks)packages/@webex/webex-core/src/lib/services/constants.js(0 hunks)packages/@webex/webex-core/src/lib/services/index.js(1 hunks)packages/@webex/webex-core/src/lib/services/interceptors/hostmap.js(0 hunks)packages/@webex/webex-core/src/lib/services/interceptors/service.js(0 hunks)packages/@webex/webex-core/src/lib/services/service-host.js(1 hunks)packages/@webex/webex-core/src/lib/services/service-registry.js(1 hunks)packages/@webex/webex-core/src/lib/services/service-state.js(1 hunks)packages/@webex/webex-core/src/lib/services/services.js(1 hunks)packages/@webex/webex-core/src/webex-core.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(4 hunks)packages/@webex/webex-core/test/unit/spec/services-v2/service-detail.ts(1 hunks)packages/@webex/webex-core/test/unit/spec/services-v2/services-v2.ts(2 hunks)
💤 Files with no reviewable changes (6)
- packages/@webex/webex-core/src/lib/services/constants.js
- packages/@webex/webex-core/src/lib/services-v2/index.ts
- packages/@webex/webex-core/src/lib/services/interceptors/hostmap.js
- packages/@webex/webex-core/src/lib/services-v2/constants.ts
- packages/@webex/webex-core/src/lib/services-v2/interceptors/server-error.js
- packages/@webex/webex-core/src/lib/services/interceptors/service.js
🧰 Additional context used
🧠 Learnings (16)
packages/@webex/webex-core/src/lib/services/service-state.js (1)
Learnt from: Kesari3008
PR: webex/webex-js-sdk#4258
File: packages/@webex/plugin-cc/src/webex.js:0-0
Timestamp: 2025-05-13T17:01:05.340Z
Learning: In the webex-js-sdk project, `PACKAGE_VERSION` is a constant that's injected by the build system and doesn't need to be explicitly imported in the source files.
packages/@webex/webex-core/src/lib/services/service-host.js (1)
Learnt from: Kesari3008
PR: webex/webex-js-sdk#4258
File: packages/@webex/plugin-cc/src/webex.js:0-0
Timestamp: 2025-05-13T17:01:05.340Z
Learning: In the webex-js-sdk project, `PACKAGE_VERSION` is a constant that's injected by the build system and doesn't need to be explicitly imported in the source files.
packages/@webex/webex-core/src/lib/interceptors/server-error.js (2)
Learnt from: robstax
PR: webex/webex-js-sdk#3948
File: packages/@webex/webex-core/test/unit/spec/webex-core.js:178-254
Timestamp: 2024-11-01T00:30:53.442Z
Learning: In the webex-js-sdk project, when reviewing changes in `packages/@webex/webex-core/test/unit/spec/webex-core.js`, if the user declines to add negative test cases for invalid interceptor configurations, accept their decision and do not insist.
Learnt from: Kesari3008
PR: webex/webex-js-sdk#4258
File: packages/@webex/plugin-cc/src/webex.js:0-0
Timestamp: 2025-05-13T17:01:05.340Z
Learning: In the webex-js-sdk project, `PACKAGE_VERSION` is a constant that's injected by the build system and doesn't need to be explicitly imported in the source files.
packages/@webex/webex-core/test/unit/spec/services-v2/service-detail.ts (3)
Learnt from: robstax
PR: webex/webex-js-sdk#3948
File: packages/@webex/webex-core/test/unit/spec/webex-core.js:178-254
Timestamp: 2024-11-01T00:30:53.442Z
Learning: In the webex-js-sdk project, when reviewing changes in `packages/@webex/webex-core/test/unit/spec/webex-core.js`, if the user declines to add negative test cases for invalid interceptor configurations, accept their decision and do not insist.
Learnt from: pagour98
PR: webex/webex-js-sdk#4086
File: packages/@webex/plugin-cc/test/unit/spec/services/task/dialer.ts:52-57
Timestamp: 2025-02-12T10:13:06.265Z
Learning: In packages/@webex/plugin-cc tests, catch blocks in test cases follow CC Desktop pattern using `expect(true).toBe(true)` for consistency.
Learnt from: Kesari3008
PR: webex/webex-js-sdk#4258
File: packages/@webex/plugin-cc/src/webex.js:0-0
Timestamp: 2025-05-13T17:01:05.340Z
Learning: In the webex-js-sdk project, `PACKAGE_VERSION` is a constant that's injected by the build system and doesn't need to be explicitly imported in the source files.
packages/@webex/webex-core/src/webex-core.js (2)
Learnt from: robstax
PR: webex/webex-js-sdk#3948
File: packages/@webex/webex-core/test/unit/spec/webex-core.js:178-254
Timestamp: 2024-11-01T00:30:53.442Z
Learning: In the webex-js-sdk project, when reviewing changes in `packages/@webex/webex-core/test/unit/spec/webex-core.js`, if the user declines to add negative test cases for invalid interceptor configurations, accept their decision and do not insist.
Learnt from: Kesari3008
PR: webex/webex-js-sdk#4258
File: packages/@webex/plugin-cc/src/webex.js:0-0
Timestamp: 2025-05-13T17:01:05.340Z
Learning: In the webex-js-sdk project, `PACKAGE_VERSION` is a constant that's injected by the build system and doesn't need to be explicitly imported in the source files.
packages/@webex/webex-core/src/lib/services/service-registry.js (2)
Learnt from: Kesari3008
PR: webex/webex-js-sdk#4258
File: packages/@webex/plugin-cc/src/webex.js:0-0
Timestamp: 2025-05-13T17:01:05.340Z
Learning: In the webex-js-sdk project, `PACKAGE_VERSION` is a constant that's injected by the build system and doesn't need to be explicitly imported in the source files.
Learnt from: robstax
PR: webex/webex-js-sdk#3948
File: packages/@webex/webex-core/test/unit/spec/webex-core.js:178-254
Timestamp: 2024-11-01T00:30:53.442Z
Learning: In the webex-js-sdk project, when reviewing changes in `packages/@webex/webex-core/test/unit/spec/webex-core.js`, if the user declines to add negative test cases for invalid interceptor configurations, accept their decision and do not insist.
packages/@webex/webex-core/src/lib/services/services.js (2)
Learnt from: Kesari3008
PR: webex/webex-js-sdk#4258
File: packages/@webex/plugin-cc/src/webex.js:0-0
Timestamp: 2025-05-13T17:01:05.340Z
Learning: In the webex-js-sdk project, `PACKAGE_VERSION` is a constant that's injected by the build system and doesn't need to be explicitly imported in the source files.
Learnt from: robstax
PR: webex/webex-js-sdk#3948
File: packages/@webex/webex-core/test/unit/spec/webex-core.js:178-254
Timestamp: 2024-11-01T00:30:53.442Z
Learning: In the webex-js-sdk project, when reviewing changes in `packages/@webex/webex-core/test/unit/spec/webex-core.js`, if the user declines to add negative test cases for invalid interceptor configurations, accept their decision and do not insist.
packages/@webex/webex-core/test/integration/spec/services-v2/service-catalog.js (3)
Learnt from: robstax
PR: webex/webex-js-sdk#3948
File: packages/@webex/webex-core/test/unit/spec/webex-core.js:178-254
Timestamp: 2024-11-01T00:30:53.442Z
Learning: In the webex-js-sdk project, when reviewing changes in `packages/@webex/webex-core/test/unit/spec/webex-core.js`, if the user declines to add negative test cases for invalid interceptor configurations, accept their decision and do not insist.
Learnt from: pagour98
PR: webex/webex-js-sdk#4086
File: packages/@webex/plugin-cc/test/unit/spec/services/task/dialer.ts:52-57
Timestamp: 2025-02-12T10:13:06.265Z
Learning: In packages/@webex/plugin-cc tests, catch blocks in test cases follow CC Desktop pattern using `expect(true).toBe(true)` for consistency.
Learnt from: Kesari3008
PR: webex/webex-js-sdk#4258
File: packages/@webex/plugin-cc/src/webex.js:0-0
Timestamp: 2025-05-13T17:01:05.340Z
Learning: In the webex-js-sdk project, `PACKAGE_VERSION` is a constant that's injected by the build system and doesn't need to be explicitly imported in the source files.
packages/@webex/webex-core/test/unit/spec/services-v2/services-v2.ts (3)
Learnt from: robstax
PR: webex/webex-js-sdk#3948
File: packages/@webex/webex-core/test/unit/spec/webex-core.js:178-254
Timestamp: 2024-11-01T00:30:53.442Z
Learning: In the webex-js-sdk project, when reviewing changes in `packages/@webex/webex-core/test/unit/spec/webex-core.js`, if the user declines to add negative test cases for invalid interceptor configurations, accept their decision and do not insist.
Learnt from: pagour98
PR: webex/webex-js-sdk#4086
File: packages/@webex/plugin-cc/test/unit/spec/services/task/dialer.ts:52-57
Timestamp: 2025-02-12T10:13:06.265Z
Learning: In packages/@webex/plugin-cc tests, catch blocks in test cases follow CC Desktop pattern using `expect(true).toBe(true)` for consistency.
Learnt from: Kesari3008
PR: webex/webex-js-sdk#4258
File: packages/@webex/plugin-cc/src/webex.js:0-0
Timestamp: 2025-05-13T17:01:05.340Z
Learning: In the webex-js-sdk project, `PACKAGE_VERSION` is a constant that's injected by the build system and doesn't need to be explicitly imported in the source files.
packages/@webex/webex-core/test/integration/spec/services-v2/services-v2.js (3)
Learnt from: robstax
PR: webex/webex-js-sdk#3948
File: packages/@webex/webex-core/test/unit/spec/webex-core.js:178-254
Timestamp: 2024-11-01T00:30:53.442Z
Learning: In the webex-js-sdk project, when reviewing changes in `packages/@webex/webex-core/test/unit/spec/webex-core.js`, if the user declines to add negative test cases for invalid interceptor configurations, accept their decision and do not insist.
Learnt from: pagour98
PR: webex/webex-js-sdk#4086
File: packages/@webex/plugin-cc/test/unit/spec/services/task/dialer.ts:52-57
Timestamp: 2025-02-12T10:13:06.265Z
Learning: In packages/@webex/plugin-cc tests, catch blocks in test cases follow CC Desktop pattern using `expect(true).toBe(true)` for consistency.
Learnt from: Kesari3008
PR: webex/webex-js-sdk#4258
File: packages/@webex/plugin-cc/src/webex.js:0-0
Timestamp: 2025-05-13T17:01:05.340Z
Learning: In the webex-js-sdk project, `PACKAGE_VERSION` is a constant that's injected by the build system and doesn't need to be explicitly imported in the source files.
packages/@webex/webex-core/src/lib/services-v2/service-catalog.ts (1)
Learnt from: robstax
PR: webex/webex-js-sdk#3948
File: packages/@webex/webex-core/test/unit/spec/webex-core.js:178-254
Timestamp: 2024-11-01T00:30:53.442Z
Learning: In the webex-js-sdk project, when reviewing changes in `packages/@webex/webex-core/test/unit/spec/webex-core.js`, if the user declines to add negative test cases for invalid interceptor configurations, accept their decision and do not insist.
packages/@webex/webex-core/src/lib/services/index.js (2)
Learnt from: robstax
PR: webex/webex-js-sdk#3948
File: packages/@webex/webex-core/test/unit/spec/webex-core.js:178-254
Timestamp: 2024-11-01T00:30:53.442Z
Learning: In the webex-js-sdk project, when reviewing changes in `packages/@webex/webex-core/test/unit/spec/webex-core.js`, if the user declines to add negative test cases for invalid interceptor configurations, accept their decision and do not insist.
Learnt from: Kesari3008
PR: webex/webex-js-sdk#4258
File: packages/@webex/plugin-cc/src/webex.js:0-0
Timestamp: 2025-05-13T17:01:05.340Z
Learning: In the webex-js-sdk project, `PACKAGE_VERSION` is a constant that's injected by the build system and doesn't need to be explicitly imported in the source files.
packages/@webex/internal-plugin-mercury/test/unit/spec/mercury.js (2)
Learnt from: robstax
PR: webex/webex-js-sdk#3948
File: packages/@webex/webex-core/test/unit/spec/webex-core.js:178-254
Timestamp: 2024-11-01T00:30:53.442Z
Learning: In the webex-js-sdk project, when reviewing changes in `packages/@webex/webex-core/test/unit/spec/webex-core.js`, if the user declines to add negative test cases for invalid interceptor configurations, accept their decision and do not insist.
Learnt from: pagour98
PR: webex/webex-js-sdk#4086
File: packages/@webex/plugin-cc/test/unit/spec/services/task/dialer.ts:52-57
Timestamp: 2025-02-12T10:13:06.265Z
Learning: In packages/@webex/plugin-cc tests, catch blocks in test cases follow CC Desktop pattern using `expect(true).toBe(true)` for consistency.
packages/@webex/webex-core/src/lib/constants.js (1)
Learnt from: Kesari3008
PR: webex/webex-js-sdk#4258
File: packages/@webex/plugin-cc/src/webex.js:0-0
Timestamp: 2025-05-13T17:01:05.340Z
Learning: In the webex-js-sdk project, `PACKAGE_VERSION` is a constant that's injected by the build system and doesn't need to be explicitly imported in the source files.
packages/@webex/webex-core/src/index.js (2)
Learnt from: robstax
PR: webex/webex-js-sdk#3948
File: packages/@webex/webex-core/test/unit/spec/webex-core.js:178-254
Timestamp: 2024-11-01T00:30:53.442Z
Learning: In the webex-js-sdk project, when reviewing changes in `packages/@webex/webex-core/test/unit/spec/webex-core.js`, if the user declines to add negative test cases for invalid interceptor configurations, accept their decision and do not insist.
Learnt from: Kesari3008
PR: webex/webex-js-sdk#4258
File: packages/@webex/plugin-cc/src/webex.js:0-0
Timestamp: 2025-05-13T17:01:05.340Z
Learning: In the webex-js-sdk project, `PACKAGE_VERSION` is a constant that's injected by the build system and doesn't need to be explicitly imported in the source files.
packages/@webex/webex-core/src/lib/services-v2/services-v2.ts (1)
Learnt from: Kesari3008
PR: webex/webex-js-sdk#4258
File: packages/@webex/plugin-cc/src/webex.js:0-0
Timestamp: 2025-05-13T17:01:05.340Z
Learning: In the webex-js-sdk project, `PACKAGE_VERSION` is a constant that's injected by the build system and doesn't need to be explicitly imported in the source files.
🧬 Code Graph Analysis (5)
packages/@webex/webex-core/test/unit/spec/services-v2/service-detail.ts (1)
packages/@webex/test-helper-mock-webex/src/index.js (1)
MockWebex(126-222)
packages/@webex/webex-core/test/integration/spec/services-v2/service-catalog.js (2)
packages/@webex/webex-core/src/lib/interceptors/service.js (1)
ServiceInterceptor(12-101)packages/@webex/webex-core/src/lib/interceptors/server-error.js (1)
ServerErrorInterceptor(10-48)
packages/@webex/webex-core/test/unit/spec/services-v2/services-v2.ts (2)
packages/@webex/test-helper-mock-webex/src/index.js (1)
MockWebex(126-222)packages/@webex/webex-core/test/fixtures/host-catalog-v2.ts (1)
serviceHostmapV2(1-121)
packages/@webex/webex-core/src/lib/services-v2/service-catalog.ts (1)
packages/@webex/webex-core/src/lib/services-v2/types.ts (1)
ServiceGroup(3-3)
packages/@webex/webex-core/src/lib/services-v2/services-v2.ts (1)
packages/@webex/webex-core/src/lib/services-v2/types.ts (1)
ActiveServices(12-12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (27)
packages/@webex/webex-core/src/lib/services/service-host.js (1)
3-3: LGTM: Consistent import path update.The import path update aligns with the constants consolidation effort described in the AI summary.
packages/@webex/webex-core/src/lib/services/service-state.js (1)
1-1: LGTM: Consistent import path update.The import path update aligns with the constants consolidation effort across service modules.
packages/@webex/webex-core/src/lib/services/service-registry.js (1)
1-1: LGTM: Consistent import path update.The import path update maintains consistency with other service modules and supports the constants consolidation effort.
packages/@webex/webex-core/test/unit/spec/services-v2/service-detail.ts (1)
17-17: MockWebex instantiation change is safe and consistent.The switch from
new MockWebex()toMockWebex({})matches existing patterns elsewhere and does not alter behavior:• In
packages/@webex/webex-core/test/unit/spec/services-v2/services-v2.ts, we already callwebex = MockWebex({ … }).
• Several other tests (e.g., plugin-cc, plugin-meetings) similarly invokeMockWebex({...})withoutnew.
• Under the hood,MockWebexreturns a fully configured instance whether or not it’s invoked withnew, so passing an empty object preserves the defaults.No changes required.
packages/@webex/webex-core/src/lib/interceptors/server-error.js (1)
6-6: LGTM: Import path update for interceptor reorganization.The import path change reflects the move of interceptors to the dedicated
lib/interceptorsdirectory as described in the AI summary.packages/@webex/webex-core/src/lib/services/services.js (1)
6-6: LGTM: Import path updates align with constants consolidation.The import paths for
METRICSandCOMMERCIAL_ALLOWED_DOMAINShave been correctly updated to reflect the consolidation of constants into the parent directory. No functional changes were made.Also applies to: 11-11
packages/@webex/webex-core/test/integration/spec/services-v2/service-catalog.js (1)
45-46: LGTM: Interceptor updates align with V2 interceptor removal.The test has been correctly updated to use the original
ServiceInterceptor.createandServerErrorInterceptor.createinstead of their V2 counterparts, aligning with the broader cleanup of deprecated V2 interceptors.packages/@webex/webex-core/src/webex-core.js (1)
34-34: LGTM: Import path updated for interceptor reorganization.The
HostMapInterceptorimport path has been correctly updated to reflect its new location in the dedicated interceptors directory as part of the interceptor reorganization.packages/@webex/internal-plugin-mercury/src/mercury.js (1)
71-81: LGTM: Mercury event listener properly implements cluster status handling.The new event listener for
ActiveClusterStatusEventcorrectly:
- Uses defensive programming with optional chaining and null checks
- Validates the presence of both the service method and event data
- Passes the
activeClustersdata to theswitchActiveClusterIdsmethod- Includes appropriate commenting about the experimental nature
This implementation aligns perfectly with the PR objectives for handling Mercury cluster update notifications.
packages/@webex/webex-core/src/lib/services-v2/types.ts (1)
66-66: LGTM: Interface update enables flexible service lookups.Making the
serviceGroupparameter optional in theIServiceCatalog.getmethod is a good design decision that:
- Maintains backward compatibility for existing code
- Enables more flexible service lookups by cluster ID alone
- Supports the new cluster management functionality being introduced
packages/@webex/webex-core/src/lib/services-v2/service-catalog.ts (1)
335-335: LGTM - Improved API usability.Making the
timeoutparameter optional with a sensible default of 60 seconds improves the API usability while maintaining backward compatibility. The validation logic properly handles edge cases.packages/@webex/webex-core/test/integration/spec/services-v2/services-v2.js (5)
12-12: Import path updated correctly.The change from
serviceConstantsV2toserviceConstantsaligns with the broader refactoring to consolidate constants.
22-25: Test fixtures imported appropriately.The addition of these imports supports the new test suite for
switchActiveClusterIdsfunctionality.
63-64: Interceptor references updated correctly.The change from V2 interceptors to regular interceptors is consistent with the broader refactoring.
299-299: Constants reference updated correctly.The change to use
serviceConstants.COMMERCIAL_ALLOWED_DOMAINSis consistent with the constants consolidation.
500-546: Comprehensive test coverage for new functionality.The test suite properly validates the
switchActiveClusterIdsmethod behavior:
- Tests the scenario where cluster ID exists and updates active services
- Tests the scenario where cluster ID doesn't exist and triggers service refresh
- Uses appropriate mocking and assertions
The test structure and assertions are well-designed.
packages/@webex/webex-core/test/unit/spec/services-v2/services-v2.ts (2)
26-26: MockWebex instantiation corrected.The change from
new MockWebex()toMockWebex()suggests the mock is designed as a factory function rather than a constructor, which is the correct usage pattern.
271-311: Well-structured unit tests for new method.The test suite covers both critical paths of
switchActiveClusterIds:
- When cluster ID exists - verifies
_updateActiveServicesis called andupdateServicesis not called- When cluster ID doesn't exist - verifies
updateServicesis called withforceRefresh: trueThe test setup, mocking, and assertions are appropriate and comprehensive.
packages/@webex/webex-core/src/lib/services/index.js (1)
7-8: Interceptor import paths verified as correct.Both
ServerErrorInterceptorandServiceInterceptorare present inpackages/@webex/webex-core/src/lib/interceptorsand have been removed from the oldservices/interceptorslocation. No further changes are needed.packages/@webex/internal-plugin-mercury/test/unit/spec/mercury.js (4)
78-78: Mock services updated appropriately.The addition of
switchActiveClusterIds: sinon.stub()to the mock services object properly supports testing the new integration between Mercury and the services layer.
159-172: Test data structure is well-defined.The
activeClusterEventEnvelopeprovides realistic test data that matches the expected structure for ActiveClusterStatusEvent, supporting comprehensive testing of the new functionality.
182-193: Integration test validates new event handling.The test properly verifies that:
- ActiveClusterStatusEvent triggers the switchActiveClusterIds method
- The correct data is passed to the method
- The method is called exactly once
This ensures the Mercury-to-services integration works as expected.
197-210: Negative test case ensures proper validation.The test confirms that
switchActiveClusterIdsis not called when an empty envelope is received, which demonstrates proper data validation in the event handler. The updated test description accurately reflects that bothupdateFeatureandswitchActiveClusterIdsare not called for empty envelopes.packages/@webex/webex-core/src/lib/constants.js (1)
8-34: LGTM! Good consolidation of service constants.The centralization of constants from multiple service modules into a single location improves maintainability and follows the DRY principle. The constant definitions are clear and appropriately named.
packages/@webex/webex-core/src/index.js (1)
27-28: LGTM! Improved modularization and separation of concerns.The reorganization of exports properly separates interceptors, constants, and core services into their respective modules. This makes the codebase more maintainable and imports more explicit.
Also applies to: 59-61
packages/@webex/webex-core/src/lib/services-v2/services-v2.ts (2)
6-6: LGTM! Updated import paths align with constants consolidation.The import path changes correctly reference the new consolidated constants module.
Also applies to: 9-9
780-780: LGTM! Improved type safety with explicit parameter and return types.The method signature refinements add better type safety and clarity to the method contracts.
Also applies to: 808-808
| /** | ||
| * Update cluster id via mercury service update. If the cluster id does not exist, | ||
| * fetch new catalog. | ||
| * | ||
| * @param {ActiveServices} newActiveClusters - The new active clusters to switch to. | ||
| * @returns {Promsie<void>} | ||
| * */ | ||
| switchActiveClusterIds(newActiveClusters: ActiveServices): Promise<void> { | ||
| this.logger.info('services: switching active cluster ids'); | ||
|
|
||
| const newActiveClusterIds = Object.values(newActiveClusters); | ||
| let missingCluseterIds = false; | ||
|
|
||
| newActiveClusterIds.forEach((clusterId) => { | ||
| // if the clusterId does not exist in the catalog, fetch the catalog | ||
| if (!this._services.find((service) => service.id === clusterId)) { | ||
| missingCluseterIds = true; | ||
| } | ||
| }); | ||
|
|
||
| if (missingCluseterIds) { | ||
| this.logger.warn( | ||
| 'services: some cluster ids do not exist in the catalog, fetching the catalog' | ||
| ); | ||
|
|
||
| // fetch the catalog | ||
| return this.updateServices({forceRefresh: true}); | ||
| } | ||
| // update the active services | ||
| this._updateActiveServices(newActiveClusters); | ||
| this.logger.info('services: active cluster ids updated successfully'); | ||
|
|
||
| return Promise.resolve(); | ||
| }, |
There was a problem hiding this comment.
Fix typo and improve error handling in cluster switching logic.
The implementation looks correct overall, but there are issues that need to be addressed:
- Typo: Variable name has a spelling error
- Error handling: Missing error handling for
updateServicesfailure
Apply this diff to fix the typo:
- let missingCluseterIds = false;
+ let missingClusterIds = false;
newActiveClusterIds.forEach((clusterId) => {
// if the clusterId does not exist in the catalog, fetch the catalog
if (!this._services.find((service) => service.id === clusterId)) {
- missingCluseterIds = true;
+ missingClusterIds = true;
}
});
- if (missingCluseterIds) {
+ if (missingClusterIds) {Additional consideration: When updateServices({forceRefresh: true}) is called, the active services are not updated in the current execution path. Consider whether this is intentional or if error handling should be added to update active services after successful catalog refresh.
📝 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.
| /** | |
| * Update cluster id via mercury service update. If the cluster id does not exist, | |
| * fetch new catalog. | |
| * | |
| * @param {ActiveServices} newActiveClusters - The new active clusters to switch to. | |
| * @returns {Promsie<void>} | |
| * */ | |
| switchActiveClusterIds(newActiveClusters: ActiveServices): Promise<void> { | |
| this.logger.info('services: switching active cluster ids'); | |
| const newActiveClusterIds = Object.values(newActiveClusters); | |
| let missingCluseterIds = false; | |
| newActiveClusterIds.forEach((clusterId) => { | |
| // if the clusterId does not exist in the catalog, fetch the catalog | |
| if (!this._services.find((service) => service.id === clusterId)) { | |
| missingCluseterIds = true; | |
| } | |
| }); | |
| if (missingCluseterIds) { | |
| this.logger.warn( | |
| 'services: some cluster ids do not exist in the catalog, fetching the catalog' | |
| ); | |
| // fetch the catalog | |
| return this.updateServices({forceRefresh: true}); | |
| } | |
| // update the active services | |
| this._updateActiveServices(newActiveClusters); | |
| this.logger.info('services: active cluster ids updated successfully'); | |
| return Promise.resolve(); | |
| }, | |
| /** | |
| * Update cluster id via mercury service update. If the cluster id does not exist, | |
| * fetch new catalog. | |
| * | |
| * @param {ActiveServices} newActiveClusters - The new active clusters to switch to. | |
| * @returns {Promise<void>} | |
| * */ | |
| switchActiveClusterIds(newActiveClusters: ActiveServices): Promise<void> { | |
| this.logger.info('services: switching active cluster ids'); | |
| const newActiveClusterIds = Object.values(newActiveClusters); | |
| let missingClusterIds = false; | |
| newActiveClusterIds.forEach((clusterId) => { | |
| // if the clusterId does not exist in the catalog, fetch the catalog | |
| if (!this._services.find((service) => service.id === clusterId)) { | |
| missingClusterIds = true; | |
| } | |
| }); | |
| if (missingClusterIds) { | |
| this.logger.warn( | |
| 'services: some cluster ids do not exist in the catalog, fetching the catalog' | |
| ); | |
| // fetch the catalog | |
| return this.updateServices({forceRefresh: true}); | |
| } | |
| // update the active services | |
| this._updateActiveServices(newActiveClusters); | |
| this.logger.info('services: active cluster ids updated successfully'); | |
| return Promise.resolve(); | |
| }, |
🤖 Prompt for AI Agents
In packages/@webex/webex-core/src/lib/services-v2/services-v2.ts between lines
349 and 382, fix the typo by renaming the variable "missingCluseterIds" to
"missingClusterIds" for clarity. Add error handling for the call to
updateServices({forceRefresh: true}) by chaining a .then() to update active
services upon successful catalog refresh and a .catch() to log or handle errors
if the update fails. This ensures that active services are updated only after a
successful refresh and errors during the update are properly managed.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
COMPLETES #< https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-686428 >
This pull request addresses
Mercury will send messages notifying DNSsec users that active clusters have changed, sdk needs to handle this switch and fetch catalog if the new active cluster id does not exist in catalog
by making the following changes
Change Type
The following scenarios were tested
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
Make sure to have followed the contributing guidelines before submitting.