[AIT-27, PUB-1197] LiveObjects REST client

[VeskeR]

Resolves PUB-1197, AIT-27

Summary by CodeRabbit

• New Features

  • Added REST API support for LiveObjects plugin, enabling access via RestChannel.object
  • Introduced RestObject with get() and publish() methods for REST-based live object operations
  • Extended LiveObjects plugin to support REST channels alongside realtime channels

• Tests

  • Added comprehensive test coverage for REST LiveObjects operations

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

This PR extends LiveObjects plugin with REST client support by relocating the plugin reference from BaseRealtime to BaseClient, implementing a new RestObject class with get/publish methods for REST-based object operations with wire-format encoding/decoding, and providing comprehensive type definitions and test coverage.

Changes

Cohort / File(s)	Summary
Plugin Wiring & Client Integration src/common/lib/client/baseclient.ts, src/common/lib/client/baserealtime.ts	Moved _liveObjectsPlugin field from BaseRealtime to BaseClient to make the plugin available to REST clients; removed import and initialization from BaseRealtime.
Channel RestObject Access src/common/lib/client/realtimechannel.ts, src/common/lib/client/restchannel.ts	Updated RealtimeChannel to reference client._liveObjectsPlugin instead of options path; added public object getter to RestChannel that provides RestObject instance and throws error if plugin unavailable.
RestObject Implementation src/plugins/liveobjects/restobject.ts, src/plugins/liveobjects/index.ts	Introduced new RestObject class with get() and publish() methods supporting wire-format encoding/decoding, robust error handling, and REST API integration; exported from plugin module.
Type Definitions liveobjects.d.ts	Added extensive REST-specific types: RestObject, RestObjectOperation unions, operation types (MapCreate, MapSet, MapRemove, CounterCreate, CounterInc), RestCompactObjectData, RestLiveObject, RestLiveMap, RestLiveCounter, GetObjectParams, PrimitiveOrObjectReference; augmented RestChannel to expose RestObject property.
Test Utilities & Helpers test/common/modules/liveobjects_helper.js, test/common/modules/shared_helper.js	Added async helper methods to LiveObjectsHelper: waitFixtureChannelIsReady, waitForMapKeyUpdate, waitForCounterUpdate, waitForObjectOperation, waitForObjectSync; refactored test wrapper factory in shared_helper.
REST LiveObjects Test Suite test/rest/liveobjects.test.js, test/support/browser_file_list.js	Introduced comprehensive test suite covering RestObject.get() and publish() operations with various data types, object references, error scenarios, and complex workflows; added test file to browser file list.
Request Test Updates test/rest/request.test.js	Converted 'request_version' test callback to async with manual promise coordination for header assertion verification.
Build Configuration scripts/moduleReport.ts	Updated checkLiveObjectsPluginFiles allowlist to include new restobject.ts file in LiveObjects bundle.

Sequence Diagram(s)

sequenceDiagram
    participant RC as RestChannel
    participant RO as RestObject
    participant HC as HTTP Client
    participant Backend as Backend Service

    rect rgba(100, 150, 200, 0.5)
      Note over RC,Backend: RestObject.get() flow
      RC->>RO: get(params?)
      activate RO
      RO->>RO: _basePath(objectId)
      RO->>HC: GET /objects/{path}?compact=...
      activate HC
      HC->>Backend: HTTP GET request
      Backend-->>HC: Response (msgpack/JSON)
      HC-->>RO: Wire format response
      deactivate HC
      RO->>RO: Decode wire format
      alt Is Live Map
        RO->>RO: _decodeWireRestLiveMap()
      else Is Live Object
        RO->>RO: _decodeWireLiveObject()
      else Is Object Data
        RO->>RO: _decodeWireObjectData()
      end
      RO-->>RC: RestLiveObject | undefined
      deactivate RO
    end

    rect rgba(150, 100, 200, 0.5)
      Note over RC,Backend: RestObject.publish() flow
      RC->>RO: publish(op | op[])
      activate RO
      RO->>RO: _constructPublishBody(op, format)
      RO->>RO: Validate operation fields
      RO->>RO: _encodePrimitive() for each value
      RO->>HC: POST /objects/{path}/publish
      activate HC
      HC->>Backend: HTTP POST (encoded operations)
      Backend-->>HC: Response with results
      HC-->>RO: Wire format response
      deactivate HC
      RO->>RO: Decode wire response
      RO-->>RC: RestObjectPublishResult
      deactivate RO
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• [PUB-1242] Rename "LiveObjects" to "Objects" when referencing a channel feature #1982: Related rename/refactor of LiveObjects plugin surfaces and client/channel plugin wiring that touches the same integration points.

