Removed sessionId usage - #5092
Conversation
| /** | ||
| * Pending practice session datachannel token, passed to webinar for its LLM channel. | ||
| */ | ||
| private _pendingPracticeSessionDatachannelToken?: string; |
There was a problem hiding this comment.
would it make more sense to keep this inside Webinar class?
There was a problem hiding this comment.
ok I see you moved it, but it's now public and name starting with "_" - probably best to make it private and add a public setter
There was a problem hiding this comment.
I see the setter added, but the prop is still public
| * | ||
| * @returns {LLMChannel} A new LLM connection instance | ||
| */ | ||
| public createConnection(): LLMChannel { |
There was a problem hiding this comment.
the function is called "createConnection" but it returns an LLMChannel - it would be better to settle on one name and use it consistently everywhere - either "connection" or "channel"
note that there are more places that use "connection" like getConnectionByDatachannelUrl, getAllConnections, etc
| /** | ||
| * The Voicea channel for transcription/captions, bound to this meeting's LLM channel. | ||
| */ | ||
| voiceaChannel?: VoiceaChannel; |
There was a problem hiding this comment.
should this maybe be private?
There was a problem hiding this comment.
I see this is now private, but I also see it's being accessed in lots of places by the web app in the web app PR, so feels like this shouldn't be private after all? or we need some public getter
| const channel = new LLMChannel({parent: this.webex}); | ||
|
|
||
| this.channels.add(channel); | ||
| channel.onDisconnect = () => this.channels.delete(channel); |
There was a problem hiding this comment.
This is not great. As onDisconnect is public anyone else can overwrite it and the LLM plugin's one won't get called. We should do this properly via an event registration and emit
|
|
||
| // Re-announce and re-enable captions if they were on | ||
| if (captionsWereOn) { | ||
| await this.turnOnCaptions(spokenLanguage); |
There was a problem hiding this comment.
question: just wondering if maybe we need to do something with currentCaptionLanguage too?
| this.annotation.registerChannel(this.llmChannel); | ||
| // Register annotation channel only if not in practice session | ||
| // (practice session manages its own annotation channel via updatePSDataChannel) | ||
| if (!this.webinar?.isPracticeSessionLLMChannelConnected()) { |
There was a problem hiding this comment.
shouldn't this be only checking if we're in practice session no matter if the PS LLM channel is connected or not? I mean there can be a case of being in PS but with disconnected LLM channel and in that case we still want annotation to stay on the PS LLM channel and not use the main one, right?
marcin-bazyl
left a comment
marcin-bazyl
left a comment
There was a problem hiding this comment.
approving, so that you're not blocked when I'm on PTO, but there are 6 open comment threads right now, so please either address them by replying in this PR if no code change is needed or do the code change in another PR and reply here with just the PR link
2 participants
COMPLETES #< INSERT LINK TO ISSUE >
This pull request addresses
< DESCRIBE THE CONTEXT OF THE ISSUE >
Before refactor:
Main Session:
Uses webex.internal.llm singleton with session ID 'llm-default-session'
URL from [locusInfo.info.datachannelUrl]
Events: ['event:relay.event'] ['locus.llm'], 'online'
Practice Session:
Uses same webex.internal.llm singleton with session ID 'llm-practice-session'
URL from [locusInfo.info.practiceSessionDatachannelUrl]
Events: ['event:relay.event:llm-practice-session'], ['locus.llm:llm-practice-session']
Waits for main session 'online' before connecting
Voicea:
Single [webex.internal.voicea] singleton shared by both sessions
[announce()] / [turnOnCaptions()]operates on whichever session is active
Caption state isCaptionBoxOn is global
Refactor:
Main Session:
[meeting.llmChannel]= dedicated [LLMChannel] instance owned by meeting
[meeting.voiceaChannel]= dedicated [VoiceaChannel] instance bound to [llmChannel]
Events emitted directly on the channel instance (no suffix)
Practice Session:
[webinar.practiceSessionLLMChannel] = dedicated [LLMChannel] instance owned by webinar
[webinar.practiceSessionVoiceaChannel]= dedicated [VoiceaChannel] instance bound to PS channel
Still waits for main session 'online' before connecting
In both architectures, when Practice Session is active:
Channel Ownership Summary
Regular Meeting:
meeting.llmChannel
meeting.voiceaChannel → listens to meeting.llmChannel
Webinar (main session only):
meeting.llmChannel
meeting.voiceaChannel → listens to meeting.llmChannel
Webinar (practice session active):
meeting.llmChannel - main session (stays connected)
webinar.practiceSessionLLMChannel - practice session (ADDITIONAL)
meeting.voiceaChannel → SWITCHES to practiceSessionLLMChannel
(preserves caption state, re-announces)
On practice session exit:
meeting.voiceaChannel.switchLLMChannel(meeting.llmChannel) → switches back
webinar.practiceSessionLLMChannel → disconnected & cleaned up
by making the following changes
< DESCRIBE YOUR 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.