[PUB-2061] Add path based subscriptions support for LiveObjects #2097
Conversation
WalkthroughAdds path- and instance-level subscription APIs and a PathObjectSubscriptionRegister; replaces LiveObject update callbacks with EventCallback-based events carrying full ObjectMessage payloads; adds parent-reference tracking and rebuild after sync; adds _type discriminants to map/counter updates; broadens buffer/value typings; and adds tests for subscription behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant SRV as Server
participant RT as RealtimeObject
participant LO as LiveObject
participant REG as PathObjectSubscriptionRegister
participant IL as InstanceListener
participant PL as PathListener
rect rgb(245,248,255)
note right of RT: Sync batch arrives with ObjectMessage
SRV->>RT: Sync batch (updates + ObjectMessage)
RT->>LO: applySync -> notifyUpdated(update)
LO->>IL: instanceSubscribe callbacks (event { object, message })
LO->>REG: notifyPathEvents(PathEvent{ path, update, message, bubbles })
REG->>PL: PathObjectSubscriptionEvent{ object, message } -- filtered by depth/bubbling -->
end
sequenceDiagram
autonumber
participant C as Client
participant PO as PathObject
participant RT as RealtimeObject
participant REG as Registry
C->>PO: subscribe(listener, options)
PO->>RT: getPathObjectSubscriptionRegister()
PO->>REG: subscribe(path[], listener, options)
REG-->>C: SubscribeResponse (unsubscribe function)
sequenceDiagram
autonumber
participant C as Client
participant INST as DefaultInstance
participant LO as LiveObject
C->>INST: subscribe(listener)
INST->>LO: instanceSubscribe(mappedListener)
Note right of LO: tombstone triggers notify + instance unsubscribe cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
temporarily deployed
to
staging/pull/2097/features
github-actions
bot
temporarily deployed
to
staging/pull/2097/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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ably.d.ts (1)
2885-2893: Fix generic type on instance subscriptions.
InstanceSubscriptionEventis declared as a generic (InstanceSubscriptionEvent<T extends Value>), but this signature drops the type argument, which is what the TS2314 failures in CI are complaining about. Please thread the generic through so the callback sees the correct shape:- subscribe(listener: EventCallback<InstanceSubscriptionEvent>): SubscribeResponse; + subscribe(listener: EventCallback<InstanceSubscriptionEvent<T>>): SubscribeResponse;Do the same for any overloads so all declarations compile.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
ably.d.ts(10 hunks)scripts/moduleReport.ts(1 hunks)src/plugins/objects/instance.ts(2 hunks)src/plugins/objects/livecounter.ts(7 hunks)src/plugins/objects/livemap.ts(12 hunks)src/plugins/objects/liveobject.ts(9 hunks)src/plugins/objects/objectmessage.ts(2 hunks)src/plugins/objects/objectspool.ts(3 hunks)src/plugins/objects/pathobject.ts(2 hunks)src/plugins/objects/pathobjectsubscriptionregister.ts(1 hunks)src/plugins/objects/realtimeobject.ts(10 hunks)test/realtime/objects.test.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
src/plugins/objects/instance.ts (3)
ably.d.ts (5)
ObjectMessage(3091-3138)EventCallback(1653-1653)InstanceSubscriptionEvent(3081-3086)LiveObject(2451-2451)Instance(3045-3051)src/plugins/objects/objectmessage.ts (1)
ObjectMessage(338-434)src/plugins/objects/liveobject.ts (2)
LiveObjectUpdate(18-23)SubscribeResponse(31-33)
src/plugins/objects/pathobjectsubscriptionregister.ts (5)
ably.d.ts (4)
EventCallback(1653-1653)PathObjectSubscriptionEvent(3056-3061)PathObjectSubscriptionOptions(3066-3076)ObjectMessage(3091-3138)src/plugins/objects/objectmessage.ts (1)
ObjectMessage(338-434)src/plugins/objects/liveobject.ts (2)
LiveObjectUpdate(18-23)SubscribeResponse(31-33)src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(41-586)src/plugins/objects/pathobject.ts (2)
path(41-44)DefaultPathObject(23-305)
src/plugins/objects/livecounter.ts (1)
src/plugins/objects/liveobject.ts (1)
LiveObjectUpdateNoop(25-29)
src/plugins/objects/livemap.ts (1)
src/plugins/objects/liveobject.ts (1)
LiveObjectUpdateNoop(25-29)
src/plugins/objects/objectmessage.ts (2)
src/plugins/objects/liveobject.ts (1)
LiveObjectUpdate(18-23)ably.d.ts (1)
ObjectMessage(3091-3138)
src/plugins/objects/pathobject.ts (2)
ably.d.ts (4)
Value(2458-2458)EventCallback(1653-1653)PathObjectSubscriptionEvent(3056-3061)PathObjectSubscriptionOptions(3066-3076)src/plugins/objects/liveobject.ts (1)
SubscribeResponse(31-33)
ably.d.ts (2)
src/plugins/objects/liveobject.ts (1)
SubscribeResponse(31-33)src/plugins/objects/objectmessage.ts (1)
ObjectMessage(338-434)
src/plugins/objects/realtimeobject.ts (3)
src/plugins/objects/pathobjectsubscriptionregister.ts (1)
PathObjectSubscriptionRegister(40-209)src/plugins/objects/pathobject.ts (2)
DefaultPathObject(23-305)value(80-113)src/plugins/objects/livemap.ts (1)
LiveMap(51-1102)
src/plugins/objects/liveobject.ts (5)
ably.d.ts (4)
LiveObject(2451-2451)EventCallback(1653-1653)ObjectMessage(3091-3138)LiveMapType(2405-2407)src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(41-586)src/plugins/objects/instance.ts (1)
InstanceEvent(9-14)src/plugins/objects/livemap.ts (1)
LiveMapUpdate(45-48)src/plugins/objects/pathobjectsubscriptionregister.ts (1)
PathEvent(23-32)
src/plugins/objects/objectspool.ts (2)
ably.d.ts (4)
LiveMapType(2405-2407)AblyDefaultObject(3147-3154)LiveMap(2435-2438)LiveObject(2451-2451)src/plugins/objects/livemap.ts (1)
LiveMap(51-1102)
test/realtime/objects.test.js (5)
src/plugins/objects/livemap.ts (1)
LiveMap(51-1102)objects.d.ts (2)
LiveMap(16-29)LiveCounter(34-45)src/plugins/objects/pathobject.ts (1)
path(41-44)src/plugins/objects/liveobject.ts (1)
unsubscribe(111-122)src/plugins/objects/livecounter.ts (1)
LiveCounter(23-374)
🪛 GitHub Actions: Lint
ably.d.ts
[error] 2889-2889: TS2314: Generic type 'InstanceSubscriptionEvent' requires 1 type argument(s).
🪛 GitHub Actions: Test NPM package
ably.d.ts
[error] 2889-2889: TypeScript error TS2314: Generic type 'InstanceSubscriptionEvent' requires 1 type argument(s).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (16.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-browser (webkit)
🔇 Additional comments (5)
src/plugins/objects/livecounter.ts (5)
17-20: LGTM! Discriminant field added to LiveCounterUpdate interface.The addition of
_type: 'LiveCounterUpdate'creates a discriminated union pattern that enables type-safe runtime discrimination. This is a breaking change to the public interface, so ensure all consumers are updated accordingly.
201-201: LGTM! Explicit noop initialization improves safety.Initializing
updateto{ noop: true }instead of leaving it potentially undefined eliminates null-reference risks and makes the intent clearer.
313-316: LGTM! Consistent _type discriminant propagation.All methods returning
LiveCounterUpdatenow include the_type: 'LiveCounterUpdate'discriminant, ensuring type safety and consistency across the update generation pipeline. The propagation ofclientIdandconnectionIdfromObjectMessageenriches the update context for subscribers.Also applies to: 318-335, 365-373
229-229: LGTM – ObjectMessage now propagated to subscribers.
217-220: Deletion notifications are correctly emitted via tombstone; no update assignment needed. Removing the assignment is intentional—_applyObjectDeleteinvokestombstone, which emits the delete event and callsnotifyUpdatedinternally.
VeskeR
force-pushed
the
PUB-2061/pathobject-subscription
branch
from
547581c to
953bb0f
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2097/features
VeskeR
force-pushed
the
PUB-2061/pathobject-subscription
branch
from
953bb0f to
113304e
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2097/features
github-actions
bot
temporarily deployed
to
staging/pull/2097/bundle-report
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
test/realtime/objects.test.js (1)
4415-4441: Fix nested event path expectation.Still expecting
'nested', but the emitted path for the nested update is'nested.newKey', so the test will keep failing until the assertion is corrected.
🧹 Nitpick comments (2)
src/plugins/objects/objectmessage.ts (1)
430-431: Address the TODO comment about payload type.The TODO comment indicates that the payload type representation is incomplete. The current implementation passes the
updateobject (which isOmit<LiveObjectUpdate, '_type'>) directly as the payload, but the comment suggests a REST API-like type that describes operations on objects would be more appropriate.Do you want me to open an issue to track this task or propose a more complete payload structure based on the REST API patterns?
src/plugins/objects/realtimeobject.ts (1)
419-467: LGTM with performance consideration.The changes correctly enable full
ObjectMessagepropagation to subscriptions and ensure parent references are rebuilt after sync. The parent reference rebuilding is necessary because objects may reference other objects that weren't in the pool when initially created.One consideration:
_rebuildAllParentReferences()iterates all objects in the pool twice (clear + rebuild). For large object pools, this could impact sync performance. Consider monitoring sync latency in production.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
ably.d.ts(12 hunks)scripts/moduleReport.ts(1 hunks)src/plugins/objects/instance.ts(2 hunks)src/plugins/objects/livecounter.ts(7 hunks)src/plugins/objects/livemap.ts(12 hunks)src/plugins/objects/liveobject.ts(9 hunks)src/plugins/objects/objectmessage.ts(2 hunks)src/plugins/objects/objectspool.ts(3 hunks)src/plugins/objects/pathobject.ts(3 hunks)src/plugins/objects/pathobjectsubscriptionregister.ts(1 hunks)src/plugins/objects/realtimeobject.ts(10 hunks)test/realtime/objects.test.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/moduleReport.ts
🧰 Additional context used
🧬 Code graph analysis (11)
src/plugins/objects/pathobjectsubscriptionregister.ts (5)
ably.d.ts (4)
EventCallback(1653-1653)PathObjectSubscriptionEvent(3056-3061)PathObjectSubscriptionOptions(3066-3076)ObjectMessage(3091-3138)src/plugins/objects/objectmessage.ts (1)
ObjectMessage(338-434)src/plugins/objects/liveobject.ts (2)
LiveObjectUpdate(18-23)SubscribeResponse(31-33)src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(41-586)src/plugins/objects/pathobject.ts (2)
path(41-44)DefaultPathObject(23-305)
src/plugins/objects/objectmessage.ts (2)
src/plugins/objects/liveobject.ts (1)
LiveObjectUpdate(18-23)ably.d.ts (1)
ObjectMessage(3091-3138)
src/plugins/objects/livecounter.ts (1)
src/plugins/objects/liveobject.ts (1)
LiveObjectUpdateNoop(25-29)
src/plugins/objects/objectspool.ts (2)
ably.d.ts (4)
LiveMapType(2405-2407)AblyDefaultObject(3147-3154)LiveMap(2435-2438)LiveObject(2451-2451)src/plugins/objects/livemap.ts (1)
LiveMap(51-1102)
src/plugins/objects/livemap.ts (1)
src/plugins/objects/liveobject.ts (1)
LiveObjectUpdateNoop(25-29)
src/plugins/objects/realtimeobject.ts (3)
src/plugins/objects/pathobjectsubscriptionregister.ts (1)
PathObjectSubscriptionRegister(40-209)src/plugins/objects/pathobject.ts (2)
DefaultPathObject(23-305)value(80-113)src/plugins/objects/objectmessage.ts (1)
ObjectMessage(338-434)
ably.d.ts (2)
src/plugins/objects/liveobject.ts (1)
SubscribeResponse(31-33)src/plugins/objects/objectmessage.ts (1)
ObjectMessage(338-434)
src/plugins/objects/instance.ts (3)
ably.d.ts (5)
ObjectMessage(3091-3138)EventCallback(1653-1653)InstanceSubscriptionEvent(3081-3086)LiveObject(2451-2451)Instance(3045-3051)src/plugins/objects/objectmessage.ts (1)
ObjectMessage(338-434)src/plugins/objects/liveobject.ts (2)
LiveObjectUpdate(18-23)SubscribeResponse(31-33)
test/realtime/objects.test.js (5)
src/plugins/objects/livemap.ts (1)
LiveMap(51-1102)src/plugins/objects/pathobject.ts (1)
path(41-44)src/plugins/objects/liveobject.ts (1)
unsubscribe(111-122)src/common/lib/client/realtimechannel.ts (1)
unsubscribe(453-465)src/plugins/objects/livecounter.ts (1)
LiveCounter(23-374)
src/plugins/objects/liveobject.ts (3)
ably.d.ts (4)
LiveObject(2451-2451)EventCallback(1653-1653)ObjectMessage(3091-3138)LiveMapType(2405-2407)src/plugins/objects/instance.ts (1)
InstanceEvent(9-14)src/plugins/objects/pathobjectsubscriptionregister.ts (1)
PathEvent(23-32)
src/plugins/objects/pathobject.ts (3)
ably.d.ts (7)
Value(2458-2458)AnyPathObject(2664-2695)Instance(3045-3051)LiveObject(2451-2451)EventCallback(1653-1653)PathObjectSubscriptionEvent(3056-3061)PathObjectSubscriptionOptions(3066-3076)src/plugins/objects/instance.ts (2)
value(53-75)DefaultInstance(16-149)src/plugins/objects/liveobject.ts (1)
SubscribeResponse(31-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (16.x)
🔇 Additional comments (19)
src/plugins/objects/objectspool.ts (2)
39-45: LGTM!The
getAll()method correctly exposes pool iteration for internal operations like parent reference rebuilding.
67-67: LGTM!Using
getRoot()instead of direct pool access improves consistency and centralizes root object retrieval logic.src/plugins/objects/instance.ts (2)
9-14: LGTM!The
InstanceEventinterface correctly structures internal subscription events, appropriately omitting_typefrom the update payload since instance subscriptions already know the object type.
138-148: LGTM!The
subscribe()method correctly:
- Validates that the value is a
LiveObject- Maps internal
InstanceEventto publicInstanceSubscriptionEvent<T>- Transforms
ObjectMessageto user-facing format with appropriate channel contextsrc/plugins/objects/livecounter.ts (4)
17-20: LGTM!Adding the
_typediscriminator toLiveCounterUpdateenables proper type narrowing in the discriminated union and aligns with the subscription event architecture introduced in this PR.
201-201: LGTM!Initializing
updateto{ noop: true }ensures it's always defined, preventing potential undefined access. This is a good defensive programming practice.
229-229: LGTM!Passing the
msgparameter tonotifyUpdated()enables subscription callbacks to receive the fullObjectMessagecontext, fulfilling a key PR objective.
313-316: LGTM!Consistently adding
_type: 'LiveCounterUpdate'to all update objects ensures proper type discrimination and event routing throughout the subscription system.src/plugins/objects/realtimeobject.ts (5)
57-71: LGTM!The
PathObjectSubscriptionRegisteris properly initialized in the constructor and follows established patterns for similar components like_objectsPool.
94-94: LGTM!Using
getRoot()instead of direct pool access withROOT_OBJECT_IDimproves consistency and centralizes root object retrieval.
546-569: LGTM!The
_rebuildAllParentReferences()method correctly rebuilds parent-child relationships after sync operations. The two-pass approach (clear then rebuild) ensures clean state. The logic correctly handles onlyLiveMapobjects as parents sinceLiveCounterinstances don't contain references to other objects.
238-243: LGTM!The
getPathObjectSubscriptionRegister()accessor follows established patterns for exposing internal components.
419-435: LGTM!Extending
existingObjectUpdatesto track themessagealongsideobjectandupdateenables fullObjectMessagecontext to be passed to subscriptions, fulfilling a key PR objective.src/plugins/objects/pathobject.ts (5)
24-24: LGTM!Changing
_clienttoprivateappropriately restricts visibility sinceDefaultPathObjectis not designed for inheritance.
115-121: LGTM!The return type change from
AnyInstance<T>toInstance<T>correctly aligns with the public API definitions. The cast is necessary due to the type hierarchy.
126-132: LGTM!The error handling correctly returns
undefinedfor path resolution failures while preserving unexpected errors. This matches the pattern used invalue()and provides consistent API semantics.
246-262: LGTM!The documentation clearly explains the PathObject subscription model, including bubbling behavior and depth control. This will help developers understand this complex feature.
264-269: LGTM!The
subscribe()implementation correctly delegates to thePathObjectSubscriptionRegister, maintaining clean separation of concerns. The default empty options object ensures the register always receives a defined options parameter.src/plugins/objects/objectmessage.ts (1)
419-433: Enforce presence of id and timestamp before non-null assertions
Both properties on ObjectMessage are optional; using this.id! and this.timestamp! in toUserFacingMessage can throw if they’re unset. Either make them required at decode time (e.g. adjust fromValues to always include them) or add runtime checks before calling this method.
There was a problem hiding this comment.
Some high level comments, but I'd like to talk through the event bubbling strategy before I review further, and discuss the extend to which we can fully replace the old subscription event updates when delivering this new implementation. We can discuss on our call tomorrow
ably.d.ts
Outdated
| /** | ||
| * The operation payload of the object message. | ||
| */ | ||
| payload: any; |
There was a problem hiding this comment.
Can we ensure this is fully typed? The shape of operations is known so we should be able to type it. Happy to ticket this separately.
There was a problem hiding this comment.
As agreed on the call, I've removed ObjectMessage.payload and added ObjectMessage.operation instead.
If ObjectMessage is present in the subscription event, it always represents an operation on an object (not a sync message). We do not expose object sync messages, just trigger subscription listeners for them.
See: 54f8ae2
src/plugins/objects/liveobject.ts
Outdated
| return { unsubscribe }; | ||
| } | ||
|
|
||
| // TODO: replace obsolete .subscribe call with this one when we completely remove previous API and switch to path-based one |
There was a problem hiding this comment.
Can we make these changes now and remove the old subscription approach? This solution depends on the old subscription approach with the old subscription event payload, which is a subset of the data we actually want to surface. Instead of doing that, can we bubble up the underlying ObjectMessage (with a transformation step in the middle if needed) through to the path or instance object subscription, instead of translating the old subscription event into the new?
There was a problem hiding this comment.
The now obsolete LiveObject subscription API was removed in 6024f4b.
Only PathObject and Instance subscriptions API is now exposed.
Note that internally I decided to preserve the concept of the "update" object which results in the event to be sent to the path object or instance subscriptions. This "update" object now carries the ObjectMessage directly, and some kind of a diff that explains what was changed on an object. Even though we don't expose delta publicly at the moment, the internal diff object is still useful for two reasons:
- The diff object is useful when emitting events for direct key changes in collections as a result of object sync sequence - when a map was changed as a result of sync sequence, the ObjectMessage is not sufficient enough to identify the keys that were changed, we need to know specific keys that got updated. The previous implementation already did that, so I kept it as is internally and resused it to emit PathObject subscription events for direct key changes.
- When we eventually implement delta for subscription events, we can reuse existing internals that calculated diffs for objects, so no point to remove it now just to re-add it later.
VeskeR
force-pushed
the
PUB-2060/value-types-object-creation
branch
6 times, most recently
from
0ffc803 to
c2dd505
Compare
VeskeR
force-pushed
the
PUB-2061/pathobject-subscription
branch
from
113304e to
6d712ac
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2097/features
github-actions
bot
temporarily deployed
to
staging/pull/2097/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/2097/typedoc
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
src/plugins/objects/liveobject.ts (1)
369-401: Non-bubbling per-key path events drop the key’s payload (reintroduces ambiguity) — scope the update per keyExact-path listeners receive ObjectMessage with empty payload, making it impossible to distinguish updated vs removed when values can be undefined. Scope the LiveMap update to the single key and attach it to the event.
Apply this diff:
// For LiveMapUpdate, also create non-bubbling events for each updated key if (update._type === 'LiveMapUpdate') { const updatedKeys = Object.keys((update as LiveMapUpdate<LiveMapType>).update); for (const key of updatedKeys) { for (const basePath of paths) { + const change = (update as LiveMapUpdate<LiveMapType>).update[ + key as keyof LiveMapType & string + ]; + const scopedUpdate: Omit<LiveObjectUpdate, '_type'> = { + update: { [key]: change } as Record<string, 'updated' | 'removed'>, + clientId: (update as LiveObjectUpdate).clientId, + connectionId: (update as LiveObjectUpdate).connectionId, + }; pathEvents.push({ path: [...basePath, key], message: objectMessage, + update: scopedUpdate, bubbles: false, - // Don't include update object as it may include updates for other keys }); } } }Additionally, consider de-duplicating pathEvents when the same object is reachable via multiple parents to avoid duplicate callbacks.
test/realtime/objects.test.js (1)
4479-4486: Fix nested event path: expect full path'nested.newKey'Root PathObject subscriptions emit the full nested path for
set('newKey', ...). The second event should assert'nested.newKey', not'nested'.- expect(event.object.path()).to.equal('nested', 'Check nested event path'); + expect(event.object.path()).to.equal('nested.newKey', 'Check nested event path');Based on relevant code snippets (pathobject.ts describes escaped-dot full path emission).
ably.d.ts (4)
2446-2470: Preserve generic type in PathObject.subscribe and link ObjectMessage in docs
- Type info for the emitted object is lost. Make the event generic so object is PathObject<_T>.
- Link to ObjectMessage in JSDoc (per prior feedback).
Apply:
- /** - * Registers a listener that is called each time the object or a primitive value at this path is updated. - * - * The provided listener receives an object message that carried an operation. + /** + * Registers a listener that is called each time the object or a primitive value at this path is updated. + * + * The provided listener receives an {@link ObjectMessage} that carried the operation that led to the change, + * as well as a {@link PathObject} representing the path that was updated. * * By default, subscriptions observe nested changes, but you can configure the observation depth * using the `options` parameter. * * A PathObject subscription observes whichever value currently exists at this path. * The subscription remains active even if the path temporarily does not resolve to any value * (for example, if an entry is removed from a map). If the object instance at this path changes, * the subscription automatically switches to observe the new instance and stops observing the old one. * * @param listener - An event listener function. * @param options - Optional subscription configuration. * @returns A {@link SubscribeResponse} object that allows the provided listener to be deregistered from future updates. * @experimental */ - subscribe( - listener: EventCallback<PathObjectSubscriptionEvent>, - options?: PathObjectSubscriptionOptions, - ): SubscribeResponse; + subscribe( + listener: EventCallback<PathObjectSubscriptionEvent<_T>>, + options?: PathObjectSubscriptionOptions, + ): SubscribeResponse;
2865-2900: Instance.subscribe: consider doc link and clarify behavior on delete
- Link to ObjectMessage in JSDoc (per prior feedback).
- You mention auto-unsubscribe on delete; add one sentence noting the callback will receive the delete operation before unsubscription.
Apply:
- /** - * Registers a listener that is called each time this instance is updated. - * - * The provided listener receives an object message that carried an operation. + /** + * Registers a listener that is called each time this instance is updated. + * + * The provided listener receives the {@link ObjectMessage} that carried the operation that led to the change, + * as well as an {@link Instance} providing a reference to the instance that was updated. * * Instance subscriptions track a specific object instance regardless of its location. * The subscription follows the instance if it is moved within the broader structure * (for example, between map entries). * * If the instance is deleted from the channel object entirely (i.e., tombstoned), - * the listener is called with the corresponding delete operation before - * automatically deregistering. + * the listener is called with the corresponding delete operation before automatically deregistering.
3097-3106: Make PathObjectSubscriptionEvent generic and link ObjectMessageExpose the concrete PathObject type in callbacks and link the message type.
Apply:
-export type PathObjectSubscriptionEvent = { - /** The {@link PathObject} representing the updated path. */ - object: PathObject; - /** The object message describing the operation that triggered the update, if available. */ - message?: ObjectMessage; -}; +export type PathObjectSubscriptionEvent<T extends Value = Value> = { + /** The {@link PathObject} representing the updated path. */ + object: PathObject<T>; + /** The {@link ObjectMessage} describing the operation that triggered the update, if available. */ + message?: ObjectMessage; +};
3125-3131: Link ObjectMessage in InstanceSubscriptionEvent docsMinor doc tweak for consistency.
Apply:
- /** The object message describing the operation that triggered the update, if available. */ + /** The {@link ObjectMessage} describing the operation that triggered the update, if available. */
🧹 Nitpick comments (6)
src/plugins/objects/livecounter.ts (1)
130-159: Avoid redundant noop notify after deleteOBJECT_DELETE triggers tombstone(), which emits the delete update and then unsubscribes. The subsequent notifyUpdated(updateNoop, msg) is redundant.
Apply this diff:
- case ObjectOperationAction.OBJECT_DELETE: - this._applyObjectDelete(msg); - break; + case ObjectOperationAction.OBJECT_DELETE: + this._applyObjectDelete(msg); + return; // tombstone() already emitted; skip trailing notifyUpdatedtest/realtime/objects.test.js (2)
4856-4890: Make the complex path subscription actually assert an emitted eventThis test currently never triggers an update at the subscribed path and relies on a timeout. Drive an update and await the event to verify behavior deterministically.
const complexPathObject = entryPathObject.at('nested\\.key.key\\.with\\.dots'); - - let eventReceived = false; - const subscriptionPromise = new Promise((resolve) => { - complexPathObject.subscribe((event) => { - eventReceived = true; - expect(event.object.path()).to.equal( - 'nested\\.key.key\\.with\\.dots', - 'Check complex path subscription event', - ); - resolve(); - }); - }); - - // This would be challenging to test directly since we need to trigger an event - // at this exact path. For now, just verify the subscription was created successfully. - expect(eventReceived).to.be.false; // No events yet - - // Clean up by resolving the promise - setTimeout(() => { - if (!eventReceived) { - subscriptionPromise.then?.(() => {}).catch?.(() => {}); - } - }, 50); + const subscriptionPromise = new Promise((resolve, reject) => { + complexPathObject.subscribe((event) => { + try { + expect(event.object.path()).to.equal( + 'nested\\.key.key\\.with\\.dots', + 'Check complex path subscription event', + ); + resolve(); + } catch (e) { + reject(e); + } + }); + }); + const keyUpdatedPromise = waitForMapKeyUpdate(nestedMap, 'key.with.dots'); + await nestedMap.set('key.with.dots', 'updatedValue'); + await keyUpdatedPromise; + await subscriptionPromise;
4529-4533: Reduce flakiness: avoid time-based waits in subscription testsReplace setTimeout-based assertions with deterministic waits (e.g., await the producing operation, then a microtask tick) or use a Promise that rejects on unexpected events. This makes tests stable across runtimes.
Also applies to: 4575-4581
ably.d.ts (2)
2712-2712: Cross-references in operation docs are helpfulThe added references to PathObject.subscribe / Instance.subscribe improve discoverability. Consider linking them explicitly with {@link ...} for consistency.
Also applies to: 2731-2731, 2753-2753, 2788-2788, 2827-2827
3135-3182: ObjectMessage.payload is untyped; consider a discriminated unionGiven update shapes are known, replace
anywith a union (e.g., LiveMapUpdate | LiveCounterUpdate | LiveObjectUpdateNoop), or introduce a genericObjectMessage<TPayload>. If not in this PR, please ticket it.Additionally, ensure parity by adding
_type: 'LiveCounterUpdate'toLiveCounterUpdatein this file for consistency withLiveMapUpdate:export declare interface LiveCounterUpdate extends LiveObjectUpdate { _type: 'LiveCounterUpdate'; update: { amount: number }; }Would you like me to raise a follow-up PR for these typings?
src/plugins/objects/livemap.ts (1)
383-421: Avoid emitting a noop update on OBJECT_DELETEIn Line 410 you call
_applyObjectDelete(msg)but still fall through tonotifyUpdated(update, msg)whereupdateis{ noop: true }. This risks emitting a spurious noop event (or double-notifying if delete already notifies).Suggestion: return immediately after applying delete.
case ObjectOperationAction.OBJECT_DELETE: - this._applyObjectDelete(msg); - break; + this._applyObjectDelete(msg); + return; ... - this.notifyUpdated(update, msg); + this.notifyUpdated(update, msg);Please verify that
notifyUpdatedignores noop updates if you keep the current flow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
ably.d.ts(12 hunks)scripts/moduleReport.ts(1 hunks)src/plugins/objects/instance.ts(2 hunks)src/plugins/objects/livecounter.ts(7 hunks)src/plugins/objects/livemap.ts(12 hunks)src/plugins/objects/liveobject.ts(9 hunks)src/plugins/objects/objectmessage.ts(2 hunks)src/plugins/objects/objectspool.ts(3 hunks)src/plugins/objects/pathobject.ts(3 hunks)src/plugins/objects/pathobjectsubscriptionregister.ts(1 hunks)src/plugins/objects/realtimeobject.ts(10 hunks)test/realtime/objects.test.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- scripts/moduleReport.ts
- src/plugins/objects/objectmessage.ts
- src/plugins/objects/pathobjectsubscriptionregister.ts
- src/plugins/objects/objectspool.ts
🧰 Additional context used
🧬 Code graph analysis (8)
src/plugins/objects/realtimeobject.ts (2)
src/plugins/objects/pathobjectsubscriptionregister.ts (1)
PathObjectSubscriptionRegister(40-209)src/plugins/objects/pathobject.ts (2)
DefaultPathObject(23-359)value(116-155)
src/plugins/objects/livemap.ts (1)
src/plugins/objects/liveobject.ts (1)
LiveObjectUpdateNoop(25-29)
src/plugins/objects/livecounter.ts (1)
src/plugins/objects/liveobject.ts (1)
LiveObjectUpdateNoop(25-29)
src/plugins/objects/instance.ts (3)
ably.d.ts (5)
ObjectMessage(3135-3182)EventCallback(1653-1653)InstanceSubscriptionEvent(3125-3130)LiveObject(2416-2416)Instance(3089-3095)src/plugins/objects/objectmessage.ts (1)
ObjectMessage(338-434)src/plugins/objects/liveobject.ts (2)
LiveObjectUpdate(18-23)SubscribeResponse(31-33)
src/plugins/objects/pathobject.ts (3)
ably.d.ts (7)
Value(2423-2423)AnyPathObject(2649-2680)Instance(3089-3095)LiveObject(2416-2416)EventCallback(1653-1653)PathObjectSubscriptionEvent(3100-3105)PathObjectSubscriptionOptions(3110-3120)src/plugins/objects/instance.ts (2)
value(55-83)DefaultInstance(16-159)src/plugins/objects/liveobject.ts (1)
SubscribeResponse(31-33)
ably.d.ts (2)
src/plugins/objects/liveobject.ts (1)
SubscribeResponse(31-33)src/plugins/objects/objectmessage.ts (1)
ObjectMessage(338-434)
test/realtime/objects.test.js (7)
ably.d.ts (2)
LiveMap(2400-2403)LiveCounter(2408-2411)src/plugins/objects/livemap.ts (1)
LiveMap(49-1007)objects.d.ts (2)
LiveMap(16-28)LiveCounter(33-43)src/plugins/objects/pathobject.ts (1)
path(43-46)src/plugins/objects/liveobject.ts (1)
unsubscribe(111-122)src/common/lib/client/realtimechannel.ts (1)
unsubscribe(453-465)src/plugins/objects/livecounter.ts (1)
LiveCounter(15-303)
src/plugins/objects/liveobject.ts (5)
ably.d.ts (4)
LiveObject(2416-2416)EventCallback(1653-1653)ObjectMessage(3135-3182)LiveMapType(2857-2859)src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(41-523)src/plugins/objects/instance.ts (1)
InstanceEvent(9-14)src/plugins/objects/livemap.ts (1)
LiveMapUpdate(43-46)src/plugins/objects/pathobjectsubscriptionregister.ts (1)
PathEvent(23-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (16.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (chromium)
🔇 Additional comments (31)
src/plugins/objects/liveobject.ts (6)
98-109: New instance-level subscriptions look goodConfiguration guard + dedicated emitter + unsubscribe wrapper are solid.
184-206: Tombstone flow ordering is correctEmitting delete update before deregistering listeners ensures delivery; also passes ObjectMessage. Good.
231-280: Parent-reference tracking API is clear and efficientMap<parent, Set> with explicit add/remove/all/clear is appropriate.
287-325: Path resolution via iterative DFS with cycle check looks safeVisited-per-branch prevents loops; returning [] for root path is expected.
355-363: Instance and legacy update emissions are consistentStripping
_typefor public update shape and wrapping InstanceEvent with optional message is consistent with API.
346-349: No callers rely on a return value from _applyObjectDelete — all invocations are standalone calls.src/plugins/objects/instance.ts (2)
9-14: InstanceEvent shape aligns with public APICarries optional message and compact update (without
_type). Good.
148-158: Correctly forwards object + user-facing ObjectMessageValidation for LiveObject, reuse of underlying instanceSubscribe, and toUserFacingMessage mapping are correct.
src/plugins/objects/livecounter.ts (2)
242-245: Consistent update shape for diffsEmits amount delta with
_type. Looks good.
296-301: Increment update includes client/connection and typeCorrectly shaped for downstream consumers.
src/plugins/objects/realtimeobject.ts (6)
71-72: Path subscription registry initialized in constructorWiring looks correct.
94-94: Returning root from pool is fineMatches pool API and simplifies call sites.
106-107: PathObject created from pool rootIntegration with DefaultPathObject is correct.
175-181: Accessor for path subscription registerInternal accessor is clear and focused.
356-361: Apply-sync now batches updates with messages, rebuilds parents before emittingGood ordering: rebuild parent links, then notify; ensures path discovery is accurate during emission.
Also applies to: 372-405
483-506: Parent-reference rebuild is pragmaticTwo-pass clear-then-link for LiveMap children is straightforward; no action for counters is correct.
src/plugins/objects/pathobject.ts (2)
157-164: Instance return type updated and scoped to LiveObject onlyReturning DefaultInstance as public Instance is appropriate.
300-323: PathObject.subscribe delegates correctly to registryOptions plumbed, path preserved, unsubscribe handled by register. Docstrings match depth semantics implemented in register.
ably.d.ts (3)
1648-1654: New EventCallback looks goodSimple, generic, and consistent with existing callback types.
2679-2679: AnyPathObject.instance typing is appropriateReturning Instance | undefined matches runtime.
3110-3120: Depth option docs are clearConsider noting that values < 1 are treated as 1 at runtime, if that is the intended behavior.
src/plugins/objects/livemap.ts (10)
43-46: Add _type discriminator on LiveMapUpdateGood addition; enables safer narrowing downstream.
506-508: Parent reference sync after object-state overrideGood call to update parent references based on the diff; prevents stale backlinks after SYNC.
528-547: clearData(): cleans up parent referencesSolid lifecycle hygiene before delegating to super.
554-607: Update object from diff includes _typeConsistent with new discriminant strategy. LGTM.
616-652: Aggregated MAP_CREATE merge update carries _typeLooks correct; preserves client/connection metadata too.
732-764: MAP_SET: maintain parent referencesCorrectly removes old and adds new backlinks when changing object references. Nice.
766-775: MAP_SET: include _type and metadata in updateLooks good.
811-819: MAP_REMOVE: remove parent referencesBacklink cleanup on removals is correct.
837-845: MAP_REMOVE: include _type and metadata in updateLooks good.
968-1006: Helper to adjust parent references from diffsWell-scoped utility used during object-state overrides. LGTM.
VeskeR
force-pushed
the
PUB-2061/pathobject-subscription
branch
from
6d712ac to
69b0b16
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2097/features
github-actions
bot
temporarily deployed
to
staging/pull/2097/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/2097/typedoc
github-actions
bot
temporarily deployed
to
staging/pull/2097/typedoc
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/plugins/objects/liveobject.ts (1)
382-395: Non-bubbling path events lack per-key update payload.The per-key non-bubbling events are pushed without the specific key's change type (
'updated'or'removed'), so subscribers at exact key paths cannot distinguish between updates and removals. This is especially problematic when legitimate values can beundefined.Based on the past review comment from coderabbitai[bot], please scope each key's update before pushing the event:
// For LiveMapUpdate, also create non-bubbling events for each updated key if (update._type === 'LiveMapUpdate') { const updatedKeys = Object.keys((update as LiveMapUpdate<LiveMapType>).update); for (const key of updatedKeys) { + const change = (update as LiveMapUpdate<LiveMapType>).update[key as keyof LiveMapType & string]; + for (const basePath of paths) { pathEvents.push({ path: [...basePath, key], message: objectMessage, bubbles: false, }); } } }However, note that the
PathEventinterface (frompathobjectsubscriptionregister.ts) doesn't currently have anupdatefield. You'll need to either:
- Add an optional
updatefield toPathEvent, or- Modify the
objectMessageto scope it to the single key for these non-bubbling eventsPlease clarify the intended design for propagating per-key update information to exact-path subscribers.
ably.d.ts (1)
3247-3308: Previous review concern about generic parameters not addressed.According to the past review comments and the internal implementation shown in
objectmessage.ts(which usesObjectOperation<ObjectData>), the following types should be generic with default parameters:
ObjectOperation(line 3247)ObjectsMapOp(line 3271)ObjectsMap(line 3289)ObjectsMapEntry(line 3299)The previous review suggested making these generic to match the internal implementation:
export interface ObjectOperation<TData = ObjectData> { mapOp?: ObjectsMapOp<TData>; map?: ObjectsMap<TData>; // ... other fields } export interface ObjectsMapOp<TData = ObjectData> { data?: TData; // ... other fields } export interface ObjectsMap<TData = ObjectData> { entries?: Record<string, ObjectsMapEntry<TData>>; // ... other fields } export interface ObjectsMapEntry<TData = ObjectData> { data?: TData; // ... other fields }Please clarify: should the public API types be generic to match the internal implementation, or is the difference intentional?
🧹 Nitpick comments (1)
ably.d.ts (1)
2446-2446: Consider improving the return type ofcompact().The method returns
any, which loses type safety. While this might be intentional given the dynamic nature of the compact representation, consider if you could use a more specific type likeunknown(forcing consumers to narrow the type) or a union type that better describes the possible return values.Example:
- compact(): any; + compact(): unknown;Or if you know the possible shapes:
- compact(): any; + compact(): Record<string, unknown> | Primitive | undefined;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
ably.d.ts(12 hunks)src/plugins/objects/instance.ts(2 hunks)src/plugins/objects/liveobject.ts(9 hunks)src/plugins/objects/objectmessage.ts(8 hunks)src/plugins/objects/pathobjectsubscriptionregister.ts(1 hunks)src/plugins/objects/realtimeobject.ts(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/plugins/objects/pathobjectsubscriptionregister.ts
🧰 Additional context used
🧬 Code graph analysis (5)
src/plugins/objects/instance.ts (3)
ably.d.ts (5)
ObjectMessage(3195-3242)EventCallback(1653-1653)InstanceSubscriptionEvent(3129-3134)LiveObject(2416-2416)Instance(3093-3099)src/plugins/objects/objectmessage.ts (1)
ObjectMessage(360-456)src/plugins/objects/liveobject.ts (1)
SubscribeResponse(31-33)
src/plugins/objects/liveobject.ts (5)
ably.d.ts (4)
LiveObject(2416-2416)EventCallback(1653-1653)ObjectMessage(3195-3242)LiveMapType(2858-2860)src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(41-523)src/plugins/objects/instance.ts (1)
InstanceEvent(9-12)src/plugins/objects/livemap.ts (1)
LiveMapUpdate(42-45)src/plugins/objects/pathobjectsubscriptionregister.ts (1)
PathEvent(23-30)
src/plugins/objects/objectmessage.ts (1)
ably.d.ts (8)
ObjectOperationAction(3169-3175)ObjectsMapSemantics(3190-3190)PrimitiveObjectValue(3539-3539)JsonArray(3554-3554)JsonObject(3559-3559)ObjectOperation(3247-3266)ObjectData(3321-3326)ObjectMessage(3195-3242)
ably.d.ts (2)
src/plugins/objects/liveobject.ts (1)
SubscribeResponse(31-33)src/plugins/objects/objectmessage.ts (3)
ObjectMessage(360-456)ObjectOperation(133-168)ObjectData(42-47)
src/plugins/objects/realtimeobject.ts (3)
src/plugins/objects/pathobjectsubscriptionregister.ts (1)
PathObjectSubscriptionRegister(38-207)src/plugins/objects/pathobject.ts (2)
DefaultPathObject(23-359)value(116-155)src/plugins/objects/livemap.ts (1)
LiveMap(48-1006)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (16.x)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-browser (firefox)
🔇 Additional comments (26)
src/plugins/objects/instance.ts (2)
9-12: LGTM!The
InstanceEventinterface correctly defines the subscription event payload with an optionalObjectMessage.
146-156: LGTM!The
subscribemethod correctly:
- Validates that the instance represents a LiveObject
- Delegates to the LiveObject's instance subscription mechanism
- Transforms internal events to user-facing
InstanceSubscriptionEvent<T>with the properObjectMessageconversionsrc/plugins/objects/realtimeobject.ts (5)
57-57: LGTM!The
PathObjectSubscriptionRegisteris correctly declared and initialized in the constructor to enable path-based subscriptions.Also applies to: 71-71
175-180: LGTM!The getter correctly exposes the path subscription register for internal use.
94-94: LGTM!The migration from
ROOT_OBJECT_IDconstant tothis._objectsPool.getRoot()provides better encapsulation and aligns with the updated ObjectsPool API.Also applies to: 106-106
483-506: LGTM!The two-phase rebuild (clear, then re-establish) ensures parent references are consistent after sync. The iteration through LiveMap entries correctly re-links child objects to their parent maps.
398-404: LGTM!The parent reference rebuild is correctly positioned after sync application but before notifications, ensuring that path-based subscriptions have accurate parent linkage when events are dispatched.
src/plugins/objects/liveobject.ts (8)
19-19: LGTM!The
_typediscriminant enables runtime type checking of updates, which is necessary for conditional path event handling.
51-51: LGTM!The separate
_instanceSubscriptionsEventEmitter and_parentReferencesMap are correctly declared and initialized, enabling instance-level subscriptions and parent tracking for path-based events.Also applies to: 63-67, 75-75, 83-83
98-109: LGTM!The
instanceSubscribemethod correctly implements instance-level subscription using a separate EventEmitter channel. The TODO comment indicates this is part of the migration to the new path-based subscription API.
231-279: LGTM!The parent reference management methods correctly maintain the
Map<LiveObject, Set<string>>structure, with proper cleanup of empty entries to avoid memory leaks.
281-325: LGTM!The iterative DFS with per-path visited tracking correctly handles:
- Multiple paths to the same object
- Cycle detection
- Root identification (zero parent references)
169-177: LGTM!The refactored
notifyUpdatedcorrectly delegates to separate notification helpers, enabling both instance-level and path-based subscription events with properObjectMessagecontext.
184-206: LGTM!The tombstone flow correctly notifies all subscriptions (both old-style and instance-level) with the delete message before cleaning up listeners.
350-362: LGTM!The helper correctly maintains backward compatibility by stripping the
_typediscriminant for old-style subscriptions while providing the fullInstanceEvent(includingObjectMessage) to instance subscribers.src/plugins/objects/objectmessage.ts (5)
8-17: LGTM!The mapping arrays correctly convert internal enum values to user-facing API strings for operation actions and map semantics.
342-353: LGTM!The
stringifyOperationhelper correctly transforms internal operation representations to user-facing API format with proper enum-to-string mappings.
36-36: LGTM!The type narrowing from generic
Bufferliketo specificBuffer | ArrayBufferimproves type safety and aligns with the platform's buffer handling.Also applies to: 60-60
441-455: LGTM!The
toUserFacingMessagemethod correctly constructs the user-facingObjectMessagewith channel context and properly serialized operation details.
770-775: LGTM!The decoding type updates are consistent with the narrowed buffer types, ensuring proper type checking during message decoding.
ably.d.ts (6)
1648-1653: LGTM! Clean generic event callback type.The
EventCallback<T>type provides a clean, reusable signature for event listeners that receive a single event object. This is a good abstraction for the subscription APIs.
2448-2470: LGTM! Well-documented subscription API.The
subscribemethod signature is correct and the documentation thoroughly explains:
- The event payload structure
- Subscription depth configuration via options
- Automatic instance switching behavior when paths resolve to different objects
- Automatic cleanup on instance deletion
2866-2866: LGTM! Correct instance subscription API.Good changes:
- Changing the type parameter from
_TtoTis correct since it's now used in thesubscribemethod signature.- The
subscribemethod correctly usesEventCallback<InstanceSubscriptionEvent<T>>for instance-level subscriptions.- Documentation clearly explains instance tracking behavior and automatic cleanup on deletion.
The same concern about
compact(): anyapplies here as mentioned in the PathObjectBase review.Also applies to: 2883-2903
3101-3134: LGTM! Well-designed subscription event types.The event payload types are well-structured:
PathObjectSubscriptionEventandInstanceSubscriptionEvent<T>both provide the updated object and optional messagePathObjectSubscriptionOptionsprovides clear depth control with good documentation- Appropriate use of generics in
InstanceSubscriptionEvent<T>
3193-3242: LGTM! Complete ObjectMessage interface.The
ObjectMessageinterface is well-defined with:
- All necessary identification fields (id, clientId, connectionId, etc.)
- Clear documentation for each field
- Proper typing with
operation: ObjectOperation- Appropriate optional fields marked with
?
3136-3190: LGTM! Well-structured supporting types.The supporting types for object operations are well-designed:
ObjectOperationActionsnamespace provides clear, type-safe action constantsObjectOperationActionproperly unions all action typesObjectsMapSemanticsNamespaceandObjectsMapSemanticsfollow the same patternObjectDataclearly represents either an object reference or a primitive valueObjectsCounterOpandObjectsCounterare appropriately simpleThe namespace pattern is a good choice for grouping related constants while maintaining type safety.
Also applies to: 3278-3327
Implements: - LiveObject parent tracking - Path-based LiveObject event emission - PathObject subscriptions path matching with deep subscriptions - Path event emission for LiveMap key updates - Full ObjectMessage (user-facing type) argument for subscription callbacks Resolves PUB-2061
Stricter typing removes `ArrayBufferView` from the union type to align with the publicly exposed API, which only uses a union of `Buffer` and `ArrayBuffer`. This change enables the internal `ObjectMessage.operation` object to be exposed as the public `ObjectOperation` type in the following commit without buffer type mismatches for underlying values.
VeskeR
force-pushed
the
PUB-2061/pathobject-subscription
branch
from
af14113 to
36d30a4
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2097/features
github-actions
bot
temporarily deployed
to
staging/pull/2097/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/2097/typedoc
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (5)
src/plugins/objects/objectspool.ts (1)
34-36: Add runtime safety for getRoot()Unsafe cast can mask a missing/invalid root. Verify existence and type before returning.
Apply this diff:
getRoot<T extends API.LiveMapType = API.AblyDefaultObject>(): LiveMap<T> { - return this._pool.get(ROOT_OBJECT_ID) as LiveMap<T>; + const root = this._pool.get(ROOT_OBJECT_ID); + if (!root || !(root instanceof LiveMap)) { + throw new this._client.ErrorInfo('Root object is missing or invalid', 50000, 500); + } + return root as LiveMap<T>; }test/realtime/objects.test.js (3)
4454-4467: Fix expected PathObject event path for direct root updateFor a set on root key 'testKey', the event PathObject should point to that key, not the root path.
- expect(event.object.path()).to.equal('', 'Check event object path is root'); + expect(event.object.path()).to.equal('testKey', 'Check event object path is the updated key');
4476-4512: Fix expected paths for deep events emitted to a root subscriptionRoot subscriptions emit events at the mutated path (full path), not at root/ancestor-only.
- if (eventCount === 1) { - expect(event.object.path()).to.equal('', 'First event is at root path'); - } else if (eventCount === 2) { - expect(event.object.path()).to.equal('nested', 'Second event is at nested path'); - } else if (eventCount === 3) { - expect(event.object.path()).to.equal('nested.child', 'Third event is at nested.child path'); + if (eventCount === 1) { + expect(event.object.path()).to.equal('nested', 'First event is at path of the created key'); + } else if (eventCount === 2) { + expect(event.object.path()).to.equal('nested.child', 'Second event is at nested.child path'); + } else if (eventCount === 3) { + expect(event.object.path()).to.equal('nested.child.foo', 'Third event is at nested.child.foo path');
4908-4926: Event PathObject path should be the updated key, not rootFor a root subscription, setting key 'foo' should produce event.object.path() === 'foo'.
- expect(event.object.path()).to.equal('', 'Check event object has correct path'); + expect(event.object.path()).to.equal('foo', 'Check event object has correct path');src/plugins/objects/liveobject.ts (1)
383-395: Non-bubbling path events may lack key-specific update payload.For per-key, non-bubbling events (lines 389-393), the
PathEventincludes the fullobjectMessagebut doesn't scope the update to the specific key. Subscribers at an exact path (e.g.,['user', 'name']) receive the entire LiveMapUpdate, which may include changes to multiple keys. If the subscriber needs to determine whether the specific key was updated or removed, they must manually filter the update object, which is less ergonomic and error-prone.The past review comment on this code segment suggested scoping the update to the single key before pushing the event. However, the
PathEventinterface currently doesn't include anupdatefield, so this would require extending the interface or embedding the scoped update within themessage.Please verify whether:
- The
PathEventinterface should be extended to include a scopedupdatefield for per-key events.- Subscribers at exact key paths need the scoped update, or if they can derive it from the full
objectMessage.#!/bin/bash # Verify PathEvent interface definition and usage in subscription callbacks rg -nP --type=ts -C5 'interface PathEvent'
🧹 Nitpick comments (7)
src/plugins/objects/batchcontext.ts (1)
26-30: Guard against missing root wrapper in get()Assumes the root always exists; add a runtime check to avoid undefined cast.
Apply this diff:
get<T extends API.LiveMapType = API.AblyDefaultObject>(): BatchContextLiveMap<T> { this._realtimeObject.throwIfInvalidAccessApiConfiguration(); this.throwIfClosed(); - return this.getWrappedObject(ROOT_OBJECT_ID) as BatchContextLiveMap<T>; + const wrapped = this.getWrappedObject(ROOT_OBJECT_ID); + if (!wrapped || !(wrapped instanceof BatchContextLiveMap)) { + throw new this._client.ErrorInfo('Root object is missing or invalid', 50000, 500); + } + return wrapped as BatchContextLiveMap<T>; }src/plugins/objects/instance.ts (2)
9-12: Avoid name ambiguity for internal event type.Consider renaming
InstanceEventto something likeInternalInstanceEventto avoid confusion with the publicInstanceSubscriptionEvent<T>. Also ensure its shape matches whatLiveObject.instanceSubscribe()emits (single source of truth).Would you confirm
LiveObject.instanceSubscribe’s event type matches{ message?: ObjectMessage }?
146-156: Isolate listener errors (align with PathObject subscriptions).Wrap the user listener to prevent exceptions from breaking internal dispatch, similar to
PathObjectSubscriptionRegister._processEvent().Apply:
- return this._value.instanceSubscribe((event: InstanceEvent) => { - listener({ - object: this as unknown as Instance<T>, - message: event.message?.toUserFacingMessage(this._realtimeobject.getChannel()), - }); - }); + return this._value.instanceSubscribe((event: InstanceEvent) => { + try { + listener({ + object: this as unknown as Instance<T>, + message: event.message?.toUserFacingMessage(this._realtimeObject.getChannel()), + }); + } catch (error) { + this._client.Logger.logAction( + this._client.logger, + this._client.Logger.LOG_MINOR, + 'DefaultInstance.subscribe()', + `Error in Instance subscription listener; error=${error}`, + ); + } + });src/plugins/objects/realtimeobject.ts (1)
57-57: Prefer readonly for subscription register.Field is assigned once; mark as
readonlyfor clarity.- private _pathObjectSubscriptionRegister: PathObjectSubscriptionRegister; + private readonly _pathObjectSubscriptionRegister: PathObjectSubscriptionRegister;Also applies to: 71-71
src/plugins/objects/pathobject.ts (1)
157-164: Avoid generic shadowing; improve readability.Method generic
Tshadows the class genericT. Rename toUto reduce confusion.- instance<T extends Value = Value>(): Instance<T> | undefined { + instance<U extends Value = Value>(): Instance<U> | undefined { try { const value = this._resolvePath(this._path); if (value instanceof LiveObject) { - return new DefaultInstance(this._realtimeObject, value) as unknown as Instance<T>; + return new DefaultInstance(this._realtimeObject, value) as unknown as Instance<U>; } return undefined;test/realtime/objects.test.js (2)
4661-4663: Remove unused extra argument to waitForCounterUpdatewaitForCounterUpdate(counter) accepts a single argument. The second is ignored.
- const counterUpdatedPromise = waitForCounterUpdate(root.get('escaped\\key').get('key.with.dots'), 'key1'); + const counterUpdatedPromise = waitForCounterUpdate(root.get('escaped\\key').get('key.with.dots'));
4670-4673: Fill in or skip the TODO scenarioThis test is a TODO with an empty action; consider implementing or marking it skipped to avoid false positives.
I can draft the test to replace the underlying object and assert the subscription continues receiving events. Want me to propose it?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
ably.d.ts(12 hunks)scripts/moduleReport.ts(1 hunks)src/platform/web/lib/util/bufferutils.ts(1 hunks)src/plugins/objects/batchcontext.ts(1 hunks)src/plugins/objects/constants.ts(1 hunks)src/plugins/objects/instance.ts(2 hunks)src/plugins/objects/livecounter.ts(7 hunks)src/plugins/objects/livemap.ts(13 hunks)src/plugins/objects/liveobject.ts(9 hunks)src/plugins/objects/objectmessage.ts(8 hunks)src/plugins/objects/objectspool.ts(3 hunks)src/plugins/objects/pathobject.ts(3 hunks)src/plugins/objects/pathobjectsubscriptionregister.ts(1 hunks)src/plugins/objects/realtimeobject.ts(9 hunks)test/realtime/objects.test.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/moduleReport.ts
- src/plugins/objects/pathobjectsubscriptionregister.ts
🧰 Additional context used
🧬 Code graph analysis (11)
src/plugins/objects/objectmessage.ts (1)
ably.d.ts (8)
ObjectOperationAction(3169-3175)ObjectsMapSemantics(3190-3190)PrimitiveObjectValue(3539-3539)JsonArray(3554-3554)JsonObject(3559-3559)ObjectOperation(3247-3266)ObjectData(3321-3326)ObjectMessage(3195-3242)
src/plugins/objects/instance.ts (3)
ably.d.ts (5)
ObjectMessage(3195-3242)EventCallback(1653-1653)InstanceSubscriptionEvent(3129-3134)LiveObject(2416-2416)Instance(3093-3099)src/plugins/objects/objectmessage.ts (1)
ObjectMessage(360-456)src/plugins/objects/liveobject.ts (1)
SubscribeResponse(32-34)
test/realtime/objects.test.js (5)
src/plugins/objects/livemap.ts (1)
LiveMap(48-1006)objects.d.ts (2)
LiveMap(16-28)LiveCounter(33-43)src/plugins/objects/livecounter.ts (1)
LiveCounter(15-303)src/plugins/objects/liveobject.ts (1)
unsubscribe(112-123)src/plugins/objects/pathobject.ts (1)
instance(157-176)
src/plugins/objects/livemap.ts (2)
ably.d.ts (2)
JsonArray(3554-3554)JsonObject(3559-3559)src/plugins/objects/liveobject.ts (1)
LiveObjectUpdateNoop(26-30)
src/plugins/objects/liveobject.ts (6)
ably.d.ts (4)
LiveObject(2416-2416)EventCallback(1653-1653)ObjectMessage(3195-3242)LiveMapType(2858-2860)src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(41-523)src/plugins/objects/instance.ts (1)
InstanceEvent(9-12)src/plugins/objects/livemap.ts (1)
LiveMapUpdate(42-45)src/plugins/objects/constants.ts (1)
ROOT_OBJECT_ID(1-1)src/plugins/objects/pathobjectsubscriptionregister.ts (1)
PathEvent(23-30)
src/plugins/objects/livecounter.ts (1)
src/plugins/objects/liveobject.ts (1)
LiveObjectUpdateNoop(26-30)
src/plugins/objects/realtimeobject.ts (2)
src/plugins/objects/pathobjectsubscriptionregister.ts (1)
PathObjectSubscriptionRegister(38-207)src/plugins/objects/pathobject.ts (2)
DefaultPathObject(23-359)value(116-155)
src/platform/web/lib/util/bufferutils.ts (1)
src/platform/nodejs/lib/util/bufferutils.ts (1)
Output(5-5)
src/plugins/objects/objectspool.ts (2)
ably.d.ts (4)
LiveMapType(2858-2860)AblyDefaultObject(3335-3342)LiveMap(2400-2403)LiveObject(2416-2416)src/plugins/objects/constants.ts (1)
ROOT_OBJECT_ID(1-1)
src/plugins/objects/pathobject.ts (2)
ably.d.ts (6)
Value(2423-2423)Instance(3093-3099)LiveObject(2416-2416)EventCallback(1653-1653)PathObjectSubscriptionEvent(3104-3109)PathObjectSubscriptionOptions(3114-3124)src/plugins/objects/liveobject.ts (1)
SubscribeResponse(32-34)
ably.d.ts (2)
src/plugins/objects/liveobject.ts (1)
SubscribeResponse(32-34)src/plugins/objects/objectmessage.ts (8)
ObjectMessage(360-456)ObjectOperation(133-168)ObjectsMapOp(73-78)ObjectsCounterOp(84-87)ObjectsMap(113-118)ObjectsCounter(124-127)ObjectData(42-47)ObjectsMapEntry(93-107)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (16.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-browser (webkit)
🔇 Additional comments (32)
src/platform/web/lib/util/bufferutils.ts (1)
9-9: ApproveOutputtype refinement toArrayBuffer.
All methods already returnArrayBufferand documentation reflects this. Ensure any consumers of the exportedOutputtype tolerate the narrowedArrayBufferversus the formerBufferSource.src/plugins/objects/livemap.ts (4)
505-507: Parent references synced after state override — goodApplying parent-reference updates from the computed diff here is correct and avoids full scans.
527-546: clearData parent-reference cleanup — goodCleaning parent links before delegating to super.clearData() prevents dangling edges.
731-764: MAP_SET parent-reference transitions handled in-place — goodRemoving old parent link then adding new is correct; covers object→primitive and primitive→object transitions.
552-606: Update discriminant_typeand diff calc — goodIncluding
_type: 'LiveMapUpdate'and precise per-key diffing is consistent and helpful for consumers.ably.d.ts (2)
1648-1654: EventCallback addition — goodUnified callback shape improves subscription API clarity.
3101-3134: Subscription event payloads — goodPathObject/Instance subscription event types expose full ObjectMessage; aligns with PR goals.
src/plugins/objects/constants.ts (1)
1-1: Centralized ROOT_OBJECT_ID — goodExporting this constant from a dedicated module removes duplication.
src/plugins/objects/objectspool.ts (2)
42-44: getAll() iterator — goodUseful internal utility for pool-wide operations.
64-72: resetToInitialPool uses getRoot() — goodKeeps root instance stable while clearing others; aligns with API expectations.
src/plugins/objects/livecounter.ts (3)
130-159: OBJECT_DELETE handling and notifyUpdated signature — goodNo assignment from _applyObjectDelete is correct (base now handles notifications). Passing msg to notifyUpdated standardizes callbacks.
242-245: Discriminant_typeadded — goodProvides a stable tag for narrowing LiveCounter updates.
294-302: COUNTER_INC update payload shape — goodIncludes clientId/connectionId and
_type; consistent with LiveMap.src/plugins/objects/instance.ts (1)
2-2: LGTM on import surface expansion.New types and ObjectMessage import fit the new subscription flow.
Also applies to: 5-6
src/plugins/objects/realtimeobject.ts (3)
94-94: LGTM: use ObjectsPool.getRoot().Switching to
getRoot()removes hardcoded IDs and improves encapsulation.Also applies to: 106-106
175-181: LGTM: internal accessor.Accessor cleanly exposes the registry internally.
398-401: Verified parent-reference APIs and non-bubbling overwrite events
LiveObject.clearParentReferences() and addParentReference() are implemented and invoked during sync rebuild; overwrite-related PathEvent instances correctly set bubbles:false.src/plugins/objects/pathobject.ts (3)
3-12: LGTM on imports and SubscribeResponse exposure.Matches the new subscription API surface.
Also applies to: 16-16
24-24: LGTM:_clientvisibility.Making
_clientprivate is fine.
300-324: LGTM: PathObject.subscribe implementation and docs.Delegation to the registry with optional depth is clear; returns
SubscribeResponseas expected.Please confirm emitters set
bubbles=falsefor overwrite events so depth/parent behavior matches the docs.src/plugins/objects/liveobject.ts (7)
3-8: LGTM!The new imports are correctly aligned with the path-based and instance-level subscription features being introduced.
19-24: LGTM!Adding the
_typediscriminant enables type narrowing in TypeScript, allowing consumers to distinguish betweenLiveMapUpdateandLiveCounterUpdateat runtime. This is a best practice for tagged unions.
64-68: LGTM!Using
Map<LiveObject, Set<string>>efficiently tracks parent-child relationships, supporting multiple parents referencing the same object at different keys. This structure enables fast lookups and modifications.Also applies to: 84-84
99-110: LGTM!The
instanceSubscribemethod correctly follows the same pattern as the existingsubscribemethod, providing a consistent API for instance-level subscriptions.
237-280: LGTM!The parent reference management methods are correctly implemented. The cleanup logic in
removeParentReference(removing empty Sets) prevents memory leaks, and the API provides appropriate granularity for different lifecycle scenarios.
288-326: LGTM!The iterative DFS algorithm correctly traverses the parent graph, handles cyclic references with the visited set, and builds paths from root to the current object. The implementation is sound.
351-363: LGTM!The helper methods correctly maintain backward compatibility with the old subscription API (emitting updates without
_type) while supporting the new instance subscription API (emitting events withmessage).src/plugins/objects/objectmessage.ts (5)
8-17: LGTM!The mapping arrays correctly translate internal enum values to API string representations, with array indices matching the enum numeric values.
36-36: LGTM!The type expansions correctly align with the public API and support both JSON (Base64-encoded string) and msgpack (Buffer/ArrayBuffer) protocols.
Also applies to: 60-60
342-353: LGTM!The
stringifyOperationfunction correctly transforms the internal operation representation to the API format, mapping enum values to strings and handling undefined semantics gracefully.
770-770: LGTM!The type annotations and casts are consistent with the
WireObjectData.bytestype changes and correctly handle both JSON and msgpack protocols.Also applies to: 775-775
441-455: Ensure non-null assertions in toUserFacingMessage are safe
toUserFacingMessage usesthis.id!,this.timestamp!, andthis.operation!; confirm every call site only invokes it on messages where those fields are guaranteed (e.g. only OBJECT-action messages), or add null checks/log errors.
github-actions
bot
temporarily deployed
to
staging/pull/2097/features
github-actions
bot
temporarily deployed
to
staging/pull/2097/bundle-report
…iveObject subscriptions LiveObject subscriptions (via PathObject and Instance APIs) now expose the full operation object that caused the change in the `event.message.operation` field.
VeskeR
force-pushed
the
PUB-2061/pathobject-subscription
branch
from
a691e56 to
6024f4b
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2097/features
github-actions
bot
temporarily deployed
to
staging/pull/2097/bundle-report
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/plugins/objects/objectmessage.ts (1)
413-435: Fix ArrayBuffer encoding in encodeObjectDataFn.
ArrayBufferandTypedArrayvalues are not explicitly handled and would fall through to thetypeof objectbranch (line 426), causing them to be incorrectlyJSON.stringify'd as{}instead of being treated as binary data.Add explicit
ArrayBuffer/TypedArrayhandling before the JSON branch:const encodeObjectDataFn: EncodeObjectDataFunction = (data: ObjectData) => { const encodedObjectData: WireObjectData = { objectId: data.objectId }; if (client.Platform.BufferUtils.isBuffer(data.value)) { // bytes encoding happens later when WireObjectMessage is encoded for wire transmission encodedObjectData.bytes = data.value; + } else if (data.value instanceof ArrayBuffer || ArrayBuffer.isView(data.value)) { + // Handle ArrayBuffer and TypedArray (Uint8Array, etc.) as binary data + encodedObjectData.bytes = client.Platform.BufferUtils.toBuffer(data.value as ArrayBuffer | ArrayBufferView); } else if (typeof data.value === 'string') { encodedObjectData.string = data.value; } else if (typeof data.value === 'boolean') { encodedObjectData.boolean = data.value; } else if (typeof data.value === 'number') { encodedObjectData.number = data.value; } else if (typeof data.value === 'object' && data.value !== null) { encodedObjectData.json = JSON.stringify(data.value); } return encodedObjectData; };
🧹 Nitpick comments (2)
ably.d.ts (2)
2439-2439: Consider usingunknowninstead ofanyfor better type safety.The
compact()method returnsany, which bypasses type checking. Consider usingunknowninstead, which would require callers to perform type assertions or guards, improving type safety while still allowing flexibility for the varied structure of compacted objects.- compact(): any; + compact(): unknown;
2874-2874: Consider usingunknowninstead ofanyfor better type safety.Similar to
PathObjectBase.compact(), this method returnsany. Consider usingunknownto require explicit type handling by callers.- compact(): any; + compact(): unknown;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
ably.d.ts(15 hunks)src/plugins/objects/instance.ts(2 hunks)src/plugins/objects/livecounter.ts(7 hunks)src/plugins/objects/livemap.ts(12 hunks)src/plugins/objects/liveobject.ts(8 hunks)src/plugins/objects/objectmessage.ts(8 hunks)src/plugins/objects/pathobject.ts(4 hunks)src/plugins/objects/pathobjectsubscriptionregister.ts(1 hunks)src/plugins/objects/realtimeobject.ts(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
src/plugins/objects/pathobject.ts (2)
ably.d.ts (6)
Value(2416-2416)Instance(3086-3092)LiveObject(2409-2409)EventCallback(1653-1653)PathObjectSubscriptionEvent(3097-3102)PathObjectSubscriptionOptions(3107-3117)src/plugins/objects/instance.ts (2)
value(61-89)DefaultInstance(22-165)
src/plugins/objects/instance.ts (2)
ably.d.ts (5)
ObjectMessage(3188-3235)EventCallback(1653-1653)InstanceSubscriptionEvent(3122-3127)LiveObject(2409-2409)Instance(3086-3092)src/plugins/objects/objectmessage.ts (1)
ObjectMessage(360-464)
src/plugins/objects/objectmessage.ts (1)
ably.d.ts (8)
ObjectOperationAction(3162-3168)ObjectsMapSemantics(3183-3183)PrimitiveObjectValue(3520-3520)JsonArray(3535-3535)JsonObject(3540-3540)ObjectOperation(3240-3259)ObjectData(3314-3319)ObjectMessage(3188-3235)
src/plugins/objects/pathobjectsubscriptionregister.ts (4)
ably.d.ts (4)
EventCallback(1653-1653)PathObjectSubscriptionEvent(3097-3102)PathObjectSubscriptionOptions(3107-3117)ObjectMessage(3188-3235)src/plugins/objects/objectmessage.ts (1)
ObjectMessage(360-464)src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(41-522)src/plugins/objects/pathobject.ts (2)
path(44-47)DefaultPathObject(24-360)
src/plugins/objects/livemap.ts (2)
ably.d.ts (2)
JsonArray(3535-3535)JsonObject(3540-3540)src/plugins/objects/liveobject.ts (1)
LiveObjectUpdateNoop(27-31)
src/plugins/objects/livecounter.ts (1)
src/plugins/objects/liveobject.ts (1)
LiveObjectUpdateNoop(27-31)
src/plugins/objects/realtimeobject.ts (3)
src/plugins/objects/pathobjectsubscriptionregister.ts (1)
PathObjectSubscriptionRegister(42-211)src/plugins/objects/pathobject.ts (2)
DefaultPathObject(24-360)value(117-156)src/plugins/objects/livemap.ts (1)
LiveMap(48-1002)
src/plugins/objects/liveobject.ts (6)
ably.d.ts (4)
ObjectMessage(3188-3235)LiveObject(2409-2409)EventCallback(1653-1653)LiveMapType(2851-2853)src/plugins/objects/objectmessage.ts (1)
ObjectMessage(360-464)src/plugins/objects/instance.ts (1)
InstanceEvent(17-20)src/plugins/objects/livemap.ts (1)
LiveMapUpdate(42-45)src/plugins/objects/constants.ts (1)
ROOT_OBJECT_ID(1-1)src/plugins/objects/pathobjectsubscriptionregister.ts (1)
PathEvent(27-34)
ably.d.ts (1)
src/plugins/objects/objectmessage.ts (8)
ObjectMessage(360-464)ObjectOperation(133-168)ObjectsMapOp(73-78)ObjectsCounterOp(84-87)ObjectsMap(113-118)ObjectsCounter(124-127)ObjectData(42-47)ObjectsMapEntry(93-107)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (16.x)
- GitHub Check: test-node (18.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-browser (webkit)
🔇 Additional comments (23)
src/plugins/objects/instance.ts (1)
154-164: LGTM: Instance subscription implementation is solid.The
subscribemethod correctly:
- Validates that the underlying value is a
LiveObjectbefore subscribing- Delegates to
instanceSubscribeon theLiveObject- Transforms the internal
ObjectMessageto the user-facing format usingtoUserFacingMessage- Returns the unsubscribe handle
src/plugins/objects/livecounter.ts (3)
11-11: LGTM: _type discriminator added.The
_type: 'LiveCounterUpdate'discriminator enables runtime type identification of update payloads, supporting the path-based subscription system introduced in this PR.
130-158: LGTM: Update initialization and handling.The changes correctly:
- Initialize
updatewith a noop object by default (line 130)- No longer assign
_applyObjectDeleteresult (line 147), which is correct since it now returnsvoidand handles notifications internally- Pass the update to
notifyUpdatedwhich now handles both instance and path subscriptionsBased on learnings.
224-224: LGTM: Update payloads enriched with _type and objectMessage.Consistently adding
_type: 'LiveCounterUpdate'andobjectMessageto all update payloads supports the goal of providing fullObjectMessagecontext to subscription callbacks.Also applies to: 244-244, 259-262, 295-299
src/plugins/objects/pathobject.ts (2)
158-177: LGTM: instance() method refined for public API.The return type change from
AnyInstance<T>toInstance<T>aligns with the public API surface. The casting is safe because:
DefaultInstanceimplements the instance contract- Returns
undefinedfor non-LiveObjectvalues- Type resolution is handled at the call site
319-324: LGTM: PathObject subscription integration.The
subscribemethod correctly delegates to thePathObjectSubscriptionRegister, passing the path array, listener, and options. This integrates cleanly with the new subscription system.src/plugins/objects/realtimeobject.ts (2)
398-403: LGTM: Parent references rebuilt before update notifications.Calling
_rebuildAllParentReferences()after applying sync updates but before notifying ensures that:
- All object-to-object references are correctly established
- Path resolution via
getFullPaths()works correctly- Subscription notifications see the accurate parent hierarchy
487-505: Add tests covering parent reference rebuild edge casesNo tests currently target
_rebuildAllParentReferences,clearParentReferences, oraddParentReference. Please add tests to validate:
- Objects created during sync but not yet linked
- Objects removed from the pool during sync
- Circular reference handling
src/plugins/objects/pathobjectsubscriptionregister.ts (3)
59-90: LGTM: Subscription registration with depth validation.The
subscribemethod:
- Validates options are an object (line 64-66)
- Ensures depth is positive or undefined (lines 68-74)
- Copies the path array to prevent external mutations (line 80)
- Returns an unsubscribe function that removes the entry
152-178: LGTM: Depth-based filtering logic is correct.The depth calculation and filtering:
- For non-bubbling events, requires exact path match (lines 159-161)
- For bubbling events, checks path prefix (lines 164-166)
- Calculates relative depth as
eventPath.length - subPath.length + 1(line 174)- Allows infinite depth when
depthis undefined (lines 169-171)This correctly implements the depth semantics where:
- depth=1 means only the subscribed path itself
- depth=2 means one level of children
- depth=undefined means all descendants
106-129: LGTM: Fault-tolerant event processing.The
_processEventmethod:
- Iterates all subscriptions and filters using
_shouldNotifySubscription- Creates a user-facing
PathObjectSubscriptionEventwith proper message transformation- Wraps listener calls in try-catch to prevent one subscription failure from affecting others
- Logs errors without re-throwing
src/plugins/objects/livemap.ts (4)
729-772: LGTM: Parent reference management in MAP_SET.The
_applyMapSetlogic correctly manages parent references:
- Removes parent reference from the old object if replacing an objectId entry (lines 731-737)
- Adds parent reference to the new object if the new value is an objectId (lines 756-761)
- Handles the case where objects don't exist in the pool yet (lines 733, 757)
807-842: LGTM: Parent reference management in MAP_REMOVE.The
_applyMapRemovelogic correctly removes parent references when a map entry is tombstoned:
- Checks if the existing entry references an object (line 809)
- Removes the parent reference from the referenced object (lines 811-814)
- Handles missing objects gracefully (line 811)
531-544: LGTM: clearData override handles parent reference cleanup.When a
LiveMapis tombstoned,clearDatacorrectly:
- Iterates all entries and removes parent references for objectId entries (lines 533-540)
- Then delegates to the parent class implementation (line 543)
This ensures parent references are cleaned up before the map's data is cleared.
966-1001: LGTM: Batch parent reference update after diff.
_updateParentReferencesFromUpdatecorrectly processes the diff:
- For 'removed' keys: removes the old parent reference (lines 968-977)
- For 'updated' keys: removes old reference and adds new reference (lines 979-999)
- Handles both objectId entries and primitive values
- Checks for object existence in the pool
This provides a centralized way to update parent references after computing a diff, used in
overrideWithObjectState.src/plugins/objects/objectmessage.ts (1)
342-353: LGTM: User-facing message transformation.The
stringifyOperationhelper maps internal enum values to user-facing strings (lines 342-353), andtoUserFacingMessageconstructs the publicAPI.ObjectMessagewith the stringified operation (lines 449-463). This correctly provides fullObjectMessagepayloads to subscription callbacks.Also applies to: 449-463
src/plugins/objects/liveobject.ts (4)
251-289: LGTM: getFullPaths implements safe cycle-aware traversal.The
getFullPathsmethod uses iterative DFS with per-path visited tracking:
- Maintains a stack of
{obj, currentPath, visited}entries- Creates new visited sets for each path branch to detect cycles without incorrectly pruning valid paths
- Returns when reaching the root object (line 270-273)
- Handles orphaned objects by returning empty array (checked in caller at line 329-332)
200-243: LGTM: Parent reference management API.The parent reference tracking methods provide a clean API:
addParentReference: Adds key to parent's set, creating entry if neededremoveParentReference: Removes key from parent's set, deletes entry if emptyremoveParentReferenceAll: Removes all references from a specific parentclearParentReferences: Clears all references (used during rebuild)The use of
Map<LiveObject, Set<string>>efficiently handles multiple parents and keys.
326-358: LGTM: Path-based subscription notifications.The
_notifyPathSubscriptionsmethod correctly:
- Retrieves all paths to this object via
getFullPaths()(line 327)- Filters out sync messages, keeping only operation messages (line 335)
- Creates bubbling events for each path (lines 336-340)
- For
LiveMapUpdate, creates non-bubbling per-key events (lines 342-355)The per-key events correctly use
bubbles: falseso they don't propagate to parent subscriptions. TheObjectMessagecontains the full operation details (includingmapOp.keyfor map operations), so subscribers can determine which key was affected.
149-170: LGTM: tombstone updated to return void and notify.The
tombstonemethod now:
- Sets the tombstone flag and timestamp (lines 150-161)
- Calls
clearData()to compute the diff (line 162)- Attaches the
objectMessageto the update (line 163)- Emits the lifecycle event (line 165)
- Calls
notifyUpdatedto trigger instance and path subscriptions (line 168)- Unsubscribes all listeners (line 169)
This ensures subscribers are notified of the deletion before being unsubscribed, and the
objectMessageprovides full context about the delete operation.ably.d.ts (3)
3094-3319: LGTM! Comprehensive type definitions for object operations and subscriptions.The new type definitions are well-structured and thoroughly documented:
- Event types clearly define the payload structure for path and instance subscriptions
- Operation types cover all necessary actions (map/counter operations, object lifecycle)
- Message types include appropriate required and optional fields
- The type hierarchy is logical and consistent
2705-2706: LGTM! Documentation correctly updated to reference new subscription APIs.The JSDoc updates consistently guide users to the appropriate subscription methods (
PathObjectBase.subscribeorInstanceBase.subscribe), helping them understand when to use path-based vs instance-based subscriptions.Also applies to: 2724-2725, 2747-2748, 2781-2782, 2800-2801, 2820-2821
2673-2673: LGTM! Correct return type for instance() method.The change from
AnyInstance<T>toInstance<T>is correct, asInstance<T>is the proper conditional type that dispatches to the appropriate instance interface based on the value type.
There was a problem hiding this comment.
Sorry it took me some time to review - overall this looks good to me, thanks!
Left a couple of small comments. Also tests are not passing yet, are you expecting to address that in this PR?
I will approve now going to unblock dependencies since this is not targetting main.
| } | ||
|
|
||
| let update: LiveCounterUpdate | LiveObjectUpdateNoop; | ||
| let update: LiveCounterUpdate | LiveObjectUpdateNoop = { noop: true }; |
There was a problem hiding this comment.
This logic is a bit hard to follow - we call notifyUpdate for non-OBJECT_DELETE ops here, but treat the OBJECT_DELETE op as a no-op update; however we do in fact call notifyUpdate with an appropriate update for the delete inside tombstone() on the LiveObject.
Could we instead make all notifyUpdate calls from a single place, i.e. here? We should be able to deregister the subscription as required here as well rather than on the parent class.
| * @returns A {@link SubscribeResponse} object that allows the provided listener to be deregistered from future updates. | ||
| * @experimental | ||
| */ | ||
| subscribe(listener: LiveObjectUpdateCallback<TUpdate>): SubscribeResponse; |
There was a problem hiding this comment.
We should remove the typedoc references to these methods elsewhere in this file
VeskeR
force-pushed
the
PUB-2061/pathobject-subscription
branch
from
6024f4b to
07ec19d
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2097/features
github-actions
bot
temporarily deployed
to
staging/pull/2097/bundle-report
This was replaced by PathObject/Instance subscription API from previous commits
VeskeR
force-pushed
the
PUB-2061/pathobject-subscription
branch
from
07ec19d to
5406c87
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/plugins/objects/livemap.ts (1)
26-26: ArrayBuffer/Buffer encoding needs encoder updateThe
ValueObjectData.valuetype now includesBufferandArrayBuffer, but theObjectMessage.encodemethod (insrc/plugins/objects/objectmessage.ts) only handles Node Buffers viaBufferUtils.isBuffer(). ArrayBuffers and TypedArrays will fall through to the JSON branch and be stringified incorrectly.Based on past review feedback, add explicit ArrayBuffer/TypedArray handling in
encodeObjectDataFnbefore the JSON branch:const encodeObjectDataFn: EncodeObjectDataFunction = (data: ObjectData) => { const encodedObjectData: WireObjectData = { objectId: data.objectId }; if (client.Platform.BufferUtils.isBuffer(data.value)) { encodedObjectData.bytes = data.value; + } else if (data.value instanceof ArrayBuffer || ArrayBuffer.isView(data.value)) { + encodedObjectData.bytes = client.Platform.BufferUtils.toBuffer(data.value as ArrayBuffer | ArrayBufferView); } else if (typeof data.value === 'string') { encodedObjectData.string = data.value;ably.d.ts (1)
3237-3301: Critical: Object operation types must be generic to match internal implementationThis is a duplicate of previous review comments that were marked as addressed, but the fix is not present in this code. The public API defines
ObjectOperation,ObjectsMapOp,ObjectsMap, andObjectsMapEntryas non-generic, but:
- Internal implementation in
src/plugins/objects/objectmessage.tsdefines them as generic (e.g.,ObjectOperation<TData>)- This causes TypeScript TS2315 errors when these types are referenced with type arguments
- This breaks type safety for data flowing through object operations
Apply this diff to add generic parameters with
ObjectDatadefaults:/** * An operation that was applied to an object on a channel. */ -export interface ObjectOperation { +export interface ObjectOperation<TData = ObjectData> { /** The operation action, one of the {@link ObjectOperationAction} enum values. */ action: ObjectOperationAction; /** The ID of the object the operation was applied to. */ objectId: string; /** The payload for the operation if it is a mutation operation on a map object. */ - mapOp?: ObjectsMapOp; + mapOp?: ObjectsMapOp<TData>; /** The payload for the operation if it is a mutation operation on a counter object. */ counterOp?: ObjectsCounterOp; /** * The payload for the operation if the action is {@link ObjectOperationActions.MAP_CREATE}. * Defines the initial value of the map object. */ - map?: ObjectsMap; + map?: ObjectsMap<TData>; /** * The payload for the operation if the action is {@link ObjectOperationActions.COUNTER_CREATE}. * Defines the initial value of the counter object. */ counter?: ObjectsCounter; } /** * Describes an operation that was applied to a map object. */ -export interface ObjectsMapOp { +export interface ObjectsMapOp<TData = ObjectData> { /** The key that the operation was applied to. */ key: string; /** The data assigned to the key if the operation is {@link ObjectOperationActions.MAP_SET}. */ - data?: ObjectData; + data?: TData; } /** * Describes the initial value of a map object. */ -export interface ObjectsMap { +export interface ObjectsMap<TData = ObjectData> { /** The conflict-resolution semantics used by the map object, one of the {@link ObjectsMapSemantics} enum values. */ semantics?: ObjectsMapSemantics; /** The map entries, indexed by key. */ - entries?: Record<string, ObjectsMapEntry>; + entries?: Record<string, ObjectsMapEntry<TData>>; } /** * Describes a value at a specific key in a map object. */ -export interface ObjectsMapEntry { +export interface ObjectsMapEntry<TData = ObjectData> { /** Indicates whether the map entry has been removed. */ tombstone?: boolean; /** The {@link ObjectMessage.serial} value of the last operation applied to the map entry. */ timeserial?: string; /** A timestamp derived from the {@link timeserial} field. Present only if {@link tombstone} is `true`. */ serialTimestamp?: number; /** The value associated with this map entry. */ - data?: ObjectData; + data?: TData; }Note: Default generic parameters (
= ObjectData) maintain backward compatibility while enabling proper typing.Based on learnings
🧹 Nitpick comments (3)
src/plugins/objects/pathobject.ts (1)
25-25: Access restriction to private is appropriateChanging
_clientfromprotectedtoprivateappropriately restricts its visibility, as it's not accessed by any subclasses.ably.d.ts (2)
2439-2439: Consider narrowing the return type fromanyThe
compact()method returnsany, which loses all type safety. Consider usingunknown(requires type checking before use) or a more specific type likeJsonObject | Primitiveif the structure is known.- compact(): any; + compact(): unknown;
2874-2874: Consider narrowing the return type fromanySimilar to
PathObjectBase.compact(), this method returnsanywhich loses type safety. Considerunknownor a more specific type.- compact(): any; + compact(): unknown;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
ably.d.ts(15 hunks)src/plugins/objects/instance.ts(2 hunks)src/plugins/objects/livecounter.ts(7 hunks)src/plugins/objects/livemap.ts(12 hunks)src/plugins/objects/liveobject.ts(8 hunks)src/plugins/objects/objectmessage.ts(8 hunks)src/plugins/objects/pathobject.ts(4 hunks)src/plugins/objects/pathobjectsubscriptionregister.ts(1 hunks)src/plugins/objects/realtimeobject.ts(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
src/plugins/objects/pathobjectsubscriptionregister.ts (3)
ably.d.ts (4)
EventCallback(1653-1653)PathObjectSubscriptionEvent(3097-3102)PathObjectSubscriptionOptions(3107-3117)ObjectMessage(3188-3235)src/plugins/objects/objectmessage.ts (1)
ObjectMessage(360-464)src/plugins/objects/pathobject.ts (2)
path(44-47)DefaultPathObject(24-360)
src/plugins/objects/livemap.ts (2)
ably.d.ts (2)
JsonArray(3535-3535)JsonObject(3540-3540)src/plugins/objects/liveobject.ts (1)
LiveObjectUpdateNoop(27-31)
src/plugins/objects/pathobject.ts (2)
ably.d.ts (6)
Value(2416-2416)Instance(3086-3092)LiveObject(2409-2409)EventCallback(1653-1653)PathObjectSubscriptionEvent(3097-3102)PathObjectSubscriptionOptions(3107-3117)src/plugins/objects/instance.ts (2)
value(61-89)DefaultInstance(22-165)
src/plugins/objects/livecounter.ts (1)
src/plugins/objects/liveobject.ts (1)
LiveObjectUpdateNoop(27-31)
src/plugins/objects/instance.ts (2)
ably.d.ts (5)
ObjectMessage(3188-3235)EventCallback(1653-1653)InstanceSubscriptionEvent(3122-3127)LiveObject(2409-2409)Instance(3086-3092)src/plugins/objects/objectmessage.ts (1)
ObjectMessage(360-464)
src/plugins/objects/liveobject.ts (6)
ably.d.ts (4)
ObjectMessage(3188-3235)LiveObject(2409-2409)EventCallback(1653-1653)LiveMapType(2851-2853)src/plugins/objects/objectmessage.ts (1)
ObjectMessage(360-464)src/plugins/objects/instance.ts (1)
InstanceEvent(17-20)src/plugins/objects/livemap.ts (1)
LiveMapUpdate(42-45)src/plugins/objects/constants.ts (1)
ROOT_OBJECT_ID(1-1)src/plugins/objects/pathobjectsubscriptionregister.ts (1)
PathEvent(27-34)
src/plugins/objects/objectmessage.ts (1)
ably.d.ts (8)
ObjectOperationAction(3162-3168)ObjectsMapSemantics(3183-3183)PrimitiveObjectValue(3520-3520)JsonArray(3535-3535)JsonObject(3540-3540)ObjectOperation(3240-3259)ObjectData(3314-3319)ObjectMessage(3188-3235)
src/plugins/objects/realtimeobject.ts (2)
src/plugins/objects/pathobjectsubscriptionregister.ts (1)
PathObjectSubscriptionRegister(42-211)src/plugins/objects/pathobject.ts (2)
DefaultPathObject(24-360)value(117-156)
ably.d.ts (1)
src/plugins/objects/objectmessage.ts (8)
ObjectMessage(360-464)ObjectOperation(133-168)ObjectsMapOp(73-78)ObjectsCounterOp(84-87)ObjectsMap(113-118)ObjectsCounter(124-127)ObjectData(42-47)ObjectsMapEntry(93-107)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (16.x)
- GitHub Check: test-browser (chromium)
🔇 Additional comments (26)
src/plugins/objects/livemap.ts (4)
526-544: LGTM: Parent reference cleanup on tombstoneThe
clearData()override correctly removes all parent references before delegating to the superclass. The pattern of cleanup-then-delegate ensures proper resource management when the LiveMap is tombstoned.
729-761: LGTM: Parent reference management in MAP_SETThe parent reference management correctly:
- Removes the old parent reference when an existing entry is updated (lines 730-737)
- Adds a new parent reference for the incoming objectId (lines 755-761)
This ensures the parent graph stays consistent when map entries are updated.
807-815: LGTM: Parent reference cleanup on MAP_REMOVEThe parent reference removal logic correctly handles the case when a map entry is tombstoned, ensuring referenced objects no longer have this map as a parent.
962-1001: LGTM: Comprehensive parent reference synchronizationThe
_updateParentReferencesFromUpdatemethod correctly handles both removal and update scenarios:
- Removed keys: Cleans up old parent references from the previous state
- Updated keys: Removes old parent references and adds new ones based on the diff
The logic correctly distinguishes between primitive values and object references.
src/plugins/objects/instance.ts (1)
154-164: LGTM: Instance subscription implementationThe
subscribe()method correctly:
- Validates that the underlying value is a
LiveObjectbefore subscribing- Delegates to the LiveObject's
instanceSubscribemethod- Wraps the internal event with the Instance reference and user-facing ObjectMessage
The implementation is consistent with other Instance methods and provides proper type safety.
src/plugins/objects/realtimeobject.ts (2)
482-505: LGTM: Parent reference rebuild after syncThe
_rebuildAllParentReferences()method correctly:
- Clears all existing references to start from a clean state
- Rebuilds references by traversing all LiveMap entries and establishing parent links for LiveObject values
The two-phase approach ensures consistency after sync operations when objects may reference other objects that weren't initially available in the pool.
398-400: LGTM: Parent reference rebuild timing in sync flowThe placement of
_rebuildAllParentReferences()is correct:
- Called after all sync objects are created/updated in the pool
- Called before notifying subscribers of updates
This ensures all object references are properly established before any subscription callbacks are invoked.
src/plugins/objects/livecounter.ts (2)
130-147: LGTM: OBJECT_DELETE handling is correctThe
OBJECT_DELETEcase correctly calls_applyObjectDelete(msg)without assigning its result. Per the method signature inLiveObject,_applyObjectDeletereturnsvoidand handles update notification internally throughtombstone(), which callsnotifyUpdated()with the appropriate update.Based on learnings.
224-224: LGTM: Consistent update structure with _type and objectMessageAll update-returning methods consistently include:
_type: 'LiveCounterUpdate'discriminator for type safetyobjectMessagefield where applicable for subscription event payloadsThis provides a unified update structure across LiveCounter operations.
Also applies to: 244-244, 258-262, 295-299
src/plugins/objects/pathobject.ts (1)
301-324: LGTM: PathObject subscription delegationThe
subscribe()method correctly delegates to the path subscription register, passing the current path and subscription options. The comprehensive JSDoc clearly explains:
- Bubbling behavior for nested changes
- Non-bubbling events for direct value overwrites
- Depth parameter semantics
src/plugins/objects/pathobjectsubscriptionregister.ts (3)
59-90: LGTM: Subscription registration with proper validationThe
subscribe()method correctly:
- Validates options structure and depth parameter
- Creates a defensive copy of the path array to prevent external mutations
- Generates unique subscription IDs using an incrementing counter
- Returns an unsubscribe function for cleanup
152-178: LGTM: Depth-based filtering with correct bubbling semanticsThe
_shouldNotifySubscription()method correctly implements:
- Non-bubbling events: Only exact path matches trigger notifications
- Bubbling events: Prefix matching with depth constraints
- Depth calculation:
eventPath.length - subPath.length + 1correctly computes relative depth
- Same level: depth = 1
- One level deeper: depth = 2
- Two levels deeper: depth = 3
- Infinite depth: When
depthisundefined, all nested events are observedThe JSDoc examples clearly illustrate the behavior.
106-129: LGTM: Fault-tolerant event processingThe
_processEvent()method correctly:
- Constructs a
PathObjectSubscriptionEventwith the appropriate PathObject and user-facing message- Wraps listener invocation in try-catch to isolate errors
- Logs errors without disrupting other subscriptions
This ensures one failing subscription doesn't prevent others from receiving events.
src/plugins/objects/liveobject.ts (4)
195-243: LGTM: Parent reference tracking APIThe parent reference management methods correctly maintain the
Map<LiveObject, Set<string>>structure:
addParentReference: Creates sets on-demand and adds keysremoveParentReference: Removes keys and cleans up empty setsremoveParentReferenceAll: Bulk removal for a specific parentclearParentReferences: Full reset for rebuild scenariosThis API enables efficient parent-child graph traversal.
251-289: LGTM: Iterative DFS with cycle detectionThe
getFullPaths()method correctly computes all paths from root to this object:
- Iterative DFS avoids stack overflow for deep hierarchies
- Cycle detection using a visited set per path prevents infinite loops
- Multiple paths are correctly handled when an object has multiple parents
- Path construction builds the path array from root to leaf by prepending keys
The implementation is robust and handles complex object graphs.
326-358: LGTM: Path-based event emission with per-key granularityThe
_notifyPathSubscriptions()method correctly:
- Emits bubbling events for each path to this object, allowing parent subscriptions to observe nested changes
- Emits non-bubbling per-key events for
LiveMapUpdate, enabling subscriptions atpath.keyto receive updates without bubbling to parent paths- Filters out sync messages, exposing only operation messages to maintain event semantics
This provides the right level of granularity for path-based subscriptions.
149-170: LGTM: Tombstone lifecycle with internal notificationThe
tombstone()method correctly:
- Marks the object as tombstoned with timestamp
- Clears data and populates
objectMessageon the update- Emits the
deletedlifecycle event- Calls
notifyUpdated()to deliver the final update to subscribers- Unsubscribes all listeners after notification
The sequence ensures proper cleanup and final event delivery.
src/plugins/objects/objectmessage.ts (2)
342-353: LGTM: Enum-to-string conversion for user-facing APIThe
stringifyOperation()function correctly converts internal enum values to user-facing string representations:
- Maps
ObjectOperationActionenum to action strings- Maps
ObjectsMapSemanticsenum to semantics strings- Handles optional fields and provides 'unknown' fallback
449-463: LGTM: User-facing message conversionThe
toUserFacingMessage()method correctly constructs the public APIObjectMessagerepresentation:
- Populates all required fields
- Converts the operation to user-facing format via
stringifyOperation()- Includes the channel name from the RealtimeChannel reference
The comment correctly notes that operation messages are the only type exposed to users.
ably.d.ts (7)
1649-1653: LGTM - Clean generic callback typeThe
EventCallback<T>type is well-defined and follows TypeScript best practices for event handler signatures.
2441-2463: LGTM - Well-documented subscription methodThe
subscribe()method signature is clean and the documentation clearly explains the subscription behavior, including depth configuration and instance tracking semantics.
2876-2896: LGTM - Clear instance subscription semanticsThe
subscribe()method documentation clearly explains how instance subscriptions track specific object instances and handle deletion (automatic unsubscribe on tombstone). The generic type parameterTproperly flows through toInstanceSubscriptionEvent<T>.
3094-3127: LGTM - Well-structured subscription typesThe subscription event types and options are cleanly defined:
PathObjectSubscriptionEventandInstanceSubscriptionEvent<T>consistently provide both the updated object and the optional triggering messagePathObjectSubscriptionOptions.depthis clearly documented with sensible defaults
3130-3183: LGTM - Well-organized operation action typesThe
ObjectOperationActionsnamespace andObjectsMapSemanticstypes are cleanly defined with descriptive type literals. The naming follows the<entity>.<action>pattern consistently.
3185-3235: LGTM - Comprehensive ObjectMessage interfaceThe
ObjectMessageinterface is well-documented and includes all necessary metadata fields. Theoperationfield properly referencesObjectOperation(though that type needs to be made generic as noted in the separate comment).
3271-3319: LGTM - Counter and data interfaces are correctly typedThe
ObjectsCounterOp,ObjectsCounter, andObjectDatainterfaces are appropriately defined:
ObjectsCounterOp.amountandObjectsCounter.countcorrectly usenumberObjectDataproperly represents the discriminated union of object references vs primitive values
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/plugins/objects/livemap.ts (1)
24-27: ArrayBuffer support requires encoder changes in ObjectMessage.You widened values to include
ArrayBuffer, butObjectMessage.encode()only treats Node Buffers as bytes; ArrayBuffers/TypedArrays will be JSON‑stringified. Add explicit handling (convert to Buffer) before the JSON branch. See prior review; re‑raising as it still applies.Proposed encoder handling (in
src/plugins/objects/objectmessage.ts):if (client.Platform.BufferUtils.isBuffer(data.value)) { encodedObjectData.bytes = data.value; } else if (data.value instanceof ArrayBuffer || ArrayBuffer.isView(data.value)) { const buf = client.Platform.BufferUtils.toBuffer(data.value as ArrayBuffer | ArrayBufferView); encodedObjectData.bytes = buf; } else if (typeof data.value === 'string') { encodedObjectData.string = data.value; } else if (typeof data.value === 'boolean') { encodedObjectData.boolean = data.value; } else if (typeof data.value === 'number') { encodedObjectData.number = data.value; } else if (typeof data.value === 'object' && data.value !== null) { encodedObjectData.json = JSON.stringify(data.value); }
🧹 Nitpick comments (7)
src/plugins/objects/objectmessage.ts (1)
449-463: Guard user-facing conversion against non-operation messages.Avoid asserting non-null with
!. Throw if called for sync messages to prevent accidental misuse.- toUserFacingMessage(channel: RealtimeChannel): API.ObjectMessage { + toUserFacingMessage(channel: RealtimeChannel): API.ObjectMessage { + if (!this.isOperationMessage()) { + throw new Error('toUserFacingMessage() requires an operation message'); + } return { id: this.id!, clientId: this.clientId, connectionId: this.connectionId, timestamp: this.timestamp!, channel: channel.name, // we expose only operation messages to users, so operation field is always present operation: stringifyOperation(this.operation!), serial: this.serial, serialTimestamp: this.serialTimestamp, siteCode: this.siteCode, extras: this.extras, }; }src/plugins/objects/instance.ts (1)
17-21: Avoid public name collision with API types.Rename or stop exporting this internal event type to avoid confusion with the public
InstanceSubscriptionEvent.-export interface InstanceEvent { +interface InternalInstanceEvent { /** Object message that caused this event */ message?: ObjectMessage; }Update the local callback type accordingly.
src/plugins/objects/liveobject.ts (4)
19-25: Type the update payload via a discriminated union instead ofany.
LiveObjectUpdate.update: anyweakens safety. Prefer per‑type payloads keyed by_type(e.g.,LiveMapUpdate['update'],LiveCounterUpdate['update']) to catch shape mismatches at compile time.
149-170: Tombstone clean‑up: consider clearing parent references post‑notification.After emitting the delete event and unsubscribing, drop
_parentReferencesto aid GC and avoid stale graph edges on long‑lived pools.this.notifyUpdated(update); - this._subscriptions.off(); + this._subscriptions.off(); + // No further updates will originate from this instance; release parent edges. + this.clearParentReferences();
251-289: Cycle‑safe, but consider worst‑case path explosion.
getFullPaths()duplicates traversal state per branch (by design). If objects are highly shared, path count can blow up. If this becomes hot, consider an optional cap or memoizing subpaths by(obj, suffix)to dedupe identical tails.
326-358: Per‑key path events carry only the base operation message.For sync‑derived diffs (
objectMessageis a sync),messageis omitted by design, so exact‑key listeners receive an event without a message. If product expectations require anObjectMessageon all events, we’ll need a scoped synthetic op or an auxiliary delta descriptor. Otherwise, current behaviour matches “operations only”.src/plugins/objects/livemap.ts (1)
551-605: Buffer/ArrayBuffer equality viadequal.Deep‑equal on binary types can be costly and may misclassify different views over same bytes. If you see churn, special‑case Node
Buffer.compare()andBufferUtils.equals(ArrayBuffer|View)before falling back todequal.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
ably.d.ts(15 hunks)src/plugins/objects/instance.ts(2 hunks)src/plugins/objects/livecounter.ts(7 hunks)src/plugins/objects/livemap.ts(12 hunks)src/plugins/objects/liveobject.ts(8 hunks)src/plugins/objects/objectmessage.ts(8 hunks)src/plugins/objects/pathobject.ts(4 hunks)src/plugins/objects/pathobjectsubscriptionregister.ts(1 hunks)src/plugins/objects/realtimeobject.ts(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/plugins/objects/pathobject.ts
🧰 Additional context used
🧬 Code graph analysis (8)
src/plugins/objects/instance.ts (2)
ably.d.ts (5)
ObjectMessage(3188-3235)EventCallback(1653-1653)InstanceSubscriptionEvent(3122-3127)LiveObject(2409-2409)Instance(3086-3092)src/plugins/objects/objectmessage.ts (1)
ObjectMessage(360-464)
src/plugins/objects/realtimeobject.ts (2)
src/plugins/objects/pathobjectsubscriptionregister.ts (1)
PathObjectSubscriptionRegister(42-211)src/plugins/objects/pathobject.ts (2)
DefaultPathObject(24-360)value(117-156)
src/plugins/objects/livecounter.ts (1)
src/plugins/objects/liveobject.ts (1)
LiveObjectUpdateNoop(27-31)
src/plugins/objects/liveobject.ts (6)
ably.d.ts (4)
ObjectMessage(3188-3235)LiveObject(2409-2409)EventCallback(1653-1653)LiveMapType(2851-2853)src/plugins/objects/objectmessage.ts (1)
ObjectMessage(360-464)src/plugins/objects/instance.ts (1)
InstanceEvent(17-20)src/plugins/objects/livemap.ts (1)
LiveMapUpdate(42-45)src/plugins/objects/constants.ts (1)
ROOT_OBJECT_ID(1-1)src/plugins/objects/pathobjectsubscriptionregister.ts (1)
PathEvent(27-34)
src/plugins/objects/objectmessage.ts (1)
ably.d.ts (8)
ObjectOperationAction(3162-3168)ObjectsMapSemantics(3183-3183)PrimitiveObjectValue(3515-3515)JsonArray(3530-3530)JsonObject(3535-3535)ObjectOperation(3240-3259)ObjectData(3314-3319)ObjectMessage(3188-3235)
src/plugins/objects/pathobjectsubscriptionregister.ts (3)
ably.d.ts (4)
EventCallback(1653-1653)PathObjectSubscriptionEvent(3097-3102)PathObjectSubscriptionOptions(3107-3117)ObjectMessage(3188-3235)src/plugins/objects/objectmessage.ts (1)
ObjectMessage(360-464)src/plugins/objects/pathobject.ts (2)
path(44-47)DefaultPathObject(24-360)
src/plugins/objects/livemap.ts (2)
ably.d.ts (2)
JsonArray(3530-3530)JsonObject(3535-3535)src/plugins/objects/liveobject.ts (1)
LiveObjectUpdateNoop(27-31)
ably.d.ts (1)
src/plugins/objects/objectmessage.ts (8)
ObjectMessage(360-464)ObjectOperation(133-168)ObjectsMapOp(73-78)ObjectsCounterOp(84-87)ObjectsMap(113-118)ObjectsCounter(124-127)ObjectData(42-47)ObjectsMapEntry(93-107)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (16.x)
- GitHub Check: test-node (18.x)
- GitHub Check: test-node (20.x)
🔇 Additional comments (19)
src/plugins/objects/objectmessage.ts (2)
778-792: Widened binary decode looks good.Early-return for
objectIdand support forBuffer | ArrayBufferin msgpack/json paths are consistent with the widened types.Also applies to: 784-790
728-749: No issues found—Bufferliketype includesArrayBufferin both platforms.The
Bufferliketype is defined asBuffer | ArrayBuffer | ArrayBufferViewin Node.js andBufferSource(which includesArrayBuffer | ArrayBufferView) in the browser. BothPlatform.BufferUtils.isBuffer()andbyteLength()explicitly handleArrayBufferwith type checks and the.byteLengthproperty. The code correctly supportsArrayBufferinputs without size miscalculation.src/plugins/objects/realtimeobject.ts (4)
94-95: Root resolution via pool LGTM.Switching to
getRoot()simplifies root access and aligns with pool ownership.
106-108: PathObject creation from pool root LGTM.Consistent with root resolution change and path API.
398-401: Parent reference rebuild: ordering and scope are good.Clearing then re-linking after sync, and before emitting updates, prevents stale parent edges.
Also applies to: 482-505
175-181: Accessor for PathObjectSubscriptionRegister LGTM.Internal getter is fine for wiring path subscriptions.
Please confirm producers invoke
notifyPathEvents(...)on updates (e.g., inLiveMap.set/removeand object creation/deletion) so subscribers actually receive events.src/plugins/objects/pathobjectsubscriptionregister.ts (3)
59-90: Subscription registration and validation LGTM.Options validation and unsubscribe handling are sound.
106-129: Event translation LGTM.Wrapping path and converting messages to user-facing shape is correct; listener errors are contained.
152-178: Depth and bubbling logic LGTM.Prefix check plus relative-depth calculation matches documented behavior.
src/plugins/objects/livecounter.ts (1)
130-159: No double notification occurs—the original concern is incorrect.
notifyUpdatedhas an explicit guard: it checksif (this._isNoopUpdate(update))and returns early without emitting. TheOBJECT_DELETEcase leavesupdateas{ noop: true }, so the call after the switch statement returns immediately without triggering any event. The actual notification fromtombstone()(which callsnotifyUpdatedwith the real update data) is what subscribers receive. Tests confirm this behavior: the subscription callback is called exactly once with the expected delete event.The code is correct as written. No refactoring needed.
src/plugins/objects/instance.ts (1)
154-164: Auto-unsubscribe on object deletion is working as expected.Verification confirms the subscription lifecycle: when
tombstone()is called on a deleted object, it notifies existing subscribers of the deletion event vianotifyUpdated(), then immediately deregisters all listeners withthis._subscriptions.off()(line 169 in liveobject.ts). The instance forwarding code correctly preserves this behavior and forwards the subscription event to the user's listener.src/plugins/objects/livemap.ts (4)
526-545: Nice: clearing child parent refs on tombstone.
clearData()removes back‑edges from referenced objects before delegating, preventing dangling ancestry. This aligns with the new path subscription model.
729-768: Parent reference swap on MAP_SET looks correct.Old ref removed, new ref added, and
_type/objectMessagepopulated for downstream listeners. Ensure poolget()returns created zero‑value objects for unseen IDs soaddParentReferencenever silently no‑ops.
807-838: Parent reference removal on MAP_REMOVE is consistent.Back‑edge cleared and update tagged as
'removed'. Matches path propagation expectations.
499-507: Good: setobjectMessageon override and sync parent refs.Attaching the source message while updating parent references ensures instance listeners get op messages only, while path listeners still receive sync‑originated events without leaking sync details.
ably.d.ts (4)
1649-1654: Event callback alias looks good.
EventCallback<T> = (event: T) => voidaligns the new subscription surfaces.
2439-2464: Public subscribe surface reads well and returnsSubscribeResponse.Docs and signatures match the new path subscription model.
2859-2897: Instance subscriptions API: docs and shape LGTM.Follows instance regardless of location and auto‑unsubscribes on delete.
3188-3320: Types are correctly exported as non-generic; no TS2315 errors will occur.Verification confirms all four interfaces (
ObjectOperation,ObjectsMapOp,ObjectsMap,ObjectsMapEntry) are exported without type parameters and used consistently without generic instantiations throughout the file (e.g., line 3212 usesObjectOperationdirectly, line 3286 usesObjectsMapEntryas a type argument toRecordwithout generic parameters). No breaking type errors will result from these exports.
Implements:
Resolves PUB-2061
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Documentation
Tests