Suggested reviewers

• mschristensen

Poem

🐰 REST objects hop and play,
With get and publish here to stay,
Wire formats dance in compact grace,
Maps and counters find their place—
Live data flows through REST's new face! 🎉

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 6.25% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title '[AIT-27, PUB-1197] LiveObjects REST client' directly summarizes the main change: implementing a REST client for LiveObjects. It is concise, specific, and clearly communicates the primary objective.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch PUB-1197/liveobjects-rest-api

📝 Coding Plan

• Generate coding plan for human review comments

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[mschristensen]

LGTM, although I dont think these tests run with the useBinaryProtocol option set to true, given

ably-js/test/common/modules/objects_helper.js

Line 33 in 5f7807f

this._rest = helper.AblyRest({ useBinaryProtocol: false });

Can we try running these tests with msgpack to see if it works? I did find this ticket which may indicate an issue with msgpack over rest currently: https://ably.atlassian.net/browse/PUB-1835

[kaschula]

Something I noticed when testing this branch for Msgpack requests. The SDK currently isn't setting the correct content type header of application/msgpack it was being set to JSON still

[coderabbitai]

Actionable comments posted: 4

♻️ Duplicate comments (3)

test/rest/objects.test.js (2)

214-362: MAP_SET test operations still missing required key field

In several publish tests, the map.set operations are constructed without a key:

const operation = {
  operation: 'map.set',
  path: 'testKey',
  value: 'testValue',
};

and in the batch test:

{
  operation: 'map.set',
  path: 'key1',
  value: 'value1',
},
{
  operation: 'map.set',
  path: 'key2',
  value: 42,
},

Given the earlier review comment that the REST API requires a key for map set and, for setting a key on the root, expects path: '' and key: 'testKey', these operations are still malformed and likely to fail REST validation. The current _constructPublishBody also expects op.key to be present when building data.key.

Recommend updating all root-level map.set operations in these tests along the lines of:

-const operation = {
-  operation: 'map.set',
-  path: 'testKey',
-  value: 'testValue',
-};
+const operation = {
+  operation: 'map.set',
+  path: '',
+  key: 'testKey',
+  value: 'testValue',
+};

and similarly for the batch:

-{
-  operation: 'map.set',
-  path: 'key1',
-  value: 'value1',
-},
+{
+  operation: 'map.set',
+  path: '',
+  key: 'key1',
+  value: 'value1',
+},
-{
-  operation: 'map.set',
-  path: 'key2',
-  value: 42,
-},
+{
+  operation: 'map.set',
+  path: '',
+  key: 'key2',
+  value: 42,
+},

---

366-412: Fix undefined result and align MAP_SET operation shape with API

In the “publish multiple operations and verify state changes” test:

const operation = {
  operation: 'map.set',
  path: 'integrationKey',
  value: 'integrationValue',
};

await channel.object.publish(operation);

// ...

expect(result).to.exist;
expect(result.integrationKey).to.equal('integrationValue');

Issues:

1. result is never defined in this scope; this will throw a ReferenceError. You probably meant to assert against rootResult (retrieved via channel.object.get()).
2. The map.set operation again omits the key field and uses path as if it were the key, conflicting with the REST API shape and the earlier review comment.

Suggested fix:

-        const operation = {
-          operation: 'map.set',
-          path: 'integrationKey',
-          value: 'integrationValue',
-        };
+        const operation = {
+          operation: 'map.set',
+          path: '',
+          key: 'integrationKey',
+          value: 'integrationValue',
+        };
@@
-        // Get root object to verify counter reference
+        // Get root object to verify counter reference and map value
         const rootResult = await channel.object.get();
         expect(rootResult).to.exist;
         expect(rootResult.testCounter).to.exist;
         expect(rootResult.testCounter.objectId).to.equal(counterObjectId);
 
-        expect(result).to.exist;
-        expect(result.integrationKey).to.equal('integrationValue');
+        expect(rootResult.integrationKey).to.equal('integrationValue');

src/plugins/objects/restobject.ts (1)

100-163: Map/counter publish body construction: fix operation names and rely on wire-level enums

Two issues here:

