fix(filmstrip) Ensures dominant speaker is always visible in filmstrip#16901
fix(filmstrip) Ensures dominant speaker is always visible in filmstrip#16901jallamsetty1 merged 10 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where dominant speakers weren't always visible in the filmstrip when there were limited visible slots. The fix refactors the filmstrip participant ordering logic to explicitly reserve a slot for the dominant speaker and maintains alphabetical sorting for stable ordering.
Changes:
- Refactored filmstrip ordering logic to reserve a slot for the dominant speaker before allocating slots to other active speakers
- Replaced
speakersListMap withactiveSpeakersSet in the participants reducer for simpler state management - Added comprehensive test coverage with 7 participants to test limited slot scenarios, screensharing interactions, and stable alphabetical ordering
- Extended test infrastructure to support 7 concurrent browser instances (p1-p7)
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/wdio.conf.ts | Added browser instances p5, p6, p7 to support tests with 7 participants |
| tests/specs/media/activeSpeaker.spec.ts | Added 3 comprehensive tests for dominant speaker visibility with limited slots, screensharing, and stable ordering |
| tests/helpers/types.ts | Extended IContext and IParticipantOptions types to include p5, p6, p7 |
| tests/helpers/participants.ts | Added ensureSevenParticipants helper and updated hangupAllParticipants to support 7 participants |
| tests/helpers/Participant.ts | Added P5, P6, P7 constants |
| react/features/filmstrip/functions.any.ts | Refactored updateRemoteParticipants to explicitly handle dominant speaker slot reservation and alphabetical sorting |
| react/features/base/participants/reducer.ts | Replaced speakersList Map with activeSpeakers Set for cleaner state management |
| react/features/base/participants/functions.ts | Removed obsolete getActiveSpeakersToBeDisplayed function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (dominant && !dominant.local && participantsWithScreenShare.indexOf(dominant.id) === -1) { | ||
| dominantSpeakerSlot = 1; | ||
| remoteParticipants.delete(dominant.id); | ||
| speakers.push(dominant.id); | ||
| } | ||
|
|
||
| return acc; | ||
| }, []); | ||
| // Find the number of slots available for speakers. | ||
| const slotsForSpeakers | ||
| = visibleRemoteParticipants.size - (screenShareParticipants.length * 2) - sharedVideos.length - dominantSpeakerSlot; | ||
|
|
||
| // Construct the list of speakers to be shown. | ||
| if (slotsForSpeakers > 0) { | ||
| Array.from(activeSpeakers).slice(0, slotsForSpeakers).forEach((speakerId: string) => { | ||
| speakers.push(speakerId); | ||
| remoteParticipants.delete(speakerId); | ||
| }); |
There was a problem hiding this comment.
The dominant speaker is added to the speakers array (line 68) and removed from remoteParticipants (line 67), but is NOT removed from activeSpeakers. Then on lines 77-80, speakers from activeSpeakers are added to the speakers array. Since activeSpeakers (from the reducer, line 160) includes the dominant speaker in the previousSpeakers array, the dominant speaker could be added to the speakers array twice - once at line 68 and again at line 78.
This would cause the dominant speaker to appear twice in the final reorderedParticipants array at line 88-93. To fix this, after line 68, add: activeSpeakers.delete(dominant.id); (though note this requires fixing the Redux state mutation issue first by making activeSpeakers a mutable copy).
There was a problem hiding this comment.
active speakers contains the list of the most recent speakers and excludes the dominant speaker.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
jallamsetty1
force-pushed
the
fix-filmstrip-reordering
branch
from
717f7ba to
28164e3
Compare
2 participants
No description provided.