[WIP] [DTP-1104] Surface live objects to the end users only when they become valid #1932
Conversation
WalkthroughThe pull request introduces significant modifications across several classes within the LiveObjects plugin, including Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
🔇 Additional comments (6)src/plugins/liveobjects/livemap.ts (6)
The enhanced documentation and validation logic effectively prevent invalid objects from being surfaced to end users, which aligns well with the PR objectives. Also applies to: 88-118
The size calculation now correctly excludes non-valid objects, maintaining consistency with the get method behavior.
The implementation correctly buffers MAP_SET operations that reference invalid objects, preventing them from being surfaced to end users prematurely.
Handle potential null reference safely The non-null assertion operator usage on the referenced object is risky, even though Apply this diff to handle the case safely: - const { off } = this._liveObjects
- .getPool()
- .get(refObjectId)!
- .onceValid(() => {
+ const refObject = this._liveObjects.getPool().get(refObjectId);
+ if (!refObject) {
+ return;
+ }
+ const { off } = refObject.onceValid(() => {
Ensure buffered operation cleanup on error The buffered operation cleanup should happen in the error case as well to prevent memory leaks. Move the cleanup to the finally block: try {
const update = this._applyMapSet(op, opOriginTimeserial);
this.notifyUpdated(update);
} catch (error) {
this._client.Logger.logAction(
this._client.logger,
this._client.Logger.LOG_ERROR,
`LiveMap._handleMapSetWithInvalidObjectReference()`,
`error applying buffered MAP_SET operation: ${this._client.Utils.inspectError(error)}`,
);
- } finally {
- this._bufferedOperations.delete(bufferedOperation);
}
+ } finally {
+ this._bufferedOperations.delete(bufferedOperation);
+ off(); // Ensure subscription is cleaned up
+ }
Need to verify if buffered operations are properly cleaned up when the LiveMap is destroyed to prevent memory leaks. ✅ Verification successfulLet me gather more information to verify the cleanup handling in LiveMap. Let me check the parent class LiveObject since LiveMap extends it and we found cleanup in LiveObject. Buffered operations are properly cleaned up The verification shows that buffered operations cleanup is handled properly:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for cleanup of buffered operations in destructor or cleanup methods
ast-grep --pattern 'class LiveMap {
$$$
destroy() {
$$$
this._bufferedOperations$$$
$$$
}
$$$
}'
# Search for any other cleanup patterns
rg -A 5 "_bufferedOperations\.clear\(\)"
Length of output: 574 Script: #!/bin/bash
# Search for LiveMap destroy method implementation
ast-grep --pattern 'class LiveMap {
$$$
destroy() {
$$$
}
$$$
}'
# Search for any cleanup or disposal patterns in LiveMap
rg -A 10 "class LiveMap" src/plugins/liveobjects/livemap.ts
# Check if LiveMap extends any base class that might handle cleanup
rg "class LiveMap(\s+extends|\s+implements)" -A 2
# Search for buffered operations initialization
ast-grep --pattern 'class LiveMap {
$$$
private _bufferedOperations = $_;
$$$
}'
Length of output: 598 Script: #!/bin/bash
# Check LiveObject's destroy method implementation
ast-grep --pattern 'class LiveObject {
$$$
destroy() {
$$$
}
$$$
}'
# Check how buffered operations are handled in LiveObject
rg -A 10 "_bufferedOperations" src/plugins/liveobjects/liveobject.ts
# Check if LiveMap overrides or extends destroy method
ast-grep --pattern 'class LiveMap extends LiveObject {
$$$
destroy() {
$$$
}
$$$
}'
Length of output: 1634 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
temporarily deployed
to
staging/pull/1932/features
github-actions
bot
temporarily deployed
to
staging/pull/1932/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1932/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: 2
🧹 Outside diff range and nitpick comments (1)
src/plugins/liveobjects/statemessage.ts (1)
321-326: LGTM! Consider enhancing JSDoc.The implementation correctly identifies MAP_SET operations that reference other objects, which is crucial for the PR objective. Consider enhancing the JSDoc to document the return value:
/** * Returns true if this state message is a state operation with `MAP_SET` action and it sets a map entry to point to another objectId. + * @returns {boolean} true if the operation is MAP_SET and references another object via objectId, false otherwise */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/plugins/liveobjects/livecounter.ts(2 hunks)src/plugins/liveobjects/livemap.ts(8 hunks)src/plugins/liveobjects/liveobject.ts(6 hunks)src/plugins/liveobjects/liveobjects.ts(5 hunks)src/plugins/liveobjects/liveobjectspool.ts(1 hunks)src/plugins/liveobjects/statemessage.ts(1 hunks)test/realtime/live_objects.test.js(4 hunks)
🧰 Additional context used
📓 Learnings (1)
src/plugins/liveobjects/livecounter.ts (1)
Learnt from: VeskeR
PR: ably/ably-js#1897
File: src/plugins/liveobjects/livecounter.ts:93-93
Timestamp: 2024-11-12T07:31:53.691Z
Learning: In the `LiveCounter` class's `_applyCounterCreate` method, it's intentional to increment the counter's value using `+=` instead of initializing it with `=` because the counter may have a pre-existing non-zero value.
🪛 Biome (1.9.4)
test/realtime/live_objects.test.js
[error] 965-965: 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 (12)
src/plugins/liveobjects/liveobjectspool.ts (1)
65-66: Implementation of cancelBufferedOperations is correct
The cancelBufferedOperations method effectively iterates over all LiveObject instances in the pool and calls their cancelBufferedOperations methods, ensuring that buffered operations are properly canceled across all live objects.
src/plugins/liveobjects/liveobject.ts (7)
8-8: Addition of Valid event enhances event handling
Introducing the Valid event to LiveObjectEvents allows for better tracking of an object's validity state, enabling listeners to react appropriately when an object becomes valid.
29-31: OnEventResponse interface provides clarity
The OnEventResponse interface standardizes the structure for event subscription management, ensuring consistent implementation of the off method for unsubscribing.
54-54: Initialization of _bufferedOperations set is appropriate
Initializing _bufferedOperations as a Set<BufferedOperation> ensures efficient handling of buffered operations, allowing for quick addition, iteration, and removal of operations.
116-129: isValid method accurately reflects object validity
The isValid method correctly determines the validity of the LiveObject based on whether its create operation has been merged, which is essential for ensuring only valid objects are exposed to end users.
131-142: onceValid method facilitates one-time event listeners
The onceValid method effectively allows consumers to register a callback that is invoked when the object becomes valid, and provides an off method to unsubscribe if needed.
147-150: Proper cancellation and clearing of buffered operations
The cancelBufferedOperations method correctly iterates over all buffered operations, calls their cancel methods, and clears the set to prevent memory leaks and ensure that obsolete operations do not interfere with the object's state.
171-179: _setCreateOperationIsMerged correctly emits Valid event
By emitting the Valid event only when the create operation changes from not merged to merged, the method ensures that listeners are notified precisely when the object becomes valid without redundant notifications.
src/plugins/liveobjects/liveobjects.ts (1)
238-277: Robust handling of state messages in _applyStateMessages
The method _applyStateMessages properly checks for the presence of the operation field in each StateMessage and processes supported actions accordingly. This ensures that the system remains stable even when encountering incomplete or unsupported messages.
test/realtime/live_objects.test.js (1)
965-965:
Avoid assignments within expressions
At line 965, there's an assignment within an if condition, which can lead to confusion and potential bugs because it's easy to overlook the assignment when reading the code.
Refactor the code to separate the assignment from the condition for better readability and maintainability:
- if (currentUpdateIndex = expectedMapUpdates.length - 1) {
+ if (currentUpdateIndex === expectedMapUpdates.length - 1) {Ensure that you use the comparison operator === instead of the assignment operator =. If the intent was to compare values, this change corrects the logic. If the intent was to assign and then compare, it's better to separate these actions:
- if (currentUpdateIndex = expectedMapUpdates.length - 1) {
+ currentUpdateIndex = expectedMapUpdates.length - 1;
+ if (currentUpdateIndex) {
// ... code
}However, assigning within a condition is discouraged.
Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 965-965: 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/livecounter.ts (2)
152-152: LGTM! Consistent with encapsulation pattern.
The change maintains the intentional counter increment behavior while improving state management through the setter method.
126-126: LGTM! Verify event emission on state change.
The change to use _setCreateOperationIsMerged improves encapsulation and aligns with the PR objective of controlling object validity state.
Let's verify the event emission implementation:
✅ Verification successful
State change notification is implemented correctly
The implementation in liveobject.ts shows that _setCreateOperationIsMerged properly manages state changes. When setting the flag to true, it detects the state transition and triggers notification through the shouldNotifyValid check, which aligns with the PR's objective of controlling object validity state.
- The method is used consistently across
livecounter.tsandlivemap.ts - State transitions from
falsetotrueare properly detected - The implementation follows the encapsulation pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that _setCreateOperationIsMerged emits events
# Expected: Find implementation showing event emission when flag changes
ast-grep --pattern 'function _setCreateOperationIsMerged($_) {
$$$
emit($_, $_)
$$$
}'
Length of output: 94
Script:
#!/bin/bash
# Let's try to find the implementation of _setCreateOperationIsMerged using a broader pattern
ast-grep --pattern '_setCreateOperationIsMerged($_) {
$$$
}'
# Also search for the method definition with ripgrep for better context
rg "_setCreateOperationIsMerged" -A 5
Length of output: 2769
| // map entry points to another object, get it from the pool | ||
| const refObject: LiveObject | undefined = this._liveObjects.getPool().get(data.objectId); | ||
| if (!refObject) { | ||
| return undefined as T[TKey]; | ||
| } | ||
|
|
||
| if (!refObject.isValid()) { | ||
| // non-valid objects must not be surfaced to the end users | ||
| return undefined as T[TKey]; | ||
| } | ||
|
|
||
| return refObject as API.LiveObject as T[TKey]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure safe handling of undefined references in get method
In the get method, the non-null assertion operator ! is used when accessing the refObject. Although createZeroValueObjectIfNotExists ensures the object exists, it's prudent to handle the possibility of undefined to prevent runtime errors.
Consider updating the code to handle potential undefined values gracefully:
- const refObject: LiveObject | undefined = this._liveObjects.getPool().get(data.objectId);
- if (!refObject) {
- return undefined as T[TKey];
- }
-
- if (!refObject.isValid()) {
- // non-valid objects must not be surfaced to the end users
- return undefined as T[TKey];
- }
-
- return refObject as API.LiveObject as T[TKey];
+ const refObject = this._liveObjects.getPool().get(data.objectId);
+ if (refObject && refObject.isValid()) {
+ return refObject as API.LiveObject as T[TKey];
+ } else {
+ // Non-valid or undefined objects must not be surfaced to the end users
+ return undefined as T[TKey];
+ }This change ensures that refObject is both defined and valid before returning it.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // map entry points to another object, get it from the pool | |
| const refObject: LiveObject | undefined = this._liveObjects.getPool().get(data.objectId); | |
| if (!refObject) { | |
| return undefined as T[TKey]; | |
| } | |
| if (!refObject.isValid()) { | |
| // non-valid objects must not be surfaced to the end users | |
| return undefined as T[TKey]; | |
| } | |
| return refObject as API.LiveObject as T[TKey]; | |
| // map entry points to another object, get it from the pool | |
| const refObject = this._liveObjects.getPool().get(data.objectId); | |
| if (refObject && refObject.isValid()) { | |
| return refObject as API.LiveObject as T[TKey]; | |
| } else { | |
| // Non-valid or undefined objects must not be surfaced to the end users | |
| return undefined as T[TKey]; | |
| } |
| private _handleMapSetWithInvalidObjectReference(op: StateMapOp, opOriginTimeserial: string | undefined): void { | ||
| const refObjectId = op?.data?.objectId!; | ||
| // ensure referenced object always exist so we can subscribe to it becoming valid | ||
| this._liveObjects.getPool().createZeroValueObjectIfNotExists(refObjectId); | ||
|
|
||
| // wait until the referenced object becomes valid, then apply MAP_SET operation, | ||
| // as it will now point to the existing valid object | ||
| const { off } = this._liveObjects | ||
| .getPool() | ||
| .get(refObjectId)! | ||
| .onceValid(() => { | ||
| try { | ||
| const update = this._applyMapSet(op, opOriginTimeserial); | ||
| this.notifyUpdated(update); | ||
| } catch (error) { | ||
| this._client.Logger.logAction( | ||
| this._client.logger, | ||
| this._client.Logger.LOG_ERROR, | ||
| `LiveMap._handleMapSetWithInvalidObjectReference()`, | ||
| `error applying buffered MAP_SET operation: ${this._client.Utils.inspectError(error)}`, | ||
| ); | ||
| } finally { | ||
| this._bufferedOperations.delete(bufferedOperation); | ||
| } | ||
| }); | ||
|
|
||
| const bufferedOperation: BufferedOperation = { | ||
| cancel: () => off(), | ||
| }; | ||
| this._bufferedOperations.add(bufferedOperation); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential null references in _handleMapSetWithInvalidObjectReference
In _handleMapSetWithInvalidObjectReference, the non-null assertion operator ! is used when accessing the refObject. To enhance reliability, consider adding checks to confirm that refObject is defined before proceeding.
Modify the code to include a check for refObject:
- const { off } = this._liveObjects
- .getPool()
- .get(refObjectId)!
- .onceValid(() => {
+ const refObject = this._liveObjects.getPool().get(refObjectId);
+ if (!refObject) {
+ return;
+ }
+ const { off } = refObject.onceValid(() => {
// ... rest of the code remains the same
});This ensures that the onceValid method is only called on a defined refObject, preventing potential runtime errors.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private _handleMapSetWithInvalidObjectReference(op: StateMapOp, opOriginTimeserial: string | undefined): void { | |
| const refObjectId = op?.data?.objectId!; | |
| // ensure referenced object always exist so we can subscribe to it becoming valid | |
| this._liveObjects.getPool().createZeroValueObjectIfNotExists(refObjectId); | |
| // wait until the referenced object becomes valid, then apply MAP_SET operation, | |
| // as it will now point to the existing valid object | |
| const { off } = this._liveObjects | |
| .getPool() | |
| .get(refObjectId)! | |
| .onceValid(() => { | |
| try { | |
| const update = this._applyMapSet(op, opOriginTimeserial); | |
| this.notifyUpdated(update); | |
| } catch (error) { | |
| this._client.Logger.logAction( | |
| this._client.logger, | |
| this._client.Logger.LOG_ERROR, | |
| `LiveMap._handleMapSetWithInvalidObjectReference()`, | |
| `error applying buffered MAP_SET operation: ${this._client.Utils.inspectError(error)}`, | |
| ); | |
| } finally { | |
| this._bufferedOperations.delete(bufferedOperation); | |
| } | |
| }); | |
| const bufferedOperation: BufferedOperation = { | |
| cancel: () => off(), | |
| }; | |
| this._bufferedOperations.add(bufferedOperation); | |
| } | |
| private _handleMapSetWithInvalidObjectReference(op: StateMapOp, opOriginTimeserial: string | undefined): void { | |
| const refObjectId = op?.data?.objectId!; | |
| // ensure referenced object always exist so we can subscribe to it becoming valid | |
| this._liveObjects.getPool().createZeroValueObjectIfNotExists(refObjectId); | |
| // wait until the referenced object becomes valid, then apply MAP_SET operation, | |
| // as it will now point to the existing valid object | |
| const refObject = this._liveObjects.getPool().get(refObjectId); | |
| if (!refObject) { | |
| return; | |
| } | |
| const { off } = refObject.onceValid(() => { | |
| try { | |
| const update = this._applyMapSet(op, opOriginTimeserial); | |
| this.notifyUpdated(update); | |
| } catch (error) { | |
| this._client.Logger.logAction( | |
| this._client.logger, | |
| this._client.Logger.LOG_ERROR, | |
| `LiveMap._handleMapSetWithInvalidObjectReference()`, | |
| `error applying buffered MAP_SET operation: ${this._client.Utils.inspectError(error)}`, | |
| ); | |
| } finally { | |
| this._bufferedOperations.delete(bufferedOperation); | |
| } | |
| }); | |
| const bufferedOperation: BufferedOperation = { | |
| cancel: () => off(), | |
| }; | |
| this._bufferedOperations.add(bufferedOperation); | |
| } |
Valid live objects are those for which the realtime client has seen the corresponding CREATE operation. Resolves DTP-1104
VeskeR
force-pushed
the
DTP-1104/surface-only-valid-objects
branch
from
92c8f11 to
449ab52
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1932/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1932/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1932/features
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/plugins/liveobjects/liveobjects.ts (1)
238-277: Consider enhancing error handling in operation application.The implementation is solid, with good handling of different operation types and proper logging. However, the error handling for operation application could be more robust.
Consider wrapping the operation application in a try-catch block:
switch (stateOperation.action) { case StateOperationAction.MAP_CREATE: case StateOperationAction.COUNTER_CREATE: case StateOperationAction.MAP_SET: case StateOperationAction.MAP_REMOVE: case StateOperationAction.COUNTER_INC: - this._liveObjectsPool.createZeroValueObjectIfNotExists(stateOperation.objectId); - this._liveObjectsPool.get(stateOperation.objectId)!.applyOperation(stateOperation, stateMessage); + try { + this._liveObjectsPool.createZeroValueObjectIfNotExists(stateOperation.objectId); + const targetObject = this._liveObjectsPool.get(stateOperation.objectId); + if (!targetObject) { + throw new this._client.ErrorInfo( + `Failed to create zero-value object for id=${stateOperation.objectId}`, + 50000, + 500 + ); + } + targetObject.applyOperation(stateOperation, stateMessage); + } catch (error) { + this._client.Logger.logAction( + this._client.logger, + this._client.Logger.LOG_ERROR, + 'LiveObjects._applyStateMessages()', + `Failed to apply operation: ${this._client.Utils.inspectError(error)}` + ); + } break;test/realtime/live_objects.test.js (3)
707-733: Improve code structure and readabilityConsider these improvements to make the code more maintainable:
- Use consistent indentation
- Consider using a more concise array declaration
- const expectedMaps = [ - { exists: false }, // MAP_CREATE not applied, object is not valid and we should get undefined - { exists: false }, // MAP_CREATE not applied, object is not valid and we should get undefined - { exists: true, data: { foo: 'bar', baz: 'qux' } }, // applied MAP_CREATE, object is valid - { exists: true, data: { foo: 'bar', baz: 'qux' } }, // applied MAP_CREATE, object is valid - { exists: true, data: { foo: 'bar', baz: 'qux' } }, // applied MAP_CREATE, object is valid - ]; + const expectedMaps = Array(5).fill(null).map((_, i) => + i < 2 + ? { exists: false } // MAP_CREATE not applied + : { exists: true, data: { foo: 'bar', baz: 'qux' } } // applied MAP_CREATE + );
1406-1427: Improve code structure for consistencySimilar to the earlier suggestion, consider improving the code structure:
- const expectedCounters = [ - { exists: false }, // COUNTER_CREATE not applied, object is not valid and we should get undefined - { exists: false }, // COUNTER_CREATE not applied, object is not valid and we should get undefined - { exists: true, value: 11 }, // applied COUNTER_CREATE - { exists: true, value: 11 }, // applied COUNTER_CREATE - { exists: true, value: 11 }, // applied COUNTER_CREATE - ]; + const expectedCounters = Array(5).fill(null).map((_, i) => + i < 2 + ? { exists: false } // COUNTER_CREATE not applied + : { exists: true, value: 11 } // applied COUNTER_CREATE + );
1713-1713: Consider usingmaterialisedEntriesconsistentlyThe code uses an empty
initialEntriesobject while actual entries are defined inmaterialisedEntries. For consistency and clarity, consider:
- Either move all entries to
materialisedEntries- Or document why some entries are in
initialEntriesvsmaterialisedEntries
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/plugins/liveobjects/livecounter.ts(2 hunks)src/plugins/liveobjects/livemap.ts(8 hunks)src/plugins/liveobjects/liveobject.ts(6 hunks)src/plugins/liveobjects/liveobjects.ts(5 hunks)src/plugins/liveobjects/liveobjectspool.ts(1 hunks)src/plugins/liveobjects/statemessage.ts(1 hunks)test/realtime/live_objects.test.js(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/plugins/liveobjects/statemessage.ts
- src/plugins/liveobjects/livecounter.ts
- src/plugins/liveobjects/liveobject.ts
🧰 Additional context used
🪛 Biome (1.9.4)
test/realtime/live_objects.test.js
[error] 1009-1009: 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 (1)
65-67: LGTM! Method correctly handles buffered operations cleanup.
The implementation properly delegates cancellation to each object in the pool. This is crucial for maintaining consistency during sync operations.
Let's verify the usage of this method:
✅ Verification successful
Implementation verified and correctly used in sync operations
The method is properly implemented and used in a critical context within LiveObjects sync operations. It's called specifically when starting a new sync to cancel buffered operations that would become invalid, maintaining data consistency. The implementation chain from LiveObjects -> LiveObjectsPool -> LiveObject ensures proper cleanup of buffered operations at all levels.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check where cancelBufferedOperations is called
rg -A 3 "cancelBufferedOperations"
Length of output: 1030
Script:
#!/bin/bash
# Let's check the implementation of LiveObject's cancelBufferedOperations
ast-grep --pattern 'class LiveObject {
$$$
cancelBufferedOperations() {
$$$
}
$$$
}'
# And let's see where it's used in the sync context
rg -B 3 "cancelBufferedOperations" src/plugins/liveobjects/liveobjects.ts
Length of output: 483
src/plugins/liveobjects/liveobjects.ts (1)
152-153: LGTM! Appropriate placement of buffered operations cleanup.
The addition of cancelBufferedOperations here ensures a clean state before starting a new sync operation.
src/plugins/liveobjects/livemap.ts (4)
Line range hint 80-118: Previous review comment about undefined handling is still applicable.
The implementation should handle undefined references more safely as suggested in the previous review.
129-134: LGTM! Proper handling of invalid objects in size calculation.
The implementation correctly excludes invalid objects from the count, maintaining consistency with the get method behavior.
169-177: LGTM! Well-implemented handling of invalid object references.
The implementation correctly buffers operations for invalid object references, preventing inconsistent state exposure to users.
378-408: Previous review comment about null reference handling is still applicable.
Additionally, let's verify the implementation of buffered operations across the codebase.
#!/bin/bash
# Description: Check implementation of buffered operations
# Look for other implementations of buffered operations
ast-grep --pattern 'interface BufferedOperation {
$$$
}'
# Check for usage of buffered operations
rg -A 3 "BufferedOperation"test/realtime/live_objects.test.js (1)
903-994: Well-structured test case that directly addresses the PR objective
This test case effectively verifies that MAP_SET operations with references to invalid objects are properly buffered until the objects become valid. The test coverage is comprehensive and includes:
- Verification of initial invalid state
- Buffering of operations
- Proper surfacing of objects only after they become valid
The implementation aligns perfectly with the PR's goal of surfacing live objects only when they become valid.
| subscribeCallbackCalledCount++; | ||
| Object.keys(update).forEach((x) => (keyUpdated[x] = true)); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid assignment within expressions
The current code assigns values within a forEach expression, which can be confusing and error-prone. Consider restructuring to make the assignment more explicit:
- root.subscribe(({ update }) => {
- subscribeCallbackCalledCount++;
- Object.keys(update).forEach((x) => (keyUpdated[x] = true));
- });
+ root.subscribe(({ update }) => {
+ subscribeCallbackCalledCount++;
+ Object.keys(update).forEach((x) => {
+ keyUpdated[x] = true;
+ });
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| subscribeCallbackCalledCount++; | |
| Object.keys(update).forEach((x) => (keyUpdated[x] = true)); | |
| }); | |
| subscribeCallbackCalledCount++; | |
| Object.keys(update).forEach((x) => { | |
| keyUpdated[x] = true; | |
| }); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 1009-1009: 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)
github-actions
bot
temporarily deployed
to
staging/pull/1932/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/1932/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/1932/features
…for keys that reference other LiveObjects Referenced live objects can be not valid (haven't seen a create op) and we should not surface such objects to the end user and return `undefined` instead.
VeskeR
force-pushed
the
DTP-1104/surface-only-valid-objects
branch
from
8005fd2 to
6ef073d
Compare
VeskeR
changed the title
|
Putting this PR in draft for now as https://ably.atlassian.net/browse/DTP-1104 is put on hold, see https://ably.atlassian.net/wiki/spaces/LOB/pages/3602874370/LODR-030+Surfacing+of+non-valid+objects+to+end+users+in+the+Realtime+Client |
Tried/thought about other implementation options that wouldn't work here:
buffering
MAP_SEToperations at the mainLiveObjectsclass level (not within a specificLiveMapinstance): does not work because other operations for the object may arrive, updating its site timeserials. As a result, when we attempt to applyMAP_SET, it will be rejected due to an earlier site timeserial.creating a zero-value object for an object ID that does not yet exist and setting a key in a map to point to that zero-value object while postponing the update event on the map until the referenced object becomes valid. This also doesn't work: the value on the map has already changed (now returning
undefineddue to the invalid object), and it no longer returns the previous value (we effectively have overwritten it). However, we did not invoke the subscription callback for the user, so they are unaware that the value for the key in the map is nowundefined. This could lead to all sorts of nasty bugs in users' applications. Calling subscription callback two times (first when it becomesundefined, second time when object is valid) is also not ideal as it provides poor DXResolves DTP-1104
Summary by CodeRabbit
Release Notes
New Features
LiveCounter,LiveMap, andLiveObjectclasses.LiveObject.StateMessageto check forMAP_SEToperations referencingobjectId.LiveMapto improve return type clarity.Bug Fixes
LiveMapoperations.Tests
MAP_SETandMAP_REMOVEoperations.