[MM-68559][MM-68560][MM-68562] Browser screen share with audio publish, subscribe, and end detection#1179
Merged
Merged
Conversation
Implement logic to remove entries from the hosts and didRingForCalls states when a call ends, ensuring proper state management for active calls.
- Added support for screen sharing events, including handling of remote and local screen streams. - Updated action types and reducers to manage screen sharing state more effectively. - Refactored websocket handlers to dispatch screen sharing actions. - Cleaned up unused code and improved type definitions for better maintainability.
- Renamed reducer imports for consistency and clarity. - Removed obsolete reducer files for screen sharing IDs and session management, streamlining the codebase. - Ensured proper state management by consolidating reducer logic.
| session_id: call.screen_sharing_session_id, | ||
| }, | ||
| }); | ||
| actions.push(userScreenShared(channelID, call.screen_sharing_session_id, call.owner_id)); |
Contributor
There was a problem hiding this comment.
I think the last argument should be the user ID of the user sharing screen. So maybe:
const sharer = call.screen_sharing_session_id
? call.sessions.find((s) => s.session_id === call.screen_sharing_session_id)
: undefined;
actions.push(userScreenShared(channelID, call.screen_sharing_session_id, sharer?.user_id ?? ''));
| @@ -0,0 +1,3 @@ | |||
| // Copyright (c) 2020-present Mattermost, Inc. All Rights Reserved. | |||
| // See LICENSE.txt for license information. | |||
|
|
|||
Contributor
There was a problem hiding this comment.
I presume you plan to put something here, or maybe using these four files are following convention?
|
|
||
| logDebug(`CallClient: remote track published from ${userID}`, remoteTrackPublication); | ||
| if (remoteTrackPublication.source === Track.Source.ScreenShare || remoteTrackPublication.source === Track.Source.ScreenShareAudio) { | ||
| // Screen-share publications do not carry stream state before subscription. We wait for TrackSubscribed, |
Contributor
There was a problem hiding this comment.
Missing second comment line?
| logDebug('CallClient.shareScreen: another participant is already sharing, skipping'); | ||
| return null; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
What about if this client is already sharing?
| const {userID, sessionID} = this.parseUserIdAndSessionIdFromIdentity(localParticipant); | ||
| this.emit(CALL_EVENT.LOCAL_SCREEN_STREAM, screenShareStream, sessionID, userID); | ||
| logDebug(`CallClient: local screen share stream published for user ${userID}`, localTrackPublication); | ||
| } |
Contributor
There was a problem hiding this comment.
When sharing with audio, this will emit LOCAL_SCREEN_STREAM twice, once when screen video is published, and once when screen audio track is published. This second emission will hold both screen video and audio tracks. So this looks OK, but it's complicated, maybe add a comment?
|
|
||
| const emptyState: State = {}; | ||
|
|
||
| export const reducer: Reducer<State, Actions> = (initialState = emptyState, action) : State => { |
Contributor
There was a problem hiding this comment.
claude seems to think that "initialState" is a poor choice of variable name, and prefers "state". I leave it to you to decide.
bgardner8008
previously requested changes
May 15, 2026
Contributor
bgardner8008
left a comment
bgardner8008
left a comment
There was a problem hiding this comment.
Looks great, thanks. I think there is only one thing that needs fixing, passing the screen sharer ID. Rest are just comments.
…eliably by checking the session list, ensuring accurate state management during call reconnections.
…sure correct state updates.
…e publications before subscription.
…is already sharing, improving efficiency and user experience.
…g teardown before notifying the server, improving state consistency.
- Updated unshareScreen to return early without sending a WebSocket message when not connected. - Added error handling to emit an ERROR event if setScreenShareEnabled rejects. - Improved shareScreen to return the existing stream if already sharing and adjusted systemAudio handling based on the withAudio parameter.
…to mm_68559_68560_68562
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds full screen-sharing: CallClient implements share/unshare and stream composition with LiveKit; new Redux slice tracks per-channel sharer; CALL_EVENT constants standardize screen-stream events; components and websocket handlers wired to new actions; comprehensive tests and small config updates. ChangesScreen-Sharing Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webapp/src/actions.ts`:
- Around line 594-595: Guard the unconditional dispatch of userScreenShared
inside loadCallState: check that call.screen_sharing_session_id is truthy and
that call.sessions contains a matching session with a non-empty user_id (the
screenSharer found by call.sessions.find where session.session_id ===
call.screen_sharing_session_id). Only push actions.push(userScreenShared(...))
when both the session exists and screenSharer.user_id is present; otherwise skip
emitting the "screen on" action to avoid dispatching with empty IDs.
In `@webapp/src/state/screen_sharing_ids/reducer.ts`:
- Around line 40-43: The reducer currently sets the "not sharing" state by
assigning an empty-string sentinel with [action.data.channelID]: '' (using
initialState), which leaves stale keys; instead remove the channel key entirely:
in the reducer (the function handling this action that references initialState
and action.data.channelID) build a new state object that omits the channel key
(e.g., spread the existing state into newState and delete
newState[action.data.channelID] or use object destructuring to omit the key) and
return that new object; make the same change in the other branch that currently
uses an empty string (the block around the lines referenced as 58-61) so both
branches delete the channel key rather than set it to ''.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b888a420-b6bf-4bf4-88f8-78330359b08a
📒 Files selected for processing (22)
.gitignorestandalone/src/recording/components/recording_view/index.tsxwebapp/src/action_types.tswebapp/src/actions.tswebapp/src/clients/call/call_client.test.tswebapp/src/clients/call/call_client.tswebapp/src/clients/call/constants.tswebapp/src/clients/calls/index.tswebapp/src/clients/websocket/index.tswebapp/src/components/call_widget/component.tsxwebapp/src/components/expanded_view/call_settings.tsxwebapp/src/components/expanded_view/component.tsxwebapp/src/reducers.tswebapp/src/selectors.tswebapp/src/state/screen_sharing_ids/action_types.tswebapp/src/state/screen_sharing_ids/actions.tswebapp/src/state/screen_sharing_ids/reducer.tswebapp/src/state/screen_sharing_ids/selector.tswebapp/src/state/session/actions.tswebapp/src/state/session/reducer.tswebapp/src/state/session/selectors.tswebapp/src/websocket_handlers.ts
💤 Files with no reviewable changes (2)
- webapp/src/action_types.ts
- webapp/src/components/expanded_view/call_settings.tsx
… only if screen sharing session ID and user ID are present, enhancing state management. - Simplified reducer logic to delete channelID from state instead of setting it to an empty string, improving state clarity.
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This pull request includes several improvements and refactors related to screen sharing events and state management in the call/recording components and their tests. The main changes focus on consolidating screen sharing action handling, updating event usage for consistency, and improving test mocks.
Ticket Link
https://mattermost.atlassian.net/browse/MM-68562
https://mattermost.atlassian.net/browse/MM-68560
https://mattermost.atlassian.net/browse/MM-68559
Screenshots
Release Note