[DTP-950] Handle initial state sync sequence

[VeskeR]

This PR adds necessary realtime client changes to be able to handle new STATE_SYNC messages by the LiveObjects plugin. The pool initialization itself is implemented in #1890. I would suggest reviewing this PR commit by commit.
Bit flag indexes for new bit flags are based on this: https://github.com/ably/realtime/blob/e3d86b386850b81f09745c3ad3360c976173e0b1/go/realtime/lib/channel/flags.go#L22

Tests for SYNC sequence are added in #1894.

Resolves DTP-950

Summary by CodeRabbit

Summary by CodeRabbit

• New Features

  • Introduced the LiveObjectsPlugin for enhanced state management and message handling.
  • New types and properties added for improved channel modes and options.
  • Enhanced message decoding capabilities with new methods for state operations.
  • New classes and methods for managing live object data and synchronization processes.
  • Updated handling of protocol messages to include live object data.

• Bug Fixes

  • Updated protocol message handling to ensure consistency across various platforms.

• Documentation

  • Updated documentation to reflect new interfaces and methods related to the LiveObjectsPlugin.

[coderabbitai]

Actionable comments posted: 19

🧹 Outside diff range and nitpick comments (15)

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

16-18: Consider adding a type annotation for the _liveObjects parameter

To improve type safety and code readability, consider adding a type annotation for the _liveObjects parameter in the constructor.

