[DTP-1035] Add Batch write API for LiveObjects

[VeskeR]

Resolves DTP-1035

Summary by CodeRabbit

• New Features

  • Introduced batch processing functionality for Live Objects.
  • Added support for batch context management with new classes:
    • BatchContext
    • BatchContextLiveCounter
    • BatchContextLiveMap
  • Implemented ability to batch multiple operations in a single channel message.

• Bug Fixes

  • Enhanced error messaging for unknown live object types.

• Tests

  • Added comprehensive test scenarios for the batch API, focusing on synchronous behavior, state management, and error handling.

• Chores

  • Updated allowed files for LiveObjects plugin bundle.

[coderabbitai]

Warning

There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure.

🔧 ast-grep (0.31.1)

test/realtime/live_objects.test.js

An unexpected error occurred while running ast-grep.

Walkthrough

The pull request introduces a batch processing mechanism for Live Objects in the CodeRabbit system. It adds three new classes—BatchContext, BatchContextLiveCounter, and BatchContextLiveMap—to support batch operations. Additionally, a new batch method is introduced in the LiveObjects class, enabling developers to group multiple operations into a single channel message. The changes enhance the plugin's functionality by providing a structured approach to manage and execute batched live object modifications.

Changes

File	Change Summary
scripts/moduleReport.ts	Added 3 new files to allowedFiles set for LiveObjects plugin
src/plugins/liveobjects/batchcontext.ts	Introduced BatchContext class for managing batch context and wrapped objects
src/plugins/liveobjects/batchcontextlivecounter.ts	Added BatchContextLiveCounter for managing live counter operations in batch
src/plugins/liveobjects/batchcontextlivemap.ts	Implemented BatchContextLiveMap for batch live map operations
src/plugins/liveobjects/liveobjects.ts	Added BatchCallback type and batch method to LiveObjects class
test/realtime/live_objects.test.js	Added new test scenarios for batch API functionality and behavior

Assessment against linked issues

Objective	Addressed	Explanation
Implement Batch write API [DTP-1035]	✅

Possibly related PRs

• [DTP-1024] GC tombstoned map entries for LiveMap and objects in the global pool #1937: The main PR adds new entries to the allowedFiles set in the checkLiveObjectsPluginFiles function, which is related to the management of LiveObjects, while the retrieved PR focuses on garbage collection for LiveMap and objects, indicating a shared context in managing LiveObjects.

Suggested reviewers

• mschristensen

Poem

🐰 Batch by batch, we hop along,
LiveObjects dancing to our song
Counters, maps in harmony sway
CodeRabbit's magic on display!
Operations grouped with rabbit flair 🚀

✨ Finishing Touches

• 📝 Generate Docstrings (Beta)

---

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 generate docstrings to generate docstrings for this PR. (Beta)
• @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.

[coderabbitai]

Caution

Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/plugins/liveobjects/liveobjects.ts (1)

60-74: Add error handling and improve documentation.

While the implementation is solid, consider these improvements:

1. Add error handling in the try block
2. Document potential error scenarios in JSDoc

+/**
+ * Provides access to the synchronous write API for LiveObjects that can be used to batch multiple operations together in a single channel message.
+ * @param callback - Function that receives a BatchContext and performs batch operations
+ * @throws {Error} When the channel is in an invalid state
+ * @throws {Error} When batch operations fail
+ * @returns Promise that resolves when all batch operations are completed
+ */
 async batch(callback: BatchCallback): Promise<void> {
   const root = await this.getRoot();
   const context = new BatchContext(this, root);

   try {
     callback(context);
     await context.flush();
+  } catch (error) {
+    throw error;
   } finally {
     context.close();
   }
 }

🛑 Comments failed to post (1)

src/plugins/liveobjects/batchcontextlivecounter.ts (1)

36-36: ⚠️ Potential issue

Remove unnecessary return statement in decrement method

The decrement method is declared to return void, but it includes a return statement. Since the increment method also returns void, the return statement is redundant and may cause confusion.

Apply this diff to fix the issue:

-    return this.increment(-amount);
+    this.increment(-amount);

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    this.increment(-amount);

🧰 Tools

🪛 Biome (1.9.4)

