-
Notifications
You must be signed in to change notification settings - Fork 346
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(plugin-meetings): moderated unmute when client is remotely muted #3995
base: next
Are you sure you want to change the base?
Conversation
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
console.log( | ||
`marcin: shouldIgnoreRemoteMuteUpdate: flag=${this.state.isRemoteUnmutePendingLocusDtoUpdate} remoteMute=${remoteMute}, isModifiedBySelf=${isModifiedBySelf}` | ||
); | ||
if (this.state.isRemoteUnmutePendingLocusDtoUpdate && isModifiedBySelf && !remoteMute) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only ever send remote unmute requests (not mute), so here I'm only checking for !remoteMute
, but the method that sends out these requests (sendRemoteMuteRequestToServer()
) is written in a way that suggests that it could do either mute or unmute, so I think I'll rename sendRemoteMuteRequestToServer()
to sendRemoteUnmuteRequestToServer()
and hardcode remoteMute = false
in it to make it more clear that we only do remote unmute, then this will be more in-line with the hardcoded assumption here in shouldIgnoreRemoteMuteUpdate()
bab60b0
to
0a56661
Compare
@@ -419,20 +419,27 @@ SelfUtils.mutedByOthersChanged = (oldSelf, changedSelf) => { | |||
throw new ParameterError('New self must be defined to determine if self was muted by others.'); | |||
} | |||
|
|||
const isModifiedBySelf = changedSelf.selfIdentity === changedSelf.modifiedBy; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't do the full check if we should ignore or not here, because selfUtils.ts doesn't have a reference to the meeting or MuteState class, so I've added the field isModifiedBySelf
that's emitted with the SELF_REMOTE_MUTE_STATUS_UPDATED
event and I've put all the logic to decide if we ignore the Locus DTO update or not in MuteState.shouldIgnoreRemoteMuteUpdate()
This PR reverts #3797 and improves the fix from #3621 by having more specific condition to decide when to ignore the Locus update