- constructor(private _liveObjects: LiveObjects) {
+ constructor(private _liveObjects: LiveObjects) {

---

1-35: Solid foundation for live object data management

The overall design of this module provides a good foundation for managing live object data. The separation of the LiveObjectDataEntry interface and the SyncLiveObjectsDataPool class allows for flexibility and potential extension in the future.

Consider the following suggestions for future improvements:

1. Add methods for adding, updating, and removing entries from the pool.
2. Implement error handling for operations that might fail (e.g., accessing non-existent entries).
3. Consider adding a method to retrieve a single entry by key.
4. If thread-safety becomes a concern, consider implementing synchronization mechanisms.

These suggestions are not critical for the current implementation but might be valuable as the system evolves.

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

3-5: Consider using a more specific type for data.

Exporting the LiveObjectData interface is a good change as it allows for better type checking and reusability across the project. However, the use of any for the data property might be too permissive.

Consider using a more specific type or a generic type parameter for data to improve type safety. For example:

export interface LiveObjectData<T = unknown> {
  data: T;
}

This allows users of the interface to specify the type of data when needed, while still providing flexibility with a default of unknown.

---

10-10: Add documentation for the _regionalTimeserial property.

The addition of the _regionalTimeserial property is approved. However, its purpose and usage are not immediately clear from the context.

Consider adding a JSDoc comment to explain the purpose and expected format of the _regionalTimeserial property. For example:

/**
 * Represents the regional timeserial for this live object.
 * Format: [Add format description here]
 */
protected _regionalTimeserial?: string;

---

35-47: Consider adding validation to setData and setRegionalTimeserial methods.

The addition of setData and setRegionalTimeserial methods is approved. They provide controlled ways to update the protected properties.

Consider adding validation to these methods to ensure data integrity:

1. For setData, you might want to validate that newDataRef conforms to the expected structure of T.
2. For setRegionalTimeserial, you could add a check for the format of the regionalTimeserial string.

Example for setRegionalTimeserial:

setRegionalTimeserial(regionalTimeserial: string): void {
  if (!/^[a-zA-Z0-9:]+$/.test(regionalTimeserial)) {
    throw new Error('Invalid regional timeserial format');
  }
  this._regionalTimeserial = regionalTimeserial;
}

Please adjust the regex pattern according to the expected format of the regional timeserial.

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

25-27: Improved LiveMapData interface enhances type safety and functionality.

The modification to the LiveMapData interface is a positive change:

1. Extending LiveObjectData likely provides additional properties or methods, enhancing the interface's capabilities.
2. This change aligns well with the PR objectives of handling initial state sync sequence.

Consider adding a brief comment above the interface to explain the purpose of extending LiveObjectData and any new properties or methods it introduces. This would improve code documentation and make it easier for other developers to understand the changes.

---

Line range hint 32-46: Consider enhancing error handling and type safety in the get method.

While the get method implementation remains unchanged and works with the new LiveMapData interface, there's an opportunity for improvement:

1. Add type guards to ensure type safety when dealing with element.data.
2. Consider adding error handling for cases where element.data.objectId doesn't exist in the local pool.

Here's a suggested implementation with improved type safety and error handling:

get(key: string): LiveObject | StateValue | undefined {
  const element = this._dataRef.data.get(key);

  if (element === undefined) {
    return undefined;
  }

  if ('value' in element.data) {
    return element.data.value;
  } else if ('objectId' in element.data) {
    const liveObject = this._liveObjects.getPool().get(element.data.objectId);
    if (!liveObject) {
      console.warn(`LiveObject with id ${element.data.objectId} not found in the local pool.`);
    }
    return liveObject;
  }

  console.error(`Invalid element data structure for key: ${key}`);
  return undefined;
}

This implementation adds type guards, improves error handling, and provides more informative console messages for debugging.

src/platform/web/index.ts (1)

48-48: Approve export changes and suggest documentation update.

The export statements have been correctly updated to use the new makeProtocolMessageFromDeserialized function name, maintaining consistency with the import change.

Consider updating any relevant documentation or API references to reflect this change in function name and potentially its new behavior as a factory function.

Also applies to: 55-55

src/platform/react-native/index.ts (1)

Line range hint 1-59: Summary of changes and potential impact.

The changes in this file are part of a larger refactoring effort to adopt a factory pattern for creating protocol messages. The modifications are consistent and well-contained within the file. However, these changes may have a broader impact on the codebase.

Key points:

1. The import statement has been updated to use makeFromDeserializedWithDependencies.
2. The export object now includes makeProtocolMessageFromDeserialized.

To ensure a smooth transition:

1. Update all dependent modules to use the new function name.
2. Review and update any documentation or comments referencing the old function name.
3. Consider adding a deprecation notice for the old function name if backward compatibility is a concern.
4. Update any relevant unit tests to reflect these changes.

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

Line range hint 1-93: Suggestion: Add tests for createPM function

While the existing tests cover the main functionality of LiveObjects, there are no tests that directly use the createPM variable. To ensure the correctness of the new protocol message creation method, consider adding tests that utilize this function.

Here's a suggested test to add:

it('creates a protocol message with LiveObjectsPlugin', function() {
  const testMessage = { name: 'test', data: 'data' };
  const protocolMessage = createPM(testMessage);
  expect(protocolMessage).to.have.property('name', 'test');
  expect(protocolMessage).to.have.property('data', 'data');
  // Add more assertions based on the expected structure of the protocol message
});

This test will verify that the createPM function correctly creates a protocol message with the LiveObjectsPlugin integration.

src/common/lib/client/baserealtime.ts (1)

23-23: LGTM: Correct implementation of _LiveObjectsPlugin property

The new _LiveObjectsPlugin property is correctly implemented:

1. It's declared as readonly, ensuring immutability.
2. The type typeof LiveObjectsPlugin | null allows for cases where the plugin isn't provided.
3. The initialization in the constructor uses optional chaining, which is a safe approach.

The implementation is consistent with how the _RealtimePresence plugin is handled.

For consistency, consider moving the _LiveObjectsPlugin property declaration next to the _RealtimePresence property declaration.

Also applies to: 63-63

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

Line range hint 294-301: LGTM: API usage updated correctly

The changes in this test case correctly implement the new Ably.makeProtocolMessageFromDeserialized() API, maintaining consistency with previous test cases.

Consider refactoring the common setup code (lines 294-301) into a helper function to reduce duplication across test cases. This would improve maintainability and readability. For example:

function setupChannelWithProtocolMessage(helper, realtime, channelName) {
  const channel = realtime.channels.get(channelName);
  helper.recordPrivateApi('call.makeProtocolMessageFromDeserialized');
  helper.recordPrivateApi('call.channel.processMessage');
  return channel.processMessage(
    createPM({
      action: 11,
      channel: channelName,
      flags: 1,
    })
  );
}

This function could then be called at the beginning of each test case, reducing code duplication.

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

Line range hint 1-1149: Overall impact of protocol message creation method change

The change from protocolMessageFromDeserialized to makeProtocolMessageFromDeserialized() has been consistently applied in this test file. This modification appears to be part of a larger refactoring effort in the Ably library. The change is localized and doesn't seem to affect the logic of the individual test cases.

However, it's important to ensure that:

1. The new makeProtocolMessageFromDeserialized() method behaves identically to the old protocolMessageFromDeserialized for all test cases.
2. This change has been applied consistently across the entire codebase, not just in this test file.
3. Any documentation or API references have been updated to reflect this change.

Consider adding a test case that specifically verifies the behavior of the new makeProtocolMessageFromDeserialized() method to ensure it meets all the requirements of the old method.

src/common/lib/types/message.ts (2)

Line range hint 166-281: Initialize lastProcessedEncodingIndex to prevent potential undefined values

The variable lastProcessedEncodingIndex is declared but not initialized before being used and cast to a number in the finally block (line 264). This could lead to runtime errors if the variable remains undefined. Please initialize lastProcessedEncodingIndex to ensure it always holds a numeric value.

Apply this diff to initialize lastProcessedEncodingIndex:

 function decodeData(
   data: any,
   encoding: string | null | undefined,
   inputContext: CipherOptions | EncodingDecodingContext | ChannelOptions,
 ): Promise<{
   error?: ErrorInfo;
   data: any;
   encoding: string | null | undefined;
 }> {
   const context = normaliseContext(inputContext);
   let lastPayload = data;
   let decodedData = data;
   let finalEncoding: string | null | undefined = encoding;
   let decodingError: ErrorInfo | undefined = undefined;

   if (encoding) {
     const xforms = encoding.split('/');
+    let lastProcessedEncodingIndex = 0;
     let encodingsToProcess = xforms.length;
     let xform = '';

---

258-263: Preserve original error information when rethrowing

When catching an error during decoding, the original error is wrapped in a new ErrorInfo object. To aid in debugging, consider preserving the original error's stack trace and relevant information by setting it as the cause.

Modify the error handling to include the original error:

 const err = e as ErrorInfo;
-decodingError = new ErrorInfo(
+decodingError = new ErrorInfo(
   `Error processing the ${xform} encoding, decoder returned '${err.message}'`,
   err.code || 40013,
   400,
+  undefined,
+  err
 );

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 8783b8b and 3a34601.

📒 Files selected for processing (31)

• ably.d.ts (2 hunks)
• scripts/moduleReport.ts (2 hunks)
• src/common/lib/client/baserealtime.ts (2 hunks)
• src/common/lib/client/realtimechannel.ts (6 hunks)
• src/common/lib/transport/comettransport.ts (1 hunks)
• src/common/lib/transport/connectionmanager.ts (1 hunks)
• src/common/lib/transport/protocol.ts (1 hunks)
• src/common/lib/transport/transport.ts (1 hunks)
• src/common/lib/transport/websockettransport.ts (1 hunks)
• src/common/lib/types/message.ts (5 hunks)
• src/common/lib/types/protocolmessage.ts (7 hunks)
• src/platform/nativescript/index.ts (2 hunks)
• src/platform/nodejs/index.ts (2 hunks)
• src/platform/react-native/index.ts (2 hunks)
• src/platform/web/index.ts (2 hunks)
• src/plugins/liveobjects/index.ts (1 hunks)
• src/plugins/liveobjects/livecounter.ts (1 hunks)
• src/plugins/liveobjects/livemap.ts (2 hunks)
• src/plugins/liveobjects/liveobject.ts (2 hunks)
• src/plugins/liveobjects/liveobjects.ts (2 hunks)
• src/plugins/liveobjects/liveobjectspool.ts (1 hunks)
• src/plugins/liveobjects/statemessage.ts (1 hunks)
• src/plugins/liveobjects/syncliveobjectsdatapool.ts (1 hunks)
• test/common/modules/private_api_recorder.js (1 hunks)
• test/realtime/channel.test.js (6 hunks)
• test/realtime/connection.test.js (2 hunks)
• test/realtime/failure.test.js (2 hunks)
• test/realtime/live_objects.test.js (1 hunks)
• test/realtime/message.test.js (2 hunks)
• test/realtime/presence.test.js (2 hunks)
• test/realtime/sync.test.js (6 hunks)

✅ Files skipped from review due to trivial changes (2)

• src/common/lib/transport/connectionmanager.ts
• test/realtime/presence.test.js

🧰 Additional context used

🪛 Biome

src/plugins/liveobjects/liveobjects.ts

[error] 128-128: The assignment should not be in an expression.

The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.

(lint/suspicious/noAssignInExpressions)

src/plugins/liveobjects/statemessage.ts

[error] 168-168: Unsafe usage of optional chaining.

If it short-circuits with 'undefined' the evaluation will throw TypeError here:

(lint/correctness/noUnsafeOptionalChaining)

---

[error] 189-189: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

🔇 Additional comments (67)

src/plugins/liveobjects/index.ts (4)

2-2: LGTM: Import statement for StateMessage

The import statement for StateMessage is correctly formatted and uses an appropriate relative path. This addition aligns with the PR objectives of handling the initial state sync sequence.

---

4-4: LGTM: Named exports updated

The named exports have been correctly updated to include StateMessage along with LiveObjects. This change makes StateMessage available for external use, which is consistent with the import and the PR objectives.

---

6-9: LGTM: Default export updated

The default export has been correctly updated to include StateMessage along with LiveObjects. This change is consistent with the named exports and makes StateMessage available through the default import as well. The formatting follows the existing style.

---

1-9: Summary: StateMessage integration complete

The changes in this file successfully integrate the StateMessage module into the liveobjects plugin. The import, named exports, and default export have all been updated consistently to include StateMessage. These changes align with the PR objectives of handling the initial state sync sequence and make the StateMessage functionality available for use in other parts of the application.

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

1-1: LGTM: Import statement updated correctly.

The import statement has been appropriately updated to include LiveObjectData. This change aligns with the modification made to the LiveCounterData interface.

---

Line range hint 7-17: Unchanged LiveCounter class: Verify compatibility with updated interface.

The LiveCounter class remains unchanged, which suggests that its current implementation is still compatible with the updated LiveCounterData interface. However, it's worth double-checking if any adjustments or optimizations could be made to take advantage of the new relationship with LiveObjectData.

Please review the LiveCounter class implementation to ensure it fully leverages the properties and methods that might be inherited from LiveObjectData through the updated interface.

---

3-5: LGTM: Interface extension looks good. Verify impact on codebase.

The LiveCounterData interface now correctly extends LiveObjectData, establishing a proper hierarchical relationship. This change is consistent with the updated import statement and maintains backwards compatibility by retaining the data property.

To ensure this change doesn't introduce any unintended side effects, please run the following script to check for any other occurrences of LiveCounterData in the codebase:

This will help identify any areas that might need adjustment due to the interface change.

✅ Verification successful

LGTM: Interface extension confirmed with no broader impacts.

The LiveCounterData interface extension is properly implemented and only affects livecounter.ts. No additional occurrences of LiveCounterData were found in the codebase, ensuring that this change is isolated and does not introduce unintended side effects.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for other occurrences of LiveCounterData in the codebase
# Expected result: Only occurrences that are compatible with the new interface extension

rg --type typescript -A 5 'LiveCounterData'

Length of output: 78

---

Script:

#!/bin/bash
# Description: Search for other occurrences of LiveCounterData in .ts and .tsx files
# Expected result: Only occurrences that are compatible with the new interface extension

rg --glob '*.ts' --glob '*.tsx' -A 5 'LiveCounterData'

Length of output: 838

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

4-8: LGTM: Well-defined interface for LiveObjectDataEntry

The LiveObjectDataEntry interface is clearly defined with appropriate types for each property. The use of a union type for objectType effectively restricts the possible values to 'LiveMap' or 'LiveCounter'.

---

13-35: LGTM: Well-implemented SyncLiveObjectsDataPool class

The SyncLiveObjectsDataPool class is well-structured and encapsulates the data pool management effectively. The use of a Map for the pool is appropriate, and the provided methods cover the necessary operations for pool management. The @internal decorator is correctly used to indicate that this class is not intended for external use.

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

28-33: LGTM: getRegionalTimeserial method looks good.

The getRegionalTimeserial method is correctly implemented and properly marked as @internal. It provides a controlled way to access the protected _regionalTimeserial property.

---

Line range hint 1-56: Overall, the changes look good with some minor suggestions for improvement.

The modifications to the LiveObjectData interface and LiveObject class introduce useful functionality for handling regional timeserials and updating data. The new methods and properties are appropriately marked as internal and follow good TypeScript practices.

Key points from the review:

1. Consider using a more specific type for the data property in LiveObjectData.
2. Add documentation for the _regionalTimeserial property to clarify its purpose and usage.
3. Consider adding validation in the setData and setRegionalTimeserial methods to ensure data integrity.

These changes will enhance the robustness and maintainability of the code.

src/platform/nodejs/index.ts (2)

49-49: LGTM! Consistent with import change.

The update to the module exports is consistent with the change in the import statement. This maintains the external API of the module, which is excellent for preserving backward compatibility.

---

6-6: Consider reviewing related changes in the codebase.

While the changes in this file are minimal and maintain backward compatibility, they suggest more significant refactoring in the protocolmessage module. It would be prudent to review other parts of the codebase that directly import from the protocolmessage module to ensure they've been updated accordingly.

To help identify potentially affected areas, you can run the following script:

#!/bin/bash
# Description: Find files that import from the protocolmessage module

# Test: Search for imports from the protocolmessage module. Expect: List of files that might need updating.
rg --type typescript 'from .*protocolmessage' --glob '!src/platform/nodejs/index.ts'

Also applies to: 49-49

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

1-2: Improved imports enhance code organization and reusability.

The changes to the import statements are beneficial:

1. Including LiveObjectData from './liveobject' prepares for its usage in the LiveMapData interface.
2. Importing StateValue from './statemessage' instead of using a local definition reduces code duplication and improves maintainability.

These modifications align well with the PR objectives and contribute to better code organization.

---

Line range hint 1-54: Overall, the changes improve code organization and type safety.

The modifications to livemap.ts align well with the PR objectives:

1. The updated imports enhance code organization and reusability.
2. The LiveMapData interface extension improves type safety and functionality.
3. The existing implementation of LiveMap remains compatible with these changes.

While the core functionality remains intact, there are opportunities for further improvements in error handling and type safety, particularly in the get method.

These changes contribute positively to the handling of initial state sync sequence and the use of bit flag indexes as outlined in the PR objectives.

src/platform/nativescript/index.ts (2)

55-55: LGTM! Export statement updated correctly.

The export statement has been properly updated to use the new function name makeProtocolMessageFromDeserialized. This change is consistent with the import statement modification and ensures that the renamed function is correctly exported from this module.

---

Line range hint 1-56: Summary of changes and potential impact

The changes in this file are focused on updating the import and export of a single function, renamed from protocolMessageFromDeserialized to makeProtocolMessageFromDeserialized. While these changes have minimal impact on this specific file, they suggest a broader refactoring in the protocolmessage module.

Points to consider:

1. The new function name makeFromDeserializedWithDependencies implies that the function now requires dependencies to be passed. This change in signature might require updates in other parts of the codebase where this function is used.
2. Any code that previously used protocolMessageFromDeserialized will need to be updated to use the new function name and potentially adjust how it's called.

To ensure a smooth transition:

1. Review and update all usages of this function throughout the project.
2. Update any relevant documentation to reflect the new function name and usage.
3. Consider adding a deprecation notice or alias for the old function name to ease migration if this is a public API.

To identify potential areas that need updating, run the following script:

#!/bin/bash
# Description: Find potential areas that need updating due to the function rename

# Search for uses of the old function name across the project
echo "Searching for uses of 'protocolMessageFromDeserialized':"
rg --type typescript 'protocolMessageFromDeserialized'

# Search for files that import from the protocolmessage module
echo "Searching for files that import from the protocolmessage module:"
rg --type typescript 'from .*protocolmessage'

This will help identify areas of the codebase that might need attention due to this change.

src/platform/web/index.ts (2)

6-6: Approve import change and verify usage.

The import statement has been updated to use makeFromDeserializedWithDependencies and alias it as makeProtocolMessageFromDeserialized. This change suggests a refactoring to potentially improve flexibility or dependency injection.

To ensure this change doesn't introduce any issues, please verify the usage of this function across the codebase. Run the following script to check for any remaining uses of the old function name:

#!/bin/bash
# Description: Check for any remaining uses of the old function name

# Test: Search for uses of the old function name
rg --type typescript 'protocolMessageFromDeserialized'

# Test: Verify the new function is used correctly
rg --type typescript 'makeProtocolMessageFromDeserialized'

---

6-6: Ensure comprehensive testing and consider creating a migration guide.

The change from protocolMessageFromDeserialized to makeProtocolMessageFromDeserialized is significant and could impact multiple parts of the codebase.

Please ensure:

1. Comprehensive unit and integration tests are in place to verify the new function works as expected in all scenarios.
2. All usages of this function across the codebase have been updated.
3. If the behavior of the function has changed significantly, consider creating a migration guide for any external code that might be using the old function.

Run the following script to help identify potential areas that need attention:

Also applies to: 48-48, 55-55

src/platform/react-native/index.ts (2)

58-58: Approve export object update and verify dependent modules.

The change in the exported object from protocolMessageFromDeserialized to makeProtocolMessageFromDeserialized is consistent with the import statement change. This ensures that the new function name is correctly exported for use in other parts of the application.

To ensure this change doesn't break existing functionality, please verify all usages of this exported function in dependent modules. Run the following script to check for any remaining occurrences of the old function name:

#!/bin/bash
# Description: Check for any remaining usage of the old exported function name in dependent modules

# Test: Search for the old function name in other TypeScript files. Expect: No results.
rg --type typescript 'protocolMessageFromDeserialized' --glob '!src/platform/react-native/index.ts'

# Test: Search for the new function name in other TypeScript files. Expect: At least one result in files that use this export.
rg --type typescript 'makeProtocolMessageFromDeserialized' --glob '!src/platform/react-native/index.ts'

---

6-6: Approve import statement change and verify usage.

The change from fromDeserializedIncludingDependencies to makeFromDeserializedWithDependencies suggests a positive refactoring towards a factory pattern. This is generally a good practice for creating objects with dependencies.

To ensure this change doesn't break existing functionality, please verify all usages of this function across the codebase. Run the following script to check for any remaining occurrences of the old function name:

✅ Verification successful

Verified import changes and updated function usages.

All instances of fromDeserializedIncludingDependencies have been successfully replaced with makeFromDeserializedWithDependencies across the codebase, ensuring consistency and preventing potential runtime errors.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining usage of the old function name

# Test: Search for the old function name. Expect: No results.
rg --type typescript 'fromDeserializedIncludingDependencies'

# Test: Search for the new function name. Expect: At least one result (the import statement we just changed).
rg --type typescript 'makeFromDeserializedWithDependencies'

Length of output: 273

---

Script:

#!/bin/bash
# Description: Check for any remaining usage of the old function name in TypeScript files

# Test: Search for the old function name in .ts and .tsx files. Expect: No results.
rg --glob '*.ts' --glob '*.tsx' 'fromDeserializedIncludingDependencies'

# Test: Search for the new function name in .ts and .tsx files. Expect: At least one result (the import statement we just changed).
rg --glob '*.ts' --glob '*.tsx' 'makeFromDeserializedWithDependencies'

Length of output: 913

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

11-11: Approved: Update to protocol message creation method

The change from Ably.protocolMessageFromDeserialized to Ably.makeProtocolMessageFromDeserialized({ LiveObjectsPlugin }) aligns with the PR objective of handling the initial state sync sequence. This modification incorporates the LiveObjectsPlugin into the protocol message creation process.

To ensure this change doesn't have unintended consequences, please verify the usage of createPM in other parts of the codebase:

Consider adding a comment explaining the purpose of the createPM variable, as it's not used within this file:

+// createPM is used for creating protocol messages in other test files
 var createPM = Ably.makeProtocolMessageFromDeserialized({ LiveObjectsPlugin });

✅ Verification successful

Verified: Protocol Message Creation Update

The change in test/realtime/live_objects.test.js to initialize createPM with { LiveObjectsPlugin } is appropriate and aligns with the PR's objectives. This modification does not affect other test files where createPM is used without parameters.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for usage of createPM variable
rg 'createPM' --type js

Length of output: 1470

src/common/lib/transport/websockettransport.ts (1)

143-143: LGTM. Verify consistency and update documentation.

The addition of this.connectionManager.realtime._LiveObjectsPlugin as a parameter to deserializeProtocolMessage aligns with the PR objectives for handling initial state sync sequence. This change enhances the deserialization process to incorporate live objects data.

To ensure consistency, please verify that all other calls to deserializeProtocolMessage across the codebase have been updated similarly. Run the following script to check for any inconsistencies:

Don't forget to update any relevant documentation or unit tests to reflect this change in the deserializeProtocolMessage function signature.

✅ Verification successful

Verification Successful: Consistent Usage of deserializeProtocolMessage

All instances of deserializeProtocolMessage include the this.connectionManager.realtime._LiveObjectsPlugin parameter as required.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for inconsistent usage of deserializeProtocolMessage

# Test: Search for deserializeProtocolMessage calls
rg --type typescript -A 5 'deserializeProtocolMessage\('

# Note: Manually review the results to ensure all calls include the new _LiveObjectsPlugin parameter

Length of output: 93

---

Script:

#!/bin/bash
# Description: Check for inconsistent usage of deserializeProtocolMessage in .ts and .tsx files

# Search for deserializeProtocolMessage calls in .ts and .tsx files
rg --glob '*.ts' --glob '*.tsx' -A 5 'deserializeProtocolMessage\('

Length of output: 601

src/common/lib/client/baserealtime.ts (2)

16-16: LGTM: Correct import of LiveObjectsPlugin type

The import statement for the LiveObjectsPlugin type is correctly implemented. Using the type keyword ensures that this import is only for type information, which is beneficial for tree-shaking and avoiding unnecessary runtime dependencies.

---

16-16: Overall impact: Successful integration of LiveObjectsPlugin

The changes successfully integrate the LiveObjectsPlugin into the BaseRealtime class while maintaining existing patterns and consistency. The implementation is non-breaking and aligns with good TypeScript practices and modular architecture.

To ensure the changes don't introduce any unintended side effects, please run the following verification script:

Also applies to: 23-23, 63-63

✅ Verification successful

Verified: LiveObjectsPlugin integration does not affect existing functionality

The integration of LiveObjectsPlugin into the BaseRealtime class has been successfully verified. Existing functionalities remain unaffected, and the implementation aligns with best TypeScript practices and the project's modular architecture.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that the LiveObjectsPlugin integration doesn't affect existing functionality

# Test 1: Check if BaseRealtime can still be instantiated without the LiveObjectsPlugin
rg -U --multiline --type typescript 'new BaseRealtime\([^)]*\)' -A 5

# Test 2: Verify that the LiveObjectsPlugin is correctly used when provided
rg -U --multiline --type typescript 'new BaseRealtime\([^)]*\bplugins\s*:\s*\{[^}]*LiveObjects\s*:[^}]+\}[^)]*\)' -A 5

# Test 3: Check for any potential issues with the new property
rg -U --multiline --type typescript '_LiveObjectsPlugin' -A 5

Length of output: 358

---

Script:

#!/bin/bash
# Description: Verify that the LiveObjectsPlugin integration doesn't affect existing functionality

# Test 1: Check if BaseRealtime can still be instantiated without the LiveObjectsPlugin
rg -U --multiline --type ts 'new BaseRealtime\([^)]*\)' -A 5

# Test 2: Verify that the LiveObjectsPlugin is correctly used when provided
rg -U --multiline --type ts 'new BaseRealtime\([^)]*\bplugins\s*:\s*\{[^}]*LiveObjects\s*:[^}]+\}[^)]*\)' -A 5

# Test 3: Check for any potential issues with the new property
rg -U --multiline --type ts '_LiveObjectsPlugin' -A 5

Length of output: 8974

src/common/lib/transport/comettransport.ts (1)

356-360: LGTM. Verify consistency and consider error handling.

The addition of this.connectionManager.realtime._LiveObjectsPlugin as a parameter to protocolMessageFromDeserialized aligns with the PR objectives for integrating the LiveObjectsPlugin. This change appears to be part of the implementation for handling the initial state sync sequence.

Please ensure that this change is consistent across the codebase. Run the following script to verify:

Consider adding error handling or type checking to ensure this.connectionManager.realtime._LiveObjectsPlugin is available before using it. For example:

this.onProtocolMessage(
  protocolMessageFromDeserialized(
    items[i],
    this.connectionManager.realtime._RealtimePresence,
    this.connectionManager.realtime._LiveObjectsPlugin || null,
  ),
);

This change would provide a fallback value if _LiveObjectsPlugin is undefined, preventing potential runtime errors.

scripts/moduleReport.ts (2)

317-318: Approved: New files added to LiveObjects plugin allowlist

The addition of 'src/plugins/liveobjects/statemessage.ts' and 'src/plugins/liveobjects/syncliveobjectsdatapool.ts' to the allowedFiles set in the checkLiveObjectsPluginFiles function is approved. These additions align with the PR objectives for handling the initial state sync sequence in the LiveObjects plugin.

To ensure these changes are justified:

1. Could you provide a brief explanation of the purpose of these new files and why they're necessary for the LiveObjects plugin?
2. Have you measured the impact of these new files on the overall bundle size of the LiveObjects plugin?

To verify the impact of these new files on the LiveObjects plugin bundle size, please run the following script:

#!/bin/bash
# Description: Check the impact of new files on the LiveObjects plugin bundle size

# Run the module report script and capture the output
output=$(node scripts/moduleReport.ts)

# Extract the LiveObjects plugin bundle size
liveobjects_size=$(echo "$output" | grep "LiveObjects" | awk '{print $NF}')

echo "Current LiveObjects plugin bundle size: $liveobjects_size"

# Use source-map-explorer to get detailed size information
npx source-map-explorer ./build/liveobjects.js --json liveobjects_size.json

# Extract sizes of the new files
statemessage_size=$(jq '.files["src/plugins/liveobjects/statemessage.ts"].size' liveobjects_size.json)
syncliveobjectsdatapool_size=$(jq '.files["src/plugins/liveobjects/syncliveobjectsdatapool.ts"].size' liveobjects_size.json)

echo "Size contribution of new files:"
echo "statemessage.ts: $statemessage_size bytes"
echo "syncliveobjectsdatapool.ts: $syncliveobjectsdatapool_size bytes"

# Clean up
rm liveobjects_size.json

This script will help quantify the impact of the new files on the LiveObjects plugin bundle size and provide visibility into their individual contributions.

---

9-9: Approved: Bundle size threshold increase

The increase in the minimalUsefulRealtimeBundleSizeThresholdsKiB constant from { raw: 99, gzip: 30 } to { raw: 100, gzip: 31 } is approved. This minor adjustment (1 KiB for both raw and gzipped sizes) likely accommodates recent necessary changes in the codebase.

However, it's important to be cautious about gradually increasing bundle size thresholds over time. Please ensure that this increase is justified by essential improvements or additions to the codebase, and continue to monitor and optimize bundle sizes in future development.

To verify the impact of this change, please run the following script:

This script will help ensure that the actual bundle size is within the new thresholds and provide visibility into the current sizes.

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

339-339: LGTM. Verify impact on other parts of the codebase.

The addition of helper.recordPrivateApi('call.makeProtocolMessageFromDeserialized'); aligns with the refactoring of protocol message handling mentioned in the AI-generated summary. This change appears to be part of a broader update to the internal API for creating protocol messages.

To ensure consistency across the codebase, please run the following script to check for any remaining instances of the old protocolMessageFromDeserialized method:

This will help identify any places where the old method might still be in use and confirm the consistent usage of the new makeProtocolMessageFromDeserialized method.

✅ Verification successful

Verified: The old method 'protocolMessageFromDeserialized' has been successfully removed, and the new method 'makeProtocolMessageFromDeserialized' is consistently used across the test suite.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for remaining instances of 'protocolMessageFromDeserialized'

# Search for 'protocolMessageFromDeserialized' in all JavaScript files
echo "Searching for 'protocolMessageFromDeserialized':"
rg --type js 'protocolMessageFromDeserialized'

# Search for 'makeProtocolMessageFromDeserialized' to verify the new usage
echo "Verifying usage of 'makeProtocolMessageFromDeserialized':"
rg --type js 'makeProtocolMessageFromDeserialized'

Length of output: 2600

test/realtime/sync.test.js (5)

Line range hint 182-189: LGTM: Consistent API usage

The changes in this test case are consistent with the new API usage. The test logic and structure remain intact, ensuring the test's validity is maintained.

---

Line range hint 482-489: LGTM: API usage updated consistently

The changes in this test case correctly implement the new Ably.makeProtocolMessageFromDeserialized() API, maintaining consistency with previous test cases.

This completes the update of the API usage across all test cases in this file. The consistent application of this change suggests a well-executed refactoring of the Ably library API. To ensure the completeness of this update:

Run the following script to check for any remaining instances of the old API call and confirm the count of new API calls:

#!/bin/bash
# Check for any remaining old API calls
old_api=$(rg "Ably\.protocolMessageFromDeserialized" test/realtime/sync.test.js)
if [ -n "$old_api" ]; then
    echo "Warning: Found remaining old API calls:"
    echo "$old_api"
else
    echo "No remaining old API calls found."
fi

# Count new API calls
new_api_count=$(rg "Ably\.makeProtocolMessageFromDeserialized\(\)" test/realtime/sync.test.js | wc -l)
echo "Number of new API calls: $new_api_count"

This will help ensure that the API update has been applied comprehensively throughout the file.

---

Line range hint 53-58: LGTM: API usage updated correctly

The changes in this test case correctly implement the new Ably.makeProtocolMessageFromDeserialized() API. The test logic remains unchanged, maintaining the integrity of the test.

To ensure the new API is used consistently within this test file, run:

#!/bin/bash
# Count occurrences of the new API call in this file
occurrences=$(rg "Ably\.makeProtocolMessageFromDeserialized\(\)" test/realtime/sync.test.js | wc -l)
echo "Number of occurrences of new API call: $occurrences"
# This should match the number of times the old API was used

---

Line range hint 1-758: Summary: Consistent API update with potential for refactoring

The changes in this file consistently update the Ably API call from Ably.protocolMessageFromDeserialized to Ably.makeProtocolMessageFromDeserialized() across all test cases. This update maintains the integrity of the tests while adapting to the new API.

Key points:

1. The API change is applied consistently throughout the file.
2. Test logic and structure remain intact, ensuring continued validity of the tests.
3. There's an opportunity to refactor common setup code to reduce duplication and improve maintainability.

Consider creating a helper function for the common setup code used across test cases. This would reduce duplication and improve the overall structure of the test suite.

To ensure the completeness of this update across the entire codebase, run the following script:

#!/bin/bash
# Check for any remaining old API calls in all JavaScript files
old_api=$(rg "Ably\.protocolMessageFromDeserialized" --type js)
if [ -n "$old_api" ]; then
    echo "Warning: Found remaining old API calls:"
    echo "$old_api"
else
    echo "No remaining old API calls found in the codebase."
fi

# Count new API calls in all JavaScript files
new_api_count=$(rg "Ably\.makeProtocolMessageFromDeserialized\(\)" --type js | wc -l)
echo "Total number of new API calls across the codebase: $new_api_count"

This script will help verify that the API update has been applied consistently across the entire project, not just in this test file.

---

5-5: Verify API change across the codebase

The change from Ably.protocolMessageFromDeserialized to Ably.makeProtocolMessageFromDeserialized() suggests an API update in the Ably library. Ensure this change is consistently applied across all relevant files in the codebase.

To verify the consistency of this API change, run the following script:

✅ Verification successful

API change consistently applied across the codebase

All instances of Ably.protocolMessageFromDeserialized have been successfully replaced with Ably.makeProtocolMessageFromDeserialized(). A total of 6 new API calls have been verified.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining instances of the old API call
rg "Ably\.protocolMessageFromDeserialized" --type js
# Search for instances of the new API call to ensure it's being used correctly
rg "Ably\.makeProtocolMessageFromDeserialized\(\)" --type js

Length of output: 397

test/realtime/message.test.js (2)

1149-1149: Consistent update to protocol message creation method

The change from protocolMessageFromDeserialized to makeProtocolMessageFromDeserialized() has been consistently applied here as well. This maintains consistency with the earlier change on line 6.

---

6-6: Update to protocol message creation method

The method for creating protocol messages has been changed from Ably.protocolMessageFromDeserialized to Ably.makeProtocolMessageFromDeserialized(). This change appears to be consistent with the alterations mentioned in the AI-generated summary.

To ensure this change is applied consistently throughout the codebase, let's run the following verification script:

src/plugins/liveobjects/liveobjectspool.ts (8)

8-10: Properly marking LiveObjectsPool as internal

The addition of the @internal comment appropriately indicates that the LiveObjectsPool class is intended for internal use, which helps with code organization and prevents unintended external usage.

---

13-13: Updating _pool key type enhances consistency

Changing the _pool property to Map<string, LiveObject> simplifies the code by using string directly as the key type, which improves consistency since objectId is already of type string.

---

20-22: Parameter type in get method matches _pool keys

The get method's parameter objectId is correctly updated to string, aligning with the updated _pool Map's key type. This ensures type consistency and prevents potential type-related errors.

---

34-36: Addition of set method improves pool management

Introducing the set method allows for directly adding new LiveObject instances to the pool, enhancing flexibility and maintaining encapsulation within the LiveObjectsPool class.

---

38-40: reset method correctly reinitializes the pool

The reset method properly resets the _pool by calling _getInitialPool(), ensuring the pool can return to its initial state when necessary. This is useful for scenarios where the pool needs to be cleared and reinitialized.

---

Line range hint 42-47: Consistent use of string as Map key type in _getInitialPool

Updating the _getInitialPool method to return Map<string, LiveObject> aligns with the updated _pool type, ensuring type consistency throughout the class and preventing potential type mismatches.

---

24-32: ⚠️ Potential issue

Verify existence of arrSubtract in this._client.Utils

In the deleteExtraObjectIds method, this._client.Utils.arrSubtract is used to compute the difference between arrays. Please confirm that arrSubtract exists within this._client.Utils. If it does not, consider using standard JavaScript array methods like filter and includes to achieve the same functionality.

Run the following script to verify if arrSubtract is defined in the codebase:

If arrSubtract is not available, you can refactor the code as follows:

- const extraObjectIds = this._client.Utils.arrSubtract(poolObjectIds, objectIds);
+ const extraObjectIds = poolObjectIds.filter(id => !objectIds.includes(id));

✅ Verification successful

arrSubtract method is confirmed to exist in this._client.Utils.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for the definition of 'arrSubtract' method in the codebase.

# Expected: Should find the method definition within utility modules.
rg --type typescript --type javascript -A 2 "arrSubtract"

Length of output: 187

---

Script:

#!/bin/bash
# Description: Search for the definition of 'arrSubtract' method in the codebase without specifying file types.

rg -A 2 "arrSubtract"

Length of output: 521

---

12-16: Ensure getClient() method exists in LiveObjects

The introduction of the private _client member, initialized via this._liveObjects.getClient(), relies on LiveObjects having a getClient() method that returns an instance of BaseClient. Please confirm that this method exists and functions as expected.

Run the following script to verify the existence of the getClient method:

✅ Verification successful

getClient() Method Verified in LiveObjects

The LiveObjects class in src/plugins/liveobjects/liveobjects.ts includes the getClient() method, which correctly returns an instance of BaseClient as required.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for the definition of 'getClient' method in 'LiveObjects' class.

# Expected: Should find the method definition within 'LiveObjects'.
rg --type typescript -A 5 "class LiveObjects" | rg -A 5 "getClient\("

Length of output: 187

---

Script:

#!/bin/bash
# Description: Search for the definition of 'getClient' method in 'LiveObjects' class within TypeScript files.

rg "class LiveObjects" -g "*.ts" -A 10 | rg "getClient\("

Length of output: 152

---

Script:

#!/bin/bash
# Description: Search for the definition of 'getClient' method across all TypeScript files.

rg "getClient\\s*\\(" -g "*.ts" --context=2

Length of output: 976

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

175-175: Ensure proper cleanup of obsolete live objects in deleteExtraObjectIds.

In the _applySync method, after processing the received objects, the method deleteExtraObjectIds is called to remove obsolete live objects. Verify that this method correctly handles the deletion without causing side effects, such as unintended removal of active objects.

Please review the implementation of deleteExtraObjectIds to ensure it accurately identifies and deletes only the objects that are no longer present in the received data.

src/common/lib/types/protocolmessage.ts (10)

11-11: Import LiveObjectsPlugin for Type Definitions

The import statement correctly uses import type to import type definitions from plugins/liveobjects, ensuring that the type information is available without including runtime dependencies. Good practice in managing type-only imports.

---

34-35: Add new actions for STATE and STATE_SYNC

The addition of STATE: 19 and STATE_SYNC: 20 extends the actions enumeration to handle new state management actions. This enhances the protocol's capabilities.

---

78-85: Extend channelModes with state modes

Including STATE_SUBSCRIBE and STATE_PUBLISH in channelModes allows for the correct decoding of modes from flags. This ensures that channels can recognize and handle state-related modes.

---

93-97: Update deserialize function signature

The deserialize function now accepts liveObjectsPlugin as a parameter, similar to presenceMessagePlugin. This modification is necessary to handle deserialization of state messages when the LiveObjectsPlugin is utilized.

---

Line range hint 103-127: Handle state messages in fromDeserialized function

The inclusion of logic to process the state property when liveObjectsPlugin is provided correctly extends the deserialization to handle state messages. This ensures that state messages are properly instantiated using the plugin.

---

136-145: Introduce makeFromDeserializedWithDependencies function

Adding makeFromDeserializedWithDependencies provides a way to create a deserialization function with dependencies injected, such as the LiveObjectsPlugin. This approach enhances flexibility and testability.

---

Line range hint 152-172: Update stringify function to include state messages

By updating stringify to handle msg.state when liveObjectsPlugin is provided, state messages are properly serialized for transmission. This ensures consistency between serialization and deserialization processes.

---

204-211: Add state property to ProtocolMessage

Including state?: LiveObjectsPlugin.StateMessage[] in the ProtocolMessage class allows it to carry state messages. The accompanying documentation comments clarify when this property is used.

---

60-66: Update MODE_ALL to include state flags

Adding STATE_SUBSCRIBE and STATE_PUBLISH to flags.MODE_ALL ensures that state-related modes are included in the combined mode flags. Confirm that MODE_ALL is used appropriately throughout the codebase.

Run the following script to find usages of MODE_ALL:

---

55-57: Define new flags for state management

Introducing STATE_SUBSCRIBE, STATE_PUBLISH, and HAS_STATE flags expands the flag definitions to support state functionalities. Ensure these flag values do not conflict with existing flags.

Run the following script to check for flag value overlaps:

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

1-309: LGTM!

The StateMessage class and related types and interfaces provide a well-structured approach to managing state messages and operations. The code is modular, follows best practices, and includes appropriate error handling and encoding/decoding logic.

🧰 Tools

🪛 Biome

[error] 168-168: Unsafe usage of optional chaining.

If it short-circuits with 'undefined' the evaluation will throw TypeError here:

(lint/correctness/noUnsafeOptionalChaining)

---

[error] 189-189: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

test/realtime/failure.test.js (2)

Line range hint 647-655: Confirm the compatibility of the simulated protocol message

At lines 647-655, you're simulating a protocol message using createPM:

helper.recordPrivateApi('call.makeProtocolMessageFromDeserialized');
helper.recordPrivateApi('call.connectionManager.activeProtocol.getTransport');
helper.recordPrivateApi('call.transport.onProtocolMessage');
connectionManager.activeProtocol.getTransport().onProtocolMessage(
  createPM({
    action: 6,
    error: {
      message: 'fake placement constraint',
      code: 50320,
      statusCode: 503,
    },
  }),
);

Ensure that the object returned by createPM({...}) aligns with the expected structure of a protocol message and that it's compatible with the onProtocolMessage handler. This is crucial for the test to accurately simulate the placement constraint scenario.

---

6-6: Ensure correct usage of makeProtocolMessageFromDeserialized

At line 6, you've replaced the assignment of createPM with:

var createPM = Ably.makeProtocolMessageFromDeserialized();

Please verify that makeProtocolMessageFromDeserialized() returns a function that can be invoked as createPM(message). This ensures that createPM functions as intended when creating protocol messages later in the tests.

To confirm that makeProtocolMessageFromDeserialized() returns a function, please run the following script:

src/common/lib/client/realtimechannel.ts (3)

537-548: Ensure Live Objects state resynchronization on continuity loss

The added code correctly handles the resynchronization of Live Objects state when continuity is lost, similar to how presence is re-synced. This maintains consistency and ensures that Live Objects state remains accurate after reconnection.

---

559-559: Pass hasState to notifyState for state awareness

Including the hasState parameter in the notifyState method call ensures that the channel state change accounts for the presence of Live Objects state. This allows notifyState to handle the state appropriately based on whether the channel has associated state data.

---

806-808: Update Live Objects on channel state changes

The addition of this._liveObjects.actOnChannelState(state, hasState) ensures that Live Objects are appropriately updated in response to channel state changes. This integration enhances the plugin's responsiveness to the channel's lifecycle.

test/realtime/channel.test.js (4)

1312-1312: Duplicate helper.recordPrivateApi call

This helper.recordPrivateApi('call.makeProtocolMessageFromDeserialized'); call duplicates a previous call on line 1262. Refer to the earlier comment about refactoring repeated calls to improve code maintainability.

---

1363-1363: Duplicate helper.recordPrivateApi call

This helper.recordPrivateApi('call.makeProtocolMessageFromDeserialized'); call is a repetition of earlier instances. Consolidating these calls can help reduce code duplication.

---

1411-1411: Duplicate helper.recordPrivateApi call

You've repeated helper.recordPrivateApi('call.makeProtocolMessageFromDeserialized'); again. Please consider the previous suggestions on refactoring to eliminate redundancy.

---

1617-1617: Duplicate helper.recordPrivateApi call

Another instance of helper.recordPrivateApi('call.makeProtocolMessageFromDeserialized'); is found here. Refactoring these repetitive calls can improve the code's readability and maintainability.

ably.d.ts (2)

874-881: New LiveObjects channel modes added correctly

The addition of STATE_PUBLISH and STATE_SUBSCRIBE in the ChannelModes namespace follows the existing pattern and is properly defined. This enhances the channel capabilities to include LiveObjects state messages.

---

896-897: ChannelMode type correctly extended with new modes

The ChannelMode type now includes ChannelModes.STATE_PUBLISH and ChannelModes.STATE_SUBSCRIBE, appropriately extending the available channel modes to support LiveObjects functionality.

[coderabbitai]

Actionable comments posted: 5

🧹 Outside diff range and nitpick comments (5)

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

1-1: LGTM! Consider using a more specific type for object IDs.

The changes to import BaseClient and add the _client property are good additions that align with the new functionality. The change from Map<ObjectId, LiveObject> to Map<string, LiveObject> simplifies the key type.

Consider creating a type alias for the object ID string (e.g., type ObjectId = string;) to maintain semantic meaning and allow for future type refinement if needed.

Also applies to: 12-13

---

24-32: LGTM! Consider optimization for large sets.

The deleteExtraObjectIds method effectively removes extra objects from the pool. The use of _client.Utils.arrSubtract for set difference is interesting and presumably consistent with other parts of the codebase.

For potential performance improvements with large sets of object IDs, consider:

1. Using a Set for objectIds to achieve O(1) lookup time.
2. Iterating over _pool once instead of creating an intermediate array.

Here's a possible optimization:

deleteExtraObjectIds(objectIds: string[]): void {
  const objectIdSet = new Set(objectIds);
  for (const [id, _] of this._pool) {
    if (!objectIdSet.has(id)) {
      this._pool.delete(id);
    }
  }
}

This approach avoids creating intermediate arrays and should be more efficient for large sets.

---

Line range hint 42-47: LGTM! Consider using ROOT_OBJECT_ID constant.

The changes to the _getInitialPool method are consistent with the earlier modifications to the _pool property type. The method correctly initializes the pool with a root LiveMap object.

For consistency and to avoid magic strings, consider using the ROOT_OBJECT_ID constant when setting the root object in the pool:

private _getInitialPool(): Map<string, LiveObject> {
  const pool = new Map<string, LiveObject>();
  const root = new LiveMap(this._liveObjects, null, ROOT_OBJECT_ID);
  pool.set(ROOT_OBJECT_ID, root);
  return pool;
}

This change would make the code more maintainable and less prone to errors if the root object ID were to change in the future.

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

24-25: Consider the default value of _syncInProgress.

The initialization of _syncLiveObjectsDataPool is correct. However, setting _syncInProgress to true by default might need reconsideration. It assumes that synchronization is always in progress when a new LiveObjects instance is created, which may not always be the case.

Consider initializing _syncInProgress to false and only set it to true when actually starting a sync operation. This approach might better reflect the initial state of the object.

---

128-128: Improve readability by separating assignment from condition.

In the _parseSyncChannelSerial method, the assignment within the if condition can reduce readability:

if (syncChannelSerial && (match = syncChannelSerial.match(/^([\w-]+):(.*)$/))) {

Consider separating the assignment from the condition for better clarity:

if (syncChannelSerial) {
  match = syncChannelSerial.match(/^([\w-]+):(.*)$/);
  if (match) {
    // ... existing code ...
  }
}

This change improves readability and aligns with best practices for avoiding assignments in expressions.

🧰 Tools

🪛 Biome

[error] 128-128: The assignment should not be in an expression.

The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.

(lint/suspicious/noAssignInExpressions)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 3a34601 and 9479d7e.

📒 Files selected for processing (8)

• scripts/moduleReport.ts (2 hunks)
• src/common/lib/client/realtimechannel.ts (6 hunks)
• src/plugins/liveobjects/livecounter.ts (1 hunks)
• src/plugins/liveobjects/livemap.ts (2 hunks)
• src/plugins/liveobjects/liveobject.ts (2 hunks)
• src/plugins/liveobjects/liveobjects.ts (2 hunks)
• src/plugins/liveobjects/liveobjectspool.ts (1 hunks)
• src/plugins/liveobjects/syncliveobjectsdatapool.ts (1 hunks)

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

• scripts/moduleReport.ts
• src/plugins/liveobjects/livecounter.ts
• src/plugins/liveobjects/livemap.ts
• src/plugins/liveobjects/liveobject.ts
• src/plugins/liveobjects/syncliveobjectsdatapool.ts

🧰 Additional context used

🪛 Biome

src/plugins/liveobjects/liveobjects.ts

[error] 128-128: The assignment should not be in an expression.

The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.

(lint/suspicious/noAssignInExpressions)

🔇 Additional comments (7)

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

15-17: LGTM! Constructor changes look good.

The initialization of the _client property in the constructor is correct and aligns with the new functionality. The order of initialization (_client before _pool) is logical and ensures that all necessary properties are set up before they're potentially used.

---

20-22: LGTM! Method signature change is consistent.

The change of the get method's parameter type from ObjectId to string is consistent with the _pool property type change. This simplification in the method signature should make the method easier to use and understand.

---

34-40: LGTM! New methods enhance pool management.

The new set and reset methods are well-implemented and serve clear purposes:

• set allows for adding new LiveObject instances to the pool.
• reset provides a way to reinitialize the pool to its initial state.

These additions improve the flexibility and manageability of the LiveObjectsPool class.

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

3-9: LGTM: New imports are appropriate for the added functionality.

The new imports are correctly added and align with the enhancements made to the LiveObjects class. They provide the necessary types and classes for handling state synchronization and live object management.

---

15-18: LGTM: New private properties enhance synchronization management.

The added private properties (_syncLiveObjectsDataPool, _syncInProgress, _currentSyncId, and _currentSyncCursor) are well-defined and crucial for managing the synchronization state of live objects. Their types are appropriate for their intended use.

---

64-84: LGTM: onAttached method handles attachment scenarios correctly.

The onAttached method effectively manages different channel attachment scenarios:

1. It properly logs the attachment event.
2. If hasState is true, it starts a new sync process.
3. If hasState is false, it correctly resets the pools and ends the sync sequence.

This implementation ensures that the live objects are properly synchronized based on the channel's state.

---

174-176: LGTM: Proper cleanup of unreceived LiveObject instances.

The final step in the _applySync method correctly removes LiveObject instances that were not received during the SYNC sequence:

this._liveObjectsPool.deleteExtraObjectIds([...receivedObjectIds]);

This cleanup is crucial for maintaining consistency between the local state and the server state, ensuring that any objects that no longer exist on the server are removed from the local pool.

[mschristensen]

This looks great to me! Although worth getting a review from @owenpearson

[owenpearson]

LGTM

[coderabbitai]

Actionable comments posted: 17

🧹 Outside diff range and nitpick comments (21)

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

10-18: LGTM: Well-structured class declaration and constructor.

The class declaration and constructor are well-implemented. The use of a Map for the pool is appropriate for efficient key-based access.

Consider adding a comment explaining the purpose of the _liveObjects parameter, as it's currently unused in the class implementation.

---

1-35: Overall assessment: Well-implemented data structure with room for enhancements.

The SyncLiveObjectsDataPool class provides a solid foundation for managing live object data entries. The implementation is type-safe and well-structured.

For future enhancements, consider:

1. Implementing methods for adding, retrieving, and removing individual entries.
2. Adding error handling for edge cases (e.g., attempting to access non-existent entries).
3. Implementing a method to update existing entries efficiently.
4. Adding comprehensive JSDoc comments to improve code documentation.

These enhancements would make the class more robust and easier to use in the broader context of the LiveObjects plugin.

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

10-10: LGTM: New _regionalTimeserial property added

The addition of the _regionalTimeserial property is in line with the PR objectives. It enhances the state management capabilities of the LiveObject class.

Consider adding a brief comment explaining the purpose and usage of this property for better code maintainability.

---

35-40: LGTM: setData method added for updating LiveObject data

The setData method provides a controlled way to update the LiveObject's data, which is essential for state management. The @internal tag correctly indicates its intended usage scope.

Consider adding a brief comment explaining any side effects or important considerations when using this method, such as whether it triggers any events or updates.

---

42-47: LGTM: setRegionalTimeserial method added

The setRegionalTimeserial method provides a controlled way to update the _regionalTimeserial property, complementing the getter method. The @internal tag correctly indicates its intended usage scope.

Consider adding input validation to ensure that only valid timeserial strings are set. This could prevent potential issues with invalid data being stored.

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

Line range hint 1-85: Consider adding tests for makeProtocolMessageFromDeserialized

While the existing tests cover the LiveObjects functionality well, it might be beneficial to add specific tests for the new makeProtocolMessageFromDeserialized function to ensure it behaves correctly with the LiveObjectsPlugin.

Would you like me to suggest a test case for the makeProtocolMessageFromDeserialized function?

src/common/lib/transport/transport.ts (1)

Line range hint 331-331: LGTM: New onAuthUpdated callback enhances authentication handling

The addition of the optional onAuthUpdated callback is a good enhancement for managing authentication state updates. It provides flexibility for handling token updates in the transport layer.

Consider adding a brief JSDoc comment to explain the purpose and usage of this callback. For example:

/**
 * Callback function to handle updates to authentication tokens.
 * @param tokenDetails The updated token details.
 */
onAuthUpdated?: (tokenDetails: API.TokenDetails) => void;

test/realtime/sync.test.js (2)

Line range hint 386-394: LGTM: Comprehensive and consistent refactoring.

This final segment completes the consistent application of the makeProtocolMessageFromDeserialized() method throughout the file. The thorough and uniform implementation of these changes across all test cases ensures the integrity of the test suite while improving the API's clarity.

Consider updating the test suite documentation to reflect this change in the protocol message creation method, ensuring that future contributors are aware of the correct usage.

---

Line range hint 1-738: Summary: Successful refactoring of protocol message creation method.

The changes in this file consistently update the protocol message creation method from Ably.protocolMessageFromDeserialized to Ably.makeProtocolMessageFromDeserialized(). This refactoring has been applied uniformly across all relevant test cases, improving the API's clarity without altering the functionality of the tests.

Key points:

1. The changes are consistent and well-executed throughout the file.
2. No functional changes to the tests were made, preserving the integrity of the test suite.
3. The new method name is more descriptive, potentially improving code readability and maintainability.

To further improve the codebase:

1. Update any relevant documentation to reflect this change in the protocol message creation method.
2. Consider running similar updates across other test files or main codebase files that might be using the old method.
3. If not already done, update any developer guidelines or contribution docs to mention the use of the new method.

src/common/lib/transport/connectionmanager.ts (1)

1808-1809: LGTM! Consider adding a comment for clarity.

The changes to the stringifyProtocolMessage call look good and align with the PR objectives for integrating the LiveObjectsPlugin.

Consider adding a brief comment explaining why these new parameters are needed for the stringifyProtocolMessage function. This would improve code readability and maintainability.

src/common/lib/types/protocolmessage.ts (1)

Line range hint 103-127: Refactor suggestion: Reduce code duplication in fromDeserialized

The logic for deserializing presence and state messages is similar. Consider refactoring to eliminate duplication by extracting shared logic into a helper function. This will enhance maintainability and readability.

Apply this refactor to extract common deserialization logic:

 function fromDeserialized(
   deserialized: Record<string, unknown>,
   presenceMessagePlugin: PresenceMessagePlugin | null,
   liveObjectsPlugin: typeof LiveObjectsPlugin | null,
 ): ProtocolMessage {
+  const processMessages = <T>(
+    messages: T[] | undefined,
+    pluginMethod: (message: T, platform: any) => T,
+  ): T[] | undefined => {
+    if (messages) {
+      for (let i = 0; i < messages.length; i++) {
+        messages[i] = pluginMethod(messages[i], Platform);
+      }
+    }
+    return messages;
+  };

   const error = deserialized.error;
   if (error) deserialized.error = ErrorInfo.fromValues(error as ErrorInfo);
   const messages = deserialized.messages as Message[];
   if (messages) for (let i = 0; i < messages.length; i++) messages[i] = messageFromValues(messages[i]);

-  const presence = presenceMessagePlugin ? (deserialized.presence as PresenceMessage[]) : undefined;
-  if (presenceMessagePlugin) {
-    if (presence && presenceMessagePlugin)
-      for (let i = 0; i < presence.length; i++)
-        presence[i] = presenceMessagePlugin.presenceMessageFromValues(presence[i], true);
-  }
+  const presence = presenceMessagePlugin
+    ? processMessages(
+        deserialized.presence as PresenceMessage[],
+        presenceMessagePlugin.presenceMessageFromValues,
+      )
+    : undefined;

   let state: LiveObjectsPlugin.StateMessage[] | undefined = undefined;
   if (liveObjectsPlugin) {
-    state = deserialized.state as LiveObjectsPlugin.StateMessage[];
-    if (state) {
-      for (let i = 0; i < state.length; i++) {
-        state[i] = liveObjectsPlugin.StateMessage.fromValues(state[i], Platform);
-      }
-    }
+    state = processMessages(
+      deserialized.state as LiveObjectsPlugin.StateMessage[],
+      liveObjectsPlugin.StateMessage.fromValues,
+    );
   }

   return Object.assign(new ProtocolMessage(), { ...deserialized, presence, state });
 }

src/common/lib/types/message.ts (2)

Line range hint 166-281: Ensure lastProcessedEncodingIndex is defined before use

In the decodeData function, there's a possibility that lastProcessedEncodingIndex might be undefined if an error occurs before it's assigned within the loop. This could lead to issues when calculating finalEncoding in the finally block.

To safeguard against this, consider initializing lastProcessedEncodingIndex to a default value or adjusting the calculation to handle undefined values. Here's a suggested fix:

-          finalEncoding =
-            (lastProcessedEncodingIndex as number) <= 0 ? null : xforms.slice(0, lastProcessedEncodingIndex).join('/');
+          finalEncoding = (lastProcessedEncodingIndex != null && lastProcessedEncodingIndex > 0)
+            ? xforms.slice(0, lastProcessedEncodingIndex).join('/')
+            : null;

---

259-259: Standardize error message formatting

The error message uses curly single quotes, which might not render correctly across different environments. For consistency and clarity, it's better to use standard single or double quotes.

Update the error message to use standard quotes:

-    `Error processing the ${xform} encoding, decoder returned ‘${err.message}’`,
+    `Error processing the ${xform} encoding, decoder returned '${err.message}'`,

test/realtime/channel.test.js (5)

Line range hint 1262-1269: Use proper mocking instead of direct method overriding

Directly overriding channel.sendMessage in tests can lead to unintended side effects and makes the tests harder to maintain. Consider using a mocking library like sinon to stub or spy on methods. This approach enhances test isolation and maintainability.

Example using sinon:

// At the beginning of your test file
const sinon = require('sinon');

// In your test setup
const sendMessageStub = sinon.stub(channel, 'sendMessage').callsFake(function (msg) {
  // Your custom logic
});

---

Line range hint 1312-1325: Use proper mocking instead of direct method overriding

In this test, you're assigning a new function to channel.sendMessage to intercept messages. Using a mocking library like sinon for stubbing can improve test clarity and prevent potential side effects.

Example:

const sinon = require('sinon');

const sendMessageStub = sinon.stub(channel, 'sendMessage').callsFake(function (msg) {
  // Your custom assertions or logic
});

---

Line range hint 1363-1369: Use proper mocking instead of direct method overriding

Again, consider using sinon to stub channel.sendMessage rather than directly overriding it. This practice makes your tests cleaner and more reliable.

---

Line range hint 1411-1418: Use proper mocking instead of direct method overriding

Consistently use mocking libraries for method stubbing to enhance test maintainability. Avoid direct overrides of methods like channel.sendMessage.

---

Line range hint 1617-1627: Use proper mocking instead of direct method overriding

Directly modifying channel.sendMessage can introduce unintended behaviors. Utilize sinon or a similar library to stub methods safely across your tests.

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

20-22: Add documentation for public method get

Consider adding JSDoc comments for the get method to enhance code readability and provide clear usage instructions for other developers.

---

34-36: Add documentation for public method set

Adding JSDoc comments for the set method will improve maintainability and help others understand its purpose.

---

38-40: Add documentation for public method reset

Including JSDoc comments for the reset method will provide clarity on its functionality and usage.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 9479d7e and 88e4736.

📒 Files selected for processing (30)

• scripts/moduleReport.ts (2 hunks)
• src/common/lib/client/baserealtime.ts (2 hunks)
• src/common/lib/client/realtimechannel.ts (6 hunks)
• src/common/lib/transport/comettransport.ts (1 hunks)
• src/common/lib/transport/connectionmanager.ts (1 hunks)
• src/common/lib/transport/protocol.ts (1 hunks)
• src/common/lib/transport/transport.ts (1 hunks)
• src/common/lib/transport/websockettransport.ts (1 hunks)
• src/common/lib/types/message.ts (5 hunks)
• src/common/lib/types/protocolmessage.ts (7 hunks)
• src/platform/nativescript/index.ts (2 hunks)
• src/platform/nodejs/index.ts (2 hunks)
• src/platform/react-native/index.ts (2 hunks)
• src/platform/web/index.ts (2 hunks)
• src/plugins/liveobjects/index.ts (1 hunks)
• src/plugins/liveobjects/livecounter.ts (1 hunks)
• src/plugins/liveobjects/livemap.ts (2 hunks)
• src/plugins/liveobjects/liveobject.ts (2 hunks)
• src/plugins/liveobjects/liveobjects.ts (2 hunks)
• src/plugins/liveobjects/liveobjectspool.ts (1 hunks)
• src/plugins/liveobjects/statemessage.ts (1 hunks)
• src/plugins/liveobjects/syncliveobjectsdatapool.ts (1 hunks)
• test/common/modules/private_api_recorder.js (1 hunks)
• test/realtime/channel.test.js (6 hunks)
• test/realtime/connection.test.js (2 hunks)
• test/realtime/failure.test.js (2 hunks)
• test/realtime/live_objects.test.js (1 hunks)
• test/realtime/message.test.js (2 hunks)
• test/realtime/presence.test.js (2 hunks)
• test/realtime/sync.test.js (6 hunks)

🧰 Additional context used

🪛 Biome

src/plugins/liveobjects/liveobjects.ts

[error] 128-128: The assignment should not be in an expression.

The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.

(lint/suspicious/noAssignInExpressions)

src/plugins/liveobjects/statemessage.ts

[error] 168-168: Unsafe usage of optional chaining.

If it short-circuits with 'undefined' the evaluation will throw TypeError here:

(lint/correctness/noUnsafeOptionalChaining)

---

[error] 189-189: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

🔇 Additional comments (64)

src/plugins/liveobjects/index.ts (4)

2-2: LGTM: StateMessage import added correctly.

The import statement for StateMessage is properly formatted and uses the correct relative path.

---

4-4: LGTM: StateMessage added to named exports.

The StateMessage is correctly added to the named exports, making it available for import by other modules.

---

6-9: LGTM: StateMessage added to default export.

The StateMessage is correctly added to the default export object, ensuring consistency with the named exports and making it available when importing the entire module.

---

1-9: Summary: Changes align with PR objectives and enhance plugin functionality.

The modifications to this file correctly integrate the new StateMessage functionality into the liveobjects plugin. These changes support the PR's objective of handling new STATE_SYNC messages introduced by the LiveObjects plugin. The file structure remains clean and consistent.

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

1-1: LGTM: Import statement updated correctly.

The import statement has been appropriately updated to include LiveObjectData, which is now used in the LiveCounterData interface. This change aligns with the PR objective of enhancing state management for live objects.

---

3-5: LGTM: Interface updated to extend LiveObjectData.

The LiveCounterData interface now extends LiveObjectData, which is a good improvement in the type hierarchy. This change aligns with the PR objective of enhancing state management and handling new STATE_SYNC messages.

To ensure this change doesn't introduce any issues, please verify the usage of LiveCounterData across the codebase. Run the following script to check for any potential impacts:

✅ Verification successful

Verification Complete: No unexpected usages of LiveCounterData found.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check usage of LiveCounterData across the codebase

# Search for LiveCounterData usage
echo "Searching for LiveCounterData usage:"
rg -n "LiveCounterData"

# Search for LiveCounter instantiations
echo "\nSearching for LiveCounter instantiations:"
rg -n "new LiveCounter"

Length of output: 641

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

1-8: LGTM: Well-structured imports and interface definition.

The import statements and the LiveObjectDataEntry interface are well-defined. The use of a union type for objectType is a good practice for type safety.

---

20-26: LGTM: Correctly implemented entries() and size() methods.

The entries() and size() methods are simple and correctly implemented, providing basic functionality for accessing the pool data.

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

3-5: LGTM: Exporting LiveObjectData interface enhances modularity

Exporting the LiveObjectData interface is a good practice. It allows other modules to use this interface, promoting code reuse and improving type safety across the codebase.

---

28-33: LGTM: getRegionalTimeserial method added

The getRegionalTimeserial method provides controlled access to the _regionalTimeserial property. The @internal tag correctly indicates its intended usage scope.

---

Line range hint 1-56: Overall assessment: Enhancements to LiveObject class improve state management

The changes to the LiveObject class, including the new _regionalTimeserial property and associated getter and setter methods, along with the setData method, significantly enhance the state management capabilities of the class. These additions align well with the PR objectives of handling new STATE_SYNC messages and improving the LiveObjects functionality.

The proper use of the @internal tag for new methods maintains good encapsulation, while exporting the LiveObjectData interface improves type safety and reusability across the codebase.

These changes lay a solid foundation for handling server-initiated sync sequences as outlined in the linked issue DTP-950.

src/platform/nodejs/index.ts (1)

49-49: LGTM! Export updated correctly.

The export statement has been updated to use the new function name makeProtocolMessageFromDeserialized, which is consistent with the import statement change. This ensures that the refactored function is correctly exported from the module.

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

1-2: Improved import structure and type organization.

The changes to the import statements enhance code organization and reusability:

1. Adding LiveObjectData import prepares for its use in LiveMapData.
2. Importing StateValue from a centralized location improves maintainability.

These changes align well with modular design principles.

---

Line range hint 1-58: Summary: Changes align well with PR objectives

The modifications in this file contribute to the overall goal of handling the initial state sync sequence:

1. The improved import structure prepares for better integration with the LiveObjects plugin.
2. The enhanced type hierarchy for LiveMapData likely facilitates more efficient state management.

These changes appear to be part of a broader refactoring effort to support the new STATE_SYNC messages and improve the realtime client's capabilities.

---

25-25: Enhanced type hierarchy for LiveMapData.

The extension of LiveObjectData by LiveMapData improves the type system:

1. It creates a clear relationship between general live objects and live maps.
2. This change likely allows for more consistent handling of different live object types.

To ensure this change is consistently applied and doesn't introduce any issues, please run the following verification:

This script will help identify any areas that might need attention due to the type hierarchy change.

✅ Verification successful

LiveMapData interface verified successfully.

All usages of LiveMapData and LiveObjectData are consistent with the updated type hierarchy and do not introduce any issues.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of LiveMapData and its compatibility with LiveObjectData

# Test 1: Check if LiveMapData is used consistently
echo "Checking LiveMapData usage:"
rg "LiveMapData" -A 5

# Test 2: Verify if there are any places where LiveObjectData is used where LiveMapData might be more appropriate
echo "Checking potential LiveObjectData usage that could be LiveMapData:"
rg "LiveObjectData" -A 5

# Test 3: Look for any type assertions or casts that might be affected by this change
echo "Checking for type assertions or casts:"
rg "(as LiveMapData|as LiveObjectData|\<LiveMapData\>|\<LiveObjectData\>)" -A 5

Length of output: 5333

---

Script:

#!/bin/bash
# Description: Verify the usage of LiveMapData and its compatibility with LiveObjectData

# Test 1: Check if LiveMapData is used consistently
echo "Checking LiveMapData usage:"
rg "LiveMapData" -A 5

# Test 2: Verify if there are any places where LiveObjectData is used where LiveMapData might be more appropriate
echo "Checking potential LiveObjectData usage that could be LiveMapData:"
rg "LiveObjectData" -A 5

# Test 3: Look for any type assertions or casts that might be affected by this change
echo "Checking for type assertions or casts:"
rg "(as LiveMapData|as LiveObjectData|<LiveMapData>|<LiveObjectData>)" -A 5

Length of output: 5811

src/platform/nativescript/index.ts (2)

55-55: LGTM! Export statement updated correctly.

The export statement has been updated to use the new function name makeProtocolMessageFromDeserialized, which is consistent with the import change. This ensures that the renamed function is correctly exposed for external use.

---

6-6: Follow-up: Verify remaining usages of protocolMessageFromDeserialized.

A previous review comment mentioned that protocolMessageFromDeserialized was still used in src/common/lib/transport/comettransport.ts. Please ensure that this instance, and any others, have been updated to use makeProtocolMessageFromDeserialized for consistency across the codebase.

Run the following script to check for any remaining usages of the old function name:

#!/bin/bash
# Description: Check for remaining usages of the old function name

echo "Checking for any remaining uses of 'protocolMessageFromDeserialized':"
rg --glob '*.ts' --glob '*.tsx' 'protocolMessageFromDeserialized'

If any results are found, please update those instances to use the new function name.

src/platform/web/index.ts (1)

Line range hint 1-55: Summary of changes and potential impact.

The changes in this file are part of a larger refactoring effort to rename protocolMessageFromDeserialized to makeProtocolMessageFromDeserialized. This modification improves code clarity by making the factory nature of the function more explicit. The changes are consistent throughout the file, affecting both import and export statements.

While these changes appear to be correct and beneficial, they may have a wider impact on the codebase. It's crucial to ensure that all files importing this module are updated accordingly to use the new function name.

To get a comprehensive view of the impact of these changes, please run the following script:

#!/bin/bash
# Description: Analyze the impact of renaming protocolMessageFromDeserialized

# Test: List all files that import from platform/web
echo "Files importing from platform/web:"
rg "from '.*platform/web'" --type ts --type js -l

# Test: Check for any remaining usage of protocolMessageFromDeserialized
echo "Checking for any remaining usage of protocolMessageFromDeserialized:"
rg 'protocolMessageFromDeserialized' --type ts --type js

# Test: Verify the usage of makeProtocolMessageFromDeserialized
echo "Verifying the usage of makeProtocolMessageFromDeserialized:"
rg 'makeProtocolMessageFromDeserialized' --type ts --type js

This script will help identify any files that might need updates due to these changes.

src/platform/react-native/index.ts (1)

58-58: LGTM: Export statement updated correctly.

The export statement has been updated to use the new function name makeProtocolMessageFromDeserialized. This change is consistent with the import statement update and ensures that the renamed function is correctly exported.

src/common/lib/transport/protocol.ts (1)

80-84: LGTM. Verify consistency across the codebase.

The addition of this.transport.connectionManager.realtime._LiveObjectsPlugin as an argument to stringifyProtocolMessage is consistent with the PR objective of handling initial state sync sequence. This change allows the LiveObjects plugin information to be included in the serialized protocol messages.

To ensure consistency, please run the following script to check for other occurrences of stringifyProtocolMessage and verify if they have been updated similarly:

This will help identify any other locations where stringifyProtocolMessage is used and ensure they are updated consistently if necessary.

✅ Verification successful

Consistency Verified Across Codebase

All instances of stringifyProtocolMessage include the _LiveObjectsPlugin argument, ensuring consistency with the PR changes.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for other occurrences of stringifyProtocolMessage and their arguments

# Test: Search for stringifyProtocolMessage calls
rg --type typescript -A 3 'stringifyProtocolMessage\('

# Test: Search for imports of stringifyProtocolMessage
rg --type typescript 'import.*stringify.*from.*protocolmessage'

Length of output: 192

---

Script:

#!/bin/bash
# Description: Check for other occurrences of stringifyProtocolMessage and their arguments

# Test: Search for stringifyProtocolMessage calls in .ts files
rg 'stringifyProtocolMessage\(' --glob '*.ts' -A 3

# Test: Search for imports of stringifyProtocolMessage in .ts files
rg 'import.*stringify.*from.*protocolmessage' --glob '*.ts'

Length of output: 1306

src/common/lib/client/baserealtime.ts (4)

16-16: LGTM: Type-only import for LiveObjectsPlugin.

The import statement is correctly using the type keyword, which is good for tree-shaking and type-checking without importing the actual implementation.

---

23-23: LGTM: New readonly property for LiveObjectsPlugin.

The _LiveObjectsPlugin property is correctly declared as readonly with an appropriate type. The underscore prefix suggests it's for internal use, which is consistent with the plugin architecture.

---

63-63: LGTM: Proper initialization of LiveObjectsPlugin in constructor.

The initialization of _LiveObjectsPlugin is implemented correctly using optional chaining and nullish coalescing operators. This approach safely handles cases where the plugin might not be provided in the options.

---

Line range hint 1-265: Overall: Solid implementation of LiveObjectsPlugin integration.

The changes in this file successfully integrate the LiveObjectsPlugin into the BaseRealtime class. The implementation is consistent with the PR objectives and follows good practices for plugin architecture. The use of type-only imports and proper initialization in the constructor demonstrates attention to detail and performance considerations.

src/common/lib/transport/transport.ts (1)

131-135: Verify the implementation of stringifyProtocolMessage

The addition of this.connectionManager.realtime._LiveObjectsPlugin as a parameter to stringifyProtocolMessage aligns with the PR objectives to handle new STATE_SYNC messages. However, we need to ensure that the stringifyProtocolMessage function is properly updated to handle this new parameter.

Please run the following script to verify the implementation:

This will help us confirm that the stringifyProtocolMessage function is correctly implemented to handle the new _LiveObjectsPlugin parameter.

src/common/lib/transport/comettransport.ts (1)

356-360: Verify the integration of LiveObjectsPlugin in protocol message deserialization

The addition of this.connectionManager.realtime._LiveObjectsPlugin as a parameter to protocolMessageFromDeserialized suggests an integration of new functionality related to live objects. This change appears to be part of a larger refactoring effort.

While the modification doesn't alter the overall logic of the onData method, it does impact how protocol messages are deserialized. Please ensure that:

1. The _LiveObjectsPlugin is properly initialized and available in all scenarios where this method is called.
2. The protocolMessageFromDeserialized function in the Protocol module has been updated to handle this new parameter correctly.
3. Any unit tests related to protocol message deserialization have been updated to reflect this change.
4. The change is consistent with similar modifications in other transport implementations, if applicable.

To confirm the correct implementation and usage of _LiveObjectsPlugin, please run the following verification script:

This script will help ensure that the _LiveObjectsPlugin is consistently implemented and used across the codebase.

scripts/moduleReport.ts (2)

9-9: Verify the need for increased bundle size thresholds.

The minimalUsefulRealtimeBundleSizeThresholdsKiB constant has been updated to allow for larger bundle sizes. While this change might be necessary due to new features or dependencies, it's important to ensure that the increase is justified.

Please run the following script to check the current bundle sizes and compare them with the new thresholds:

#!/bin/bash
# Description: Verify the current bundle sizes against the new thresholds

# Test: Build the minimal useful Realtime bundle and check its size
npm run build:modular
node -e "
const fs = require('fs');
const zlib = require('zlib');
const bundle = fs.readFileSync('./build/modular/index.mjs');
const gzipped = zlib.gzipSync(bundle);
console.log('Raw size:', (bundle.length / 1024).toFixed(2), 'KiB');
console.log('Gzipped size:', (gzipped.length / 1024).toFixed(2), 'KiB');
console.log('New thresholds - Raw: 100 KiB, Gzipped: 31 KiB');
"

---

317-318: Verify the necessity of new LiveObjects plugin files.

Two new files have been added to the allowedFiles set in the checkLiveObjectsPluginFiles function:

1. 'src/plugins/liveobjects/statemessage.ts'
2. 'src/plugins/liveobjects/syncliveobjectsdatapool.ts'

These additions suggest that these files are now expected to contribute significantly to the LiveObjects plugin bundle size.

Please run the following script to check the contribution of these new files to the bundle size:

This will help us understand if these files indeed contribute significantly (>= 100 bytes as per the threshold in the script) to the LiveObjects plugin bundle size.

test/realtime/connection.test.js (3)

339-339: LGTM. Consistent with the earlier change.

This line correctly records the call to the new makeProtocolMessageFromDeserialized method, which is consistent with the change on line 5. It's a good practice to track the usage of private APIs in tests.

---

Line range hint 1-391: Overall assessment: Changes look good and are consistent with PR objectives.

The modifications in this file are minimal and focused on updating the protocol message creation method. These changes align with the PR objectives and the broader refactoring effort mentioned in the summary. The overall structure and logic of the tests remain intact, which is a positive sign.

However, given that this change affects a core functionality (protocol message creation), it's crucial to ensure that:

1. All tests in this file pass after the changes.
2. The new makeProtocolMessageFromDeserialized method is thoroughly tested in its own unit tests.
3. The impact of this change on other parts of the codebase has been assessed and addressed.

To verify the test results, please run the following command and provide the output:

#!/bin/bash
# Description: Run the tests for the connection module

# Test: Execute the connection tests
npm test test/realtime/connection.test.js

---

5-5: LGTM. Verify impact on other parts of the codebase.

The change from Ably.protocolMessageFromDeserialized to Ably.makeProtocolMessageFromDeserialized() is consistent with the updates mentioned in the PR summary. This modification appears to be part of a broader refactoring effort.

To ensure this change doesn't introduce any regressions, please run the following script to check for any remaining uses of the old method:

✅ Verification successful

No remaining uses of protocolMessageFromDeserialized found.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining uses of the old protocolMessageFromDeserialized method

# Test: Search for any remaining uses of protocolMessageFromDeserialized
rg 'protocolMessageFromDeserialized' --type js

Length of output: 294

---

Script:

#!/bin/bash
# Description: Check for any remaining uses of the old protocolMessageFromDeserialized method

# Test: Search for any remaining uses of protocolMessageFromDeserialized
rg 'protocolMessageFromDeserialized' --type js

Length of output: 46

test/realtime/sync.test.js (4)

Line range hint 53-59: LGTM: Consistent update of protocol message creation.

The changes in this segment correctly implement the new makeProtocolMessageFromDeserialized() method. The functionality remains unchanged, ensuring the test's integrity.

---

Line range hint 182-190: LGTM: Consistent implementation of new protocol message creation method.

This segment correctly implements the makeProtocolMessageFromDeserialized() method, maintaining consistency with the earlier changes. The test's functionality remains intact.

---

Line range hint 294-302: LGTM: Consistently applied protocol message creation update.

The changes in this segment maintain the consistency of the refactoring effort. The uniform application of the makeProtocolMessageFromDeserialized() method across multiple test cases is commendable.

---

5-5: LGTM: Updated protocol message creation method.

The change from Ably.protocolMessageFromDeserialized to Ably.makeProtocolMessageFromDeserialized() is consistent with the refactoring mentioned in the summary. This update improves the API clarity by using a more descriptive method name.

To ensure consistency across the codebase, run the following script:

✅ Verification successful

Verified: All instances of protocolMessageFromDeserialized have been updated.
No remaining occurrences of protocolMessageFromDeserialized were found in the codebase, confirming the refactoring is complete.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that all occurrences of protocolMessageFromDeserialized have been updated.

# Test: Search for any remaining instances of the old method name
rg 'protocolMessageFromDeserialized' --type js

Length of output: 46

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

2099-2099: Approved: Updated protocol message creation method

The change from Ably.protocolMessageFromDeserialized to Ably.makeProtocolMessageFromDeserialized() appears to be part of a broader update in the SDK's message handling. This modification aligns with similar changes across the test suite.

To ensure the new method behaves as expected, please run the following verification script:

✅ Verification successful

Verified: Consistent update of protocol message creation method

The change from Ably.protocolMessageFromDeserialized to Ably.makeProtocolMessageFromDeserialized() is consistently applied across the test suite and related platform files, ensuring uniformity in protocol message handling.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the behavior of makeProtocolMessageFromDeserialized()

# Test: Check if the new method is defined and callable
rg -A 5 "makeProtocolMessageFromDeserialized" src/

# Test: Verify usage in other test files
rg -A 5 "makeProtocolMessageFromDeserialized" test/

Length of output: 12399

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

56-56: Implement synchronization logic in handleStateSyncMessage

The TODO comment indicates that state messages should be delegated to _syncLiveObjectsDataPool and new live and data objects should be created. Implementing this is essential for proper synchronization handling.

---

97-98: Define behavior for 'detached', 'failed', and 'suspended' channel states

The cases for 'detached', 'failed', and 'suspended' states in the actOnChannelState method currently have TODO comments. Properly handling these states is important to ensure the application can respond appropriately to channel disconnections, failures, or suspensions.

Also applies to: 101-102

---

128-128: Avoid assignments within conditional expressions for clarity

Assigning match within the if condition can reduce code readability and may lead to confusion. It's better practice to perform the assignment separately before the conditional check.

🧰 Tools

🪛 Biome

[error] 128-128: The assignment should not be in an expression.

The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.

(lint/suspicious/noAssignInExpressions)

---

166-168: Improve error handling for unknown live object types

Throwing an error for an unknown objectType in _applySync may disrupt the entire synchronization process. Consider logging the error and continuing processing the remaining objects.

src/common/lib/types/protocolmessage.ts (12)

11-11: Importing LiveObjectsPlugin for type definitions

The import statement correctly brings in the LiveObjectsPlugin types from 'plugins/liveobjects', enabling type support for state messages.

---

34-35: Adding new action constants STATE and STATE_SYNC

The new action constants STATE and STATE_SYNC are appropriately added to the actions object with unique numeric values, ensuring proper handling of state-related protocol messages.

---

55-57: Defining new flags for state functionality

The flags STATE_SUBSCRIBE, STATE_PUBLISH, and HAS_STATE are correctly defined with unique bit shifts, avoiding conflicts with existing flags and extending state capabilities.

---

60-66: Updating MODE_ALL to include state modes

The flags.MODE_ALL composite flag now includes STATE_SUBSCRIBE and STATE_PUBLISH, ensuring that all channel modes are encompassed when using this flag.

---

78-85: Extending channelModes array with state modes

The channelModes array is updated to include STATE_SUBSCRIBE and STATE_PUBLISH, maintaining consistency between channel modes and their corresponding flags.

---

93-93: Extending deserialize function to handle state messages

The deserialize function signature now includes the liveObjectsPlugin parameter, enabling deserialization of state messages when the plugin is provided.

---

103-103: Updating fromDeserialized to process state messages

The function fromDeserialized correctly accepts the liveObjectsPlugin parameter, allowing it to handle the deserialization of state messages alongside presence messages.

---

116-127: Deserializing state messages in fromDeserialized

The added code appropriately processes state messages when liveObjectsPlugin is available:

• It checks for the existence of deserialized.state.
• It iterates over each state message and uses liveObjectsPlugin.StateMessage.fromValues for proper deserialization.

---

136-145: Introducing makeFromDeserializedWithDependencies function

The new function makeFromDeserializedWithDependencies enhances modularity by allowing dependency injection of LiveObjectsPlugin, improving testability and flexibility in handling optional plugins.

---

152-156: Modifying stringify to include liveObjectsPlugin parameter

The stringify function now accepts liveObjectsPlugin, enabling it to serialize state messages when they are present.

---

170-172: Serializing state messages in stringify

The code correctly serializes state messages using:

result += '; state=' + toStringArray(liveObjectsPlugin.StateMessage.fromValuesArray(msg.state, Platform));

This ensures that state messages are properly included in the string representation of the protocol message.

---

204-211: Adding state property to ProtocolMessage class

The state property is added to the ProtocolMessage class with appropriate documentation:

• It's marked as optional (state?: LiveObjectsPlugin.StateMessage[]).
• Comments explain that it will be undefined if the user hasn't requested LiveObjects functionality.

src/common/lib/transport/websockettransport.ts (1)

143-143: LGTM!

The addition of this.connectionManager.realtime._LiveObjectsPlugin to the deserializeProtocolMessage call within onWsData ensures proper handling of live objects messages. This change aligns with the integration of the LiveObjectsPlugin and enhances the processing of incoming WebSocket data.

src/common/lib/types/message.ts (2)

157-164: Good refactoring for modularity and clarity

The extraction of the decoding logic into a separate decodeData function enhances code modularity and readability. This change improves maintainability and makes the decoding process clearer.

---

244-247: Handle potential null values for decodedData

In the vcdiff decoding case, ensure that decodedData and deltaBaseBuffer are valid buffers before proceeding, to prevent runtime exceptions.

Run the following script to check for any instances where decodedData might be null or undefined prior to this operation:

test/realtime/failure.test.js (2)

6-6: Verify the Correct Assignment of 'createPM' Function

At line 6, you are assigning createPM to the result of Ably.makeProtocolMessageFromDeserialized(). Please verify whether makeProtocolMessageFromDeserialized should be called here. If it is intended to return a function that createPM will use, then calling it is correct. Otherwise, if makeProtocolMessageFromDeserialized is the function itself, you might need to assign it directly without the parentheses.

---

647-647: Update of Helper Function Name is Consistent

The use of 'call.makeProtocolMessageFromDeserialized' in helper.recordPrivateApi at line 647 matches the updated function naming convention. This change appears consistent and correct.

src/common/lib/client/realtimechannel.ts (1)

559-559: ⚠️ Potential issue

Add missing hasBacklog parameter to notifyState method signature

The notifyState method is called with the hasBacklog parameter, but the method signature does not include it. This mismatch can lead to incorrect parameter assignments and unintended behavior.

Apply this diff to update the method signature:

- notifyState(state: API.ChannelState, reason?: ErrorInfo | null, resumed?: boolean, hasPresence?: boolean, hasState?: boolean): void {
+ notifyState(state: API.ChannelState, reason?: ErrorInfo | null, resumed?: boolean, hasPresence?: boolean, hasBacklog?: boolean, hasState?: boolean): void {

Likely invalid or redundant comment.

test/realtime/message.test.js (2)

1149-1149: No issues found with the change

The usage of helper.recordPrivateApi('call.makeProtocolMessageFromDeserialized') appears appropriate.

---

6-6: Confirm that makeProtocolMessageFromDeserialized is correctly invoked

The function makeProtocolMessageFromDeserialized is being called without parameters. Please verify that this is the intended usage and that the function does not require any arguments.

Run the following script to verify the function definition:

test/realtime/channel.test.js (2)

7-7: Duplicate: Possible incorrect invocation of makeProtocolMessageFromDeserialized()

---

1262-1262: Duplicate: Consider refactoring repeated helper.recordPrivateApi calls

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

27-32: Ensure proper cleanup of LiveObject instances when deleting from the pool

When removing LiveObject instances from the pool, ensure that any associated resources are properly released. This may include detaching event listeners or performing other cleanup operations to prevent memory leaks.

Would you like assistance in implementing cleanup logic for LiveObject instances when they are deleted?

---

38-40: ⚠️ Potential issue

Ensure disposal of existing LiveObjects when resetting the pool

When resetting the pool, the existing LiveObject instances may still hold references or event listeners. Ensure that these are properly disposed of before resetting to prevent potential memory leaks.

Consider adding cleanup procedures before reinitializing the pool in the reset method.