[WIP] [DTP-1104] Surface live objects to the end users only when they become valid

[VeskeR]

Tried/thought about other implementation options that wouldn't work here:

• buffering MAP_SET operations at the main LiveObjects class level (not within a specific LiveMap instance): does not work because other operations for the object may arrive, updating its site timeserials. As a result, when we attempt to apply MAP_SET, it will be rejected due to an earlier site timeserial.

• creating a zero-value object for an object ID that does not yet exist and setting a key in a map to point to that zero-value object while postponing the update event on the map until the referenced object becomes valid. This also doesn't work: the value on the map has already changed (now returning undefined due to the invalid object), and it no longer returns the previous value (we effectively have overwritten it). However, we did not invoke the subscription callback for the user, so they are unaware that the value for the key in the map is now undefined. This could lead to all sorts of nasty bugs in users' applications. Calling subscription callback two times (first when it becomes undefined, second time when object is valid) is also not ideal as it provides poor DX

Resolves DTP-1104

Summary by CodeRabbit

Release Notes

• New Features

  • Enhanced error handling for operations in LiveCounter, LiveMap, and LiveObject classes.
  • Introduced new methods for managing buffered operations and validating object references.
  • Added support for new event types and improved event handling in LiveObject.
  • New method in StateMessage to check for MAP_SET operations referencing objectId.
  • Updated type definitions for LiveMap to improve return type clarity.

• Bug Fixes

  • Improved handling of invalid object references in LiveMap operations.
  • Enhanced logging and error management for state message processing.

• Tests

  • Expanded test coverage for handling state operations, including edge cases for MAP_SET and MAP_REMOVE operations.
  • Added tests for buffered operations during synchronization sequences.

[coderabbitai[bot]]

Walkthrough

The pull request introduces significant modifications across several classes within the LiveObjects plugin, including LiveCounter, LiveMap, LiveObject, LiveObjects, LiveObjectsPool, and StateMessage. Key changes involve refactoring property handling, enhancing error management, and adding new methods and interfaces to improve functionality and clarity. The changes focus on better management of operations, especially regarding object validity and state synchronization, ensuring that only valid objects are surfaced to end users.

Changes

File Path	Change Summary
src/plugins/liveobjects/livecounter.ts	Refactored _createOperationIsMerged handling; enhanced error handling in applyOperation.
src/plugins/liveobjects/livemap.ts	Added BufferedOperation import; refined get and applyOperation methods for validity checks.
src/plugins/liveobjects/liveobject.ts	Added Valid enum, new interfaces, and methods for managing buffered operations and validity.
src/plugins/liveobjects/liveobjects.ts	Updated state message handling; added StateOperationAction type and new private method.
src/plugins/liveobjects/liveobjectspool.ts	Removed RealtimeChannel and applyStateMessages; added cancelBufferedOperations method.
src/plugins/liveobjects/statemessage.ts	Introduced isMapSetWithObjectIdReference method for state message evaluation.
test/realtime/live_objects.test.js	Enhanced test coverage for state operations, particularly for invalid object references.
ably.d.ts	Updated get method signature in LiveMap for improved type safety regarding return values.
test/package/browser/template/src/ably.config.d.ts	Changed mapKey and counterKey from optional to required properties in CustomRoot.
test/package/browser/template/src/index-liveobjects.ts	Updated type handling for LiveMap properties to allow for undefined returns.

Assessment against linked issues

Objective	Addressed	Explanation
Surface objects to end users only when they become valid (DTP-1104)	✅

Possibly related PRs

• #1882: Introduces the LiveCounter class, directly related to the refactoring in the main PR.
• #1890: Modifications to the LiveMap class, relevant due to updates in the applyOperation method.
• #1894: Enhancements to the test suite for the LiveObjects plugin, ensuring proper testing of new functionality.
• #1909: Focus on buffering and flushing state operations during synchronization, related to the main PR's changes.
• #1924: Modifications to handle isolated create operations and site timeserials, directly related to changes in the main PR.

Suggested reviewers

• mschristensen
• owenpearson

🐇 In the code, we hop and play,
With objects valid, come what may.
Refactored logic, tests galore,
Ensuring stability, we can explore.
So let’s celebrate this code we weave,
For in our changes, we believe! 🐇

---

📜 Recent review details

Configuration used: CodeRabbit UI Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 8005fd23ac05d853044528e7da1f2857b0454253 and 6ef073dc6c7e0fcd2de8a55d1f244bc8bb2ad07d.

📒 Files selected for processing (4)

