Skip to content

Merge main into integration/liveobjects branch 2025-04-16 #2006

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 81 commits into from
Apr 17, 2025

Conversation

VeskeR
Copy link
Contributor

@VeskeR VeskeR commented Apr 16, 2025

This PR merges latest changes from the main branch into the integration/liveobjects branch. Most of the changes were trivial to resolve, as LiveObjects changes are mostly contained in a separate plugin and do not interfere with the core library.

The merge was split into three separate merge steps to isolate a single PR (message types refactoring) that had the most conflicting changes. Most notable changes when resolving conflicts for that PR:

  • applied changes from 4b51773 to split decoding functions in basemessage.ts to use them in Objects plugin
  • applied changes from f4073b9 to split encoding and encrypting functions in basemessage.ts to use them in Objects plugin

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Introduced comprehensive support for message annotations, including publishing, subscribing, retrieving, and deleting annotations on channels via both Realtime and REST APIs.
    • Added annotation summaries and new channel modes for annotation publish/subscribe.
    • Enhanced type definitions and modular plugin system to support annotations and encrypted presence messages.
    • Added new React hook (useChannelAttach) and improved channel/presence management in React Hooks.
  • Bug Fixes

    • Fixed connectivity check logic to honor the disableConnectivityCheck option in browser environments.
    • Corrected error codes for connection failures with invalid credentials.
  • Documentation

    • Updated README and changelog to reflect new features and recent changes, including annotation support and usage examples for v2 of the library.
  • Tests

    • Added extensive tests for annotations, encrypted presence message decoding, and new presence/channel behaviors.
    • Improved and extended test utilities for async flows and error handling.
  • Refactor

    • Refactored message, presence, and annotation handling for clarity, type safety, and async/await usage.
    • Streamlined internal APIs and modular plugin architecture.
  • Chores

    • Updated package version to 2.6.5.
    • Updated CI workflows and code style rules.

SimonWoolf and others added 30 commits December 6, 2024 20:05
Previously, fromValues() took an options object with a 'stringifyAction'
bool property. If that was false, then it acted as a normal fromValues;
if true, then it was effectively doing a partial fromDeserialized,
parsing a wire protocol message into a message but without decoding
the message.data.

This seemed a bit silly. Removed the options object and replaced it with
an explicit Message.fromWireProtocol.

Also while doing that ended up changing a bunch of other things that
seemed broken:
- unnecessary type-assertions due to not having a wire-protocol type, or
  unnecessary use of any & unknown
- channeloptions being type-asserted as cipheroptions, then type-assert
  back to channeloptions, which made no sense
- standalone PresenceMessage.fromEncoded() methods were not passing in a
  Crypto object, so wouldn't be able to decrypt and encrypted presence
  message
Tests for TM2p pending serverside implementation of field name changes
being deployed
Realtime is now returning 40101 (invalid credentials) when unable to
extract an app ID from the key. The Realtime team have confirmed that
this new behaviour is correct and that the test should be updated [1].

[1] https://ably-real-time.slack.com/archives/CURL4U2FP/p1733479167314059?thread_ts=1733420816.469159&cid=CURL4U2FP
This was previously untested.

The new test in `crypto.test.js` is taken from a test Simon added
in #1923, but I wanted to separate this test from all the other work that’s
being done in that PR. I’ve also copied this new test to create a
similar test for the modular variant of the library.

Co-authored-by: Simon Woolf <[email protected]>
Mutable message field changes and Message/PresenceMessage.fromValues refactoring
chore: add release information for 2.6.0
Test that we can decrypt history messages
The test waits to receive an end message on a channel then it runs the
assertions, this can cause flakes because we dont know if the event loop
had any other pending IO which could include delivering the 2 expected
filtered messages. Run the assertions in a setImmediate so that it executes
in the next event loop run allowing any queued messages to be delivered.

When running the test with logging the messages were being delivered but
the handler was being called after the assertions had ran.

RAR-656
it's not a channel mode. why is it in here?
- extract common functionality to a new BaseMessage file
- introduce proper WireMessage and WirePresenceMessage classes, such that:
  - you just call Message.encode() to get a WireMessage, and
    WireMessage.decode() to get a Message, and the type system will tell
    you which one you have
  - the toJSON override hack only applies to internal wire-protocol uses
    and doesn't interfere with the normal public Message/PresenceMessage
    classes
  - all necessary transformations between userfriendly message and wire
    protocol messages (encoding the data and the action)
    are now done by decode() and encode(), rather than (as before) the
    encoding happening in-place and the action encoding happening only
    on json-stringification
  - ProtocolMessage is now always a wire-protocol message and always
    holds WireMessages and WirePresence; all decoding is done by the
    channel
  - The PresencePlugin interface is now just the PresenceMessage and
    WirePresenceMessage types, rather than ad-hoc individual functions
  - The fromWireProtocol, encode, and decode functions in the
    DefaultMessage interface are no longer needed and can be removed
- Cleaned up and simplified the message decoding step in the channel, which
  was unnecessarily complicated
Restructure and clean up message and presencemessage types
- Update CHANGELOG
- Update Ably package version
CHORE: Add release information for v.2.6.1
Message: fix parent-populated version not being propagated to serial
Chore: Release version 2.6.2
Because of implicit `attach()` hooks can produce additional errors e.g. when updating ably client

to avoid this situation, channels created inside `ChannelProvider` now use `attachOnSubscribe = false` flag and attach is happening explicitly.
[ECO-5184] feat: update channel hooks implementation
It seems like the Realtime connection key format, which as the removed
comment implies is not guaranteed to be stable, has changed on sandbox.
Stop using our knowledge of it.
@github-actions github-actions bot temporarily deployed to staging/pull/2006/features April 16, 2025 09:23 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/2006/bundle-report April 16, 2025 09:23 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/2006/typedoc April 16, 2025 09:23 Inactive
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
src/common/lib/types/basemessage.ts (1)

13-24: Replace Function type with explicit function signatures for better type safety.

Using the generic Function type masks the specific function signature and reduces type safety. This was flagged by static analysis tools and appears in a previous review as well.

Apply this diff to define more explicit function types:

 export type CipherOptions = {
   channelCipher: {
-    encrypt: Function;
+    encrypt: (data: ArrayBuffer | Uint8Array) => Promise<ArrayBuffer | Uint8Array>;
     algorithm: 'aes';
   };
   cipher?: {
     channelCipher: {
-      encrypt: Function;
+      encrypt: (data: ArrayBuffer | Uint8Array) => Promise<ArrayBuffer | Uint8Array>;
       algorithm: 'aes';
     };
   };
 };
🧰 Tools
🪛 Biome (1.9.4)

[error] 15-15: Don't use 'Function' as a type.

Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.

(lint/complexity/noBannedTypes)


[error] 20-20: Don't use 'Function' as a type.

Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.

(lint/complexity/noBannedTypes)

🧹 Nitpick comments (9)
src/common/lib/types/basemessage.ts (9)

36-45: Use type predicate for safer type narrowing in normaliseContext.

The current implementation uses a type assertion (as EncodingDecodingContext) which bypasses TypeScript's type checking. Using a type predicate would provide better type safety.

Consider using a type predicate instead:

