[PUB-2059] Change LiveObjects entry point to channel.object.get()#2087
Conversation
WalkthroughRenames the public API from RealtimeObjects to RealtimeObject across types, channel property, and plugin internals. Replaces getRoot() with get() on both RealtimeObject and BatchContext. Updates imports/usages, pools/sync dependencies, tests, and a build-report script. Minor additions include a decrement type check and connectionId propagation in update payloads. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant Chan as RealtimeChannel
participant Obj as RealtimeObject
participant Srv as Ably Service
App->>Chan: channel = client.channels.get("X")
App->>Chan: channel.object
Chan-->>App: RealtimeObject
App->>Obj: get<T>()
Note over Obj: Validate access/config
Obj->>Srv: Attach/Sync (OBJECT/OBJECT_SYNC)
Srv-->>Obj: Object state/messages
Obj-->>App: Promise<LiveMap<T>>
sequenceDiagram
autonumber
participant App as Application
participant Obj as RealtimeObject
participant Batch as BatchContext
participant Srv as Ably Service
App->>Obj: batch(fn)
Obj-->>App: BatchContext
App->>Batch: get<T>()
Note over Batch: Wrap root LiveMap<T>
App->>Batch: mutate map/counter
Batch->>Obj: publish batched ops
Obj->>Srv: OBJECT ops
Srv-->>Obj: ACK/Sync
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.39.5)test/realtime/objects.test.jsThanks 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/2087/features
github-actions
bot
temporarily deployed
to
staging/pull/2087/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/2087/typedoc
VeskeR
force-pushed
the
PUB-2059/lo-entry-point-change
branch
from
14e1419 to
4c3009c
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2087/features
github-actions
bot
temporarily deployed
to
staging/pull/2087/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/2087/typedoc
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/realtime/objects.test.js (1)
1008-1010: Fix invalid chained comparisons (can yield false positives)Expressions like a <= b <= c don’t work in JS; use within() or &&.
- expect( - tsBeforeMsg <= obj.tombstonedAt() <= tsAfterMsg, - `Check object's "tombstonedAt" value is set using local clock if no "serialTimestamp" provided`, - ).to.be.true; + expect(obj.tombstonedAt()).to.be.within( + tsBeforeMsg, + tsAfterMsg, + `Check object's "tombstonedAt" value is set using local clock if no "serialTimestamp" provided`, + );- expect( - tsBeforeMsg <= mapEntry.tombstonedAt <= tsAfterMsg, - `Check map entry's "tombstonedAt" value is set using local clock if no "serialTimestamp" provided`, - ).to.be.true; + expect(mapEntry.tombstonedAt).to.be.within( + tsBeforeMsg, + tsAfterMsg, + `Check map entry's "tombstonedAt" value is set using local clock if no "serialTimestamp" provided`, + );- expect( - tsBeforeMsg <= mapEntry.tombstonedAt <= tsAfterMsg, - `Check map entry's "tombstonedAt" value is set using local clock if no "serialTimestamp" provided`, - ).to.be.true; + expect(mapEntry.tombstonedAt).to.be.within( + tsBeforeMsg, + tsAfterMsg, + `Check map entry's "tombstonedAt" value is set using local clock if no "serialTimestamp" provided`, + );- expect( - tsBeforeMsg <= obj.tombstonedAt() <= tsAfterMsg, - `Check object's "tombstonedAt" value is set using local clock if no "serialTimestamp" provided`, - ).to.be.true; + expect(obj.tombstonedAt()).to.be.within( + tsBeforeMsg, + tsAfterMsg, + `Check object's "tombstonedAt" value is set using local clock if no "serialTimestamp" provided`, + );Also applies to: 1087-1089, 1661-1663, 2190-2192
🧹 Nitpick comments (15)
src/plugins/objects/syncobjectsdatapool.ts (1)
30-33: Make the injected dependency readonly.Prevents accidental reassignment and communicates intent.
Apply:
- constructor(private _realtimeObject: RealtimeObject) { + constructor(private readonly _realtimeObject: RealtimeObject) {src/plugins/objects/batchcontextlivecounter.ts (2)
9-15: Constructor migration looks good; mark dependency readonly.- constructor( + constructor( private _batchContext: BatchContext, - private _realtimeObject: RealtimeObject, + private readonly _realtimeObject: RealtimeObject, private _counter: LiveCounter, ) {
23-28: Optional early arg validation for increment.Keeps error origin closer to call site (mirrors decrement’s precheck).
increment(amount: number): void { this._realtimeObject.throwIfInvalidWriteApiConfiguration(); this._batchContext.throwIfClosed(); + if (typeof amount !== 'number' || !Number.isFinite(amount)) { + throw new this._client.ErrorInfo('Counter value increment should be a valid number', 40003, 400); + } const msg = LiveCounter.createCounterIncMessage(this._realtimeObject, this._counter.getObjectId(), amount); this._batchContext.queueMessage(msg); }ably.d.ts (1)
2291-2293: Remove TODO from public typings and rephrase doc.Avoid shipping TODOs in d.ts; tighten wording.
- /** - * TODO: how to refer to the "root" object in docs now that we avoid that term in the public API? - * + /**Alternative doc tweak (optional):
- * Retrieves the root {@link LiveMap} object for Objects on a channel. + * Retrieves the channel’s Objects map (the top-level {@link LiveMap}).src/plugins/objects/batchcontext.ts (1)
18-24: Constructor migration looks good; make dependency readonly.- constructor( - private _realtimeObject: RealtimeObject, + constructor( + private readonly _realtimeObject: RealtimeObject, private _root: LiveMap<API.LiveMapType>, ) {src/plugins/objects/livecounter.ts (1)
148-166: DRY up number validation (optional)Increment/decrement duplicate finite-number checks. Consider a tiny helper to keep messages/codes in one place.
class LiveCounter extends LiveObject<LiveCounterData, LiveCounterUpdate> { + private static _assertFiniteNumber(client: any, value: unknown, what: 'increment'|'decrement'|'initial'): void { + if (typeof value !== 'number' || !Number.isFinite(value)) { + throw new client.ErrorInfo( + `Counter value ${what} should be a valid number`, + 40003, + 400, + ); + } + } static createCounterIncMessage(realtimeObject: RealtimeObject, objectId: string, amount: number): ObjectMessage { - const client = realtimeObject.getClient(); - - if (typeof amount !== 'number' || !Number.isFinite(amount)) { - throw new client.ErrorInfo('Counter value increment should be a valid number', 40003, 400); - } + const client = realtimeObject.getClient(); + LiveCounter._assertFiniteNumber(client, amount, 'increment'); ... } static async createCounterCreateMessage(realtimeObject: RealtimeObject, count?: number): Promise<ObjectMessage> { - const client = realtimeObject.getClient(); - if (count !== undefined && (typeof count !== 'number' || !Number.isFinite(count))) { - throw new client.ErrorInfo('Counter value should be a valid number', 40003, 400); - } + const client = realtimeObject.getClient(); + if (count !== undefined) LiveCounter._assertFiniteNumber(client, count, 'initial'); ... } async decrement(amount: number): Promise<void> { - this._realtimeObject.throwIfInvalidWriteApiConfiguration(); - if (typeof amount !== 'number' || !Number.isFinite(amount)) { - throw new this._client.ErrorInfo('Counter value decrement should be a valid number', 40003, 400); - } + this._realtimeObject.throwIfInvalidWriteApiConfiguration(); + LiveCounter._assertFiniteNumber(this._client, amount, 'decrement'); return this.increment(-amount); }src/plugins/objects/livemap.ts (1)
541-548: Avoid repeated Date.now() per loop (micro)Cache now once for stable comparison and a tiny perf win.
- const keysToDelete: string[] = []; - for (const [key, value] of this._dataRef.data.entries()) { - if (value.tombstone === true && Date.now() - value.tombstonedAt! >= this._realtimeObject.gcGracePeriod) { + const keysToDelete: string[] = []; + const now = Date.now(); + for (const [key, value] of this._dataRef.data.entries()) { + if (value.tombstone === true && now - value.tombstonedAt! >= this._realtimeObject.gcGracePeriod) { keysToDelete.push(key); } }test/realtime/objects.test.js (5)
3945-3966: Batch callback shouldn’t be async (callback result is not awaited)These callbacks don’t await inside, so remove async to avoid misleading future edits.
- await realtimeObject.batch(async (ctx) => { + await realtimeObject.batch((ctx) => { const ctxRoot = ctx.get(); ... });
3885-3887: Unnecessary await on sync getterLiveMap.get returns synchronously; drop await for clarity.
- const counter1 = await root.get('counter1'); + const counter1 = root.get('counter1');
125-127: Prefer named ProtocolMessage action constants over magic numbersReplace 19/20 with ProtocolMessage.Action.OBJECT/OBJECT_SYNC (or equivalent) to avoid drift.
Also applies to: 149-151
31-34: Tighten class name check regex (nit)Anchor the pattern to avoid accidental matches.
- expect(object.constructor.name).to.match(new RegExp(`_?${className}`), msg); + expect(object.constructor.name).to.match(new RegExp(`^_?${className}$`), msg);
1008-1010: Action: convert/remove async callbacks passed to .batch or confirm .batch awaits Promises
- No chained-comparison patterns detected in the repo.
- Async callback found at test/realtime/objects.test.js:3945 — replace the async (ctx) => { … } with a synchronous callback or ensure realtimeObject.batch properly awaits returned Promises.
src/plugins/objects/objectspool.ts (2)
113-113: Optional: expose a disposer to clear the GC interval.
Prevents timers lingering after teardown in long‑lived tests or app shutdowns.export class ObjectsPool { @@ private _onGCInterval(): void { ... } + + /** @internal */ + dispose(): void { + clearInterval(this._gcInterval); + } }
83-92: Harden switch with a default case that throws on unexpected object id types.
ObjectId.fromString(client, objectId) only allows 'map' | 'counter' (it will throw for 'root'), but add this defensive default to surface parser/type drift early.Apply:
switch (parsedObjectId.type) { case 'map': { zeroValueObject = LiveMap.zeroValue(this._realtimeObject, objectId); // RTO6b2 break; } case 'counter': zeroValueObject = LiveCounter.zeroValue(this._realtimeObject, objectId); // RTO6b3 break; + default: { + throw new this._client.ErrorInfo( + `Unknown LiveObject id type "${(parsedObjectId as any).type}" for objectId=${objectId}`, + 50000, + 500, + ); + } }src/plugins/objects/batchcontextlivemap.ts (1)
15-47: Read‑path guards before access — OK.
Consistently enforcesobject_subscribeand batch open state.You can DRY the repeated guard with a tiny helper:
export class BatchContextLiveMap<T extends API.LiveMapType> { + private _ensureReadable() { + this._realtimeObject.throwIfInvalidAccessApiConfiguration(); + this._batchContext.throwIfClosed(); + } @@ - this._realtimeObject.throwIfInvalidAccessApiConfiguration(); - this._batchContext.throwIfClosed(); + this._ensureReadable();
📜 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 (16)
ably.d.ts(8 hunks)scripts/moduleReport.ts(1 hunks)src/common/lib/client/realtimechannel.ts(8 hunks)src/plugins/objects/batchcontext.ts(4 hunks)src/plugins/objects/batchcontextlivecounter.ts(1 hunks)src/plugins/objects/batchcontextlivemap.ts(2 hunks)src/plugins/objects/index.ts(1 hunks)src/plugins/objects/livecounter.ts(7 hunks)src/plugins/objects/livemap.ts(18 hunks)src/plugins/objects/liveobject.ts(3 hunks)src/plugins/objects/objectspool.ts(5 hunks)src/plugins/objects/realtimeobject.ts(6 hunks)src/plugins/objects/syncobjectsdatapool.ts(2 hunks)test/common/modules/private_api_recorder.js(4 hunks)test/package/browser/template/src/index-objects.ts(2 hunks)test/realtime/objects.test.js(71 hunks)
🧰 Additional context used
🧬 Code graph analysis (13)
src/plugins/objects/livemap.ts (3)
src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(39-529)ably.d.ts (1)
LiveMapType(2394-2394)src/plugins/objects/objectmessage.ts (1)
ObjectMessage(335-415)
src/plugins/objects/realtimeobject.ts (4)
src/plugins/objects/index.ts (1)
RealtimeObject(4-4)ably.d.ts (2)
LiveMapType(2394-2394)DefaultRoot(2403-2410)src/plugins/objects/livemap.ts (1)
LiveMap(48-928)test/realtime/objects.test.js (1)
root(170-170)
src/plugins/objects/batchcontextlivecounter.ts (3)
src/plugins/objects/batchcontext.ts (1)
BatchContext(11-107)src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(39-529)src/plugins/objects/livecounter.ts (1)
LiveCounter(22-362)
test/package/browser/template/src/index-objects.ts (1)
test/realtime/objects.test.js (5)
root(170-170)channel(166-166)channel(199-199)channel(277-277)channel(5542-5542)
src/common/lib/client/realtimechannel.ts (2)
src/plugins/objects/index.ts (1)
RealtimeObject(4-4)src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(39-529)
src/plugins/objects/syncobjectsdatapool.ts (1)
src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(39-529)
src/plugins/objects/liveobject.ts (1)
src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(39-529)
src/plugins/objects/livecounter.ts (1)
src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(39-529)
src/plugins/objects/batchcontextlivemap.ts (4)
src/plugins/objects/batchcontext.ts (1)
BatchContext(11-107)src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(39-529)src/plugins/objects/livemap.ts (1)
LiveMap(48-928)src/plugins/objects/batchcontextlivecounter.ts (1)
value(17-21)
ably.d.ts (3)
src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(39-529)src/plugins/objects/livemap.ts (1)
LiveMap(48-928)src/plugins/objects/batchcontextlivemap.ts (1)
BatchContextLiveMap(7-62)
src/plugins/objects/objectspool.ts (3)
src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(39-529)src/plugins/objects/livemap.ts (1)
LiveMap(48-928)src/plugins/objects/livecounter.ts (1)
LiveCounter(22-362)
src/plugins/objects/batchcontext.ts (3)
src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(39-529)src/plugins/objects/batchcontextlivemap.ts (1)
BatchContextLiveMap(7-62)src/plugins/objects/batchcontextlivecounter.ts (1)
BatchContextLiveCounter(6-41)
test/realtime/objects.test.js (5)
src/plugins/objects/batchcontextlivecounter.ts (1)
value(17-21)src/plugins/objects/livecounter.ts (1)
value(134-137)src/plugins/objects/batchcontextlivemap.ts (1)
entries(31-35)src/plugins/objects/livemap.ts (1)
entries(321-334)src/plugins/objects/syncobjectsdatapool.ts (1)
entries(36-38)
⏰ 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 (34)
src/plugins/objects/realtimeobject.ts (4)
39-39: LGTM! Class name correctly updated to singular.The class has been renamed from
RealtimeObjectstoRealtimeObjectas part of the consistent plural-to-singular API refactoring.
78-78: LGTM! Method documentation and signature updated correctly.The comment has been updated from "A user can provide an explicit type for the getRoot method" to "A user can provide an explicit type for the this method", and the method name has been changed from
getRoottoget, both of which align with the singular API changes.Also applies to: 82-82
99-99: LGTM! Internal method call updated consistently.The call to retrieve the root object has been properly updated from
this.getRoot()tothis.get(), maintaining consistency with the method rename.
268-268: LGTM! Logging messages updated to reflect new class name.The log messages have been correctly updated from
'RealtimeObjects.onAttached()'to'RealtimeObject.onAttached()'and from'RealtimeObjects._applyObjectMessages()'to'RealtimeObject._applyObjectMessages()', maintaining accuracy in debugging and monitoring.Also applies to: 444-444, 473-473
test/common/modules/private_api_recorder.js (1)
29-30: LGTM! Private API identifiers updated for class rename.All private API identifiers have been correctly updated from
RealtimeObjectstoRealtimeObject:
- Line 29-30: Call operations updated
- Line 86-87: Read operations updated
- Line 127-128: Replace operations updated
- Line 147-148: Write operations updated
These changes ensure the private API tracking remains accurate after the class rename.
Also applies to: 86-87, 127-128, 147-148
src/plugins/objects/syncobjectsdatapool.ts (1)
4-4: Import rename to RealtimeObject is consistent with the refactor. LGTM.src/plugins/objects/batchcontextlivecounter.ts (1)
30-37: Good type guard before negation.src/common/lib/client/realtimechannel.ts (4)
140-158: Channel surface rename: objects ➜ object implemented correctly.Instantiation, getter, and missing‑plugin error path look consistent.
621-637: OBJECT/OBJECT_SYNC routing via RealtimeObject is correct.Early return on missing plugin or state is fine; decoding uses active transport format as expected.
801-803: Channel state propagation to RealtimeObject updated. LGTM.
153-158: Verify no legacy usages of channel.objects remainLocal verification required — run the following and confirm there are no matches; ensure plugin exports include RealtimeObject and WireObjectMessage.
rg -nP --hidden --no-ignore-vcs '\.objects\b' -C2 rg -nP --hidden --no-ignore-vcs '\bgetRoot\s*\(' -C2 rg -nP --hidden --no-ignore-vcs 'export\s+.*\bRealtimeObject\b' src/plugins/objects -C2 rg -nP --hidden --no-ignore-vcs 'export\s+.*\bWireObjectMessage\b' src/plugins/objects -C2 # Fallback if ripgrep is filtered: git grep -n -- '\.objects\b' || trueably.d.ts (3)
2429-2435: BatchContext.get mirror of RealtimeObject.get is accurate.
2950-2953: Public surface: RealtimeChannel.object exported correctly.
629-631: Docs updated to reference RealtimeChannel.object. LGTM.src/plugins/objects/batchcontext.ts (3)
40-50: Wrapper instantiation switched to RealtimeObject; consistent with refactor.
99-101: Flush delegates to RealtimeObject.publish; good.
26-30: API rename getRoot ➜ get — confirm no remaining callers.
Automated searches in the sandbox returned no matches; run a repo-wide search locally (e.g., rg -nP '\bgetRoot\b' --hidden --no-ignore) and confirm there are no remaining callers or external consumers to update.src/plugins/objects/livecounter.ts (1)
11-12: API migration to RealtimeObject: LGTMConstructor/context swaps, access/write guards, and message creation/publish paths are consistent with RealtimeObject. Error handling preserved.
Also applies to: 29-31, 39-55, 60-80, 85-120, 133-136, 148-152, 157-166
src/plugins/objects/livemap.ts (1)
18-19: RealtimeObject refactor: LGTMConstructor/context updates, guard usage, and pool interactions look correct and consistent across APIs.
Also applies to: 50-55, 63-66, 74-80, 88-99, 104-139, 145-168, 173-194, 198-238, 281-303, 305-334, 357-376, 897-898, 918-919
test/realtime/objects.test.js (3)
196-201: channel.object surface in tests: LGTMRenames align with the new API: property access, get(), and type assertions behave as expected.
Also applies to: 274-279, 282-294
5220-5531: ObjectMessage size tests: LGTMCoverage is comprehensive across fields and payload types.
4865-4866: GC interval override restored: LGTMOverrides are properly reset in finally. No leak across suites.
Also applies to: 4917-4919
src/plugins/objects/liveobject.ts (3)
4-4: Rename alignment looks good (singular import).
Matches the broader API rename.
57-62: Constructor refactor to RealtimeObject is correct.
Client access viathis._realtimeObject.getClient()is consistent with the new design.
72-74: Access gating on subscribe is appropriate.
Ensures channel hasobject_subscribemode before registering listeners.src/plugins/objects/objectspool.ts (3)
7-7: Import rename to RealtimeObject — OK.
20-22: Ctor dependency swap to RealtimeObject — OK.
_clientretrieval viagetClient()is consistent.
101-101: Root zero‑value via RealtimeObject — OK.
Matches new constructor shape.src/plugins/objects/batchcontextlivemap.ts (2)
5-12: Constructor and dependency rename — OK.
Wiring matchesBatchContextand the newRealtimeObjectAPI.
50-60: Write‑path guards and message creation — OK.
Usesobject_publishchecks and passesRealtimeObjectthrough to message builders.test/package/browser/template/src/index-objects.ts (2)
45-46: Switch tochannel.object.get()— OK.
Type inference forCustomRooton the returned LiveMap checks out.
91-93: Explicit generic onobject.get<ExplicitRootType>()— OK.
Validates the override of globalAblyObjectsTypes.scripts/moduleReport.ts (1)
340-341: Updated allowed file path to realtimeobject.ts — OK.
No occurrences of "realtimeobjects.ts" or "RealtimeObjects" found; scripts/moduleReport.ts now lists 'src/plugins/objects/realtimeobject.ts' (lines ~340–341).src/plugins/objects/index.ts (1)
2-9: Public surface rename to RealtimeObject — OK (breaking change); verify no remaining plural API referencesDocs and migration notes must reflect the singular API.
File: src/plugins/objects/index.ts (lines 2-9)
import { RealtimeObject } from './realtimeobject'; export { ObjectMessage, RealtimeObject, WireObjectMessage }; export default { ObjectMessage, RealtimeObject, WireObjectMessage,Automated scan could not be completed in this environment (ripgrep returned "No files were searched"). Re-run the scan where the repository is present with:
rg -nP --hidden -uu -g '!**/dist/**' -g '!**/build/**' -C2 '\bRealtimeObjects\b|\bobjects\.getRoot\b|\bchannel\.objects\b'If rg still reports no files, run
rg --debugto diagnose why files are being skipped.
VeskeR
requested review from
lawrence-forooghian and
mschristensen
and removed request for
lawrence-forooghian and
mschristensen
VeskeR
force-pushed
the
PUB-2059/lo-entry-point-change
branch
from
4c3009c to
f02db17
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2087/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/2087/features
github-actions
bot
temporarily deployed
to
staging/pull/2087/typedoc
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)
test/realtime/objects.test.js (1)
1008-1010: Fix invalid chained comparisons; these assertions always pass in JSJavaScript does not support Python‑style chained comparisons. Use explicit range checks.
- expect( - tsBeforeMsg <= obj.tombstonedAt() <= tsAfterMsg, - `Check object's "tombstonedAt" value is set using local clock if no "serialTimestamp" provided`, - ).to.be.true; + expect( + obj.tombstonedAt(), + `Check object's "tombstonedAt" value is set using local clock if no "serialTimestamp" provided`, + ).to.be.at.least(tsBeforeMsg).and.at.most(tsAfterMsg);- expect( - tsBeforeMsg <= mapEntry.tombstonedAt <= tsAfterMsg, - `Check map entry's "tombstonedAt" value is set using local clock if no "serialTimestamp" provided`, - ).to.be.true; + expect( + mapEntry.tombstonedAt, + `Check map entry's "tombstonedAt" value is set using local clock if no "serialTimestamp" provided`, + ).to.be.at.least(tsBeforeMsg).and.at.most(tsAfterMsg);- expect( - tsBeforeMsg <= mapEntry.tombstonedAt <= tsAfterMsg, - `Check map entry's "tombstonedAt" value is set using local clock if no "serialTimestamp" provided`, - ).to.be.true; + expect( + mapEntry.tombstonedAt, + `Check map entry's "tombstonedAt" value is set using local clock if no "serialTimestamp" provided`, + ).to.be.at.least(tsBeforeMsg).and.at.most(tsAfterMsg);- expect( - tsBeforeMsg <= obj.tombstonedAt() <= tsAfterMsg, - `Check object's "tombstonedAt" value is set using local clock if no "serialTimestamp" provided`, - ).to.be.true; + expect( + obj.tombstonedAt(), + `Check object's "tombstonedAt" value is set using local clock if no "serialTimestamp" provided`, + ).to.be.at.least(tsBeforeMsg).and.at.most(tsAfterMsg);Also applies to: 1087-1089, 1661-1663, 2190-2192
🧹 Nitpick comments (7)
ably.d.ts (1)
2289-2316: Public API: RealtimeObject + get()Breaking rename (
getRoot()→get()) is correctly reflected. Ensure any migration notes call this out.Consider adding a temporary deprecated alias in JS (not in typings) behind a feature flag to ease migration of downstream samples/tests if needed.
src/plugins/objects/batchcontextlivecounter.ts (1)
30-40: Minor: tighten decrement validation for parity with increment.
increment()rejects non-finite numbers;decrement()only checks type. Add a finite check to keep error semantics consistent.if (typeof amount !== 'number') { throw new this._client.ErrorInfo('Counter value decrement should be a number', 40003, 400); } + if (!Number.isFinite(amount)) { + throw new this._client.ErrorInfo('Counter value decrement should be a valid number', 40003, 400); + }src/common/lib/client/realtimechannel.ts (1)
153-158: Getterobjectwith missing-plugin guard.Throws consistent error label (“Objects”). Consider harmonizing message text across SDKs if there’s a convention.
test/realtime/objects.test.js (2)
3945-3966: Avoid async batch callback; it isn’t awaitedbatch() doesn’t await an async callback; mark it sync to avoid timing surprises.
- await realtimeObject.batch(async (ctx) => { + await realtimeObject.batch((ctx) => { const ctxRoot = ctx.get(); // enumeration methods should not count tombstoned entries expect(ctxRoot.size()).to.equal(2, 'Check BatchContextLiveMap.size() returns expected number of keys');
3885-3886: Remove unnecessary await on synchronous get()LiveMap#get is sync; awaiting is redundant.
- const counter1 = await root.get('counter1'); + const counter1 = root.get('counter1');src/plugins/objects/livemap.ts (2)
198-206: Minor: input error message parityvalidateKeyValue throws 40013 for value types; createMapCreateMessage throws 40003 for invalid entries object. If intentional, ignore; otherwise consider aligning error codes for consistency.
321-334: Nit: micro-allocs in entries()You could store pool ref once per iterator to avoid repeated getPool() calls inside helpers, though impact is negligible.
📜 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 (16)
ably.d.ts(8 hunks)scripts/moduleReport.ts(1 hunks)src/common/lib/client/realtimechannel.ts(8 hunks)src/plugins/objects/batchcontext.ts(4 hunks)src/plugins/objects/batchcontextlivecounter.ts(1 hunks)src/plugins/objects/batchcontextlivemap.ts(2 hunks)src/plugins/objects/index.ts(1 hunks)src/plugins/objects/livecounter.ts(7 hunks)src/plugins/objects/livemap.ts(18 hunks)src/plugins/objects/liveobject.ts(3 hunks)src/plugins/objects/objectspool.ts(5 hunks)src/plugins/objects/realtimeobject.ts(6 hunks)src/plugins/objects/syncobjectsdatapool.ts(2 hunks)test/common/modules/private_api_recorder.js(4 hunks)test/package/browser/template/src/index-objects.ts(2 hunks)test/realtime/objects.test.js(71 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- test/package/browser/template/src/index-objects.ts
- src/plugins/objects/liveobject.ts
- src/plugins/objects/objectspool.ts
- src/plugins/objects/batchcontextlivemap.ts
- scripts/moduleReport.ts
- src/plugins/objects/syncobjectsdatapool.ts
- src/plugins/objects/realtimeobject.ts
- src/plugins/objects/index.ts
- src/plugins/objects/livecounter.ts
🧰 Additional context used
🧬 Code graph analysis (6)
src/plugins/objects/batchcontextlivecounter.ts (3)
src/plugins/objects/batchcontext.ts (1)
BatchContext(11-107)src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(39-529)src/plugins/objects/livecounter.ts (1)
LiveCounter(22-362)
src/common/lib/client/realtimechannel.ts (2)
src/plugins/objects/index.ts (1)
RealtimeObject(4-4)src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(39-529)
ably.d.ts (2)
src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(39-529)src/plugins/objects/batchcontextlivemap.ts (1)
BatchContextLiveMap(7-62)
src/plugins/objects/batchcontext.ts (4)
src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(39-529)src/plugins/objects/livemap.ts (1)
LiveMap(48-928)src/plugins/objects/batchcontextlivemap.ts (1)
BatchContextLiveMap(7-62)src/plugins/objects/batchcontextlivecounter.ts (1)
BatchContextLiveCounter(6-41)
src/plugins/objects/livemap.ts (1)
src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(39-529)
test/realtime/objects.test.js (4)
src/plugins/objects/batchcontextlivecounter.ts (1)
value(17-21)src/plugins/objects/livecounter.ts (1)
value(134-137)src/plugins/objects/batchcontextlivemap.ts (1)
entries(31-35)src/plugins/objects/livemap.ts (1)
entries(321-334)
⏰ 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 (webkit)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (chromium)
🔇 Additional comments (24)
ably.d.ts (6)
629-632: Docs updated for plugin entrypoint.Comment now points to
RealtimeChannel.object. Looks consistent.
1656-1659: Event callback rename matches type surface.
ObjectsEventCallbacknow referencesRealtimeObject. No issues.
1666-1672: Batch callback doc ties to RealtimeObject.batch.Good linkage; no behavior change implied.
2267-2270: ObjectsEvent → RealtimeObject event type alignment.Type and docs reflect the singular object model. LGTM.
2427-2433: BatchContext.get mirrors RealtimeObject.get.Return type and docs are consistent with the new surface.
2948-2951: Approve: RealtimeChannel.object property added — verify no remainingobjectsgettersGetter name/type match runtime changes in
realtimechannel.ts. Ripgrep returned "No files were searched", so I couldn't confirm absence of otherobjectsgetters — re-run the check from the repo root:Location: ably.d.ts lines 2948–2951
rg -n --hidden -S '\bobjects\s*[:(]' -g '!**/node_modules/**' -g '!**/dist/**' || true rg -n --hidden -S '\bobjects\b' -g '**/*.d.ts' -g '!**/node_modules/**' || truesrc/plugins/objects/batchcontextlivecounter.ts (3)
11-15: Constructor migration to RealtimeObject is correct.Client acquisition via
_realtimeObject.getClient()matches plugin composition pattern.
17-21: Access checks placed before read.Order is fine; ensures API gating is enforced inside batch wrappers.
23-28: Increment path wired to new message factory.Publishes via queued message with correct objectId context.
src/common/lib/client/realtimechannel.ts (6)
20-21: Types import switched to RealtimeObject/WireObjectMessage.Matches plugin export surface.
99-99: Channel now holds_object?: RealtimeObject.Field rename aligns with the singular model.
140-142: Plugin instantiation updated to RealtimeObject.Lazy-init respects plugin presence. Good.
553-555: HAS_OBJECTS re-sync path forwarded to RealtimeObject.Correctly triggers object resync on non-resumed attach.
621-637: OBJECT/OBJECT_SYNC handling routed through RealtimeObject.Guard for missing plugin or
message.stateis sound; decoding uses active transport format. LGTM.
801-803: Channel state propagation to RealtimeObject.
actOnChannelStatereceiveshasObjects; matches implementation.src/plugins/objects/batchcontext.ts (5)
9-10: Import switch to RealtimeObject.Consistent with broader rename.
18-24: Constructor updated to singular object + root wrapper seed.Client sourcing and initial wrapper registration are correct.
26-30: get(): access gating + wrapper retrieval.Signature mirrors typings; safe checks precede access. LGTM.
40-50: Object lookup via RealtimeObject pool.Wrapper creation path for map/counter uses the right context; error path retains client ErrorInfo.
99-101: flush(): delegates publish to RealtimeObject.Matches new surface; clears state in finally. LGTM.
test/common/modules/private_api_recorder.js (1)
29-30: Rename alignment to RealtimeObject looks good.Automated sweep failed — ripgrep returned "No files were searched". Run locally and confirm no remaining references by running:
rg -nP -S '\bRealtimeObjects\b|\bchannel\.objects\b|\bgetRoot\s*\(' --hidden --no-ignore --glob '!node_modules/**' rg -nP -S '\bRealtimeObject\b' --hidden --no-ignore --glob '!node_modules/**'Locations to check: test/common/modules/private_api_recorder.js lines 29-30 (also 86-87, 127-128, 147-148).
src/plugins/objects/livemap.ts (2)
49-55: Solid rename and integration with RealtimeObjectConstructor, factories, access/write guards, pool lookups, and GC now route via RealtimeObject; semantics preserved. LGTM.
Also applies to: 63-65, 74-80, 88-97, 105-113, 144-150, 173-193, 198-238, 281-285, 305-323, 357-361, 372-376, 542-548, 721-729, 872-880, 895-901, 917-927
18-19: Verify no capitalizedIf((likely typo) exists in the repoAutomated ripgrep in the review sandbox couldn't search files — verify locally. Check src/plugins/objects/realtimeobject.ts (livemap imports it at src/plugins/objects/livemap.ts:18-19) and the whole codebase:
rg -nP '\bIf\s*(' src/ -S
If ripgrep is unavailable:
git grep -n -E '\bIf\s*(' -- src/
If any matches are found, change
If(toif(in those files.test/realtime/objects.test.js (1)
169-173: API migration looks consistent — automated verification failed; manual re-run requiredTests reflect the new entry point and class names; reads well. Automated grep ran into errors / returned no results — re-run these checks from the repo root to confirm no remaining old APIs or chained comparisons:
rg -nP --glob '!node_modules/**' '(channel\.objects\b|\.getRoot\s*\()' . rg -nP --glob '!node_modules/**' '(?<=[\w\]\)])\s*<=\s*[^;=]+?\s*<=\s*(?=[\w\(])' .Also applies to: 196-201, 274-279, 283-294, 297-310
VeskeR
force-pushed
the
PUB-2059/lo-entry-point-change
branch
from
f02db17 to
be5eb4b
Compare
VeskeR
changed the base branch from
integration/objects-breaking-api-2025-09-16-merge-main
to
integration/objects-breaking-api-2025-09-24-merge-main
VeskeR
changed the base branch from
integration/objects-breaking-api-2025-09-24-merge-main
to
main
VeskeR
changed the base branch from
main
to
integration/objects-breaking-api-2025-09-24-merge-main
VeskeR
force-pushed
the
PUB-2059/lo-entry-point-change
branch
from
be5eb4b to
87ede4c
Compare
github-actions
bot
temporarily deployed
to
staging/pull/2087/features
github-actions
bot
temporarily deployed
to
staging/pull/2087/bundle-report
github-actions
bot
temporarily deployed
to
staging/pull/2087/typedoc
VeskeR
force-pushed
the
PUB-2059/lo-entry-point-change
branch
from
87ede4c to
eded037
Compare
VeskeR
changed the base branch from
integration/objects-breaking-api-2025-09-24-merge-main
to
integration/objects-breaking-api
…RealtimeObjects` to `RealtimeObject` Resolves https://ably.atlassian.net/browse/PUB-2059
VeskeR
force-pushed
the
PUB-2059/lo-entry-point-change
branch
from
eded037 to
e2d9486
Compare
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 (2)
src/plugins/objects/batchcontextlivemap.ts (1)
14-23: Type mismatch: get() returns BatchContext wrappers but d.ts promises LiveObjectAt runtime, when the value is a LiveObject, this method returns a BatchContext wrapper (BatchContextLiveMap/BatchContextLiveCounter). However, ably.d.ts declares the return type as T[TKey] (i.e., LiveMap/LiveCounter or primitive). This will mislead TS consumers and can cause runtime errors (e.g., calling subscribe on a BatchContext wrapper).
Adjust the ably.d.ts signature to return the wrapper types for LiveObjects. See suggested diff in the ably.d.ts comment.
test/realtime/objects.test.js (1)
1007-1010: Fix chained range assertions so the tests actually validate the boundsThe new assertions use
tsBeforeMsg <= value <= tsAfterMsg, but in JavaScript that expression is evaluated left-to-right, turning the first comparison into a boolean (0or1) before the second comparison. As a result these expectations always pass, even whenvaluelies outside the intended range, so the tests no longer verify the behaviour they are supposed to cover.Please compare the timestamp explicitly, e.g.
expect(value).to.be.within(tsBeforeMsg, tsAfterMsg), and do the same for each occurrence in this file.- expect( - tsBeforeMsg <= obj.tombstonedAt() <= tsAfterMsg, - `Check object's "tombstonedAt" value is set using local clock if no "serialTimestamp" provided`, - ).to.be.true; + expect( + obj.tombstonedAt(), + `Check object's "tombstonedAt" value is set using local clock if no "serialTimestamp" provided`, + ).to.be.within(tsBeforeMsg, tsAfterMsg);Also applies to: 1087-1090, 1661-1664, 2190-2193
🧹 Nitpick comments (2)
src/plugins/objects/livemap.ts (1)
173-193: Tighten value validation: reject NaN/Infinity for numeric valuesvalidateKeyValue permits any number, including NaN/Infinity. Elsewhere (LiveCounter.createCounterIncMessage) non-finite numbers are rejected. For consistency and to prevent encoding issues, add a finite check.
Apply:
if ( - value === null || - (typeof value !== 'string' && - typeof value !== 'number' && - typeof value !== 'boolean' && - typeof value !== 'object') + value === null || + (typeof value !== 'string' && + typeof value !== 'number' && + typeof value !== 'boolean' && + typeof value !== 'object') ) { throw new client.ErrorInfo('Map value data type is unsupported', 40013, 400); // OD4a } + if (typeof value === 'number' && !Number.isFinite(value)) { + throw new client.ErrorInfo('Map value must be a finite number', 40003, 400); + }src/plugins/objects/batchcontextlivecounter.ts (1)
30-37: Align decrement validation with increment: ensure finite numbersdecrement() accepts NaN/Infinity (typeof 'number'), only to fail later in increment() via LiveCounter.createCounterIncMessage. Validate finiteness here for consistency.
Apply:
- if (typeof amount !== 'number') { - throw new this._client.ErrorInfo('Counter value decrement should be a number', 40003, 400); - } + if (typeof amount !== 'number' || !Number.isFinite(amount)) { + throw new this._client.ErrorInfo('Counter value decrement should be a valid number', 40003, 400); + }
📜 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 (16)
ably.d.ts(8 hunks)scripts/moduleReport.ts(1 hunks)src/common/lib/client/realtimechannel.ts(8 hunks)src/plugins/objects/batchcontext.ts(4 hunks)src/plugins/objects/batchcontextlivecounter.ts(1 hunks)src/plugins/objects/batchcontextlivemap.ts(2 hunks)src/plugins/objects/index.ts(1 hunks)src/plugins/objects/livecounter.ts(7 hunks)src/plugins/objects/livemap.ts(18 hunks)src/plugins/objects/liveobject.ts(3 hunks)src/plugins/objects/objectspool.ts(5 hunks)src/plugins/objects/realtimeobject.ts(6 hunks)src/plugins/objects/syncobjectsdatapool.ts(2 hunks)test/common/modules/private_api_recorder.js(4 hunks)test/package/browser/template/src/index-objects.ts(2 hunks)test/realtime/objects.test.js(71 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- scripts/moduleReport.ts
- test/package/browser/template/src/index-objects.ts
- src/plugins/objects/objectspool.ts
- src/plugins/objects/realtimeobject.ts
- src/plugins/objects/syncobjectsdatapool.ts
- src/plugins/objects/liveobject.ts
- test/common/modules/private_api_recorder.js
- src/plugins/objects/index.ts
🧰 Additional context used
🧬 Code graph analysis (8)
src/plugins/objects/batchcontextlivemap.ts (5)
ably.d.ts (1)
LiveMapType(2392-2392)src/plugins/objects/batchcontext.ts (1)
BatchContext(11-107)src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(39-529)src/plugins/objects/livemap.ts (1)
LiveMap(48-929)src/plugins/objects/batchcontextlivecounter.ts (1)
value(17-21)
test/realtime/objects.test.js (5)
src/plugins/objects/batchcontextlivecounter.ts (1)
value(17-21)src/plugins/objects/livecounter.ts (1)
value(134-137)src/plugins/objects/batchcontextlivemap.ts (1)
entries(31-35)src/plugins/objects/livemap.ts (1)
entries(321-334)src/plugins/objects/syncobjectsdatapool.ts (1)
entries(36-38)
src/plugins/objects/livecounter.ts (1)
src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(39-529)
src/plugins/objects/livemap.ts (1)
src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(39-529)
src/plugins/objects/batchcontextlivecounter.ts (3)
src/plugins/objects/batchcontext.ts (1)
BatchContext(11-107)src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(39-529)src/plugins/objects/livecounter.ts (1)
LiveCounter(22-367)
src/common/lib/client/realtimechannel.ts (1)
src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(39-529)
ably.d.ts (2)
src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(39-529)src/plugins/objects/batchcontextlivemap.ts (1)
BatchContextLiveMap(7-62)
src/plugins/objects/batchcontext.ts (3)
src/plugins/objects/realtimeobject.ts (1)
RealtimeObject(39-529)src/plugins/objects/batchcontextlivemap.ts (1)
BatchContextLiveMap(7-62)src/plugins/objects/batchcontextlivecounter.ts (1)
BatchContextLiveCounter(6-41)
⏰ 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 (20.x)
- GitHub Check: test-node (18.x)
🔇 Additional comments (2)
src/plugins/objects/batchcontext.ts (1)
18-24: LGTM: BatchContext migrated cleanly to RealtimeObjectConstructor wiring, access/write validation, object wrapping, and publish/flush paths look correct and consistent with RealtimeObject.
Also applies to: 26-30, 40-50, 99-101
src/common/lib/client/realtimechannel.ts (1)
140-143: LGTM: channel.object surface and message handling updated correctlyInstantiation, getter, ATTACHED handling, state transitions, and OBJECT/OBJECT_SYNC routing all correctly use RealtimeObject.
Also applies to: 152-158, 621-637, 801-803
VeskeR
merged commit b7f041b
into
integration/objects-breaking-api
[PUB-2059] Change LiveObjects entry point to `channel.object.get()`
[PUB-2059] Change LiveObjects entry point to `channel.object.get()`
2 participants
Also renames
RealtimeObjectsto `RealtimeObject``Resolves PUB-2059
Summary by CodeRabbit