[error] 36-36: The function should not return a value because its return type is void.

The function is here:

'void' signals the absence of value. The returned value is likely to be ignored by the caller.

(lint/correctness/noVoidTypeReturn)

[mschristensen]

Really nice, I love the use of composition here. No tests in this PR, but since this is PRd into the other branch I'm happy for us to add this separately.

[VeskeR]

No tests in this PR,

oh I see, I think the commit got lost during the rebase/branch cleanup I was doing with all the write API PRs, let me restore it and push it

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (6)

src/plugins/liveobjects/batchcontext.ts (3)

34-59: Ensure Consistent Error Handling in getWrappedObject Method

In the getWrappedObject method, when an unknown object type is encountered, an ErrorInfo is thrown with error code 50000 and status code 500. This indicates a server error. However, since this is a client-side validation, consider using a 400 series error code to represent a client error.

Apply this diff to adjust the error code:

-          throw new this._client.ErrorInfo(
-            `Unknown Live Object instance type: objectId=${originObject.getObjectId()}`,
-            50000,
-            500,
-          );
+          throw new this._client.ErrorInfo(
+            `Unknown Live Object instance type: objectId=${originObject.getObjectId()}`,
+            40000,
+            400,
+          );

---

65-67: Improve Error Message Clarity in throwIfClosed Method

The error message "Batch is closed" might not provide enough context to the user. Consider updating the error message to be more descriptive, such as "Cannot perform operation: Batch context is already closed."

Apply this diff to enhance the error message:

-        if (this.isClosed()) {
-          throw new this._client.ErrorInfo('Batch is closed', 40000, 400);
-        }
+        if (this.isClosed()) {
+          throw new this._client.ErrorInfo('Cannot perform operation: Batch context is already closed', 40000, 400);
+        }

---

13-13: Avoid Potential Memory Leaks with WeakMap

The _wrappedObjects map holds references to wrapped objects, which could prevent them from being garbage collected if not managed carefully. Consider using a WeakMap to allow for garbage collection of wrapped objects when they are no longer in use.

Apply this diff to use WeakMap:

-  private _wrappedObjects: Map<string, BatchContextLiveCounter | BatchContextLiveMap<API.LiveMapType>> = new Map();
+  private _wrappedObjects: WeakMap<string, BatchContextLiveCounter | BatchContextLiveMap<API.LiveMapType>> = new WeakMap();

Also applies to: 57-57

src/plugins/liveobjects/batchcontextlivecounter.ts (1)

22-26: Consider Input Validation for increment Method

In the increment method, there is no type checking for the amount parameter. To prevent potential errors, consider adding a type check similar to the one in the decrement method to ensure that amount is a number.

Apply this diff to add input validation:

      increment(amount: number): void {
        this._batchContext.throwIfClosed();
+       if (typeof amount !== 'number') {
+         throw new this._client.ErrorInfo('Counter value increment should be a number', 40012, 400);
+       }
        const stateMessage = LiveCounter.createCounterIncMessage(this._liveObjects, this._counter.getObjectId(), amount);
        this._batchContext.queueStateMessage(stateMessage);
      }

src/plugins/liveobjects/liveobjects.ts (2)

64-65: Avoid Unnecessary Await in batch Method

In the batch method, there is an unnecessary await when calling this.getRoot(). Since the getRoot method is awaited but its result is not used in this context, you can remove the await to improve efficiency.