1. Operation name casing / format

The switch uses operation values like 'map.create', 'map.set', 'map.remove', 'counter.create', 'counter.inc', and then forwards op.operation directly to the wire:

const operation = op.operation;
switch (operation) {
  case 'map.create':
    return {
      operation: op.operation,
      // ...
    };
  // ...
}

Per the earlier review comment and REST docs, the wire format expects MAP_CREATE, MAP_SET, MAP_REMOVE, COUNTER_CREATE, COUNTER_INC, etc. If you send the dotted lowercase strings, REST validation will fail.

A robust approach is to map the SDK-level operations to the wire-level enums:

-  private _constructPublishBody(op: RestObjectOperation, format: Utils.Format): any {
-    const operation = op.operation;
+  private _constructPublishBody(op: RestObjectOperation, format: Utils.Format): any {
+    const wireOperationMap: Record<string, string> = {
+      'map.create': 'MAP_CREATE',
+      'map.set': 'MAP_SET',
+      'map.remove': 'MAP_REMOVE',
+      'counter.create': 'COUNTER_CREATE',
+      'counter.inc': 'COUNTER_INC',
+    };
+    const operation = wireOperationMap[op.operation];
@@
-      case 'map.create':
+      case 'MAP_CREATE':
         return {
-          operation: op.operation,
+          operation,
@@
-      case 'map.set':
+      case 'MAP_SET':
         return {
-          operation: op.operation,
+          operation,
@@
-      case 'map.remove':
+      case 'MAP_REMOVE':
@@
-      case 'counter.create':
+      case 'COUNTER_CREATE':
@@
-      case 'counter.inc':
+      case 'COUNTER_INC':
@@
-      default:
+      default:
         throw new this.channel.client.ErrorInfo('Unsupported publish operation action: ' + operation, 40003, 400);

2. Assumptions about map.set shape

The map.set case builds:

data: {
  key: op.key,
  value: {
    ...this._encodePrimitive(op.value, format),
    ...(op.encoding ? { encoding: op.encoding } : {}),
  },
},

This is consistent with the test suite’s more complex usages (e.g. objectId + key), but several tests still construct operations without key and try to overload path. Those tests should be fixed (see comments in test/rest/objects.test.js), rather than loosening this method.

🧹 Nitpick comments (2)

ably.d.ts (1)

2291-2538: REST Objects typings look consistent with runtime, but add property JSDoc to fix Typedoc warnings

The new REST object operation and data types (RestObjectOperation*, RestObject*Data, GetObjectParams, PrimitiveOrObjectReference, and the updated ObjectOperationAction) are coherent and line up with the RestObject implementation (ids, extras, map/counter payloads, and operation action strings).

The remaining issue is just documentation noise from Typedoc for the inline TargetByObjectId/TargetByPath property types. You can keep the current aliases but document the properties explicitly so the API Reference job stops warning:

-/**
- * Targets an object by its object ID.
- */
-type TargetByObjectId = { objectId: string; path?: never };
+/**
+ * Targets an object by its object ID.
+ */
+type TargetByObjectId = {
+  /** The ID of the object to target. Mutually exclusive with `path`. */
+  objectId: string;
+  /** Must not be specified when targeting by `objectId`. */
+  path?: never;
+};
@@
-/**
- * Targets an object by its location using the path.
- * Paths are expressed relative to the structure of the object as defined by the compact view of the object tree.
- */
-type TargetByPath = { path: string; objectId?: never };
+/**
+ * Targets an object by its location using the path.
+ * Paths are expressed relative to the structure of the object as defined by the compact view of the object tree.
+ */
+type TargetByPath = {
+  /** Path to the target, evaluated relative to the entrypoint object. Mutually exclusive with `objectId`. */
+  path: string;
+  /** Must not be specified when targeting by `path`. */
+  objectId?: never;
+};

This should resolve the API Reference warnings without changing the public surface.

Also applies to: 2626-2632, 3952-3960

test/common/modules/objects_helper.js (1)

42-127: Harden transport interception helpers to always restore original handler and add light guards

The new helpers are handy, but the onProtocolMessage interception is a bit fragile:

• In both waitForObjectOperation and waitForObjectSync, if onProtocolMessageOriginal throws, the catch rejects the promise but leaves transport.onProtocolMessage patched, which can cascade into later tests.
• Both helpers assume client.connection.connectionManager.activeProtocol and getTransport() are always available; if a test uses them before the connection is fully established, this will throw in a slightly opaque way.
• waitFixtureChannelIsReady assumes entryPathObject.instance() is always non‑undefined; if object.get() ever returns a PathObject that doesn’t yet resolve to an instance, entryInstance.get(key) will throw.

Consider a small defensive refactor that keeps behavior but makes these helpers safer in failing tests, for example:

-        const transport = client.connection.connectionManager.activeProtocol.getTransport();
-        const onProtocolMessageOriginal = transport.onProtocolMessage;
+        const activeProtocol = client.connection.connectionManager.activeProtocol;
+        if (!activeProtocol) {
+          return reject(new Error('No active protocol when waiting for object operation'));
+        }
+        const transport = activeProtocol.getTransport();
+        const onProtocolMessageOriginal = transport.onProtocolMessage;
@@
-        transport.onProtocolMessage = function (message) {
-          try {
-            helper.recordPrivateApi('call.transport.onProtocolMessage');
-            onProtocolMessageOriginal.call(transport, message);
-
-            if (message.action === 19 && message.state[0]?.operation?.action === waitForAction) {
-              helper.recordPrivateApi('replace.transport.onProtocolMessage');
-              transport.onProtocolMessage = onProtocolMessageOriginal;
-              resolve();
-            }
-          } catch (err) {
-            reject(err);
-          }
-        };
+        transport.onProtocolMessage = function (message) {
+          try {
+            helper.recordPrivateApi('call.transport.onProtocolMessage');
+            onProtocolMessageOriginal.call(transport, message);
+
+            if (message.action === 19 && message.state[0]?.operation?.action === waitForAction) {
+              resolve();
+            }
+          } catch (err) {
+            reject(err);
+          } finally {
+            // Always restore the original handler once this helper completes
+            helper.recordPrivateApi('replace.transport.onProtocolMessage');
+            transport.onProtocolMessage = onProtocolMessageOriginal;
+          }
+        };

and similarly for waitForObjectSync. Optionally, you could also guard entryInstance in waitFixtureChannelIsReady and fail fast with a descriptive error if it can’t be resolved.

These changes would make the tests more robust without changing their intended behavior.

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 464d6b4 and 568d0a0.

📒 Files selected for processing (11)

• ably.d.ts (3 hunks)
• objects.d.ts (2 hunks)
• src/common/lib/client/baseclient.ts (3 hunks)
• src/common/lib/client/baserealtime.ts (0 hunks)
• src/common/lib/client/realtimechannel.ts (1 hunks)
• src/common/lib/client/restchannel.ts (4 hunks)
• src/plugins/objects/index.ts (2 hunks)
• src/plugins/objects/restobject.ts (1 hunks)
• test/common/modules/objects_helper.js (1 hunks)
• test/realtime/objects.test.js (1 hunks)
• test/rest/objects.test.js (1 hunks)

💤 Files with no reviewable changes (1)

• src/common/lib/client/baserealtime.ts

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

• src/common/lib/client/realtimechannel.ts
• src/common/lib/client/baseclient.ts
• src/common/lib/client/restchannel.ts

🧰 Additional context used

🧬 Code graph analysis (4)

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

test/rest/objects.test.js (1)

• waitFixtureChannelIsReady (13-13)

test/rest/objects.test.js (2)

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

• object (153-158)

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

• object (71-76)

src/plugins/objects/restobject.ts (1)

ably.d.ts (6)

• GetObjectParams (2493-2500)
• RestCompactObjectData (2407-2415)
• RestLiveObject (2421-2421)
• RestObjectOperation (2380-2385)
• RestObjectPublishResult (2391-2401)
• PrimitiveOrObjectReference (2632-2632)

ably.d.ts (1)

src/plugins/objects/restobject.ts (1)

• RestObject (12-184)

🪛 GitHub Actions: API Reference

ably.d.ts

[warning] 1-1: Typedoc: ably.TargetByObjectId.__type.objectId does not have any documentation.

---

[warning] 1-1: Typedoc: ably.TargetByObjectId.__type.path does not have any documentation.

---

[warning] 1-1: Typedoc: ably.TargetByPath.__type.path does not have any documentation.

---

[warning] 1-1: Typedoc: ably.TargetByPath.__type.objectId does not have any documentation.

🪛 GitHub Actions: Bundle size report

src/plugins/objects/restobject.ts

[error] 1-1: Unexpected file src/plugins/objects/restobject.ts, contributes 2332B to bundle, more than allowed 100B

⏰ 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 (20.x)
• GitHub Check: test-node (16.x)
• GitHub Check: test-node (18.x)

🔇 Additional comments (8)

objects.d.ts (1)

3-12: Objects plugin docs align with new REST support

The updated imports and JSDoc correctly describe usage with both Realtime/Rest and modular BaseRealtime/BaseRest (including the Rest plugin requirement). No issues from a typings or behavior perspective.

Also applies to: 46-63

src/plugins/objects/index.ts (1)

5-22: RestObject export wiring looks correct

Importing RestObject from ./restobject and exposing it in both the named and default exports matches how the other Objects types are surfaced; nothing further needed here.

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

20-24: Reusing shared wait helpers from ObjectsHelper looks good

Aliasing these helpers from ObjectsHelper keeps this test file DRY and consistent with the REST tests. As long as the underlying helpers don’t depend on a this context (which appears true given their other usages), this change is sound and should be behavior‑preserving.

test/rest/objects.test.js (3)

47-55: Channel .object error test looks good

The “Rest without Objects plugin” test correctly asserts that accessing channel.object without the plugin throws with the expected message, matching the Utils.throwMissingPluginError('Objects') behavior from restchannel.ts.

---

66-210: RestObject.get() test coverage is comprehensive and consistent

The RestObject.get() suite (non-existent IDs/paths, root default, compact true/false behaviors, objectId and path variants, primitive type handling, and binary checks via BufferUtils) is thorough and matches the intended API semantics.

These tests should give good confidence that RestObject.get and the server responses behave correctly once the fixtures are in place.

---

415-477: Complex workflow test is solid, but depends on correct object-reference encoding

This test does a good job of exercising a realistic workflow (map.create, map.set, counter.create, then wiring a counter reference into the map) and verifying both compact and expanded views.

Note that the expectation:

expect(mapContent.map.entries.counter.data.objectId).to.equal(counterObjectId);

assumes that value: { objectId: counterObjectId } in the map.set operation is preserved as an object reference on the wire, not JSON-stringified. This needs RestObject._encodePrimitive to special‑case { objectId: string } so that the server sees it as a reference rather than generic JSON.

src/plugins/objects/restobject.ts (2)

19-56: get() flow looks reasonable; ensure headers / format match existing REST patterns

The get implementation follows the existing client patterns (choosing format from useBinaryProtocol, using envelope based on supportsLinkHeaders, and decoding via Utils.decodeBody). The 404 handling via isErrorInfoOrPartialErrorInfo and error.code === 40400 is a good way to surface “not found” as undefined.

Given the msgpack-related concerns on this branch, it’s worth double‑checking that Defaults.defaultGetHeaders(client.options) sets an appropriate Accept header (and any other necessary flags) when useBinaryProtocol is true, in the same way as other REST GETs in the SDK, so that the server returns msgpack rather than JSON.

---

1-184: Address bundlesize failure for restobject.ts

The bundlesize check reports:

Unexpected file src/plugins/objects/restobject.ts, contributes 2332B to bundle, more than allowed 100B

This new file is clearly larger than the default allowance. You’ll need to either:

• Update the bundlesize configuration to include this file with an appropriate budget, or
• Restructure / split code (e.g. moving shared helpers or types) so that the per‑file contribution is within the configured threshold.

Given this is core new functionality, adjusting the bundlesize config to account for it is likely the pragmatic fix.

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

objects.d.ts (1)

67-67: Fix TypeScript export incompatibility.

TypeScript does not allow export = (CommonJS-style export assignment) to coexist with named exports like the LiveMap and LiveCounter classes defined earlier in this file. This is causing the pipeline failure.

Consider one of these solutions:

Solution 1 (recommended): Use default export

-declare const Objects: any;
-
-export = Objects;
+export default Objects;
+declare const Objects: any;

Solution 2: Remove named exports and export everything through the default
Move LiveMap and LiveCounter class declarations to be properties of the Objects object instead of separate named exports.

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

136-142: Consider caching the Push plugin as an internal field for consistency.

The Objects plugin is now cached as client._objectsPlugin, following the pattern established for other core plugins (Crypto, Annotations). However, Push continues to be accessed directly via client.options.plugins?.Push throughout the codebase. For consistency and potential performance benefits, consider caching Push similarly—e.g., as client._pushPlugin in BaseClient's constructor, similar to line 108.

♻️ Duplicate comments (3)

test/rest/objects.test.js (1)

15-21: Add default parameter for options to prevent runtime errors.

Both helper functions spread options directly, which will throw a TypeError if called without the second argument. Several call sites (e.g., lines 108, 142, 208, 257) pass only the helper argument via RealtimeWithObjects(helper, options) where options comes from the test callback - this works, but if ever called without options it would fail.

-function RealtimeWithObjects(helper, options) {
-  return helper.AblyRealtime({ ...options, plugins: { Objects: ObjectsPlugin } });
-}
-
-function RestWithObjects(helper, options) {
-  return helper.AblyRest({ ...options, plugins: { Objects: ObjectsPlugin } });
-}
+function RealtimeWithObjects(helper, options = {}) {
+  return helper.AblyRealtime({ ...options, plugins: { Objects: ObjectsPlugin } });
+}
+
+function RestWithObjects(helper, options = {}) {
+  return helper.AblyRest({ ...options, plugins: { Objects: ObjectsPlugin } });
+}

src/plugins/objects/restobject.ts (2)

125-125: Pass format parameter to defaultPostHeaders() to fix Content-Type for MsgPack.

The publish() method calls defaultPostHeaders(client.options) without passing the format parameter. This causes the Content-Type header to default to JSON regardless of the useBinaryProtocol setting, while the body is encoded as msgpack on line 133. This is the root cause of the MsgPack Content-Type issue reported in the PR comments.

-    const headers = client.Defaults.defaultPostHeaders(client.options);
+    const headers = client.Defaults.defaultPostHeaders(client.options, { format });

---

152-158: Change path segment from /object to /objects.

The REST API endpoint for LiveObjects is /channels/{channelName}/objects (plural), but _basePath constructs paths with /object (singular). This mismatch will cause all REST API calls to fail with 404 errors.

     return (
       this._channel.client.rest.channelMixin.basePath(this._channel) +
-      '/object' +
+      '/objects' +
       (objectId ? '/' + encodeURIComponent(objectId) : '')
     );

🧹 Nitpick comments (8)

test/rest/request.test.js (1)

35-57: Consider adding a comment to explain the promise coordination pattern.

The manual promise creation (lines 35-40) and the non-settling promise return (line 50) form a coordination pattern that ensures assertions complete before the test finishes. While this works correctly, a brief comment would help future maintainers understand why this approach is necessary.

Consider adding a comment like:

       let savedResolve;
       let savedReject;
+      // Manual promise coordination: testRequestHandler returns a non-settling promise
+      // to keep the request "in progress" while we control test completion via savedResolve
       let promise = new Promise((res, rej) => {

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

20-24: Centralising wait helpers via ObjectsHelper looks good

Pulling waitFixtureChannelIsReady / waitFor* from ObjectsHelper keeps waiter behaviour consistent across test suites and removes local duplication. The alias pattern is fine here as long as those helpers don’t rely on this; if they ever do, consider binding them (ObjectsHelper.waitForMapKeyUpdate.bind(ObjectsHelper), etc.) as a small hardening step.

test/rest/objects.test.js (2)

108-108: Close Realtime client after use to prevent resource leaks.

The RealtimeWithObjects client created here is used only to wait for the fixture channel to be ready but is never closed. This pattern repeats at lines 142, 208, and 257. Consider storing the client and closing it after the wait completes.

-          await waitFixtureChannelIsReady(RealtimeWithObjects(helper, options), objectsFixturesChannel);
+          const realtimeClient = RealtimeWithObjects(helper, options);
+          await waitFixtureChannelIsReady(realtimeClient, objectsFixturesChannel);
+          realtimeClient.close();

---

279-292: Consider moving primitiveKeyData definition before its first usage.

The primitiveKeyData array is used at line 262 inside the RestObject.get() tests but defined here after those tests. While this works due to hoisting, moving the definition before its first usage (before line 250) would improve readability.

test/common/modules/objects_helper.js (2)

93-93: Replace magic numbers with named constants for clarity.

The action values 19 and 20 are protocol message action codes. Consider defining constants for these (e.g., OBJECT_ACTION = 19, OBJECT_SYNC_ACTION = 20) to improve readability and maintainability.

+  static OBJECT_ACTION = 19;
+  static OBJECT_SYNC_ACTION = 20;
+
   static async waitForObjectOperation(helper, client, waitForAction) {
     // ...
-          if (message.action === 19 && message.state[0]?.operation?.action === waitForAction) {
+          if (message.action === ObjectsHelper.OBJECT_ACTION && message.state[0]?.operation?.action === waitForAction) {

Also applies to: 117-117

---

81-127: Consider extracting shared transport interception logic.

Both waitForObjectOperation and waitForObjectSync follow the same pattern: get transport, replace onProtocolMessage, restore on match. The only difference is the condition. A shared helper could reduce duplication, though this is minor for test utilities.

src/plugins/objects/restobject.ts (2)

86-91: Simplify redundant condition check.

The format variable is always truthy (either 'msgpack' or 'json' from line 70), so the if (format) condition at line 87 will always be true. The else branch is unreachable.

-      let decodedBody: RestCompactObjectData | WireRestLiveObject | undefined;
-      if (format) {
-        decodedBody = client.Utils.decodeBody(response.body, client._MsgPack, format);
-      } else {
-        decodedBody = response.body;
-      }
+      const decodedBody = client.Utils.decodeBody(response.body, client._MsgPack, format);

---

145-149: Simplify redundant condition check in publish().

Similar to the get() method, format is always truthy here, making the condition unnecessary.

-    if (format) {
-      return client.Utils.decodeBody(response.body, client._MsgPack, format);
-    }
-
-    return response.body!;
+    return client.Utils.decodeBody(response.body, client._MsgPack, format);

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 568d0a0 and 879347f.

📒 Files selected for processing (13)

• ably.d.ts (3 hunks)
• objects.d.ts (2 hunks)
• src/common/lib/client/baseclient.ts (3 hunks)
• src/common/lib/client/baserealtime.ts (0 hunks)
• src/common/lib/client/realtimechannel.ts (1 hunks)
• src/common/lib/client/restchannel.ts (4 hunks)
• src/plugins/objects/index.ts (2 hunks)
• src/plugins/objects/restobject.ts (1 hunks)
• test/common/modules/objects_helper.js (1 hunks)
• test/common/modules/shared_helper.js (1 hunks)
• test/realtime/objects.test.js (4 hunks)
• test/rest/objects.test.js (1 hunks)
• test/rest/request.test.js (2 hunks)

💤 Files with no reviewable changes (1)

• src/common/lib/client/baserealtime.ts

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

• src/common/lib/client/baseclient.ts

🧰 Additional context used

🧬 Code graph analysis (5)

test/rest/request.test.js (2)

test/rest/http.test.js (3)

• helper (11-11)
• helper (27-27)
• helper (72-72)

test/rest/status.test.js (2)

• helper (11-11)
• rest (38-38)

src/plugins/objects/restobject.ts (1)

ably.d.ts (8)

• ObjectsMapSemantics (3975-3975)
• GetObjectParams (2493-2500)
• RestLiveObject (2421-2421)
• RestObjectOperation (2380-2385)
• RestObjectPublishResult (2391-2401)
• PrimitiveOrObjectReference (2632-2632)
• RestObjectMapEntry (2441-2444)
• RestObjectData (2450-2463)

test/rest/objects.test.js (2)

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

• object (153-158)

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

• object (71-76)

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

src/plugins/objects/restobject.ts (1)

• RestObject (59-354)

src/plugins/objects/index.ts (1)

• RestObject (12-12)

ably.d.ts (1)

src/plugins/objects/restobject.ts (1)

• RestObject (59-354)

🪛 GitHub Actions: API Reference

ably.d.ts

[warning] 1-1: Typedoc warning: ably.TargetByObjectId.__type.objectId does not have any documentation.

---

[warning] 1-1: Typedoc warning: ably.TargetByObjectId.__type.path does not have any documentation.

---

[warning] 1-1: Typedoc warning: ably.TargetByPath.__type.path does not have any documentation.

---

[warning] 1-1: Typedoc warning: ably.TargetByPath.__type.objectId does not have any documentation.

---

[warning] 1-1: Typedoc warning: ably.OperationMapCreate.__type.operation does not have any documentation.

---

[warning] 1-1: Typedoc warning: ably.OperationMapCreate.__type.entries does not have any documentation.

---

[warning] 1-1: Typedoc warning: ably.OperationMapSet.__type.operation does not have any documentation.

---

[warning] 1-1: Typedoc warning: ably.OperationMapSet.__type.key does not have any documentation.

---

[warning] 1-1: Typedoc warning: ably.OperationMapSet.__type.value does not have any documentation.

---

[warning] 1-1: Typedoc warning: ably.OperationMapSet.__type.encoding does not have any documentation.

---

[warning] 1-1: Typedoc warning: ably.OperationMapRemove.__type.operation does not have any documentation.

---

[warning] 1-1: Typedoc warning: ably.OperationMapRemove.__type.key does not have any documentation.

---

[warning] 1-1: Typedoc warning: ably.OperationCounterCreate.__type.operation does not have any documentation.

---

[warning] 1-1: Typedoc warning: ably.OperationCounterCreate.__type.count does not have any documentation.

---

[warning] 1-1: Typedoc warning: ably.OperationCounterInc.__type.operation does not have any documentation.

---

[warning] 1-1: Typedoc warning: ably.OperationCounterInc.__type.amount does not have any documentation.

---

[warning] 1-1: Typedoc warning: ably.RestCompactObjectData.__type.objectId does not have any documentation.

---

[warning] 1-1: Typedoc warning: ably.RestObject.get.params.__type.compact does not have any documentation.

---

[warning] 1-1: Typedoc warning: ably.PrimitiveOrObjectReference.__type.objectId does not have any documentation.

🪛 GitHub Actions: Bundle size report

src/plugins/objects/restobject.ts

[error] 1-1: Unexpected file src/plugins/objects/restobject.ts, contributes 4040B to bundle, more than allowed 100B. Command failed during 'npm run modulereport' (exit code 1).

🪛 GitHub Actions: Test NPM package

objects.d.ts

[error] 67-67: TS2309: An export assignment cannot be used in a module with other exported elements.

⏰ 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 (6)

objects.d.ts (1)

8-8: Documentation updates look good.

The additions of RestClient, BaseRest, and Rest to the imports and documentation accurately reflect the expanded REST support for Objects functionality.

Also applies to: 11-11, 47-63

test/common/modules/shared_helper.js (1)

385-393: LGTM! Clean refactoring for async test support.

The createTest helper unifies test invocation and enables passing the helper as a third argument to test functions. The async-only approach is appropriate for testOnJsonMsgpack, which differs from testOnAllTransportsAndProtocols that supports both callback and promise styles.

src/plugins/objects/index.ts (1)

5-5: LGTM! RestObject export added correctly.

The RestObject import and export follow the same pattern as existing exports in this file.

Also applies to: 12-12, 21-21

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

433-433: Reusing waitFixtureChannelIsReady from ObjectsHelper is an appropriate refactor

Switching these scenarios to await waitFixtureChannelIsReady(client, objectsFixturesChannel); keeps fixture-channel readiness logic in one place and preserves the existing call shape, so the tests remain easy to follow.

Also applies to: 605-605, 631-631

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

21-21: LGTM!

The RestObject integration follows the established plugin patterns used for Push and Annotations. The lazy initialization in the constructor and the defensive getter that throws when the plugin is missing are consistent with the existing codebase conventions.

Also applies to: 38-38, 52-54, 71-76

src/plugins/objects/restobject.ts (1)

250-256: LGTM on object reference handling.

The _encodePrimitive method now correctly handles object references by checking for the objectId property before falling back to JSON stringification. This addresses the previously flagged issue where object references were incorrectly being JSON-encoded.

[VeskeR]

Addressed all feedback. This ended up as a significant refactor of the public types and internal encoding/decoding logic to reuse as much as possible from the realtime ObjectMessage pipeline rather than duplicating it in the REST client.

I've split the changes into separate commits for easier review. I intend to keep every commit up to and including 73e60cf as-is, and squash everything after it once reviewed.

[VeskeR]

Expected test failures from:

"compact: false" returns all data types correctly

Due to the REST API bug, see https://ably-real-time.slack.com/archives/C09SY1AQGK0/p1773250535110709. Tested locally without empty bytes, test passes.

UPDATE: seems to be fixed

[VeskeR]

Also addressed Mike's feedback for the docs PR (ably/docs#3258 (comment)) regarding correct channel object terminology (do not use "root") and referring to "non-compact" responses as "full" everywhere. See 8d25ece

[lawrence-forooghian]

LGTM — happy to resolve / approve once commits sorted

[VeskeR]

LGTM — happy to resolve / approve once commits sorted

Cleaned up commit history