VIDCS-3442: Refactor session context #160
Conversation
| useEffect(() => { | ||
| session?.on('signal', emojiHandler); | ||
|
|
||
| return () => { | ||
| session?.off('signal', emojiHandler); | ||
| }; | ||
| }, [emojiHandler, session]); | ||
|
|
There was a problem hiding this comment.
We handle this in the session context. Removing this silences the event listener console warning.
| changedStream: null | ChangedStreamType; | ||
| connections: null | Connection[]; |
There was a problem hiding this comment.
These two weren't used anywhere.
| const handleConnectionCreated = (e: ConnectionEventType) => { | ||
| setConnections((prevConnections) => [...prevConnections, e.connection]); | ||
| }; | ||
|
|
||
| /** | ||
| * Handles the destruction of a connection. Removing it from the connections array | ||
| * @param {ConnectionEventType} e - The connection event object. | ||
| */ | ||
| const handleConnectionDestroyed = (e: ConnectionEventType) => { | ||
| setConnections((prevConnections) => | ||
| [...prevConnections].filter( | ||
| (connection) => connection.connectionId !== e.connection.connectionId | ||
| ) | ||
| ); |
There was a problem hiding this comment.
These two weren't used at all. It may have been leftovers from our call data function from the beginning.
There was a problem hiding this comment.
Most of this was moved to the VonageVideoClient class. Some are bespoke handlers in this file, see below.
| await result.current.publish(); | ||
|
|
||
| await act(async () => { | ||
| result.current.initializeLocalPublisher({}); |
There was a problem hiding this comment.
Moving this under the act silences a warning in the console when running unit tests locally
| throw new Error(`${error.name}: ${error.message}`); | ||
| } | ||
| }); | ||
| mSession.publish(publisherRef.current); |
There was a problem hiding this comment.
The error handling is in the VonageVideoClient class. It can throw, but the error will be caught and handled below.
|
|
||
| export type { ChatMessageType } from '../../types/chat'; | ||
|
|
||
| export type ChangedStreamType = { |
There was a problem hiding this comment.
This and a few other types were moved to a separate file so the VonageVideoClient class would not have a circular dependency
| @@ -146,7 +120,8 @@ const MAX_PIN_COUNT = isMobile() ? MAX_PIN_COUNT_MOBILE : MAX_PIN_COUNT_DESKTOP; | |||
| * @returns {SessionContextType} a context provider for a publisher preview | |||
| */ | |||
| const SessionProvider = ({ children }: SessionProviderProps): ReactElement => { | |||
| const session = useRef<Session | null>(null); | |||
| const [, forceUpdate] = useState<boolean>(false); // NOSONAR | |||
There was a problem hiding this comment.
Explained in more detail below, but it forces the DOM to re-render with any current changes. It seems hacky, but it looks to be pretty standard fare for React apps from what I can tell.
| await new Promise<string | undefined>((resolve, reject) => { | ||
| session.current?.connect(token, (err?: OTError) => { | ||
| if (err) { | ||
| // We ignore the following lint warning because we are rejecting with an OTError object. | ||
| reject(err); // NOSONAR | ||
| } else { | ||
| setConnected(true); | ||
| logOnConnect(apiKey, sessionId, session.current?.connection?.connectionId); | ||
| resolve(session.current?.sessionId); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Moved to the VonageVideoClient file
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the session context to extract all Vonage Video API–related logic into a dedicated TypeScript class (VonageVideoClient) and adjusts related hooks, tests, and context providers accordingly. Key changes include creating the VonageVideoClient class, updating various hooks (e.g. useScreenShare, useEmoji, useChat) to work with the new client interface, and modifying the SessionProvider and PublisherProvider to integrate the new architecture.
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/utils/VonageVideoClient/vonageVideoClient.ts | New standalone client class encapsulating Vonage Video session logic |
| frontend/src/types/session.ts | Updated type definitions and adjusted import references |
| frontend/src/hooks/* | Updates to hooks (useScreenShare, useEmoji, useChat, etc.) for new API |
| frontend/src/Context/SessionProvider/session.ts | SessionProvider refactored to use the new VonageVideoClient |
| frontend/src/Context/PublisherProvider/usePublisher/* | PublisherProvider updated to use the new API calls |
Comments suppressed due to low confidence (2)
frontend/src/Context/PublisherProvider/usePublisher/usePublisher.tsx:235
- The recursive call to publish() in the catch block could lead to a stack overflow if publishing continues to fail. Consider refactoring this retry logic into an iterative approach or using a loop with a retry counter.
publish();
frontend/src/Context/PublisherProvider/usePublisher/usePublisher.tsx:180
- [nitpick] The variable name 'mSession' is ambiguous; consider renaming it to a more descriptive identifier such as 'sessionContext' for clarity.
const mSession = useSessionContext();
| if (error) { | ||
| throw new Error(`${error.name}: ${error.message}`); | ||
| } |
There was a problem hiding this comment.
This was moved to the VonageVideoClient
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the session context by extracting the Vonage Video API logic into a standalone TypeScript class (VonageVideoClient) and updates various hooks and tests to use the new client. Key changes include:
- Introducing the VonageVideoClient class to manage session interactions and events.
- Adapting hooks (useScreenShare, useEmoji, useChat, usePublisher) to switch from the previous session object to the new VonageVideoClient.
- Updating tests and context providers to align with the refactored session logic.
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| frontend/src/utils/VonageVideoClient/vonageVideoClient.ts | New client class encapsulating Vonage Video API interactions and event management. |
| frontend/src/utils/VonageVideoClient/vonageVideoClient.spec.ts | Tests updated to validate functionality of the new VonageVideoClient. |
| Various hook and context files | Refactored usage of session properties to use the new VonageVideoClient and updated dependent logic. |
Comments suppressed due to low confidence (1)
frontend/src/Context/PublisherProvider/usePublisher/usePublisher.tsx:242
- The recursive call to publish() in the catch block could lead to infinite recursion or stack overflow if the error persists. Consider implementing a retry limit or a backoff strategy instead.
publish();
| } | ||
| }); | ||
| }); | ||
| vonageVideoClient.current = new VonageVideoClient(credential); |
There was a problem hiding this comment.
Storing the VonageVideoClient instance in a ref may not trigger re-renders when the client instance changes, which might affect consumers relying on the latest instance. Consider using a state variable or another mechanism to properly update context consumers when the client is updated.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
This is fine. We have been using a ref specifically to not trigger re-renders as parts of the object are changing.
|
| export type EmojiWrapper = { | ||
| name: string; | ||
| emoji: string; | ||
| time: number; | ||
| }; | ||
|
|
||
| const useEmoji = () => { | ||
| const { session, subscriberWrappers } = useSessionContext(); |
There was a problem hiding this comment.
We remove the session context as it's not necessary for this hook. Instead, we pass the subscriberWrappers directly. This also makes the tests simpler and easier to read.
There was a problem hiding this comment.
The changes in this file show why it was necessary to create a VonageVideoClient class. Without it, we'd be drowning in mocks for testing. With the class, we just have to emit mock events to easily test our handlers and session functionality. It's no coincidence we had very little coverage in this file before this refactor 😅
| session: null | Session; | ||
| changedStream: null | ChangedStreamType; | ||
| connections: null | Connection[]; | ||
| connect: null | ((credential: Credential) => Promise<void>); |
There was a problem hiding this comment.
Connect was only used by joinRoom
| const handleEmoji = useCallback( | ||
| (emojiEvent: SignalEvent) => { | ||
| setSubscriberWrappers((currentSubscriberWrappers) => { | ||
| onEmoji(emojiEvent, currentSubscriberWrappers); | ||
| return currentSubscriberWrappers; // Return unchanged state | ||
| }); | ||
| }, | ||
| [onEmoji] | ||
| ); |
There was a problem hiding this comment.
We need an accurate representation of the subscriberWrappers. We use the setState function to check the current state to pass it into the onEmoji handler.
| /** | ||
| * Subscribes to a stream in a session, managing the receiving audio and video from the remote party. | ||
| * We are disabling the default SDK UI to have more control on the display of the subscriber | ||
| * Ref for Vonage Unified https://vonage.github.io/conversation-docs/video-js-reference/latest/Session.html#subscribe | ||
| * Ref for Opentok https://tokbox.com/developer/sdks/js/reference/Session.html#subscribe | ||
| * @param {Stream} stream - The stream to which the user is subscribing. | ||
| * @param {Session} localSession - The session in which the stream is located. | ||
| * @param {SubscriberProperties} [options={}] - Optional properties to configure the subscriber. | ||
| * @returns {Promise<void>} A promise that resolves when the subscription is set up. | ||
| */ | ||
| const subscribe = useCallback( | ||
| async (stream: Stream, localSession: Session, options: SubscriberProperties = {}) => { | ||
| const { streamId } = stream; | ||
| if (localSession) { | ||
| const finalOptions: SubscriberProperties = { | ||
| ...options, | ||
| insertMode: 'append', | ||
| width: '100%', | ||
| height: '100%', | ||
| preferredResolution: 'auto', | ||
| style: { | ||
| buttonDisplayMode: 'off', | ||
| nameDisplayMode: 'on', | ||
| }, | ||
| insertDefaultUI: false, | ||
| }; | ||
| const isScreenshare = stream.videoType === 'screen'; | ||
| const subscriber = localSession.subscribe(stream, undefined, finalOptions); | ||
| subscriber.on('videoElementCreated', (event: VideoElementCreatedEvent) => { | ||
| const { element } = event; | ||
| const subscriberWrapper: SubscriberWrapper = { | ||
| element, | ||
| subscriber, | ||
| isScreenshare, | ||
| isPinned: false, | ||
| // subscriber.id is refers to the targetElement id and will be undefined when insertDefaultUI is false so we use streamId to track our subscriber | ||
| id: streamId, | ||
| }; | ||
| // prepend new subscriber to beginning of array so that is is visible on joining | ||
| setSubscriberWrappers((previousSubscriberWrappers) => | ||
| [subscriberWrapper, ...previousSubscriberWrappers].sort( | ||
| sortByDisplayPriority(activeSpeakerIdRef.current) | ||
| ) | ||
| ); | ||
| }); | ||
|
|
||
| subscriber.on('destroyed', () => { | ||
| activeSpeakerTracker.current.onSubscriberDestroyed(streamId); | ||
| const isNotDestroyedStreamId = ({ id }: { id: string }) => streamId !== id; | ||
| setSubscriberWrappers((prevSubscriberWrappers) => | ||
| prevSubscriberWrappers.filter(isNotDestroyedStreamId) | ||
| ); | ||
| }); | ||
|
|
||
| // Create moving average tracker and add handler for subscriber audioLevelUpdated event emitted periodically with subscriber audio volume | ||
| // See for reference: https://developer.vonage.com/en/video/guides/ui-customization/general-customization#adjusting-user-interface-based-on-audio-levels | ||
| const getMovingAverageAudioLevel = createMovingAvgAudioLevelTracker(); | ||
| subscriber.on('audioLevelUpdated', ({ audioLevel }) => { | ||
| const { logMovingAvg } = getMovingAverageAudioLevel(audioLevel); | ||
| activeSpeakerTracker.current.onSubscriberAudioLevelUpdated({ | ||
| subscriberId: streamId, | ||
| movingAvg: logMovingAvg, | ||
| }); | ||
| }); | ||
| } | ||
| }, | ||
| [] | ||
| ); | ||
|
|
||
| // handle the subscription (Receiving media from remote parties) | ||
| const handleStreamCreated = (e: StreamCreatedEvent) => { | ||
| if (session.current) { | ||
| subscribe(e.stream, session.current); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Moved to the VonageVideoClient file
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the session context to isolate the Vonage Video API logic into a dedicated plain-old TypeScript class (VonageVideoClient) and updates related hooks, tests, and the session provider accordingly. Key changes include the introduction of the VonageVideoClient class, refactored hooks (useScreenShare, useEmoji, useChat) to use the new client, and comprehensive updates to tests and context integration.
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| frontend/src/utils/VonageVideoClient/vonageVideoClient.ts | Adds a new class to manage Vonage Video session logic |
| frontend/src/hooks/ | Updates dependencies from session to vonageVideoClient in hooks |
| frontend/src/Context/SessionProvider/session.ts | Refactors session provider to use VonageVideoClient and new event flow |
| Others (tests, PublisherProvider, etc.) | Adapts tests and supporting files to the new client and context design |
Comments suppressed due to low confidence (1)
frontend/src/Context/PublisherProvider/usePublisher/usePublisher.tsx:240
- The recursive call to publish() in the catch block can lead to infinite recursion if the error persists. Add a retry limit or introduce a delay mechanism to prevent potential stack overflow.
publish();
What is this PR doing?
Refactors the session context so that the Vonage Video API parts are inside a separate non-react, plain-old
JavaScriptTypeScript class. This will allow for easier testing in the future, e.g. captions 👀 🤐. We talked about doing this since the pinning PR since that was running into issues with mocking. Thanks for all of the discussions and sorry I didn't finish this before you left, Mike 😢note: After IT wiped my computer and reinstalled some
corporate spywaresecurity software, I forgot to resume auto-signing commits. It was more trouble than it was worth to manually re-sign and cherry-pick the commits. The previous branches are here: #140 and here: #155How should this be manually tested?
What are the relevant tickets?
A maintainer will add this ticket number.
Resolves VIDCS-3442
Checklist
[✅] Branch is based on
develop(notmain).[ ] Resolves a
Known Issue.[ ] If yes, did you remove the item from the
docs/KNOWN_ISSUES.md?[ ] Resolves an item reported in
Issues.If yes, which issue? Issue Number?