fix: replace in-flight presence sync sequences#2261
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughPresence synchronization now resets residual membership state for every new sync and tracks sync identifiers to distinguish duplicate updates from new sequences. The RTP18a test is enabled unconditionally and its comments reflect the updated behavior. ChangesPresence sync lifecycle
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant PresenceUpdate
participant RealtimePresence
participant PresenceMap
PresenceUpdate->>RealtimePresence: deliver syncChannelSerial
RealtimePresence->>PresenceMap: startSync for a new syncId
PresenceMap->>PresenceMap: rebuild residualMembers
PresenceUpdate->>RealtimePresence: deliver sync end
RealtimePresence->>PresenceMap: endSync and clear syncId
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/common/lib/client/realtimepresence.ts (1)
307-309: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
onAttachedstarts a sync without setting_syncId, causing a redundantstartSync()on firstsetPresence.
onAttached()(line 358) callsthis.members.startSync()but never setsthis._syncId. When the firstsetPresencearrives with a realsyncId, the conditionsyncId !== this._syncId(string !== undefined) is true, triggering a secondstartSync()that resetsresidualMembersagain. This is currently harmless because no members are processed between the two calls, but it's an inconsistency that could become a bug if the flow changes.Consider setting
_syncIdinonAttachedor documenting that the redundant call is expected.🤖 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 `@src/common/lib/client/realtimepresence.ts` around lines 307 - 309, Update onAttached to assign the active sync identifier to _syncId when it calls members.startSync(), so the first setPresence does not trigger a redundant startSync through the syncId !== this._syncId check. Preserve the existing synchronization behavior for subsequent sync ID changes.
🤖 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.
Inline comments:
In `@src/common/lib/client/presencemap.ts`:
- Around line 140-142: Update startSync() when assigning residualMembers so the
copied map excludes entries whose action is 'absent'. Preserve all non-absent
entries and the existing sync replacement and setInProgress behavior.
---
Nitpick comments:
In `@src/common/lib/client/realtimepresence.ts`:
- Around line 307-309: Update onAttached to assign the active sync identifier to
_syncId when it calls members.startSync(), so the first setPresence does not
trigger a redundant startSync through the syncId !== this._syncId check.
Preserve the existing synchronization behavior for subsequent sync ID changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8492ebe0-9a14-49da-8f9c-55c0e5521923
📒 Files selected for processing (3)
src/common/lib/client/presencemap.tssrc/common/lib/client/realtimepresence.tstest/uts/realtime/unit/presence/presence_sync.test.ts
|
Addressed the tombstone edge case in Validation:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/common/lib/client/presencemap.ts`:
- Around line 140-148: Initialize residualMembers in the sync replacement logic
with Object.create(null) instead of a regular object, preserving the existing
filtering and assignments in the surrounding loop to prevent prototype-based
keys from affecting Object.prototype.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b161bb93-b527-4888-945e-e50357cccb03
📒 Files selected for processing (1)
src/common/lib/client/presencemap.ts
1 participant
Summary
Validation
npm run test:uts:unit(1,052 passing, 55 pending)npx mocha --no-config --require tsx/cjs test/uts/realtime/unit/presence/presence_sync.test.ts(14 passing)npx eslint src/common/lib/client/presencemap.ts src/common/lib/client/realtimepresence.tsnpx prettier --check src/common/lib/client/presencemap.ts src/common/lib/client/realtimepresence.ts test/uts/realtime/unit/presence/presence_sync.test.tsgit diff --checkFixes #2215
Summary by CodeRabbit