fix(client-presence): Announce 'attendeeConnected' for self-attendee #26123

WillieHabi · 2026-01-05T22:14:22Z

Description

Fixes AB#56686 -- self-attendee is announced via 'attendeeConnected' when the local client connects.

To test this, we added prepareDisconnectedPresence test helper which creates a presence instance in a disconnected state. This allows us to test and setup event listeners before initial connection occurs. We then refactor prepareConnectedPresence to use this as an intermediate step.

We also refactor usage of client2 as the local client in our unit testing to be more explicit. This is so when we do prepareConnectedPresence, the client is only added to the audience during the connect step.

List of test added:

- PresenceManager
    - self attendee
        - is announced via `attendeeConnected` when presence initializes while self is in Audience
        - has status "Disconnected" when presence initializes while self not in Audience
        - has status "Connected" when announced via `attendeeConnected`
        - is announced via `attendeeConnected` when local client reconnects

- self attendee - is announced via 'attendeeConnected' when local client connects - has status "Connected" when announced via 'attendeeConnected' - is announced via 'attendeeConnected' when local client reconnects

Copilot

Pull request overview

This PR fixes issue AB#56686 by ensuring the self-attendee is announced via the attendeeConnected event when the local client connects or reconnects. Previously, this event was not being emitted for the self-attendee due to a TODO comment blocking the implementation.

Key changes:

Enabled the attendeeConnected event emission for self-attendee in systemWorkspace.ts
Added prepareDisconnectedPresence test helper to support testing connection events from a disconnected state
Added three comprehensive test cases to verify self-attendee connection announcement behavior

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
packages/framework/presence/src/systemWorkspace.ts	Removed TODO comment and enabled event emission for self-attendee connection
packages/framework/presence/src/test/testUtils.ts	Added new test helper function to create presence instances in disconnected state for testing connection events
packages/framework/presence/src/test/presenceManager.spec.ts	Added test suite for self-attendee connection events and updated existing tests to exclude self-attendee from assertions

Copilot · 2026-01-05T22:19:26Z

packages/framework/presence/src/test/testUtils.ts

+			// Pass time (to mimic likely response)
+			clock.tick(broadcastJoinResponseDelaysMs.namedResponder + 20);
+
+			// Send a fake join response


The comment on line 351 is missing contextual information that would help readers understand the purpose of the fake join response. The similar comment in prepareConnectedPresence (lines 227-230) includes a helpful explanation: "There are no other attendees in the session (not realistic) but this convinces the presence manager that it now has full knowledge, which enables it to respond to other's join requests accurately." Consider adding this explanation here as well for consistency and better code maintainability.

Suggested change

// Send a fake join response

// Send a fake join response. There are no other attendees in the session (not realistic) but

// this convinces the presence manager that it now has full knowledge, which enables it to

// respond to other's join requests accurately.

jason-ha · 2026-01-06T22:25:22Z

runtime: MockEphemeralRuntime,

How is prepareConnectedPresence different from calling prepareDisconnectedPresence followed by using connectPresence that it returns?

Refers to: packages/framework/presence/src/test/testUtils.ts:141 in a11008f. [](commit_id = a11008f, deletion_comment = False)

jason-ha

self-Attendee section should also have a test for attendeeDisconnected.

packages/framework/presence/src/test/presenceManager.spec.ts

…nce with new helper functions

…elf not in Audience

WillieHabi · 2026-01-08T01:21:11Z

runtime: MockEphemeralRuntime,

How is prepareConnectedPresence different from calling prepareDisconnectedPresence followed by using connectPresence that it returns?

Refers to: packages/framework/presence/src/test/testUtils.ts:141 in a11008f. [](commit_id = a11008f, deletion_comment = False)

prepareConnectedPresence before would return instantiate presence but it wouldn't go through the full connected flow (runtime.connect). I changed this now so it does go through connect flow by using 'prepareDisconnectedPresence' + 'connect'.

jason-ha

I didn't get through looking at everything, but here are some notes.
Also still missing a self attendeeDisconnected case.

jason-ha · 2026-01-08T07:09:13Z

