fix(control-plane): dedupe presence list by participantId#622
Conversation
📝 WalkthroughWalkthroughgetPresenceList() now deduplicates presence entries by participantId, merging status as "active" if any socket is active and using the most recent lastSeen. Durable-object close logic refreshes presence when other sockets remain. Unit and integration tests verify these behaviors. ChangesPresence List Deduplication
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ColeMurray
left a comment
There was a problem hiding this comment.
[Automated Review] Requesting changes — see inline comment for the symmetric presence_leave bug not addressed by this PR. The dedupe fix here is correct, but its mirror image on disconnect partially undoes the user-visible improvement.
When a participant holds multiple WebSocket connections (e.g. two browser tabs) and one disconnects, the previous code broadcast presence_leave with the user's userId. The web client filters all entries by userId on presence_leave, so the user would disappear from every viewer's UI even though they were still connected on another tab — partially undoing the dedupe-by-participantId fix. Check whether any other authenticated socket still belongs to the same participantId before deciding which message to send. If yes, broadcast a deduped presence_update via broadcastPresence(). If not, fall back to the existing presence_leave path. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/control-plane/test/integration/websocket-client.test.ts (1)
275-278: ⚡ Quick winExtract
timeoutMsinto a named constant with unit.Both new tests restate
2000. Please define one local constant (for example,PRESENCE_MESSAGE_TIMEOUT_MS) and reuse it.As per coding guidelines, "Define each default timeout value exactly once as a named constant and import it everywhere rather than restating literal values" and "Do not use bare timeout variable names; always encode the unit (seconds for Python, milliseconds for TypeScript) in the variable name".
Also applies to: 300-303
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/control-plane/test/integration/websocket-client.test.ts` around lines 275 - 278, Introduce a named constant for the timeout (e.g., PRESENCE_MESSAGE_TIMEOUT_MS = 2000) and replace the hard-coded 2000 in the collectMessages call(s) with that constant; update both occurrences (the collector created for tab1.ws and the similar call around lines 300-303) so they use PRESENCE_MESSAGE_TIMEOUT_MS to encode milliseconds and avoid repeating the literal.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/control-plane/test/integration/websocket-client.test.ts`:
- Around line 275-278: Introduce a named constant for the timeout (e.g.,
PRESENCE_MESSAGE_TIMEOUT_MS = 2000) and replace the hard-coded 2000 in the
collectMessages call(s) with that constant; update both occurrences (the
collector created for tab1.ws and the similar call around lines 300-303) so they
use PRESENCE_MESSAGE_TIMEOUT_MS to encode milliseconds and avoid repeating the
literal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: df0ede4c-837e-4dc5-9c7d-c026a5eb9fff
📒 Files selected for processing (2)
packages/control-plane/src/session/durable-object.tspackages/control-plane/test/integration/websocket-client.test.ts
Fixes a bug where a prompt engineer would show up twice (or more) based upon how many tabs they had open. This change ensures that they only show up once in the list of prompt engineers.
Summary
getPresenceList()output byparticipantId: any active socket marks the participant active, and the most recentlastSeenacross sockets wins.Summary by CodeRabbit