[DTP-1034] Emit Live Objects lifecycle events #1958
Conversation
Currently emits `deleted` upon receiving `OBJECT_DELETE` for the object DTP-1034
WalkthroughThe pull request introduces significant improvements to the event handling mechanism in the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/plugins/liveobjects/liveobject.ts (2)
32-36: Consider adding event payload type.The lifecycle event callback and response interfaces are well-defined but could be enhanced to support future event types that might need payload data.
Consider this enhancement:
-export type LiveObjectLifecycleEventCallback = () => void; +export type LiveObjectLifecycleEventCallback<T = void> = (data?: T) => void; export interface OnLiveObjectLifecycleEventResponse { off(): void; }
166-166: Consider adding a deleted timestamp to the event payload.The
deletedevent could benefit from including the_tombstonedAttimestamp in its payload for better tracking of when the deletion occurred.- this._lifecycleEvents.emit(LiveObjectLifecycleEvent.deleted); + this._lifecycleEvents.emit(LiveObjectLifecycleEvent.deleted, { deletedAt: this._tombstonedAt });src/plugins/liveobjects/liveobjects.ts (2)
167-170: Consider adding type guard for event parameter.Similar to the
LiveObjectclass, there's a defensive check for nil arguments. Consider adding a type guard for the event parameter.- if (this._client.Utils.isNil(event) && this._client.Utils.isNil(callback)) { + if (!Object.values(LiveObjectsEvent).includes(event) || this._client.Utils.isNil(callback)) { return; }
314-315: Consider adding sync metadata to event payload.The sync events could provide valuable metadata about the sync operation to help with debugging and monitoring.
- this._eventEmitterInternal.emit(LiveObjectsEvent.sync_start); - this._eventEmitterPublic.emit(LiveObjectsEvent.sync_start); + const syncStartMetadata = { syncId: this._currentSyncId, cursor: this._currentSyncCursor }; + this._eventEmitterInternal.emit(LiveObjectsEvent.sync_start, syncStartMetadata); + this._eventEmitterPublic.emit(LiveObjectsEvent.sync_start, syncStartMetadata);- this._eventEmitterInternal.emit(LiveObjectsEvent.sync_end); - this._eventEmitterPublic.emit(LiveObjectsEvent.sync_end); + const syncEndMetadata = { + syncId: this._currentSyncId, + bufferedOperationsCount: this._bufferedStateOperations.length + }; + this._eventEmitterInternal.emit(LiveObjectsEvent.sync_end, syncEndMetadata); + this._eventEmitterPublic.emit(LiveObjectsEvent.sync_end, syncEndMetadata);Also applies to: 329-330
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/plugins/liveobjects/liveobject.ts(8 hunks)src/plugins/liveobjects/liveobjects.ts(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (16.x)
- GitHub Check: test-browser (chromium)
🔇 Additional comments (5)
src/plugins/liveobjects/liveobject.ts (3)
6-7: LGTM! Consistent event naming convention.The event names follow the lowercase snake_case convention, which is consistent across both
LiveObjectSubscriptionEventandLiveObjectLifecycleEventenums.Also applies to: 28-30
43-44: LGTM! Good separation of event emitters.The separation of subscription and lifecycle events into distinct event emitters improves the code organization and maintainability.
Also applies to: 69-70
123-126: LGTM! Defensive programming.Good defensive check to prevent accidental removal of all event listeners when both arguments are nil.
src/plugins/liveobjects/liveobjects.ts (2)
14-16: LGTM! Clear sync event naming.The sync events follow a consistent naming pattern and clearly indicate the start and end of sync operations.
32-34: LGTM! Good documentation of event emitter separation.The comment referencing RTC10 provides clear context for why there are separate internal and public event emitters.
Functionally complete but not tests yet as some of them depend on DTP-1147. Worth revieweing this PR API side while I complete tests setup.
Resolves DTP-1034
Summary by CodeRabbit
New Features
on,off, andoffAllmethodsImprovements
sync_startandsync_end