LiveObjects fixes #2028
LiveObjects fixes #2028
Conversation
See Simon's comment [1]. The `channel` property should not be expected in the wire protocol for object messages, just like it isn't present in a message or presencemessage. [1] ably/specification#279 (comment)
`MapEntry.data` will not be set for tombstone properties in a map, this commit ensures we handle it correctly.
WalkthroughThis update refactors object and message encoding logic, improves defensive checks around optional data, and restructures object pool reset/clear semantics. It also updates test helpers and scenarios, modifies method signatures, and enhances validation for client options. Several feature flags are removed from test utilities, and new test coverage is added for object sync and error cases. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Objects
participant ObjectsPool
participant SyncObjectsDataPool
participant ObjectMessage
Client->>Objects: publish(objectMessages)
Objects->>ObjectMessage: encode(message, client)
ObjectMessage->>Client: Use MessageEncoding.encodeData with isNativeDataType
ObjectMessage-->>Objects: Encoded message
Objects->>ObjectsPool: Reset/Clear pool (based on state)
Objects->>SyncObjectsDataPool: Clear pool (based on state)
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.31.1)test/realtime/objects.test.jsTip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
temporarily deployed
to
staging/pull/2028/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/2028/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/2028/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/2028/bundle-report
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/plugins/objects/objects.ts (3)
288-291: Potentially stale object instances kept afterdetached/failed
clearObjectsData(false)wipes the objects’ data but intentionally keeps the object instances and emits no update events.
Down-stream code holding references to those objects will continue to read mutated (now zero-value) state without any notification, which might be confusing.Consider one of the following:
- Emit a lightweight “invalidated” update so observers know their data is no longer reliable, or
- Replace the objects entirely (similar to
resetToInitialPool) to force consumers to re-fetch on re-attach.
301-304: In-place mutation byObjectMessage.encodecould surprise callers
ObjectMessage.encodemutates the supplied instance; you then rely on that side effect when computinggetMessageSize().
If the sameObjectMessageobjects are ever reused elsewhere (e.g., for retries), they will already be encoded, possibly with the wrongBaseClientcontext.A safer, allocation-free pattern is to let
encodereturn the encoded copy and use that copy downstream:-objectMessages.forEach((x) => ObjectMessage.encode(x, this._client)); +objectMessages = objectMessages.map((msg) => ObjectMessage.encode(msg, this._client));This keeps mutation local and explicit.
498-507: Use the existing_clientreference for consistencyThe method accesses
this._channel.client, but the class already stores the same instance inthis._client.
Using a single source reduces cognitive load and helps future refactors (e.g., if_channelbecame optional).- if (this._channel.client.options.echoMessages === false) { - throw new this._channel.client.ErrorInfo( + if (this._client.options.echoMessages === false) { + throw new this._client.ErrorInfo([nitpick]
test/realtime/objects.test.js (3)
4584-4595:client.options.echoMessages = falseafter connect may not be honouredThe Objects plug-in most likely reads
client.options.echoMessagesduring construction / attachment.
Mutating the flag later (lines 4588-4590) can yield a false-positive test: the write API may still succeed because the plug-in cached the original value.Consider spinning up a fresh client with
echoMessages:falsein its constructor instead of patching the existing instance, e.g.- client.options.echoMessages = false; + await client.close(); + const clientNoEcho = RealtimeWithObjects(helper, { ...clientOptions, echoMessages: false });Please verify the plug-in’s behaviour and adjust the scenario if needed.
4945-4953: Create / dispose a single offline client for size testsEvery iteration instantiates a new
Realtime(even ifautoConnect:false). Although they never connect, thousands of objects accumulate and enlarge heap usage. Reuse one client or explicitlyclose()it inside the loop to keep the test memory-constant and slightly faster.- const client = RealtimeWithObjects(helper, { autoConnect: false }); + if (!global._sizeTestClient) { + global._sizeTestClient = RealtimeWithObjects(helper, { autoConnect: false }); + } + const client = global._sizeTestClient;At the end of the describe-block you can
client.close()once.
4698-4714: Zero-byte expectations rely on internal knowledgeThe “nonce” and “initial value” test cases hard-code
expected:0.
If future refactors legitimately start counting these fields, the test will fail even though behaviour is still correct. Instead, compute the expected size via the same helper used in production (Utils.dataSizeBytes(value)) to lock assertions to observable output rather than implementation detail.This reduces brittle red builds on harmless internal refactors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/common/lib/types/basemessage.ts(2 hunks)src/plugins/objects/livemap.ts(1 hunks)src/plugins/objects/liveobject.ts(3 hunks)src/plugins/objects/objectmessage.ts(6 hunks)src/plugins/objects/objects.ts(6 hunks)src/plugins/objects/objectspool.ts(3 hunks)src/plugins/objects/syncobjectsdatapool.ts(1 hunks)test/common/modules/private_api_recorder.js(1 hunks)test/common/modules/testapp_manager.js(0 hunks)test/package/browser/template/src/index-objects.ts(1 hunks)test/realtime/objects.test.js(8 hunks)
💤 Files with no reviewable changes (1)
- test/common/modules/testapp_manager.js
🧰 Additional context used
🧬 Code Graph Analysis (5)
test/package/browser/template/src/index-objects.ts (1)
test/package/browser/template/src/sandbox.ts (1)
createSandboxAblyAPIKey(3-20)
src/plugins/objects/objects.ts (1)
src/plugins/objects/objectmessage.ts (1)
ObjectMessage(155-653)
src/plugins/objects/livemap.ts (1)
src/plugins/objects/objectmessage.ts (1)
ObjectData(28-42)
src/common/lib/types/basemessage.ts (1)
src/common/platform.ts (1)
Platform(18-42)
src/plugins/objects/objectmessage.ts (1)
src/plugins/objects/livemap.ts (1)
ObjectData(39-39)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (16.x)
🔇 Additional comments (20)
src/plugins/objects/syncobjectsdatapool.ts (1)
48-48: Method name change improves semantic clarity.The rename from
reset()toclear()better reflects the method's functionality and aligns with related updates in the codebase. This ensures consistent handling of pool clearing across the objects plugin.test/common/modules/private_api_recorder.js (1)
159-159: Addition of private API identifier for testing.This addition properly extends the set of private API identifiers to include the new
echoMessagesoption validation. This will allow tests to record and verify usage of this private API property.test/package/browser/template/src/index-objects.ts (1)
36-36: Simplified API key creation by removing unnecessary feature flag.Removing the
enableChannelStatefeature flag from the call tocreateSandboxAblyAPIKey()simplifies the code and aligns with broader changes in the app creation logic. This suggests the feature is now enabled by default or no longer needs explicit enablement.src/plugins/objects/livemap.ts (1)
825-838: Added defensive checks for optional data.This change improves the robustness of the code by adding proper null/undefined checks before accessing properties of
entry.data. The previous implementation assumed thatentry.datawould always exist, which could lead to runtime errors if that assumption was violated. This defensive coding practice prevents potentialTypeErrorexceptions when handling entries with missing data.src/plugins/objects/liveobject.ts (3)
162-168: Improved update handling for tombstoned objectsThe
tombstone()method now returns aTUpdateinstead ofvoid, properly representing the state change as a diffable update that can be propagated to subscribers. This provides better consistency in how state changes are handled throughout the LiveObject lifecycle.
185-192: Good encapsulation of data clearing logicThe new
clearData()method properly encapsulates the logic for resetting object data and generating the corresponding update. This separation of concerns makes the code more maintainable and reusable.
214-214: Simplified object deletion logicThe
_applyObjectDelete()method now directly returns the result oftombstone(), making the code more concise while maintaining the same behavior.src/common/lib/types/basemessage.ts (2)
99-104: Centralized native data type detectionThe native data type check is now performed once in the
encodefunction and passed down toencodeData, avoiding redundant checks and improving code efficiency.
119-121: Simplified type checking in encodeDataThe function now accepts the pre-computed
isNativeDataTypeflag instead of rechecking, which makes the code more efficient and maintainable.src/plugins/objects/objectspool.ts (4)
21-21: Renamed method with more precise intentThe method name change from
_getInitialPool()to_createInitialPool()better reflects that the method is creating a new pool rather than retrieving an existing one.
51-59: Improved pool reset mechanicsThe new
resetToInitialPoolmethod properly preserves the root object reference while clearing other objects, which maintains consistent object references for subscribers.
64-71: Better control over data clearing and event emissionThe
clearObjectsDatamethod provides fine-grained control over when update events are emitted during data clearing, which is important for ensuring proper synchronization behavior.
96-96: Consistent naming of pool initialization methodsRenaming
_getInitialPoolto_createInitialPoolaligns with the method's behavior of creating a new pool instance.src/plugins/objects/objectmessage.ts (5)
70-70: Made MapEntry.data optional for better defensive programmingMaking the
dataproperty optional in theMapEntryinterface correctly reflects that entries might not always have data, particularly for tombstoned entries. This avoids potential null reference errors when working with map entries.
189-203: Improved encoding method with explicit client contextThe
encodemethod was changed from async to sync and now accepts aBaseClientinstance instead of justMessageEncoding. This provides access to platform-specific utilities needed for native type checking. The explicit checks for native data types match those inbasemessage.ts, ensuring consistent behavior.
336-338: Added defensive check for entry.data before decodingThis check prevents potential errors when processing MapEntry objects with undefined data, improving robustness.
378-382: Added defensive check for entry.data in object operation encodingThis properly handles cases where map entries might not have data, preventing potential null reference errors during encoding.
405-408: Added defensive check for entry.data in object state encodingSimilar to the other defensive checks, this ensures that entries without data are handled safely during object state encoding.
src/plugins/objects/objects.ts (1)
268-271: Verify event-ordering side-effects when emitting updates on an empty channel
resetToInitialPool(true)deliberately emits update events before the sync sequence is formally finished.
Because_startNewSync()was invoked earlier in the same tick, the public order of events becomes:
syncing(internal + public) ‑- from_startNewSync- object‐update events from
resetToInitialPool(true)synced(internal + public) ‑- from_endSyncIf consumers assume that all object-update callbacks occur after a
syncedevent, this new ordering may break them.
Please double-check existing listeners/tests and, if necessary, defer the update events (or document the expected order).test/realtime/objects.test.js (1)
138-160: Always restoretransport.onProtocolMessageinwaitForObjectSyncIf the promise rejects or the test times out, the patched
transport.onProtocolMessageremains in place, potentially leaking into later tests that share the same connection object and altering assertions in hard-to-trace ways.transport.onProtocolMessage = function (message) { try { … - if (message.action === 20) { + if (message.action === 20) { … transport.onProtocolMessage = onProtocolMessageOriginal; resolve(); } } catch (err) { - reject(err); + transport.onProtocolMessage = onProtocolMessageOriginal; + reject(err); } };Adding a
finallystyle cleanup (ortransport.once) guarantees the original handler is always restored.
[ suggest_essential_refactor ]
…with HAS_OBJECTS=false Object references must be kept the same whenever possible so that end-user keeps the correct reference and its subscriptions to the object whenever resynchronization happens, see [1]. On channel DETACHED or FAILED only the objects data should be removed, and no update events should be emitted, see [2]. [1] https://ably.atlassian.net/wiki/spaces/LOB/pages/3382738945/LODR-022+Realtime+Client+read-only+internal+spec#7.-Decoupling-of-the-underlying-state-data-and-LiveObject-class-instances [2] https://ably.atlassian.net/wiki/spaces/LOB/pages/3784933378/LODR-032+Realtime+Client+behavior+under+channel+states
…tion is set to false
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/realtime/objects.test.js (1)
695-696: Consider consolidating duplicate buffer equality checks.These buffer comparisons appear in multiple places in the tests. Consider extracting this into a helper function to reduce duplication and improve maintainability.
-helper.recordPrivateApi('call.BufferUtils.base64Decode'); -helper.recordPrivateApi('call.BufferUtils.areBuffersEqual'); -expect( - BufferUtils.areBuffersEqual(valuesMap.get('emptyBytesKey'), BufferUtils.base64Decode('')), - 'Check values map has correct empty bytes value key', -).to.be.true; +helper.recordPrivateApi('call.BufferUtils.base64Decode'); +helper.recordPrivateApi('call.BufferUtils.areBuffersEqual'); +expectBufferEqual( + valuesMap.get('emptyBytesKey'), + '', + 'Check values map has correct empty bytes value key' +); // Add this helper function: // function expectBufferEqual(actual, expectedBase64, message) { // return expect( // BufferUtils.areBuffersEqual(actual, BufferUtils.base64Decode(expectedBase64)), // message // ).to.be.true; // }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (8)
src/plugins/objects/liveobject.ts(3 hunks)src/plugins/objects/objects.ts(6 hunks)src/plugins/objects/objectspool.ts(3 hunks)src/plugins/objects/syncobjectsdatapool.ts(1 hunks)test/common/modules/private_api_recorder.js(1 hunks)test/common/modules/testapp_manager.js(0 hunks)test/package/browser/template/src/index-objects.ts(1 hunks)test/realtime/objects.test.js(8 hunks)
💤 Files with no reviewable changes (1)
- test/common/modules/testapp_manager.js
🚧 Files skipped from review as they are similar to previous changes (6)
- src/plugins/objects/syncobjectsdatapool.ts
- test/package/browser/template/src/index-objects.ts
- test/common/modules/private_api_recorder.js
- src/plugins/objects/objects.ts
- src/plugins/objects/liveobject.ts
- src/plugins/objects/objectspool.ts
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (16.x)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-npm-package
🔇 Additional comments (7)
test/realtime/objects.test.js (7)
138-160: Good addition of thewaitForObjectSynchelper function.This helper function improves test reliability by specifically waiting for OBJECT_SYNC protocol messages (action 20). It follows the same pattern as the existing
waitForObjectOperationfunction but with a specific focus on synchronization events.
444-477: Well-structured test fixtures for primitives and counters.The creation of these fixtures (
primitiveKeyData,primitiveMapsFixtures,countersFixtures) centralizes test data and makes tests more maintainable. They provide good coverage of different data types and value ranges, including edge cases like empty values and maximum safe integers.
479-741: Good refactoring to structured scenario testing for object sync.Replacing individual test calls with the structured
objectSyncSequenceScenariosarray improves test organization and maintainability. The scenarios thoroughly test object tree building, operation application, reference preservation, and initialization with initial values from OBJECT_SYNC sequences.
4453-4596: Good validation tests for client configuration scenarios.The comprehensive coverage of various error conditions (missing object modes, invalid channel states) ensures robust API validation. The rename from
channelConfigurationScenariostoclientConfigurationScenariosbetter reflects the scope of these tests.
4578-4596: Important validation for theechoMessagesclient option.This new test properly verifies that the public write API throws an appropriate error when the
echoMessagesclient option is disabled. This is a crucial validation for ensuring correct behavior of the realtime objects API.
4698-4714: Good test coverage for new object message properties.The addition of test cases for
nonceandinitialValueproperties ensures proper message size calculations for these properties, maintaining consistency with the underlying implementation changes.
4947-4947: Updated ObjectMessage.encode to accept client instance.The change to pass the client instance to
ObjectMessage.encodealigns with updates in the source code where the staticencodemethod now requires aBaseClientinstance instead of just a message encoding type.
Some minor fixes, mostly based on coderabbit review from #2007.
Best to review by going commit by commit.
Summary by CodeRabbit
Bug Fixes
New Features
Refactor
Tests
Chores