Apply this diff to remove the unnecessary await:

      async batch(callback: BatchCallback): Promise<void> {
-       const root = await this.getRoot();
+       const root = this.getRoot();
        const context = new BatchContext(this, root);

Alternatively, if root is needed synchronously, ensure that it's properly awaited and utilized.

---

302-305: Ensure Consistent Error Codes in _applySync Method

In the _applySync method, when an unknown live object type is encountered, an ErrorInfo is thrown with error code 50000 and status code 500. Similar to previous comments, consider using a 400 series error code to represent client-side errors.

Apply this diff to adjust the error code:

            default:
              throw new this._client.ErrorInfo(`Unknown Live Object type: ${objectType}`, 50000, 500);
+             throw new this._client.ErrorInfo(`Unknown Live Object type: ${objectType}`, 40000, 400);

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2a33708 and 8901f46.

📒 Files selected for processing (5)

• scripts/moduleReport.ts (1 hunks)
• src/plugins/liveobjects/batchcontext.ts (1 hunks)
• src/plugins/liveobjects/batchcontextlivecounter.ts (1 hunks)
• src/plugins/liveobjects/batchcontextlivemap.ts (1 hunks)
• src/plugins/liveobjects/liveobjects.ts (4 hunks)

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

• scripts/moduleReport.ts
• src/plugins/liveobjects/batchcontextlivemap.ts

🧰 Additional context used

🪛 Biome (1.9.4)

src/plugins/liveobjects/batchcontextlivecounter.ts

[error] 36-36: The function should not return a value because its return type is void.

The function is here:

'void' signals the absence of value. The returned value is likely to be ignored by the caller.

(lint/correctness/noVoidTypeReturn)

⏰ Context from checks skipped due to timeout of 90000ms (7)

• GitHub Check: test-node (20.x)
• GitHub Check: test-browser (webkit)
• GitHub Check: test-node (18.x)
• GitHub Check: test-browser (firefox)
• GitHub Check: test-browser (chromium)
• GitHub Check: test-npm-package
• GitHub Check: test-node (16.x)

🔇 Additional comments (1)

src/plugins/liveobjects/batchcontextlivecounter.ts (1)

17-20: Ensure Consistent State Retrieval in value Method

The value method retrieves the current counter value without considering any queued increments or decrements within the batch context. This might lead to confusion as the returned value won't reflect pending operations. Consider documenting this behavior or adjusting the method to account for queued operations.

Would you like to adjust the value method to include pending increments/decrements within the batch context, or should we document the current behavior more clearly?

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (3)

src/plugins/liveobjects/batchcontext.ts (1)

94-101: 🛠️ Refactor suggestion

Handle exceptions during publishing in flush method

If an error occurs during this._liveObjects.publish(this._queuedMessages), the cleanup code (_wrappedObjects.clear() and _queuedMessages = []) will not execute, potentially leading to memory leaks or inconsistent state. Wrap the publish call in a try...finally block to ensure resources are cleaned up even if an error occurs.

Apply this diff to handle exceptions:

  async flush(): Promise<void> {
    this.close();
+   try {
      if (this._queuedMessages.length > 0) {
        await this._liveObjects.publish(this._queuedMessages);
      }
+   } finally {
      this._wrappedObjects.clear();
      this._queuedMessages = [];
+   }
  }

src/plugins/liveobjects/batchcontextlivecounter.ts (1)

28-37: ⚠️ Potential issue

Remove unnecessary return statement in decrement method

The decrement method is declared with a return type of void, but it contains a return statement that returns the result of this.increment(-amount). Since increment also returns void, returning it is unnecessary and may cause confusion.

Apply this diff to remove the unnecessary return:

  decrement(amount: number): void {
    this._batchContext.throwIfClosed();
    // do an explicit type safety check here before negating the amount value,
    // so we don't unintentionally change the type sent by a user
    if (typeof amount !== 'number') {
      throw new this._client.ErrorInfo('Counter value decrement should be a number', 40013, 400);
    }

-   return this.increment(-amount);
+   this.increment(-amount);
  }

🧰 Tools

🪛 Biome (1.9.4)

[error] 36-36: The function should not return a value because its return type is void.

The function is here:

'void' signals the absence of value. The returned value is likely to be ignored by the caller.

(lint/correctness/noVoidTypeReturn)

src/plugins/liveobjects/liveobjects.ts (1)

63-73: 🛠️ Refactor suggestion

Handle asynchronous operations in batch method

If the callback function performs asynchronous operations without returning a promise or being awaited, context.flush() may be called before those operations complete, leading to unexpected behavior. Consider modifying the BatchCallback type to handle asynchronous functions and await the callback to ensure all operations are completed before flushing.

Apply this diff to support asynchronous BatchCallback:

-type BatchCallback = (batchContext: BatchContext) => void;
+type BatchCallback = (batchContext: BatchContext) => Promise<void> | void;

And modify the batch method accordingly:

  async batch(callback: BatchCallback): Promise<void> {
    const root = await this.getRoot();
    const context = new BatchContext(this, root);

    try {
-     callback(context);
+     await callback(context);
      await context.flush();
    } finally {
      context.close();
    }
  }

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between aabfdc3 and da39bcb.

📒 Files selected for processing (5)

• scripts/moduleReport.ts (1 hunks)
• src/plugins/liveobjects/batchcontext.ts (1 hunks)
• src/plugins/liveobjects/batchcontextlivecounter.ts (1 hunks)
• src/plugins/liveobjects/batchcontextlivemap.ts (1 hunks)
• src/plugins/liveobjects/liveobjects.ts (4 hunks)

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

• scripts/moduleReport.ts
• src/plugins/liveobjects/batchcontextlivemap.ts

🧰 Additional context used

🪛 Biome (1.9.4)

src/plugins/liveobjects/batchcontextlivecounter.ts

[error] 36-36: The function should not return a value because its return type is void.

The function is here:

'void' signals the absence of value. The returned value is likely to be ignored by the caller.

(lint/correctness/noVoidTypeReturn)

⏰ Context from checks skipped due to timeout of 90000ms (6)

• GitHub Check: test-node (20.x)
• GitHub Check: test-browser (webkit)
• GitHub Check: test-browser (firefox)
• GitHub Check: test-node (18.x)
• GitHub Check: test-browser (chromium)
• GitHub Check: test-node (16.x)

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (2)

src/plugins/liveobjects/batchcontext.ts (1)

26-29: ⚠️ Potential issue

Handle potential undefined return value in getRoot method

In the getRoot method, getWrappedObject(ROOT_OBJECT_ID) could return undefined if the root object is not found. Casting it directly to BatchContextLiveMap<T> without checking could lead to runtime errors. Please add a check to handle the undefined case appropriately.

Apply this diff to handle the potential undefined value:

 getRoot<T extends API.LiveMapType = API.DefaultRoot>(): BatchContextLiveMap<T> {
   this.throwIfClosed();
+  const rootObject = this.getWrappedObject(ROOT_OBJECT_ID);
+  if (!rootObject) {
+    throw new this._client.ErrorInfo('Root object not found', 50000, 500);
+  }
-  return this.getWrappedObject(ROOT_OBJECT_ID) as BatchContextLiveMap<T>;
+  return rootObject as BatchContextLiveMap<T>;
 }

src/plugins/liveobjects/liveobjects.ts (1)

63-73: ⚠️ Potential issue

Ensure asynchronous callbacks are properly awaited in batch method

The batch method's callback function might perform asynchronous operations. Currently, you are not awaiting the callback execution, which could result in context.flush() being called before the asynchronous operations complete. This may lead to unexpected behavior or incomplete batch operations. Consider updating the BatchCallback type to allow asynchronous functions and await the callback.

Apply this diff to enforce an asynchronous BatchCallback and await the callback:

-type BatchCallback = (batchContext: BatchContext) => void;
+type BatchCallback = (batchContext: BatchContext) => Promise<void> | void;

And modify the batch method accordingly:

 async batch(callback: BatchCallback): Promise<void> {
   const root = await this.getRoot();
   const context = new BatchContext(this, root);

   try {
-    callback(context);
+    await callback(context);
     await context.flush();
   } finally {
     context.close();
   }
 }

🧹 Nitpick comments (1)

src/plugins/liveobjects/liveobjects.ts (1)

310-310: Ensure consistency in error messages for unknown Live Object types

The error message when encountering an unknown Live Object type is "Unknown Live Object type: ${objectType}". In batchcontext.ts, a similar error message includes the objectId. For consistency and better debugging, consider including the objectId in this error message as well.

Apply this diff to enhance the error message:

-          throw new this._client.ErrorInfo(`Unknown Live Object type: ${objectType}`, 50000, 500);
+          throw new this._client.ErrorInfo(`Unknown Live Object type: ${objectType}, objectId=${objectId}`, 50000, 500);

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between da39bcb and a58dcd0.

📒 Files selected for processing (5)

• scripts/moduleReport.ts (1 hunks)
• src/plugins/liveobjects/batchcontext.ts (1 hunks)
• src/plugins/liveobjects/batchcontextlivecounter.ts (1 hunks)
• src/plugins/liveobjects/batchcontextlivemap.ts (1 hunks)
• src/plugins/liveobjects/liveobjects.ts (4 hunks)

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

• scripts/moduleReport.ts
• src/plugins/liveobjects/batchcontextlivecounter.ts
• src/plugins/liveobjects/batchcontextlivemap.ts

⏰ Context from checks skipped due to timeout of 90000ms (6)

• GitHub Check: test-node (20.x)
• GitHub Check: test-browser (webkit)
• GitHub Check: test-browser (firefox)
• GitHub Check: test-node (18.x)
• GitHub Check: test-node (16.x)
• GitHub Check: test-browser (chromium)

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (2)

src/plugins/liveobjects/batchcontext.ts (1)

26-29: ⚠️ Potential issue

Handle undefined return value from getWrappedObject in getRoot

In the getRoot method, getWrappedObject(ROOT_OBJECT_ID) could potentially return undefined if the root object is not found. Casting it directly to BatchContextLiveMap<T> without checking could lead to runtime errors. Consider adding a check to handle the undefined case and throw an appropriate error.

Apply this diff to handle the undefined return value:

  getRoot<T extends API.LiveMapType = API.DefaultRoot>(): BatchContextLiveMap<T> {
    this.throwIfClosed();
-   return this.getWrappedObject(ROOT_OBJECT_ID) as BatchContextLiveMap<T>;
+   const rootObject = this.getWrappedObject(ROOT_OBJECT_ID);
+   if (!rootObject) {
+     throw new this._client.ErrorInfo('Root object not found', 50000, 500);
+   }
+   return rootObject as BatchContextLiveMap<T>;
  }

src/plugins/liveobjects/liveobjects.ts (1)

63-73: ⚠️ Potential issue

Ensure batch method handles asynchronous callbacks properly

In the batch method, if the callback function performs asynchronous operations without returning a promise or being awaited, context.flush() may be called before the operations complete. This could lead to unexpected behavior. Consider modifying the BatchCallback type to accept an asynchronous function and await the callback to ensure all operations are completed before flushing.

Apply this diff to enforce an asynchronous BatchCallback:

-type BatchCallback = (batchContext: BatchContext) => void;
+type BatchCallback = (batchContext: BatchContext) => Promise<void> | void;

And modify the batch method accordingly:

  async batch(callback: BatchCallback): Promise<void> {
    const root = await this.getRoot();
    const context = new BatchContext(this, root);

    try {
-     callback(context);
+     await callback(context);
      await context.flush();
    } finally {
      context.close();
    }
  }

🧹 Nitpick comments (1)

test/realtime/live_objects.test.js (1)

2856-3131: Add tests for asynchronous operations within batch method

The current tests cover synchronous operations within the batch method. To ensure the batch method handles asynchronous callbacks correctly, please add tests where the callback function passed to batch is asynchronous and performs asynchronous operations (e.g., awaits promises). These tests should verify that the batch method waits for all asynchronous operations to complete before calling context.flush().

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 45c7863 and a28901d.

📒 Files selected for processing (6)

• scripts/moduleReport.ts (1 hunks)
• src/plugins/liveobjects/batchcontext.ts (1 hunks)
• src/plugins/liveobjects/batchcontextlivecounter.ts (1 hunks)
• src/plugins/liveobjects/batchcontextlivemap.ts (1 hunks)
• src/plugins/liveobjects/liveobjects.ts (4 hunks)
• test/realtime/live_objects.test.js (1 hunks)

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

• scripts/moduleReport.ts
• src/plugins/liveobjects/batchcontextlivecounter.ts
• src/plugins/liveobjects/batchcontextlivemap.ts

⏰ Context from checks skipped due to timeout of 90000ms (6)

• GitHub Check: test-node (20.x)
• GitHub Check: test-browser (webkit)
• GitHub Check: test-node (18.x)
• GitHub Check: test-browser (firefox)
• GitHub Check: test-node (16.x)
• GitHub Check: test-browser (chromium)