fix(breakouts): failed to end breakout sessions #4432
Conversation
📝 WalkthroughWalkthroughThe LOCUS_URL_CHANGED action is rerouted from the DESYNC branch to the USE_INCOMING branch in applyLocusDeltaData, invoking onDeltaLocus(locus) instead of doLocusSync(meeting). Unit tests are updated to reflect this by calling meeting.locusInfo.onDeltaLocus(fakeLocus) directly, removing prior DTO fetches via meetingRequest.getLocusDTO and syncUrl logic. No exported/public signature changes are introduced. Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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. 🔧 ast-grep (0.38.6)packages/@webex/plugin-meetings/test/unit/spec/locus-info/index.js✨ 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
|
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
xinhyao
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: 0
🧹 Nitpick comments (2)
packages/@webex/plugin-meetings/src/locus-info/index.ts (1)
223-225: Add lightweight logging for LOCUS_URL_CHANGED pathA one-line log here will greatly help field troubleshooting when session switches fail or look slow, without adding complexity.
case USE_INCOMING: - case LOCUS_URL_CHANGED: - meeting.locusInfo.onDeltaLocus(locus); + case LOCUS_URL_CHANGED: + LoggerProxy.logger.info( + 'Locus-info:index#applyLocusDeltaData --> LOCUS_URL_CHANGED: applying incoming delta via onDeltaLocus' + ); + meeting.locusInfo.onDeltaLocus(locus); break;packages/@webex/plugin-meetings/test/unit/spec/locus-info/index.js (1)
2352-2359: Strengthen the test by asserting the locusUrl actually updatesTo ensure the regression is fully covered (messages switch to main locus), add an assertion that meeting.locusUrl is updated through the onDeltaLocus path.
Example test block you can add near this test:
it('applyLocusDeltaData(LOCUS_URL_CHANGED) updates meeting.locusUrl via onDeltaLocus path', () => { // reuse existing locusInfo/mocks from the suite const fakeLocusUrl = 'https://example.com/locus/main'; const fakeLocus = {url: fakeLocusUrl}; // Use the real locusInfo instance so updateLocusInfo -> updateLocusUrl -> updateMeeting runs const meeting = {locusInfo}; // Sanity: meeting.locusUrl is not yet set assert.isUndefined(mockMeeting.locusUrl); locusInfo.applyLocusDeltaData(LocusDeltaParser.loci.LOCUS_URL_CHANGED, fakeLocus, meeting); // Assert that the meeting received the new locusUrl assert.equal(mockMeeting.locusUrl, fakeLocusUrl); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/@webex/plugin-meetings/src/locus-info/index.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/locus-info/index.js(1 hunks)
🔇 Additional comments (2)
packages/@webex/plugin-meetings/src/locus-info/index.ts (1)
223-225: Rerouting LOCUS_URL_CHANGED to onDeltaLocus is the right fixHandling LOCUS_URL_CHANGED under USE_INCOMING ensures updateLocusUrl runs immediately (via onDeltaLocus -> updateLocusInfo -> updateLocusUrl), which addresses the BO-to-main switch timing issue without depending on a potentially stale syncUrl. This aligns with the PR objective and avoids unnecessary syncs.
packages/@webex/plugin-meetings/test/unit/spec/locus-info/index.js (1)
2352-2359: Unit test correctly reflects new control flowGood update to assert that LOCUS_URL_CHANGED now invokes onDeltaLocus directly instead of performing a sync. This mirrors the updated switch logic.
xinhyao
changed the title
| @@ -220,14 +220,14 @@ export default class LocusInfo extends EventsScope { | |||
|
|
|||
| switch (action) { | |||
| case USE_INCOMING: | |||
| case LOCUS_URL_CHANGED: | |||
There was a problem hiding this comment.
I'm not convinced that this is the right approach. I think it would be better to keep doing the full sync when Locus URL changes. I think the problem is that in this line for the LOCUS_URL_CHANGED case we put the incoming locus that had url changed into the working copy, so then when we do a sync there is a big chance that the full locus in response will be on the same version as our working copy, so I think we need modify that line to either clear the working copy completely (because we want to do a full sync anyway) or at least just reset the sequence in workingCopy so that it's empty and then any sequence value from locus sync response will be always "newer" and will result in USE_INCOMING
COMPLETES
https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-666148
This pull request addresses
When the host clicks to end breakout sessions and the countdown ends, the session cannot be successfully switched from BO to meeting. As a result, the Web client sometimes still tries to send messages to BO Locus instead of main session Locus.
When the host clicks to end BO and after the countdown ends, SDK will receive the
LOCUSEVENT.LOCUS_MERCURYevent. By parsing, the event belongs toLOCUS_URL_CHANGEDcase. According to the existing logic, it will calldoLocusSyncto do the sync with a new locus, but for this bug case, the entry value of the sequence in the new fetched locus is the same as the entry value of the sequence in theworkingCopy. This will cause thelociComparisonto beUSE_CURRENT, for theUSE_CURRENTcase, SDK will do nothing, so the switch from BO to main session is not completed when the BO ends countdown.by making the following changes
This issue has a relatively high probability of reoccurring (I tested it 10 times and reoccurred 7 times). For those successful cases, I found that when the countdown ends, the SDK will additionally receive the
LOCUSEVENT.LOCUS_MERCURYevent, by parsing, the event belongs toUSE_INCOMINGcase. According to the existing logic, it will callmeeting.locusInfo.onDeltaLocusdirectly, so locus info can be updated to complete the switch from BO to main session, but this case is not very stable and does not depend ondoLocusSync(), so my current modification is that when the received action isLOCUS_URL_CHANGED, callmeeting.locusInfo.onDeltaLocusinstead ofdoLocusSyncdirectly. In this way, locus info can be updated to complete the switch from BO to main session.Failure log:
Success log:
According to the log printed on the console, we can see that for successful cases, it is because the SDK received additional events, which triggered the
meeting.locusInfo.onDeltaLocus, but this is unstable.This PR is changed on this PR(#3239).
Change Type
The following scenarios were tested
Added unit tests and manually tested.
I tested:
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
Make sure to have followed the contributing guidelines before submitting.