-function normaliseContext(context: CipherOptions | EncodingDecodingContext | ChannelOptions): EncodingDecodingContext {
-  if (!context || !(context as EncodingDecodingContext).channelOptions) {
+function normaliseContext(context: CipherOptions | EncodingDecodingContext | ChannelOptions): EncodingDecodingContext {
+  function isEncodingDecodingContext(ctx: any): ctx is EncodingDecodingContext {
+    return ctx && 'channelOptions' in ctx;
+  }
+  
+  if (!context || !isEncodingDecodingContext(context)) {
     return {
       channelOptions: context as ChannelOptions,
       plugins: {},
       baseEncodedPreviousPayload: undefined,
     };
   }
-  return context as EncodingDecodingContext;
+  return context;
 }

47-61: Use optional chaining for better code readability.

The conditional check on line 52 could benefit from optional chaining to make the code more concise.

Apply this diff to use optional chaining:

 export function normalizeCipherOptions(
   Crypto: IUntypedCryptoStatic | null,
   logger: Logger,
   options: API.ChannelOptions | null,
 ): ChannelOptions {
-  if (options && options.cipher) {
+  if (options?.cipher) {
     if (!Crypto) Utils.throwMissingPluginError('Crypto');
     const cipher = Crypto.getCipher(options.cipher, logger);
     return {
       cipher: cipher.cipherParams,
       channelCipher: cipher.cipher,
     };
   }
   return options ?? {};
 }
🧰 Tools
🪛 Biome (1.9.4)

[error] 52-52: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


93-107: Inconsistency in async/await pattern in encode function.

The encode function awaits encodeData result implicitly through destructuring assignment but doesn't use await. This could lead to unexpected behavior if encodeData is modified to be asynchronous in the future.

Apply this diff for consistency:

 export async function encode<T extends BaseMessage>(msg: T, options: unknown): Promise<T> {
-  const { data, encoding } = encodeData(msg.data, msg.encoding);
+  const { data, encoding } = await Promise.resolve(encodeData(msg.data, msg.encoding));
   msg.data = data;
   msg.encoding = encoding;

   if (options != null && (options as CipherOptions).cipher) {
     return encrypt(msg, options as CipherOptions);
   } else {
     return msg;
   }
 }

133-135: Missing consistent error code pattern.

The error thrown on line 134 uses a hardcoded error code (40013). Consider extracting common error codes to constants for better maintainability.

Consider creating an error code enumeration or constants:

+// At the top of the file with other imports
+export enum ErrorCodes {
+  UNSUPPORTED_DATA_TYPE = 40013,
+  VCDIFF_DECODER_MISSING = 40019,
+  DELTA_DECODING_NOT_SUPPORTED = 40020,
+  VCDIFF_DELTA_DECODE_FAILED = 40018,
+  UNEXPECTED_ACTION = 40000,
+}

 // Then in the code
-  throw new ErrorInfo('Data type is unsupported', 40013, 400);
+  throw new ErrorInfo('Data type is unsupported', ErrorCodes.UNSUPPORTED_DATA_TYPE, 400);

152-271: Optimize conditional chain in decodeData function for readability.

The decoding logic in the decodeData function contains deeply nested conditions and switch cases that could be simplified for better readability.

Consider extracting the individual decoder implementations into separate functions to reduce nesting and improve maintainability:

 export async function decodeData(
   data: any,
   encoding: string | null | undefined,
   inputContext: CipherOptions | EncodingDecodingContext | ChannelOptions,
 ): Promise<{
   error?: ErrorInfo;
   data: any;
   encoding: string | null | undefined;
 }> {
   const context = normaliseContext(inputContext);
   let lastPayload = data;
   let decodedData = data;
   let finalEncoding = encoding;
   let decodingError: ErrorInfo | undefined;

   if (encoding) {
     const xforms = encoding.split('/');
     let lastProcessedEncodingIndex;
     let encodingsToProcess = xforms.length;
     let xform = '';

     try {
       while ((lastProcessedEncodingIndex = encodingsToProcess) > 0) {
         // eslint-disable-next-line security/detect-unsafe-regex
         const match = xforms[--encodingsToProcess].match(/([-\w]+)(\+([\w-]+))?/);
         if (!match) break;
         xform = match[1];
+        const decoderResult = await processTransform(xform, match, decodedData, context, lastProcessedEncodingIndex, xforms.length);
+        decodedData = decoderResult.data;
+        if (decoderResult.lastPayload) {
+          lastPayload = decoderResult.lastPayload;
+        }
-        switch (xform) {
-          case 'base64':
-            decodedData = Platform.BufferUtils.base64Decode(String(decodedData));
-            if (lastProcessedEncodingIndex == xforms.length) {
-              lastPayload = decodedData;
-            }
-            continue;
-          // ... rest of the switch cases
-        }
       }
     } catch (e) {
       // ... error handling
     }
   }
   // ... rest of the function
 }

+async function processTransform(
+  xform: string, 
+  match: RegExpMatchArray, 
+  data: any, 
+  context: EncodingDecodingContext,
+  lastProcessedEncodingIndex: number,
+  totalEncodings: number
+): Promise<{ data: any; lastPayload?: any }> {
+  switch (xform) {
+    case 'base64': {
+      const decoded = Platform.BufferUtils.base64Decode(String(data));
+      return {
+        data: decoded,
+        lastPayload: lastProcessedEncodingIndex === totalEncodings ? decoded : undefined
+      };
+    }
+    // ... other cases extracted from the original switch
+  }
+  throw new Error('Unknown encoding');
+}
🧰 Tools
🪛 Biome (1.9.4)

[error] 197-198: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


196-201: Use optional chaining for nested condition checks.

The multiple conditions checking for non-null properties could be simplified using optional chaining.

Apply this diff to use optional chaining:

           case 'cipher':
             if (
-              context.channelOptions != null &&
-              context.channelOptions.cipher &&
-              context.channelOptions.channelCipher
+              context.channelOptions?.cipher &&
+              context.channelOptions?.channelCipher
             ) {
               const xformAlgorithm = match[3],
                 cipher = context.channelOptions.channelCipher;
🧰 Tools
🪛 Biome (1.9.4)

[error] 197-198: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


213-214: Add more informative guidance for missing Vcdiff decoder.

The error message for a missing Vcdiff decoder could be enhanced with more context about how to install or enable it.

Enhance the error message with installation instructions:

           case 'vcdiff':
             if (!context.plugins || !context.plugins.vcdiff) {
-              throw new ErrorInfo('Missing Vcdiff decoder (https://github.com/ably-forks/vcdiff-decoder)', 40019, 400);
+              throw new ErrorInfo(
+                'Missing Vcdiff decoder. Please install the vcdiff-decoder plugin from https://github.com/ably-forks/vcdiff-decoder or use "npm install @ably/vcdiff-decoder"',
+                40019,
+                400
+              );
             }

326-363: Improve type safety in populateFieldsFromParent function.

The populateFieldsFromParent function uses non-null assertions (!) when accessing potentially undefined properties of the parent protocol message.

Add type guards to improve safety:

 export function populateFieldsFromParent(parent: ProtocolMessage) {
   const { id, connectionId, timestamp } = parent;

   let msgs: BaseMessage[];
   switch (parent.action) {
     case actions.MESSAGE: {
+      if (!parent.messages) {
+        throw new ErrorInfo('Protocol message with MESSAGE action must have messages', 40000, 400);
+      }
       msgs = parent.messages!;
       break;
     }
     case actions.PRESENCE:
     case actions.SYNC:
+      if (!parent.presence) {
+        throw new ErrorInfo('Protocol message with PRESENCE/SYNC action must have presence', 40000, 400);
+      }
       msgs = parent.presence!;
       break;
     case actions.ANNOTATION:
+      if (!parent.annotations) {
+        throw new ErrorInfo('Protocol message with ANNOTATION action must have annotations', 40000, 400);
+      }
       msgs = parent.annotations!;
       break;
     case actions.OBJECT:
     case actions.OBJECT_SYNC:
+      if (!parent.state) {
+        throw new ErrorInfo('Protocol message with OBJECT/OBJECT_SYNC action must have state', 40000, 400);
+      }
       msgs = parent.state!;
       break;
     default:
       throw new ErrorInfo('Unexpected action ' + parent.action, 40000, 400);
   }

386-395: Add JSDoc comments to the BaseMessage class.

The BaseMessage abstract class lacks documentation that would help developers understand its purpose and usage.

Add JSDoc comments to describe the class and its properties:

+/**
+ * Abstract base class representing a message in the Ably protocol.
+ * Provides common properties and functionality for all message types.
+ */
 export abstract class BaseMessage {
+  /** Unique identifier for the message */
   id?: string;
+  /** Timestamp when the message was sent or received (in milliseconds since epoch) */
   timestamp?: number;
+  /** ID of the client that generated the message */
   clientId?: string;
+  /** ID of the connection that the message was sent on */
   connectionId?: string;
+  /** Message payload */
   data?: any;
+  /** String describing the encoding applied to the data */
   encoding?: string | null;
+  /** Additional metadata for the message */
   extras?: any;
+  /** Size of the message in bytes */
   size?: number;
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 39f879e and 74088f0.

📒 Files selected for processing (1)
  • src/common/lib/types/basemessage.ts (1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/common/lib/types/basemessage.ts

[error] 52-52: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


[error] 197-198: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


[error] 15-15: Don't use 'Function' as a type.

Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.

(lint/complexity/noBannedTypes)


[error] 20-20: Don't use 'Function' as a type.

Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.

(lint/complexity/noBannedTypes)

⏰ Context from checks skipped due to timeout of 90000ms (6)
  • GitHub Check: test-node (20.x)
  • GitHub Check: test-browser (webkit)
  • GitHub Check: test-node (18.x)
  • GitHub Check: test-browser (firefox)
  • GitHub Check: test-node (16.x)
  • GitHub Check: test-browser (chromium)

VeskeR added 2 commits April 16, 2025 10:36
This commit merges changes from the next PRs:
- #2001
- #2002
- #2000
- #1953
- #1989
- #1988
- #1984
- #1949
- #1980
- #1979
- #1978
- #1976
- #1962
- #1959
- #1955
- #1957
- #1940
- #1925
- #1947
- #1946
- #1945
- #1944

into the integration/liveobjects branch. No significant changes made
during merge conflict resolution.
Encode function refactoring in commit f4073b9
made it so `data` and `encoding` fields will be set on a message object
even if they are undefined. This is not a problem in itself, but
current `BaseMessage.strMsg` implementation does not check for undefined
values and adds them to the result string. It should not do that
@VeskeR VeskeR force-pushed the liveobjects/merge-main branch from 74088f0 to 4a51ccd Compare April 16, 2025 09:38
@github-actions github-actions bot temporarily deployed to staging/pull/2006/bundle-report April 16, 2025 09:38 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/2006/features April 16, 2025 09:38 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/2006/typedoc April 16, 2025 09:38 Inactive
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (3)
src/platform/react-hooks/src/hooks/useChannelAttach.ts (1)

34-42: ⚠️ Potential issue

Add missing dependency in useEffect.

The effect uses the ably variable but doesn't include it in the dependency array, which could lead to stale references if the Ably client changes.

  useEffect(() => {
    if (shouldAttachToTheChannel) {
      channel.attach().catch((reason) => {
        // we use a fire-and-forget approach for attaching, but calling detach during the attaching process or while
        // suspending can cause errors that will be automatically resolved
        logError(ably, reason.toString());
      });
    }
-  }, [shouldAttachToTheChannel, channel]);
+  }, [shouldAttachToTheChannel, channel, ably]);
🧰 Tools
🪛 GitHub Check: lint

[warning] 42-42:
React Hook useEffect has a missing dependency: 'ably'. Either include it or remove the dependency array

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

244-287: Improve type safety for publish arguments.

The publish method has been refactored to properly handle different input formats, but it still accepts any[] as arguments which makes the API less clear to consumers.

Consider using method overloads to provide better type information:

- async publish(...args: any[]): Promise<void> {
+ async publish(message: Message | Message[]): Promise<void>;
+ async publish(name: string, data: unknown): Promise<void>;
+ async publish(...args: any[]): Promise<void> {
    let messages: Message[];
    let argCount = args.length;

    if (argCount == 1) {
      if (Utils.isObject(args[0])) {
        messages = [Message.fromValues(args[0])];
      } else if (Array.isArray(args[0])) {
        messages = Message.fromValuesArray(args[0]);
      } else {
        throw new ErrorInfo(
          'The single-argument form of publish() expects a message object or an array of message objects',
          40013,
          400,
        );
      }
    } else {
      messages = [Message.fromValues({ name: args[0], data: args[1] })];
    }
    // Rest of the implementation...
src/common/lib/types/basemessage.ts (1)

13-24: 🛠️ Refactor suggestion

Avoid using the Function type in CipherOptions.

Using Function can mask specific function signatures. Consider defining a more explicit function type to improve type safety and self-documentation.

export type CipherOptions = {
  channelCipher: {
-    encrypt: Function;
+    encrypt: (data: ArrayBuffer | Uint8Array) => Promise<ArrayBuffer | Uint8Array>;
    algorithm: 'aes';
  };
  cipher?: {
    channelCipher: {
-      encrypt: Function;
+      encrypt: (data: ArrayBuffer | Uint8Array) => Promise<ArrayBuffer | Uint8Array>;
      algorithm: 'aes';
    };
  };
};
🧰 Tools
🪛 Biome (1.9.4)

[error] 15-15: Don't use 'Function' as a type.

Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.

(lint/complexity/noBannedTypes)


[error] 20-20: Don't use 'Function' as a type.

Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.

(lint/complexity/noBannedTypes)

🧹 Nitpick comments (6)
test/realtime/annotations.test.js (1)

1-1: Consider removing redundant 'use strict' directive.

JavaScript modules are automatically in strict mode, making this directive redundant. However, if this file isn't using ES modules, you can safely ignore this suggestion.

-'use strict';

 define(['shared_helper', 'chai'], function (Helper, chai) {
🧰 Tools
🪛 Biome (1.9.4)

[error] 1-1: Redundant use strict directive.

The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.

(lint/suspicious/noRedundantUseStrict)

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

60-60: Optional interface extraction.
Storing pending presence items as { presence: WirePresenceMessage; callback: ErrCallback }[] is valid. For clarity, you could create a small interface or type alias, like type PendingPresenceItem = { presence: WirePresenceMessage; callback: ErrCallback };, then use an array of that type.

src/common/lib/types/annotation.ts (1)

14-33: fromEncoded and fromEncodedArray functions.

export async function fromEncoded(): Promise<Annotation> {  }
export async function fromEncodedArray(): Promise<Annotation[]> {  }

These mirror patterns from message.ts and presencemessage.ts. The code is straightforward. Consider adding tests for edge cases, such as invalid wire data or missing fields, if not already covered.

src/common/lib/types/message.ts (1)

91-101: getMessagesSize() uses getMessageSize().

export function getMessagesSize(messages: WireMessage[]): number {
  
  total += msg.size || (msg.size = getMessageSize(msg));
}

Caching the size in msg.size is a small optimization. Make sure to reset if the message’s data changes.

src/common/lib/types/basemessage.ts (2)

52-53: Use optional chaining for clearer code.

The nested conditions could be simplified using optional chaining for better readability.

- if (options && options.cipher) {
+ if (options?.cipher) {
    if (!Crypto) Utils.throwMissingPluginError('Crypto');
    const cipher = Crypto.getCipher(options.cipher, logger);
🧰 Tools
🪛 Biome (1.9.4)

[error] 52-52: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


196-212: Use optional chaining for nested conditions.

The nested condition checks could be simplified using optional chaining for better readability.

- if (
-   context.channelOptions != null &&
-   context.channelOptions.cipher &&
-   context.channelOptions.channelCipher
- ) {
+ if (context.channelOptions?.cipher && context.channelOptions?.channelCipher) {
    const xformAlgorithm = match[3],
      cipher = context.channelOptions.channelCipher;
    /* don't attempt to decrypt unless the cipher params are compatible */
    if (xformAlgorithm != cipher.algorithm) {
      throw new Error('Unable to decrypt message with given cipher; incompatible cipher params');
    }
    decodedData = await cipher.decrypt(decodedData);
    continue;
  } else {
    throw new Error('Unable to decrypt message; not an encrypted channel');
  }
🧰 Tools
🪛 Biome (1.9.4)

[error] 197-198: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 74088f0 and 4a51ccd.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (53)
  • .eslintrc.js (1 hunks)
  • .github/workflows/test-browser.yml (1 hunks)
  • CHANGELOG.md (1 hunks)
  • README.md (1 hunks)
  • ably.d.ts (19 hunks)
  • modular.d.ts (5 hunks)
  • package.json (1 hunks)
  • src/common/lib/client/baseclient.ts (3 hunks)
  • src/common/lib/client/defaultrealtime.ts (3 hunks)
  • src/common/lib/client/defaultrest.ts (3 hunks)
  • src/common/lib/client/modularplugins.ts (1 hunks)
  • src/common/lib/client/realtimeannotations.ts (1 hunks)
  • src/common/lib/client/realtimechannel.ts (19 hunks)
  • src/common/lib/client/realtimepresence.ts (9 hunks)
  • src/common/lib/client/restannotations.ts (1 hunks)
  • src/common/lib/client/restchannel.ts (7 hunks)
  • src/common/lib/transport/comettransport.ts (2 hunks)
  • src/common/lib/transport/connectionmanager.ts (2 hunks)
  • src/common/lib/transport/protocol.ts (3 hunks)
  • src/common/lib/transport/transport.ts (2 hunks)
  • src/common/lib/transport/websockettransport.ts (1 hunks)
  • src/common/lib/types/annotation.ts (1 hunks)
  • src/common/lib/types/basemessage.ts (1 hunks)
  • src/common/lib/types/defaultannotation.ts (1 hunks)
  • src/common/lib/types/message.ts (3 hunks)
  • src/common/lib/types/protocolmessage.ts (5 hunks)
  • src/common/lib/types/protocolmessagecommon.ts (1 hunks)
  • src/platform/react-hooks/src/AblyReactHooks.ts (1 hunks)
  • src/platform/react-hooks/src/ChannelProvider.tsx (2 hunks)
  • src/platform/react-hooks/src/fakes/ably.ts (3 hunks)
  • src/platform/react-hooks/src/hooks/constants.ts (1 hunks)
  • src/platform/react-hooks/src/hooks/useChannel.ts (2 hunks)
  • src/platform/react-hooks/src/hooks/useChannelAttach.test.tsx (1 hunks)
  • src/platform/react-hooks/src/hooks/useChannelAttach.ts (1 hunks)
  • src/platform/react-hooks/src/hooks/usePresence.ts (2 hunks)
  • src/platform/react-hooks/src/hooks/usePresenceListener.ts (2 hunks)
  • src/platform/react-hooks/src/utils.ts (1 hunks)
  • src/platform/web/lib/http/http.ts (1 hunks)
  • src/platform/web/modular.ts (1 hunks)
  • src/platform/web/modular/annotations.ts (1 hunks)
  • src/plugins/objects/objects.ts (1 hunks)
  • src/plugins/push/pushactivation.ts (1 hunks)
  • test/browser/connection.test.js (8 hunks)
  • test/browser/modular.test.js (20 hunks)
  • test/common/ably-common (1 hunks)
  • test/common/modules/shared_helper.js (1 hunks)
  • test/realtime/annotations.test.js (1 hunks)
  • test/realtime/channel.test.js (7 hunks)
  • test/realtime/message.test.js (3 hunks)
  • test/realtime/objects.test.js (18 hunks)
  • test/realtime/presence.test.js (6 hunks)
  • test/rest/message.test.js (1 hunks)
  • test/support/browser_file_list.js (1 hunks)
✅ Files skipped from review due to trivial changes (4)
  • package.json
  • .github/workflows/test-browser.yml
  • test/common/ably-common
  • test/support/browser_file_list.js
🚧 Files skipped from review as they are similar to previous changes (38)
  • src/platform/react-hooks/src/ChannelProvider.tsx
  • src/common/lib/transport/transport.ts
  • src/platform/web/modular.ts
  • .eslintrc.js
  • src/platform/react-hooks/src/hooks/usePresenceListener.ts
  • src/common/lib/client/defaultrest.ts
  • src/platform/react-hooks/src/fakes/ably.ts
  • src/plugins/objects/objects.ts
  • src/common/lib/transport/protocol.ts
  • src/common/lib/client/baseclient.ts
  • test/realtime/channel.test.js
  • src/platform/web/modular/annotations.ts
  • src/common/lib/client/defaultrealtime.ts
  • src/plugins/push/pushactivation.ts
  • src/platform/react-hooks/src/utils.ts
  • src/platform/web/lib/http/http.ts
  • test/common/modules/shared_helper.js
  • src/platform/react-hooks/src/hooks/useChannelAttach.test.tsx
  • modular.d.ts
  • test/rest/message.test.js
  • src/platform/react-hooks/src/hooks/constants.ts
  • src/platform/react-hooks/src/AblyReactHooks.ts
  • src/common/lib/transport/connectionmanager.ts
  • src/common/lib/types/defaultannotation.ts
  • src/platform/react-hooks/src/hooks/useChannel.ts
  • test/browser/modular.test.js
  • src/platform/react-hooks/src/hooks/usePresence.ts
  • src/common/lib/transport/comettransport.ts
  • test/browser/connection.test.js
  • test/realtime/presence.test.js
  • test/realtime/objects.test.js
  • src/common/lib/client/restchannel.ts
  • src/common/lib/client/restannotations.ts
  • src/common/lib/client/realtimeannotations.ts
  • src/common/lib/transport/websockettransport.ts
  • src/common/lib/types/protocolmessagecommon.ts
  • src/common/lib/client/modularplugins.ts
  • CHANGELOG.md
🧰 Additional context used
🧬 Code Graph Analysis (5)
src/common/lib/types/annotation.ts (5)
src/common/lib/types/message.ts (8)
  • fromEncoded (46-55)
  • fromEncodedArray (57-68)
  • _fromEncoded (72-75)
  • _fromEncodedArray (77-83)
  • fromValues (135-137)
  • fromValues (165-167)
  • encode (128-133)
  • decode (193-199)
src/common/lib/client/restchannel.ts (1)
  • logger (65-67)
ably.d.ts (3)
  • ChannelOptions (959-979)
  • Annotation (3218-3273)
  • AnnotationAction (3364-3364)
src/common/lib/util/utils.ts (1)
  • Properties (471-471)
src/common/lib/types/basemessage.ts (4)
  • encode (97-107)
  • strMsg (365-384)
  • wireToJSON (273-281)
  • decode (137-150)
src/common/lib/client/realtimechannel.ts (4)
src/common/lib/types/basemessage.ts (3)
  • CipherOptions (13-24)
  • populateFieldsFromParent (327-363)
  • MessageEncoding (319-324)
src/common/lib/types/message.ts (1)
  • getMessagesSize (93-101)
src/common/lib/types/protocolmessagecommon.ts (1)
  • actions (4-27)
src/common/lib/types/presencemessage.ts (1)
  • WirePresenceMessage (119-150)
src/common/lib/types/message.ts (10)
ably.d.ts (5)
  • MessageAction (3340-3345)
  • ChannelOptions (959-979)
  • Message (3131-3213)
  • Crypto (3513-3528)
  • Operation (3290-3303)
src/common/lib/util/utils.ts (1)
  • Properties (471-471)
src/common/types/channel.d.ts (1)
  • ChannelOptions (3-10)
src/common/lib/types/basemessage.ts (7)
  • normalizeCipherOptions (47-61)
  • CipherOptions (13-24)
  • encode (97-107)
  • strMsg (365-384)
  • wireToJSON (273-281)
  • EncodingDecodingContext (26-34)
  • decode (137-150)
src/common/lib/client/restchannel.ts (1)
  • logger (65-67)
src/platform/web/lib/http/http.ts (1)
  • logger (149-151)
src/common/types/http.ts (1)
  • logger (134-136)
src/common/lib/types/annotation.ts (5)
  • fromEncodedArray (23-33)
  • _fromEncoded (37-39)
  • _fromEncodedArray (41-50)
  • encode (63-70)
  • decode (103-114)
src/common/lib/types/presencemessage.ts (5)
  • fromEncodedArray (26-37)
  • _fromEncoded (41-46)
  • _fromEncodedArray (48-57)
  • encode (90-95)
  • decode (134-145)
src/common/types/ICryptoStatic.ts (1)
  • IUntypedCryptoStatic (23-25)
src/platform/react-hooks/src/hooks/useChannelAttach.ts (5)
ably.d.ts (1)
  • ConnectionState (149-157)
src/platform/react-hooks/src/hooks/useAbly.ts (1)
  • useAbly (5-15)
src/platform/react-hooks/src/hooks/useConnectionStateListener.ts (1)
  • useConnectionStateListener (15-28)
src/platform/react-hooks/src/hooks/constants.ts (1)
  • INACTIVE_CONNECTION_STATES (3-3)
src/platform/react-hooks/src/utils.ts (1)
  • logError (7-14)
test/realtime/annotations.test.js (5)
test/realtime/presence.test.js (5)
  • chai (4-4)
  • realtime (1619-1619)
  • realtime (1702-1702)
  • channel (1620-1620)
  • channel (1703-1703)
src/common/lib/client/baseclient.ts (1)
  • rest (108-113)
test/realtime/channel.test.js (23)
  • helper (155-155)
  • helper (175-175)
  • helper (213-213)
  • helper (240-240)
  • helper (270-270)
  • helper (308-308)
  • helper (343-343)
  • helper (384-384)
  • helper (414-414)
  • helper (448-448)
  • helper (488-488)
  • helper (510-510)
  • helper (543-543)
  • helper (606-606)
  • helper (661-661)
  • realtime (1829-1829)
  • realtime (1848-1848)
  • realtime (1872-1872)
  • realtime (1912-1912)
  • channel (1830-1830)
  • channel (1851-1851)
  • channel (1875-1875)
  • channel (1915-1915)
src/common/lib/client/realtimechannel.ts (1)
  • annotations (73-78)
src/common/lib/client/restchannel.ts (1)
  • annotations (37-42)
🪛 Biome (1.9.4)
src/common/lib/types/basemessage.ts

[error] 52-52: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


[error] 197-198: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


[error] 15-15: Don't use 'Function' as a type.

Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.

(lint/complexity/noBannedTypes)


[error] 20-20: Don't use 'Function' as a type.

Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.

(lint/complexity/noBannedTypes)

src/common/lib/client/realtimepresence.ts

[error] 132-133: This case is falling through to the next case.

Add a break or return statement to the end of this case to prevent fallthrough.

(lint/suspicious/noFallthroughSwitchClause)

test/realtime/annotations.test.js

[error] 1-1: Redundant use strict directive.

The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.

(lint/suspicious/noRedundantUseStrict)

🪛 LanguageTool
README.md

[grammar] ~77-~77: You used an adverb (‘Ably’) instead of an adjective, or a noun (‘client’) instead of another adjective.
Context: ...m/lib/ably.min-2.js"></script> ``` The Ably client library follows [Semantic Versioning](h...

(A_RB_NN)

🪛 markdownlint-cli2 (0.17.2)
README.md

77-77: Bare URL used
null

(MD034, no-bare-urls)


77-77: Bare URL used
null

(MD034, no-bare-urls)


77-77: Bare URL used
null

(MD034, no-bare-urls)


77-77: Bare URL used
null

(MD034, no-bare-urls)


77-77: Bare URL used
null

(MD034, no-bare-urls)


77-77: Spaces inside emphasis markers
null

(MD037, no-space-in-emphasis)

🪛 GitHub Check: lint
src/platform/react-hooks/src/hooks/useChannelAttach.ts

[warning] 42-42:
React Hook useEffect has a missing dependency: 'ably'. Either include it or remove the dependency array

⏰ Context from checks skipped due to timeout of 90000ms (6)
  • GitHub Check: test-browser (webkit)
  • GitHub Check: test-node (20.x)
  • GitHub Check: test-browser (firefox)
  • GitHub Check: test-node (18.x)
  • GitHub Check: test-browser (chromium)
  • GitHub Check: test-node (16.x)
🔇 Additional comments (83)
README.md (2)

73-74: Library version upgrade in CDN link.

The CDN link has been updated to reference version 2 of the Ably JavaScript client library, which aligns with the broader version 2.6.x release mentioned in the PR summary.

🧰 Tools
🪛 markdownlint-cli2 (0.17.2)

73-73: Code block style
Expected: indented; Actual: fenced

(MD046, code-block-style)


77-77: Version references updated to reflect version 2.

The documentation about semantic versioning has been appropriately updated to show examples for the v2 series (2.js, 2.6.js, 2.6.3.js).

Note: The static analysis flagged bare URLs in this line, but this is acceptable in this context as these are examples of CDN links.

🧰 Tools
🪛 LanguageTool

[grammar] ~77-~77: You used an adverb (‘Ably’) instead of an adjective, or a noun (‘client’) instead of another adjective.
Context: ...m/lib/ably.min-2.js"></script> ``` The Ably client library follows [Semantic Versioning](h...

(A_RB_NN)

🪛 markdownlint-cli2 (0.17.2)

77-77: Bare URL used
null

(MD034, no-bare-urls)


77-77: Bare URL used
null

(MD034, no-bare-urls)


77-77: Bare URL used
null

(MD034, no-bare-urls)


77-77: Bare URL used
null

(MD034, no-bare-urls)


77-77: Bare URL used
null

(MD034, no-bare-urls)


77-77: Spaces inside emphasis markers
null

(MD037, no-space-in-emphasis)

src/platform/react-hooks/src/hooks/useChannelAttach.ts (1)

12-47: Well-structured hook for channel attachment management.

This new hook provides a clean way to manage the attachment lifecycle of an Ably Realtime channel based on connection state. It tracks connection state changes and attaches the channel asynchronously when appropriate, returning the current connection state for reuse in related hooks.

🧰 Tools
🪛 GitHub Check: lint

[warning] 42-42:
React Hook useEffect has a missing dependency: 'ably'. Either include it or remove the dependency array

test/realtime/annotations.test.js (2)

30-70: Comprehensive test for annotation publishing and subscribing.

The test thoroughly validates the annotation lifecycle, including publishing from realtime and REST clients, verifying message fields and expected behavior when annotations are created.


72-124: Well-structured pagination test for REST annotations retrieval.

This test thoroughly verifies the REST API for retrieving message annotations, including pagination capabilities. The test publishes multiple annotations and then verifies that they can be retrieved with correct pagination handling and properly ordered results.

test/realtime/message.test.js (4)

1278-1314: Updated message action handling tests.

The test suite has been renamed and refactored to use async/await for calls to Message.fromEncoded and message.encode(). The action codes have been standardized (e.g., 0 for create actions), improving consistency with the implementation.


1315-1332: Added test for message metadata population.

Good addition of tests to verify that messages with action create (0) properly populate serial and createdAt fields from version and timestamp, while other actions do not. This ensures correct metadata handling across different message types.


1443-1479: Improved test reliability with setTimeout.

The timeout ensures that all pending I/O operations and message processing complete before assertions are run, avoiding potential race conditions in the test suite. This is a good practice for asynchronous testing.


1491-1529: Added robustness test for unrecognized message actions.

This new test verifies that receiving protocol messages with unrecognized actions does not close or fail the realtime connection, enhancing robustness. This is important for ensuring backward compatibility and graceful handling of unexpected messages.

src/common/lib/client/realtimepresence.ts (8)

4-5: Imports look correct.
Bringing in PresenceMessage and WirePresenceMessage from presencemessage.ts and CipherOptions from basemessage.ts is consistent with the new architecture.


118-126: Consider error handling on encode.

const presence = PresenceMessage.fromData(data);
presence.action = action;

const wirePresMsg = await presence.encode(channel.channelOptions as CipherOptions);

This block looks fine, but if presence.encode throws (e.g., due to invalid data), the error will bubble up. Verify that upstream logic can handle this gracefully.


130-138: Intentional switch fallthrough.
Lines 132-133 fall through to the attaching case. There is already an // eslint-disable-next-line no-fallthrough comment, so it appears intentional. If that’s desired, you can keep it; otherwise, insert a break or refactor to handle each case distinctly to silence linters.

🧰 Tools
🪛 Biome (1.9.4)

[error] 132-133: This case is falling through to the next case.

Add a break or return statement to the end of this case to prevent fallthrough.

(lint/suspicious/noFallthroughSwitchClause)


172-197: Check consistency for leaving presence in various states.

switch (channel.state) {
  case 'attached':
    
  case 'attaching':
    
  case 'initialized':
  case 'failed':
    throw new PartialErrorInfo('…');
  default:
    throw channel.invalidStateError();
}

Consider whether states like 'detached' or 'suspended' need handling. Ensure this logic stays consistent with _enterOrUpdateClient to avoid unexpected errors.


282-282: For-of loop is fine.
Using a for-of on presenceSet enhances clarity and avoids index-handling overhead.


313-313: Action non-null assertion.

this.subscriptions.emit(presence.action!, presence);

Ensure presence.action! is always defined at runtime. If the code or upstream logic might ever produce undefined, this could throw an error.


354-357: Promise chain for sending presence.

this.channel
  .sendPresence(presenceArray)
  .then(() => multicaster())
  .catch((err: ErrorInfo) => multicaster(err));

Looks good; straightforward chain. Consider adding a .finally() if there is any shared cleanup logic needed, but otherwise this is fine.


454-458: Conditional attach on subscribe.

if (channel.channelOptions.attachOnSubscribe !== false) {
  await channel.attach();
}

This is a helpful convenience feature. If a client wants manual attaching, they can override with attachOnSubscribe = false.

src/common/lib/types/annotation.ts (6)

1-11: Initial imports and setup.

import Logger from '../util/logger';
import { BaseMessage, encode, decode, wireToJSON, strMsg } from './basemessage';

import type { Properties } from '../util/utils';
import type RestChannel from '../client/restchannel';
import type RealtimeChannel from '../client/realtimechannel';
import type { ChannelOptions } from '../../types/channel';

All relevant utilities and types are pulled in. This file depends on basemessage.ts for core encode/decode logic, consistent with the new approach.


12-13: actions array for annotation.

const actions = ['annotation.create', 'annotation.delete'];

Defining an array for annotation actions is consistent with the approach in other message types.


35-55: Internal _fromEncoded variants.

export async function _fromEncoded()
export async function _fromEncodedArray()

These are for decoding with an existing channel, bypassing cipher normalization. This is consistent with the approach in other message types.


57-83: Annotation class basics.

class Annotation extends BaseMessage {
  
  async encode(): Promise<WireAnnotation> {  }
  static fromValues(): Annotation {  }
  
}
  • The encode() method does not pass cipher options, aligning with the comment that annotation data must be server-parseable.
  • The class extends BaseMessage for consistent encoding and decoding flow.

85-119: WireAnnotation class.

class WireAnnotation extends BaseMessage {
  
  async decode(channelOptions: ChannelOptions, logger: Logger): Promise<Annotation> {
    
    await decode(res, channelOptions);
    
  }
}
  • This mirrors WireMessage and WirePresenceMessage.
  • The catch block logs errors but does not rethrow; presumably the caller handles partial results.

121-121: Default export.

export default Annotation;

Makes sense for the primary class. Keep an eye on naming clarity if exporting both Annotation and WireAnnotation from the same file, but it seems consistent with other patterns.

src/common/lib/types/protocolmessage.ts (9)

4-4: Annotations plugin import.

import { AnnotationsPlugin } from '../client/modularplugins';

This addition aligns with the new annotation functionality.


7-9: Switch to WireMessage, WirePresenceMessage, WireAnnotation.

import { WireMessage } from './message';
import PresenceMessage, { WirePresenceMessage } from './presencemessage';
import Annotation, { WireAnnotation } from './annotation';

This reflects the central theme of wire vs. high-level message objects.


29-39: deserialize() with annotationsPlugin.

export function deserialize(
  serialized: unknown, 
  annotationsPlugin: AnnotationsPlugin | null,
  
) {
  
  return fromDeserialized(deserialized, presenceMessagePlugin, annotationsPlugin, objectsPlugin);
}

Now includes the optional annotationsPlugin argument. This is consistent with the presence approach.


41-81: fromDeserialized() extended for annotations.

let annotations: WireAnnotation[] | undefined;
if (annotationsPlugin && deserialized.annotations) {
  annotations = annotationsPlugin.WireAnnotation.fromValuesArray();
}

return Object.assign(new ProtocolMessage(), {  annotations,});
  • Good: We only decode annotations if the plugin is present.
  • The rest uses the standard approach for presence, messages, and states.

89-100: makeFromDeserializedWithDependencies() test utility.

return fromDeserialized(
  deserialized,
  {
    PresenceMessage,
    WirePresenceMessage,
  },
  { Annotation, WireAnnotation, RealtimeAnnotations, RestAnnotations },
  dependencies?.ObjectsPlugin ?? null,
);

This factory approach is consistent with prior patterns, letting tests supply a plugin environment.


103-103: fromValues() helper.

export function fromValues(values: Properties<ProtocolMessage>): ProtocolMessage {
  return Object.assign(new ProtocolMessage(), values);
}

No concerns here — matches the pattern for converting plain objects to typed instances.


107-150: stringify() extended for annotations.

if (msg.annotations && annotationsPlugin) {
  result += '; annotations=' + toStringArray();
}

Ensures annotation data is included for debugging and logs. Straightforward extension of existing approach for messages/presence.

🧰 Tools
🪛 Biome (1.9.4)

[error] 134-134: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


153-176: Additional fields in ProtocolMessage.

annotations?: WireAnnotation[];
params?: Record<string, string>;

annotations?: WireAnnotation[]; is consistent with the plugin-based approach. The new params field is also introduced. Ensure no sensitive data is stored in params inadvertently.


181-187: setFlag and fallthrough logic.

setFlag(flag: keyof typeof flags): number {
  return (this.flags = (this.flags as number) | flags[flag]);
}

This is standard bitwise logic for flags. No issues.

src/common/lib/types/message.ts (13)

2-11: Refactor to import shared utilities from basemessage.ts.

import {
  BaseMessage,
  encode,
  decode,
  wireToJSON,
  normalizeCipherOptions,
  EncodingDecodingContext,
  CipherOptions,
  strMsg,
} from './basemessage';

Centralizing encode/decode logic in basemessage.ts is a major improvement, removing duplication.


15-21: Crypto and channel definitions.

import type { IUntypedCryptoStatic } from 'common/types/ICryptoStatic';
import type { ChannelOptions } from '../../types/channel';
import type { Properties } from '../util/utils';

type Channel = RestChannel | RealtimeChannel;

Matches the approach in other message-related files, ensuring consistent references.


23-24: Message actions array.

const actions: API.MessageAction[] = [  ];

This mirrors the presence/annotation pattern of an array-based action definition.


25-27: stringifyAction() helper.

function stringifyAction(action: number | undefined): string {  }

Offers a quick fallback to 'unknown' for out-of-range action indices. Fine approach.


29-29: getMessageSize() introduced.
This function calculates approximate message size in bytes. Make sure that for complex extras or data, JSON size is measured accurately.


46-55: fromEncoded() logic.

export async function fromEncoded(logger, Crypto, encoded, inputOptions?) {
  const options = normalizeCipherOptions(Crypto, logger, inputOptions ?? null);
  const wm = WireMessage.fromValues(encoded);
  return wm.decode(options, logger);
}

Ensures a uniform decode flow with optional crypto. Nicely done.


70-75: Internal _fromEncoded variants.

export async function _fromEncoded(encoded: Properties<WireMessage>, channel: Channel): Promise<Message> {
  const wm = WireMessage.fromValues(encoded);
  return wm.decode(channel.channelOptions, channel.logger);
}

Identical logic except skipping cipher normalization. This pattern is consistent with presence and annotation code.


85-87: encodeArray() function.

export async function encodeArray(messages: Array<Message>, options: CipherOptions): Promise<Array<WireMessage>> {
  return Promise.all(messages.map((message) => message.encode(options)));
}

A straightforward batch-encoding approach.


103-134: Message class.

class Message extends BaseMessage {  }
  • Uses expandFields() for version → serial or timestamp → createdAt conversions.
  • encode() calls the shared encode utility.
  • Great example of removing duplication and centralizing the logic in basemessage.ts.

135-141: Message.fromValues() variants.

static fromValues(values: Properties<Message>): Message {  }
static fromValuesArray(values: Properties<Message>[]): Message[] {}

This remains consistent with the other wire-based classes.


143-145: toString() override.

toString() {
  return strMsg(this, 'Message');
}

Straightforward debugging helper, consistent with other message types.


148-203: WireMessage class refactoring.

class WireMessage extends BaseMessage {
  
  async decode() {
    const { decoded } = await this.decodeWithErr();
    return decoded;
  }
  
  toString() { return strMsg(this, 'WireMessage'); }
}
  • decodeWithErr pattern is flexible if partial decode must be handled.
  • The rest matches presence/annotation wire classes. This is a good design to unify message handling.

206-206: No changes visible at line 206
No content is shown in the snippet for line 206, so no relevant comment.

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

66-66: LGTM: Proper initialization of _annotations property.

Adding the _annotations property initialized to null is consistent with how the _presence property is handled, maintaining a clear pattern for plugin-dependent features.


73-78: LGTM: Well-implemented annotations getter.

The getter implementation correctly follows the established pattern for plugin-dependent features, throwing an appropriate error when the plugin is missing.


89-89: LGTM: Simplified initialization of _mode property.

Initializing _mode directly to 0 simplifies the code while maintaining the same functionality.


113-115: LGTM: Proper initialization of annotations in constructor.

This code correctly instantiates the RealtimeAnnotations class when the plugin is available, following the same pattern used for other plugins.


289-296: LGTM: Good extraction of validation logic.

Creating a separate throwIfUnpublishableState method improves code organization and readability.


429-433: LGTM: Modernized Promise-based implementation.

Refactoring to use Promise API with async/await instead of callbacks makes the code more maintainable and easier to reason about.


449-454: LGTM: Added support for conditional channel attachment.

The attachOnSubscribe option provides flexibility in how channel subscription works, which is a nice improvement to the API.


493-503: LGTM: Modernized API with Promise-based implementation.

Converting callback-based methods to return Promises improves code quality and aligns with modern JavaScript practices.


607-617: LGTM: Improved presence message handling.

Using populateFieldsFromParent and Promise.all for presence messages provides a more organized and efficient processing approach.


686-708: LGTM: Improved message decoding with better error handling.

The refactored message decoding with async/await and clearer error handling improves code maintainability and reliability.


716-729: LGTM: Well-implemented annotation message handling.

The new handling for ANNOTATION action follows the same pattern as other message types, with proper decoding and processing.


744-750: LGTM: Improved handling of unrecognized message actions.

Changing from aborting the connection to logging a major-level message for unrecognized actions improves forward compatibility and resilience.

src/common/lib/types/basemessage.ts (6)

36-45: LGTM: Well-implemented context normalization.

The normaliseContext function ensures consistent handling of different input types, providing good defaults and type safety.


97-107: LGTM: Clear and focused encode function.

The encode function has a well-defined responsibility, properly separating encoding and encryption concerns.


155-271: LGTM: Comprehensive message decoding implementation.

The decodeData function handles multiple encoding formats with careful error handling, providing a robust implementation for the complex decoding process.

🧰 Tools
🪛 Biome (1.9.4)

[error] 197-198: Change to an optional chain.

Unsafe fix: Change to an optional chain.

(lint/complexity/useOptionalChain)


319-325: LGTM: Clean export of encoding utilities.

The MessageEncoding object provides a well-organized export of related encoding functions, making the API more discoverable.


327-363: LGTM: Well-implemented field population from parent message.

The populateFieldsFromParent function handles different message types appropriately, ensuring consistent message metadata.


386-395: LGTM: Clean base message definition.

The BaseMessage abstract class provides a solid foundation for all message types with appropriate properties.

ably.d.ts (20)

553-553: Transport parameters now support boolean values

The transportParams field in ClientOptions has been updated to accept boolean values in addition to string and number values. This enhances flexibility for configuring connection parameters.


715-722: Added new message modification capabilities

New capabilities have been added to support message modification operations:

  • message-update-any: Update any messages
  • message-update-own: Update own messages only
  • message-delete-any: Delete any messages
  • message-delete-own: Delete own messages only

These new capabilities provide more granular permission control for message operations.


865-893: Updated channel modes to support both case variants

Channel modes now accept both uppercase and lowercase variants ('PUBLISH' and 'publish') for backward compatibility. New annotation-related modes have been added:

  • ANNOTATION_PUBLISH/annotation_publish: Allows publishing annotations
  • ANNOTATION_SUBSCRIBE/annotation_subscribe: Allows receiving annotations

908-909: Extended ChannelMode type with annotation support

The ChannelMode type has been updated to include the new annotation-related channel modes, enabling proper type checking when configuring channels for annotation capabilities.


911-954: Added ResolvedChannelModes namespace and type

New ResolvedChannelModes namespace and ResolvedChannelMode type have been introduced to represent the lowercase-only variants of channel modes. This is part of a planned migration path for standardizing on lowercase values in a future major release (as noted in GitHub issue #1954).


972-978: Added attachOnSubscribe option to ChannelOptions

A new attachOnSubscribe boolean option has been added to control whether a channel should be implicitly attached when subscribing to it. This provides more control over channel attachment behavior, defaulting to true to maintain backward compatibility.


1022-1032: Added GetAnnotationsParams interface for annotation retrieval

New interface for specifying parameters when retrieving message annotations, including a limit parameter to control the maximum number of annotations returned (default: 100, maximum: 1000).


2096-2194: Added RealtimeAnnotations interface

Comprehensive interface for managing message annotations in realtime:

  • Subscribe/unsubscribe to receive annotations
  • Publish new annotations to messages
  • Delete annotations from messages
  • Retrieve annotations with pagination

The interface follows Ably's consistent pattern for resource management and includes clear documentation about the need to request the annotation_subscribe channel mode to receive individual annotations.


2736-2738: Added annotations property to Channel interface

The Channel interface now includes an annotations property that provides access to RestAnnotations functionality, enabling annotation operations on REST channels.


2783-2820: Added RestAnnotations interface

New interface for managing message annotations using the REST API:

  • Publish annotations to messages
  • Retrieve annotations with pagination

Similar to RealtimeAnnotations but with functionality appropriate for REST clients.


2898-2901: Added annotations property to RealtimeChannel interface

The RealtimeChannel interface now includes an annotations property that provides access to RealtimeAnnotations functionality, enabling realtime annotation operations.


3003-3007: Extended PublishOptions to support additional properties

The PublishOptions type now supports arbitrary additional string/number/boolean properties through an index signature. This allows for forward compatibility with new server-side publish options without requiring type definition updates.


3070-3126: Added annotation summary type definitions

Comprehensive type definitions for different annotation summary formats:

  • SummaryDistinctValues: For distinct.v1 aggregation
  • SummaryUniqueValues: For unique.v1 aggregation
  • SummaryMultipleValues: For multiple.v1 aggregation
  • SummaryClientIdList: For flag.v1 aggregation
  • SummaryClientIdCounts: For counting aggregations
  • SummaryTotal: For total.v1 aggregation

These types provide strongly-typed access to different annotation summary formats in the message summary.


3198-3212: Added new Message properties for annotations support

Two new properties added to the Message interface:

  • connectionKey: Allows REST clients to publish on behalf of Realtime clients
  • summary: Contains aggregated annotation data for the message

These properties enable annotation functionality and cross-client publishing capabilities.


3215-3285: Added Annotation and OutboundAnnotation interfaces

New interfaces define the structure of annotations:

  • Annotation: Complete annotation object received from Ably
  • OutboundAnnotation: Simplified structure for creating/publishing annotations

These interfaces provide type safety when working with message annotations.


3347-3364: Added AnnotationActions namespace and type

New namespace and type defining the possible annotation actions:

  • ANNOTATION_CREATE: For creating a new annotation
  • ANNOTATION_DELETE: For deleting an existing annotation

This provides type safety when working with annotation actions.


3369-3370: Updated InboundMessage type

The InboundMessage type has been updated to:

  • Omit the connectionKey property (which is only used for outbound messages)
  • Require additional fields for type safety

This ensures type correctness when handling received messages.


3459-3477: Added AnnotationStatic interface for decoding annotations

New utility interface that provides methods for decoding annotation objects:

  • fromEncoded: Decodes a single annotation
  • fromEncodedArray: Decodes an array of annotations

Similar to existing Message and PresenceMessage static utilities, this provides consistent encoding/decoding capabilities for annotations.


3839-3842: Added Annotation static property to Rest class

The Rest class now exposes the Annotation static utilities, providing access to annotation decoding capabilities for REST clients.


3894-3897: Added Annotation static property to Realtime class

The Realtime class now exposes the Annotation static utilities, providing access to annotation decoding capabilities for Realtime clients.

@lawrence-forooghian
Copy link
Collaborator

Great idea to split it into multiple merge commits to isolate the interesting changes!

@lawrence-forooghian
Copy link
Collaborator

It looks like the browser tests are failing on LiveObjects-related functionality, could you take a look please?

@lawrence-forooghian
Copy link
Collaborator

I've had a brief look at 2a920d2, but it's still quite hard to understand what's going on there (not your fault!). I think that instead of me putting much time into understanding that, it probably makes more sense to get this PR merged (once tests are fixed) and then I do a deeper review on the into-main PR instead.

@VeskeR
Copy link
Contributor Author

VeskeR commented Apr 16, 2025

I've had a brief look at 2a920d2, but it's still quite hard to understand what's going on there (not your fault!).

Yea, totally understanable 😞 Can't really think what other dev process will help avoid these situations. Can't rebase integration/liveobjects onto main right now, as we would loose refs to all previous PRs. Due to the ongoing nature of liveobjects development we couldn't merge PRs into the main either.
The best we can do in the future is to minimize common library changes in the integration branches (like in this case, instead of refactoring encoding/decoding on the integration branch do it against main and then merge into integration branch to minimize impact). We’ll still need to do merge commits at the end of the day, but they should be resolved automatically more often - or at least require fewer manual changes.

I think that instead of me putting much time into understanding that, it probably makes more sense to get this PR merged (once tests are fixed) and then I do a deeper review on the into-main PR instead.

Makes sense. You can view the gist of the changes re encoding/decoding refactoring on a compare page: main...liveobjects/merge-main#diff-cea067adf28dca08e1effd8b2bbbc703aeb421c68529b8d9d4bee68331a657e6 (may need to switch to "Files changed" tab and scroll to src/common/lib/types/basemessage.ts file as github doesn't automatically opens it even if a link is for that specific file).
It applies the changes from 4b51773 and f4073b9 as mentioned in the merge commit.

@VeskeR
Copy link
Contributor Author

VeskeR commented Apr 16, 2025

It looks like the browser tests are failing on LiveObjects-related functionality, could you take a look please?

Seems like browser tests can't resolve the reference to the deep-equal library used in LO plugin, will fix

@lawrence-forooghian
Copy link
Collaborator

instead of refactoring encoding/decoding on the integration branch do it against main and then merge into integration branch to minimize impact

Yes, this is definitely a good idea.

@lawrence-forooghian
Copy link
Collaborator

You can view the gist of the changes re encoding/decoding refactoring on a compare page

Cool, I'll wait until the into-main PR is ready to look at it, though, so that I can leave comments on it

@VeskeR
Copy link
Contributor Author

VeskeR commented Apr 17, 2025

It looks like the browser tests are failing on LiveObjects-related functionality, could you take a look please?

Fixed in #2011 - opened a separate PR as it is not related to the merge. That is PR is based on this merge PR so should expect a green CI there

deep-equal [1] does not provide UMD-compatible bundle which causes our
playwright tests to fail as they cannot resolve the module.
Replace it with dequal [2] which has UMD-compatible bundle. (also
apparently it should be faster than deep-equal)

[1] https://www.npmjs.com/package/deep-equal
[2] https://www.npmjs.com/package/dequal
@VeskeR
Copy link
Contributor Author

VeskeR commented Apr 17, 2025

Cool, I'll wait until the into-main PR is ready to look at it, though, so that I can leave comments on it

Works for me.
Browser tests failures are fixed in #2011, and the only other notable change in this PR to review is this commit: 4a51ccd (1 line change to fix an issue after the merge due to refactred encoding functions).

Please review those changes and approve this PR when ready, thanks

…ser-tests

Replace `deep-equal` with `dequal` package
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

9 participants