improvement(client-presence): Consistent event ordering and state results #23797
Conversation
github-actions
bot
added
area: framework
There was a problem hiding this comment.
Copilot reviewed 5 out of 8 changed files in this pull request and generated no comments.
Files not reviewed (3)
- packages/framework/presence/src/internalTypes.ts: Evaluated as low risk
- packages/framework/presence/src/latestMapValueManager.ts: Evaluated as low risk
- packages/framework/presence/src/latestValueManager.ts: Evaluated as low risk
Comments suppressed due to low confidence (1)
packages/framework/presence/src/notificationsManager.ts:254
- The return type of the
updatemethod should be consistent. If there are no listeners, it should return an empty array. This ensures that the caller can always expect an array, regardless of whether there are listeners or not.
return [];
rajatch-ff
requested review from
a team,
pragya91,
markfields,
jatgarg,
kian-thompson,
rajatch-ff and
MarioJGMsoft
and removed request for
a team
| }; | ||
| describe("State eventing", () => { |
There was a problem hiding this comment.
Not just state - order consistency should apply to notifications too. And beyond that the ordering matters for workspaceActivated (those need to happen early and let local registrations happen immediately, so they get updates especially for Notifications)
Also add a vertical space here.
There was a problem hiding this comment.
Copilot reviewed 5 out of 8 changed files in this pull request and generated no comments.
Files not reviewed (3)
- packages/framework/presence/src/systemWorkspace.ts: Evaluated as low risk
- packages/framework/presence/src/latestValueManager.ts: Evaluated as low risk
- packages/framework/presence/src/presenceStates.ts: Evaluated as low risk
| testPermutation(["latest", "notifications", "latestMap"]); | ||
| testPermutation(["latestMap", "latest", "notifications"]); | ||
| testPermutation(["latestMap", "notifications", "latest"]); |
There was a problem hiding this comment.
I don't think the cases with notification in the middle is legitimate. At least not while latest and latestMap cases share a workspace. All workspace updates will arrive together; so, notifications will never be in between.
It is good to have test case for two vm updates in a single workspace.
Wouldn't be a bad idea to have two states workspaces as well - then notifications could split them.
| // Test all possible permutations | ||
| testPermutation(["latest", "latestMap"]); | ||
| testPermutation(["latestMap", "latest"]); | ||
| testPermutation(["latest", "notifications"]); | ||
| testPermutation(["notifications", "latest"]); | ||
| testPermutation(["latestMap", "notifications"]); | ||
| testPermutation(["notifications", "latestMap"]); | ||
| testPermutation(["latest", "latestMap", "notifications"]); | ||
| testPermutation(["latest", "notifications", "latestMap"]); | ||
| testPermutation(["latestMap", "latest", "notifications"]); | ||
| testPermutation(["latestMap", "notifications", "latest"]); | ||
| testPermutation(["notifications", "latest", "latestMap"]); | ||
| testPermutation(["notifications", "latestMap", "latest"]); |
There was a problem hiding this comment.
I don't think we want to test all possible with the set we have. It is okay to have cases for in states workspace order permutation and then not have all of those permutations when testing multiple workspaces.
With that reduction, then there is space to test some other things like multiple LatestVMs in one workspace and multiple states workspaces. A NotificationsVM can also be in a states workspace.
| const attendeeUpdate = { | ||
| "clientToSessionId": { | ||
| "client1": { | ||
| "rev": 0, | ||
| "timestamp": 0, | ||
| "value": "sessionId-1", | ||
| }, | ||
| }, | ||
| }; | ||
| const latestUpdate = { | ||
| "latest": { | ||
| "sessionId-1": { | ||
| "rev": 1, | ||
| "timestamp": 0, | ||
| "value": { x: 1, y: 1, z: 1 }, | ||
| }, | ||
| }, | ||
| }; | ||
| const latestMapUpdate = { | ||
| "latestMap": { | ||
| "sessionId-1": { | ||
| "rev": 1, | ||
| "items": { | ||
| "key1": { | ||
| "rev": 1, | ||
| "timestamp": 0, | ||
| "value": { a: 1, b: 1 }, | ||
| }, | ||
| "key2": { | ||
| "rev": 1, | ||
| "timestamp": 0, | ||
| "value": { c: 1, d: 1 }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }; | ||
| const notificationsUpdate = { | ||
| "testEvents": { | ||
| "sessionId-1": { | ||
| "rev": 0, | ||
| "timestamp": 0, | ||
| "value": { "name": "newId", "args": [42] }, | ||
| "ignoreUnmonitored": true, | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Since these should never be changed, try adding as const to the end of all of them.
| const createUpdatePermutation = (order: string[]): Record<string, UpdateContent> => { | ||
| const updates: Record<string, UpdateContent> = { | ||
| "system:presence": attendeeUpdate, | ||
| }; | ||
|
|
||
| for (const key of order) { | ||
| switch (key) { | ||
| case "latest": { | ||
| const existingUpdates = updates["s:name:testWorkspace"] ?? {}; | ||
| updates["s:name:testWorkspace"] = { | ||
| ...existingUpdates, | ||
| ...latestUpdate, | ||
| }; | ||
| break; | ||
| } | ||
| case "latestMap": { | ||
| const existingUpdates = updates["s:name:testWorkspace"] ?? {}; | ||
| updates["s:name:testWorkspace"] = { | ||
| ...existingUpdates, | ||
| ...latestMapUpdate, | ||
| }; | ||
| break; | ||
| } | ||
| case "notifications": { | ||
| updates["n:name:testWorkspace"] = notificationsUpdate; | ||
| break; | ||
| } | ||
| default: { | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return updates; | ||
| }; |
There was a problem hiding this comment.
Generation is cool but it lead to an invalid pair of test cases. It also doesn't make it easy to see what the incoming data is - can't see what matters.
The alternative is to have an explicit array of update data. Usually that is good to pair with a simple description.
Here is a series of three:
testPermutation("latest before latestMap in single workspace", {
"s:name:testWorkspace": { ...latestUpdate, ...latestMapUpdate },
});
testPermutation("latestMap before latest in single workspace", {
"s:name:testWorkspace": { ...latestMapUpdate, ...latestUpdate },
});
testPermutation("Notifications workspace before States workspace", {
"n:name:testWorkspace": { ...notificationsUpdate },
"s:name:testWorkspace": { ...latestUpdate },
});
testPermutation("States workspace before Notifications workspace", {
"s:name:testWorkspace": { ...latestUpdate },
"n:name:testWorkspace": { ...notificationsUpdate },
});testPermutation could be
function testPermutation(desc:string, data: Record<"s:name:testWorkspace"|"n:name:testWorkspace",Partial<typeof latestUpdate & typeof latestMapUpdate & typeof notificationsUpdate>): void {
it(desc, async () => {
...
if ("latest" in (data["s:name:testWorkspace"] ?? {})) { assert(latestSpy.calledOnce...
...(Of course, when authored like that where description doesn't need formatted we prefer it("specific desc", async () => testPermutation({ ... }); because that lets us use .skip or .only.)
I chopped off "handles update order:" as I think it can go in an outer scope. Will make other comment.
| }, | ||
| }; | ||
|
|
||
| describe("ValueManager eventing", () => { |
There was a problem hiding this comment.
All Presence tests are placed under a Presence block.
Also, I think this can be "events are fired with consistent and final state". Maybe add "when" at end to align with the case names suggested elsewhere.
| const latestSpy = spy(() => { | ||
| const attendee = presence.getAttendee("client1"); | ||
| verifyFinalState(attendee, permutation); | ||
| }); | ||
|
|
||
| const latestMapSpy = spy(() => { | ||
| const attendee = presence.getAttendee("client1"); | ||
| verifyFinalState(attendee, permutation); | ||
| }); | ||
|
|
||
| const notificationsSpy = spy(() => { | ||
| const attendee = presence.getAttendee("client1"); | ||
| verifyFinalState(attendee, permutation); | ||
| }); |
There was a problem hiding this comment.
These implementations are all identical. Extract to helper function:
const verifyStateOnEvent = ...;
const latestSpy = spy(verifyStateOnEvent);
...| }); | ||
|
|
||
| latest.events.on("updated", latestSpy); | ||
| latestMap.events.on("updated", latestMapSpy); |
There was a problem hiding this comment.
LatestMapVM has two other events that should get covered. (The itemRemoved event will take more setup and cannot be mixed with attendeeJoined checking [unless you disconnect that client after the extra setup].)
|
nit: in PR description avoid "This PR" as the PR is merged with squash and description will be the commit description. You can say "This change". |
Description
For consistent data reads, events from value manager updates should be placed in a queue to be processed directly after the incoming signal is processed. Then any data explorations a customer may trigger from event will get current values.
This PR makes every workspace return a list of "post update actions" that must be executed after every updated is processed from the incoming signal. This makes sure that if a client gets receives event from one update, all other updates within the same datastore update message will be consistently reflected.
Tests:
Fixes AB#29542