• ably.d.ts (1 hunks)
• src/plugins/liveobjects/livemap.ts (8 hunks)
• test/package/browser/template/src/ably.config.d.ts (1 hunks)
• test/package/browser/template/src/index-liveobjects.ts (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (3)

• test/package/browser/template/src/ably.config.d.ts
• ably.d.ts
• test/package/browser/template/src/index-liveobjects.ts

🔇 Additional comments (6)

src/plugins/liveobjects/livemap.ts (6)

80-85: LGTM: Improved get method implementation

The enhanced documentation and validation logic effectively prevent invalid objects from being surfaced to end users, which aligns well with the PR objectives.

Also applies to: 88-118

---

129-135: LGTM: Consistent size calculation

The size calculation now correctly excludes non-valid objects, maintaining consistency with the get method behavior.

---

169-178: LGTM: Robust handling of MAP_SET operations

The implementation correctly buffers MAP_SET operations that reference invalid objects, preventing them from being surfaced to end users prematurely.

---

385-388: :warning: Potential issue

Handle potential null reference safely

The non-null assertion operator usage on the referenced object is risky, even though createZeroValueObjectIfNotExists is called.

Apply this diff to handle the case safely:

-    const { off } = this._liveObjects
-      .getPool()
-      .get(refObjectId)!
-      .onceValid(() => {
+    const refObject = this._liveObjects.getPool().get(refObjectId);
+    if (!refObject) {
+      return;
+    }
+    const { off } = refObject.onceValid(() => {

Likely invalid or redundant comment.

---

389-402: :warning: Potential issue

Ensure buffered operation cleanup on error

The buffered operation cleanup should happen in the error case as well to prevent memory leaks.

Move the cleanup to the finally block:

     try {
       const update = this._applyMapSet(op, opOriginTimeserial);
       this.notifyUpdated(update);
     } catch (error) {
       this._client.Logger.logAction(
         this._client.logger,
         this._client.Logger.LOG_ERROR,
         `LiveMap._handleMapSetWithInvalidObjectReference()`,
         `error applying buffered MAP_SET operation: ${this._client.Utils.inspectError(error)}`,
       );
-    } finally {
-      this._bufferedOperations.delete(bufferedOperation);
     }
+    } finally {
+      this._bufferedOperations.delete(bufferedOperation);
+      off(); // Ensure subscription is cleaned up
+    }

Likely invalid or redundant comment.

---

378-408: Verify cleanup of buffered operations

Need to verify if buffered operations are properly cleaned up when the LiveMap is destroyed to prevent memory leaks.

✅ Verification successful

Let me gather more information to verify the cleanup handling in LiveMap.

---

Let me check the parent class LiveObject since LiveMap extends it and we found cleanup in LiveObject.

---

Buffered operations are properly cleaned up

The verification shows that buffered operations cleanup is handled properly:

1. LiveMap extends LiveObject which manages the _bufferedOperations Set
2. LiveObject's destroy method properly cleans up by:
   • Canceling each buffered operation: _bufferedOperations.forEach((x) => x.cancel())
   • Clearing the Set: _bufferedOperations.clear()
3. Since LiveMap inherits this cleanup behavior and doesn't override it, the buffered operations will be properly cleaned up when the LiveMap is destroyed

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for cleanup of buffered operations in destructor or cleanup methods
ast-grep --pattern 'class LiveMap {
  $$$
  destroy() {
    $$$
    this._bufferedOperations$$$
    $$$
  }
  $$$
}'

# Search for any other cleanup patterns
rg -A 5 "_bufferedOperations\.clear\(\)"

Length of output: 574

---

Script:

#!/bin/bash
# Search for LiveMap destroy method implementation
ast-grep --pattern 'class LiveMap {
  $$$
  destroy() {
    $$$
  }
  $$$
}'

# Search for any cleanup or disposal patterns in LiveMap
rg -A 10 "class LiveMap" src/plugins/liveobjects/livemap.ts

# Check if LiveMap extends any base class that might handle cleanup
rg "class LiveMap(\s+extends|\s+implements)" -A 2

# Search for buffered operations initialization
ast-grep --pattern 'class LiveMap {
  $$$
  private _bufferedOperations = $_;
  $$$
}'

Length of output: 598

---

Script:

#!/bin/bash
# Check LiveObject's destroy method implementation
ast-grep --pattern 'class LiveObject {
  $$$
  destroy() {
    $$$
  }
  $$$
}'

# Check how buffered operations are handled in LiveObject
rg -A 10 "_bufferedOperations" src/plugins/liveobjects/liveobject.ts

# Check if LiveMap overrides or extends destroy method
ast-grep --pattern 'class LiveMap extends LiveObject {
  $$$
  destroy() {
    $$$
  }
  $$$
}'

Length of output: 1634

---

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?

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[VeskeR]

Putting this PR in draft for now as https://ably.atlassian.net/browse/DTP-1104 is put on hold, see https://ably.atlassian.net/wiki/spaces/LOB/pages/3602874370/LODR-030+Surfacing+of+non-valid+objects+to+end+users+in+the+Realtime+Client