packages/framework/presence/src/test/testUtils.ts

 	processSignal: ProcessSignalFunction;
 	localAvgLatency: number;
 } {
 	const localAvgLatency = 10;


Doesn't seem to be connected to anything anymore beyond return.

packages/framework/presence/src/test/testUtils.ts

jason-ha · 2026-01-08T08:49:40Z

packages/framework/presence/src/test/mockEphemeralRuntime.ts

+			// Add to quorum as a write client (consistent with buildClientDataArray creating write clients)
+			const quorumSize = this.quorum.getMembers().size;
+			this.quorum.addMember(newClientId, {
+				client: newMember,
+				sequenceNumber: 10 * quorumSize,
+			});


Hmm. I think there are a number of questions for this change:

why is it needed / desired?

assuming it is kept, where is the related removal?

the timing does not appear to be correct. quorum is an op-based construct. This connect is should only go up to minimal connection status. Ops that change quorum would come later.

There is no check to see that client is a write client and would appear in quorum.

Note that the Audience that Presence cares about is the Signal-only Audience.

I'll ended up adding a syncWithQuorum method to MockEphemeralRuntime instead of doing this. I think that's better for testing purposes and more just correct, but here's the responses I wrote before and my thought process.

When we connect/disconnect our local client in our testing, there's currently no mechanism to keep quorum in sync. This is desirable/needed for example when we test responding to ClientJoin -- since quorum is not synced with connect flow, the local client does not appear in quorum and cannot compute its join order. This only worked before because we used client2 as the designated local client and tested against it's hardcoded quorum sequenceNumber/join order. But if we had a test where we disconnect + reconnect with new clientID and try to test the same scenario it would all fall apart since quorum is not synced with the local connection flow.

I saw we handle removal of clients from audience and quorum in runtime.removeMember(). But in runtime.disconnect() we don't handle removal of local client from either audience or quorum, which doesn't seem right since it's impossible for local client to disconnect and still be in quorum. I think maybe calling removeMember(this.clientId) in disconnect() would make sense here(?)

Yeah this callout makes sense to me, the timing of audience and quorum removal at the same time is not right.

We build the new client doing buildClientDataArray([clientId], 1 \* numWriteClients *\) so we always connect as write client.

Note that Quorum is a funny beast. When you join a document as a writer, it takes note of you being there. When you join later Quorum and traditional Audience will show add- and removeMember for everyone in the history (I think we don't see it too often because Summarization is aggressive and squashes the history.)
We largely do not require Quorum any longer. We do leverage it as fallback when other Join mechanism don't perform as intended. I hope that is very rare now. And things should be functional whether our local write client is in Quorum at time of demand or not. Ideally, we have coverage for both. What does our coverage look like for presenceDatastoreManager.ts lines 857 to 877 running just the Presence > protocol handling tests?

packages/framework/presence/src/test/presenceManager.spec.ts

packages/framework/presence/src/test/testUtils.ts

jason-ha · 2026-01-09T19:41:16Z

packages/framework/presence/src/test/mockEphemeralRuntime.ts

 		this.audience.removeMember(clientId);
 	}

+	public syncWithQuorum(): void {


This says "sync" but it doesn't do anything with quorum members that are no longer in audience.
Mostly it needs comments.

jason-ha · 2026-01-09T19:42:50Z

packages/framework/presence/src/test/mockEphemeralRuntime.ts

+				const quorumSize = this.quorum.getMembers().size;
+				this.quorum.addMember(clientId, {
+					client,
+					sequenceNumber: 10 * quorumSize,


This sequence is questionable. sequenceNumber should always be going up, even if in the past quorum members were removed. At least add comments.

jason-ha · 2026-01-09T19:44:48Z

packages/framework/presence/src/test/mockEphemeralRuntime.ts

-		// finally add new connection
+		// finally add new connection to both audience


What does "both audience" mean?
(I can assign it meaning, but without extra knowledge it is hard to know what it might mean.)

jason-ha · 2026-01-09T19:51:19Z

packages/framework/presence/src/test/testUtils.ts

+ * Note: This is intentionally not in the initial audience so that tests can
+ * properly simulate connecting a client that wasn't previously connected.


I think initialLocalClientConnectionId should be in audience most of the time at Presence connection. At least I think that is the common case and it will be the exception that it is not in audience.
Can we have it in by default and pull it out for the special test cases?

jason-ha · 2026-01-09T21:49:02Z

packages/framework/presence/src/test/schemaValidation/protocol.spec.ts

 			// Process remote client update signal (attendeeId-1 is then part of local client's known session).
 			const attendee1UpdateSendTimestamp = deltaToStart - 20;
 			const attendee1AvgLatency = 20;
+			const localAvgLatency = 10;


Prefer not to have this hard coded. There is magic number in connect that matters. We can restore that, right?

jason-ha · 2026-01-09T22:36:20Z

packages/framework/presence/src/test/testUtils.ts

-			}
-		}
-	}
-	// From PresenceDatastoreManager.joinSession:


"// From PresenceDatastoreManager.joinSession" remains dropped

jason-ha · 2026-01-09T22:37:37Z

packages/framework/presence/src/test/testUtils.ts

-		clock.tick(broadcastJoinResponseDelaysMs.namedResponder + 20);
+	const connect = (clientConnectionId: ClientConnectionId): void => {
+		// This logic needs to be kept in sync with datastore manager.
+		// From PresenceDatastoreManager.getInteractiveMembersExcludingSelf:


pre-existing: comment is stale and should be updated (it was renamed to getAudienceInformation some time back.)

jason-ha · 2026-01-09T22:44:51Z

packages/framework/presence/src/test/presenceManager.spec.ts

 							// Simulate remote client disconnect (while local is disconnected)
 							runtime.audience.removeMember(knownAttendee.getConnectionId());
-							runtime.connect("client8", "client2"); // Simulate local client reconnect with new connection id
+							runtime.connect("client8", initialLocalClientConnectionId); // Simulate local client reconnect with new connection id


Feels like "client8" should get similar treatment improving clarity.

WillieHabi added 3 commits January 5, 2026 13:23

Add prepareDisconnectedPresence util to testUtils.ts

cfded76

Add self-attendee tests to presenceManager.spec.ts:

1e15826

- self attendee - is announced via 'attendeeConnected' when local client connects - has status "Connected" when announced via 'attendeeConnected' - is announced via 'attendeeConnected' when local client reconnects

emit "attendeeConnected" for self-attendee

a11008f

Copilot AI review requested due to automatic review settings January 5, 2026 22:14

Copilot started reviewing on behalf of WillieHabi January 5, 2026 22:14 View session

Copilot AI reviewed Jan 5, 2026

View reviewed changes

WillieHabi requested a review from jason-ha January 5, 2026 23:19

jason-ha reviewed Jan 6, 2026

View reviewed changes

packages/framework/presence/src/test/presenceManager.spec.ts Outdated Show resolved Hide resolved

packages/framework/presence/src/test/presenceManager.spec.ts Outdated Show resolved Hide resolved

packages/framework/presence/src/test/presenceManager.spec.ts Outdated Show resolved Hide resolved

WillieHabi added 5 commits January 7, 2026 14:45

remove duplicate code in testUtils by composing prepareConnectedPrese…

499857c

…nce with new helper functions

fix assert to be more clear

3a15cf6

restructure attendeeConnected assert

ed55c2c

Add test: has status "Disconnected" when presence initializes while s…

75ab983

…elf not in Audience

replace usage of selfAttendeeId and selfClientConnectionId

40372df

WillieHabi requested a review from jason-ha January 8, 2026 04:02

jason-ha reviewed Jan 8, 2026

View reviewed changes

WillieHabi added 3 commits January 8, 2026 15:18

requested changes

0ef728f

no need to specify initial when naming attendeeId since it is stable

87c473e

attendeeDisconnected test

cce825b

WillieHabi requested a review from jason-ha January 9, 2026 17:32

jason-ha reviewed Jan 9, 2026

View reviewed changes

-			// Send a fake join response
+			// Send a fake join response. There are no other attendees in the session (not realistic) but
+			// this convinces the presence manager that it now has full knowledge, which enables it to
+			// respond to other's join requests accurately.

		// finally add new connection
		// finally add new connection to both audience

		* Note: This is intentionally not in the initial audience so that tests can
		* properly simulate connecting a client that wasn't previously connected.

fix(client-presence): Announce 'attendeeConnected' for self-attendee #26123

Are you sure you want to change the base?

fix(client-presence): Announce 'attendeeConnected' for self-attendee #26123

Conversation

WillieHabi commented Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 5, 2026

Choose a reason for hiding this comment

Uh oh!

jason-ha commented Jan 6, 2026

Uh oh!

jason-ha left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

WillieHabi commented Jan 8, 2026

Uh oh!

jason-ha left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WillieHabi Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

WillieHabi commented Jan 5, 2026 •

edited

Loading

WillieHabi Jan 8, 2026 •

edited

Loading