fix(client-presence): Remove connectionId for self attendee disconnect #23022
Conversation
github-actions
bot
added
area: framework
There was a problem hiding this comment.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no suggestions.
Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more
There was a problem hiding this comment.
Code Coverage Summary
↓ packages.framework.presence.src:
Line Coverage Change: -0.02% Branch Coverage Change: No change
| Metric Name | Baseline coverage | PR coverage | Coverage Diff |
|---|---|---|---|
| Branch Coverage | 84.05% | 84.05% | → No change |
| Line Coverage | 62.14% | 62.12% | ↓ -0.02% |
Baseline commit: 25935ec
Baseline build: 304843
Happy Coding!!
Code coverage comparison check passed!!
⯅ @fluid-example/bundle-size-tests: +245 Bytes
Baseline commit: 25935ec |
| runtime.on("disconnected", () => { | ||
| if (runtime.clientId !== undefined) { | ||
| this.removeClientConnectionId(runtime.clientId); | ||
| } | ||
| }); |
There was a problem hiding this comment.
We can keep this runtime dependency out of here and place it in PresenceManagerDataObject where the existing call to removeClientConnectionId is. (We want to keep the runtime dependencies to a minimum.) I assume from this that audience never says "removeMember" for self.
There was a problem hiding this comment.
I think I made the wrong comment above. It is cool to have the call here (perhaps better at least for now). The more important thing is consistency. We can move the PresenceManagerDataObject call to here since audience is being added to IEmphemeralRuntime (in #23006) and remove ClientConnectionManager that requires removeClientConnectionId to be public. Related comment from past: #22833 (comment)
|
I think the best way to test this would be through unit testing and manual testing of external controller. I will redirect conversation to this PR #23006 since this change is apart of it and this is where all the test reworking is. |
Description
We currently use Audience to listen for when remote clients disconnect from the session before removing their connection Id and setting their status to disconnected.
This works perfectly fine for remote clients, but for the local client this is not possible since "removeMember" event is not sent to the local client that disconnected.
To fix this we can listen to "disconnected" event on the ephemeral runtime (same way we listen to "connected") to know when the local client disconnects.