materialisation: Added new message fields.

[splindsay-92]

To support the work related to materialization, this pull request introduces new message fields and actions handling.

Added MessageAction enum to represent different message actions.
Added serial to the message, this is the unique serial (including index) of the message.
Publish now correctly sets the message action to message_create

• Added the following fields related to message annotations
  • refSerial
  • refType
• Added the following fields related to message updates/deletes
  • operation
  • updatedAt
  • deletedAt

This PR will need to wait until the related realtime work is done to support the new action fields in published messages. (ongoing)

Related to CHA-575

Summary by CodeRabbit

Release Notes

• New Features

  • Enhanced messaging capabilities with new properties in the Message interface for better tracking.
  • Introduced an Operation interface to carry additional information about message operations.
  • Added a new script for selective test execution using grep.
  • New tests validate the stringification behavior of the action property in messages.
  • Improved functionality of the test runner to allow dynamic filtering of tests based on environment variables.

• Bug Fixes

  • Improved error handling in message encoding/decoding tests.

• Documentation

  • Updated type definitions and method signatures for clarity and functionality.

• Chores

  • Adjusted bundle size thresholds for Realtime functionality.

[paddybyers]

• editedAt

Not updatedAt ? https://docs.google.com/document/d/14yBHC7Nkh6YkDneTauqkPSNEw2YVCvZNr_RplMvm40I/edit#heading=h.5df1tga3tfjx

[splindsay-92]

• editedAt

Not updatedAt ? https://docs.google.com/document/d/14yBHC7Nkh6YkDneTauqkPSNEw2YVCvZNr_RplMvm40I/edit#heading=h.5df1tga3tfjx

This was just a mistype in the PR body, the code uses updatedAt and I have now corrected the typo.

[coderabbitai]

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 543b360 and a8dd4cf.

📒 Files selected for processing (1)

• src/common/lib/types/message.ts (7 hunks)

🧰 Additional context used

🪛 Biome

src/common/lib/types/message.ts

[error] 287-288: The assignment should not be in an expression.

The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.

(lint/suspicious/noAssignInExpressions)

🔇 Additional comments (1)

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

312-312: ⚠️ Potential issue

Add type validation before type assertion

The type assertion values[i] as Record<string, unknown> could be unsafe if the input doesn't match the expected type.

- result[i] = fromValues(values[i] as Record<string, unknown>);
+ const value = values[i];
+ if (!value || typeof value !== 'object') {
+   throw new ErrorInfo('Invalid message value', 40013, 400);
+ }
+ result[i] = fromValues(value as Record<string, unknown>);

Likely invalid or redundant comment.

[AndyTWF]

Can we add a new test, or update an existing test, that covers the new fields please?

Perhaps a unit test on fromValues as that's where a lot of the magic happens?

[coderabbitai]

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (1)

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

12-20: Add JSDoc documentation for the message action constants

Consider adding documentation to explain the purpose and usage of these message actions, especially since they're part of the public API.

/**
 * Array of valid message actions in order of their numeric representation.
 * Used for converting between action numbers and their string representations.
 * @internal
 */
const MessageActionArray: API.MessageAction[] = [
  'message_unset',
  'message_create',
  // ... rest of the actions
];

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between a8dd4cf and 431e31a.

📒 Files selected for processing (1)

• src/common/lib/types/message.ts (7 hunks)

🧰 Additional context used

🪛 Biome

src/common/lib/types/message.ts

[error] 287-288: The assignment should not be in an expression.

The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.

(lint/suspicious/noAssignInExpressions)

🔇 Additional comments (2)

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

376-382: ⚠️ Potential issue

Add error handling for action conversion in toJSON

The action conversion could throw an error if the action is invalid. Consider handling this case gracefully.

-      action: toMessageActionNumber(this.action as API.MessageAction) || this.action,
+      action: (() => {
+        try {
+          return toMessageActionNumber(this.action as API.MessageAction) || this.action;
+        } catch (error) {
+          Logger.logAction(
+            Logger,
+            Logger.LOG_ERROR,
+            'Message.toJSON()',
+            `Invalid action: ${this.action}`,
+          );
+          return undefined;
+        }
+      })(),

Likely invalid or redundant comment.

---

28-34: ⚠️ Potential issue

Add input validation to conversion functions

The conversion functions should validate their inputs to fail fast and provide clear error messages.

 function toMessageActionString(actionNumber: number): API.MessageAction | undefined {
+  if (typeof actionNumber !== 'number' || !Number.isInteger(actionNumber) || actionNumber < 0) {
+    throw new ErrorInfo('Invalid action number', 40013, 400);
+  }
   return ReverseMessageActionMap.get(actionNumber);
 }

 function toMessageActionNumber(messageAction?: API.MessageAction): number | undefined {
+  if (messageAction && !MessageActionMap.has(messageAction)) {
+    throw new ErrorInfo('Invalid message action', 40013, 400);
+  }
   return messageAction ? MessageActionMap.get(messageAction) : undefined;
 }

Likely invalid or redundant comment.

[coderabbitai]

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (3)

test/support/runPlaywrightTests.js (1)

71-84: Consider documenting the grep functionality.

This new filtering capability is a valuable addition to the test infrastructure. Consider:

1. Adding a comment block explaining the grep functionality and its usage
2. Updating the README or testing documentation to include examples of using MOCHA_GREP
3. Adding examples of common grep patterns for frequently used test scenarios

Example documentation:

/**
 * Test filtering via MOCHA_GREP
 * 
 * Usage:
 * MOCHA_GREP="MyTestSuite" npm test
 * MOCHA_GREP="should handle.*errors" npm test
 * 
 * The grep pattern is applied across all browser types and supports
 * regular expressions for flexible test filtering.
 */

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

12-34: LGTM! The implementation follows the dot notation format and uses Maps for efficient lookups.

The use of Maps for bidirectional conversion provides O(1) lookup time, which is an improvement over array iteration. However, consider adding input validation to prevent runtime errors.

 function toMessageActionString(actionNumber: number): API.MessageAction | undefined {
+  if (typeof actionNumber !== 'number' || !Number.isInteger(actionNumber) || actionNumber < 0) {
+    throw new ErrorInfo('Invalid action number', 40013, 400);
+  }
   return ReverseMessageActionMap.get(actionNumber);
 }

 function toMessageActionNumber(messageAction?: API.MessageAction): number | undefined {
+  if (messageAction && !MessageActionMap.has(messageAction)) {
+    throw new ErrorInfo('Invalid message action', 40013, 400);
+  }
   return messageAction ? MessageActionMap.get(messageAction) : undefined;
 }

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

1275-1324: LGTM! Well-structured test suite for message action stringification.

The test suite thoroughly covers various scenarios for message action handling:

• Numeric to string conversion
• Pre-stringified actions
• Edge cases (undefined/unknown actions)
• Consistent JSON serialization

The implementation aligns well with the PR objectives for enhancing message action support.

Consider adding the following test cases to improve coverage:

• Invalid action values (e.g., negative numbers, non-numeric strings)
• Boundary values for action enum
• Case sensitivity handling for stringified actions

// Additional test cases
{
  description: 'should handle invalid numeric action',
  action: -1,
  options: { stringifyAction: true },
  expectedString: '[Message; action=-1]',
  expectedJSON: { action: -1 }
},
{
  description: 'should handle case-sensitive action strings',
  action: 'MESSAGE.CREATE',
  options: { stringifyAction: true },
  expectedString: '[Message; action=MESSAGE.CREATE]',
  expectedJSON: { action: undefined }
}

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 431e31a and e25751e.

📒 Files selected for processing (10)

• ably.d.ts (1 hunks)
• package.json (1 hunks)
• scripts/moduleReport.ts (1 hunks)
• src/common/lib/client/restchannel.ts (1 hunks)
• src/common/lib/types/defaultmessage.ts (2 hunks)
• src/common/lib/types/message.ts (7 hunks)
• src/common/lib/types/protocolmessage.ts (1 hunks)
• test/realtime/crypto.test.js (2 hunks)
• test/realtime/message.test.js (2 hunks)
• test/support/runPlaywrightTests.js (1 hunks)

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

• package.json
• scripts/moduleReport.ts
• src/common/lib/client/restchannel.ts
• src/common/lib/types/defaultmessage.ts
• src/common/lib/types/protocolmessage.ts

🧰 Additional context used

🪛 Biome

src/common/lib/types/message.ts

[error] 287-288: The assignment should not be in an expression.

The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.

(lint/suspicious/noAssignInExpressions)

🔇 Additional comments (9)

test/support/runPlaywrightTests.js (1)

71-72: LGTM: Clean implementation of grep configuration.

The addition of the grep parameter through environment variables follows testing best practices and maintains clean code structure.

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

312-312: ⚠️ Potential issue

Add type validation before type assertion.

The current implementation assumes the input values are of the correct type without validation.

- result[i] = fromValues(values[i] as Record<string, unknown>);
+ const value = values[i];
+ if (!value || typeof value !== 'object') {
+   throw new ErrorInfo('Invalid message value', 40013, 400);
+ }
+ result[i] = fromValues(value as Record<string, unknown>);

Likely invalid or redundant comment.

---

405-411: ⚠️ Potential issue

Use explicit null checks in toString method.

Using truthy checks can miss zero values. Consider using explicit null checks for more accurate string representation.

-    if (this.action) result += '; action=' + this.action;
-    if (this.serial) result += '; serial=' + this.serial;
-    if (this.refSerial) result += '; refSerial=' + this.refSerial;
-    if (this.refType) result += '; refType=' + this.refType;
-    if (this.updatedAt) result += '; updatedAt=' + this.updatedAt;
-    if (this.deletedAt) result += '; deletedAt=' + this.deletedAt;
-    if (this.operation) result += '; operation=' + JSON.stringify(this.operation);
+    if (this.action != null) result += '; action=' + this.action;
+    if (this.serial != null) result += '; serial=' + this.serial;
+    if (this.refSerial != null) result += '; refSerial=' + this.refSerial;
+    if (this.refType != null) result += '; refType=' + this.refType;
+    if (this.updatedAt != null) result += '; updatedAt=' + this.updatedAt;
+    if (this.deletedAt != null) result += '; deletedAt=' + this.deletedAt;
+    if (this.operation != null) result += '; operation=' + JSON.stringify(this.operation);

Likely invalid or redundant comment.

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

398-398: LGTM: Consistent handling of message actions across encryption modes.

The addition of { stringifyAction: true } option in both 128-bit and 256-bit msgpack tests ensures proper stringification of the new message action field during decoding. This change aligns with the PR's objective of supporting operations in messages.

Let's verify the consistency of this change across the codebase:

Also applies to: 443-443

✅ Verification successful

Let me gather more context about the stringifyAction option usage to ensure completeness of the verification.

---

The stringifyAction option is correctly used in the crypto tests

The verification confirms that the stringifyAction option is being used appropriately in the crypto tests. The codebase shows:

1. The option is part of the Message class's design, converting numeric action values to their string representations using MessageAction enum.
2. It's consistently used across the codebase in message decoding scenarios:
   • In protocol message handling: messageFromValues(messages[i], { stringifyAction: true })
   • In message decoding: fromValues(encoded, { stringifyAction: true })
   • In the crypto tests for both 128-bit and 256-bit encryption

The implementation in the crypto tests aligns with the established pattern for handling message actions throughout the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of stringifyAction option in message decoding
# Test: Search for other message decoding calls that might need the same option

# Look for Message.fromValues calls
ast-grep --pattern 'Message.fromValues($$$)'

# Look for msgpack.decode calls
rg 'msgpack\.decode'

Length of output: 1304

---

Script:

#!/bin/bash
# Search for stringifyAction usage across the codebase
rg "stringifyAction" -B 2 -A 2

# Look for Message class definition to understand the option handling
ast-grep --pattern 'class Message {
  $$$
}'

# Search for message action related code
rg "MessageAction" -B 2 -A 2

Length of output: 16492

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

1278-1315: Verify the consistency of message action values.

The test cases assume specific numeric values for actions (e.g., 1 for 'message.create', 2 for 'message.update'). Let's verify these values are consistent with the implementation.

✅ Verification successful

The verification results show that the message action values are defined in src/common/lib/types/message.ts using a zero-based index array MessageActionArray. The array defines the actions in order: 'message.unset' (0), 'message.create' (1), 'message.update' (2), which perfectly matches the test cases' assumptions where 'message.create' corresponds to 1 and 'message.update' corresponds to 2.

Let me verify one more aspect - the actual usage of these maps in the message implementation:

---

Test cases correctly reflect the message action handling implementation

The verification confirms that the test cases are consistent with the implementation:

1. The action values are correctly defined in MessageActionArray with 'message.create' at index 1 and 'message.update' at index 2
2. The Message class implementation in src/common/lib/types/message.ts shows:
   • toJSON() converts string actions to numbers using toMessageActionNumber
   • toString() directly uses the action value (string or number)

This matches the test cases' expectations where:

• Numeric action 1 maps to 'message.create'
• Numeric action 2 maps to 'message.update'
• String actions are preserved in toString() but converted to numbers in toJSON()

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify message action values are consistent across the codebase

# Test: Search for action enum/constant definitions
echo "Searching for message action definitions..."
rg -A 5 "(?:enum|const)\s+(?:Message)?Action"

# Test: Search for specific action string literals
echo "Searching for action string literals..."
rg -A 2 "(?:message\.create|message\.update)"

Length of output: 2533

---

Script:

#!/bin/bash
# Search for the message implementation that uses these action maps
ast-grep --pattern 'class $_ {
  $$$
  toString() {
    $$$
  }
  $$$
}'

# Also check for toJSON implementation
ast-grep --pattern 'class $_ {
  $$$
  toJSON() {
    $$$
  }
  $$$
}'

Length of output: 6443

ably.d.ts (4)

2338-2365: LGTM: Message interface changes are well-documented and properly typed.

The new fields (action, serial, refSerial, etc.) are correctly marked as optional since they may not be set when building a message, while being required in InboundMessage type. This aligns with the materialization enhancement objectives.

---

2435-2435: LGTM: InboundMessage type correctly requires new fields.

The addition of 'serial' and 'action' to the required fields is appropriate as these will be set on all messages across all namespaces, as confirmed in previous discussions.

---

2380-2383: 🛠️ Refactor suggestion

Consider using a more flexible type for metadata.

The current type Record<string, string> restricts metadata values to strings only. Consider using Record<string, unknown> or a more flexible type to allow for structured metadata.

Apply this diff to allow more flexible metadata values:

-  metadata?: Record<string, string>;
+  metadata?: Record<string, unknown>;

Likely invalid or redundant comment.

---

2369-2370: ⚠️ Potential issue

Fix typo in Operation interface comment.

The comment should read "update or deletion" instead of "update of deletion".

Apply this diff to fix the typo:

- * Contains the details of an operation, such as update of deletion, supplied by the actioning client.
+ * Contains the details of an operation, such as update or deletion, supplied by the actioning client.

Likely invalid or redundant comment.

[coderabbitai]

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (3)

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

28-30: Add validation for action number range

The function should validate that the action number is within the valid range before attempting to retrieve from the map.

 function toMessageActionString(actionNumber: number): API.MessageAction | undefined {
+  if (typeof actionNumber !== 'number' || actionNumber < 0 || actionNumber >= MessageActionArray.length) {
+    return undefined;
+  }
   return ReverseMessageActionMap.get(actionNumber);
 }

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

Line range hint 32-38: Consider adding test cases for new message fields

While the msgpack serialization changes look good, consider adding test cases to verify the handling of the new message fields (refSerial, refType, operation, updatedAt, deletedAt) in encrypted messages. This would ensure that these fields are properly encrypted/decrypted and serialized/deserialized.

Would you like me to help generate additional test cases for these new message fields?

Also applies to: 398-443

ably.d.ts (1)

2369-2369: Typo in comment: 'update of deletion' should be 'update or deletion'

The comment on line 2369 contains a typographical error. It should read "update or deletion" instead of "update of deletion".

Apply this diff to fix the typo:

 /**
- * Contains the details of an operation, such as update of deletion, supplied by the actioning client.
+ * Contains the details of an operation, such as update or deletion, supplied by the actioning client.
  */

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between e25751e and e28bfcc.

📒 Files selected for processing (10)

• ably.d.ts (1 hunks)
• package.json (1 hunks)
• scripts/moduleReport.ts (1 hunks)
• src/common/lib/client/restchannel.ts (1 hunks)
• src/common/lib/types/defaultmessage.ts (2 hunks)
• src/common/lib/types/message.ts (7 hunks)
• src/common/lib/types/protocolmessage.ts (1 hunks)
• test/realtime/crypto.test.js (2 hunks)
• test/realtime/message.test.js (2 hunks)
• test/support/runPlaywrightTests.js (1 hunks)

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

• package.json
• scripts/moduleReport.ts
• src/common/lib/client/restchannel.ts
• src/common/lib/types/defaultmessage.ts
• src/common/lib/types/protocolmessage.ts
• test/support/runPlaywrightTests.js

🧰 Additional context used

🪛 Biome

src/common/lib/types/message.ts

[error] 287-288: The assignment should not be in an expression.

The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.

(lint/suspicious/noAssignInExpressions)

🔇 Additional comments (9)

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

376-382: ⚠️ Potential issue

Add error handling for action conversion in toJSON

The action conversion could throw if the action is invalid. Consider handling this case gracefully.

-      action: toMessageActionNumber(this.action as API.MessageAction) || this.action,
+      action: (() => {
+        try {
+          return toMessageActionNumber(this.action as API.MessageAction) || this.action;
+        } catch (error) {
+          Logger.logAction(Logger.LOG_ERROR, 'Message.toJSON()', `Invalid action: ${this.action}`);
+          return undefined;
+        }
+      })(),

Likely invalid or redundant comment.

---

287-287: ⚠️ Potential issue

Improve readability by separating the assignment

The current implementation combines assignment and declaration, which reduces readability and has been flagged by static analysis.

-    const msg = (body[i] = fromValues(body[i], { stringifyAction: true }));
+    body[i] = fromValues(body[i], { stringifyAction: true });
+    const msg = body[i];

Likely invalid or redundant comment.

🧰 Tools

🪛 Biome

[error] 287-288: The assignment should not be in an expression.

The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.

(lint/suspicious/noAssignInExpressions)

---

405-411: ⚠️ Potential issue

Guard against potential undefined access in toString

The toString method should check for truthy values before stringifying them to prevent potential errors.

-    if (this.action) result += '; action=' + this.action;
-    if (this.serial) result += '; serial=' + this.serial;
-    if (this.refSerial) result += '; refSerial=' + this.refSerial;
-    if (this.refType) result += '; refType=' + this.refType;
-    if (this.updatedAt) result += '; updatedAt=' + this.updatedAt;
-    if (this.deletedAt) result += '; deletedAt=' + this.deletedAt;
-    if (this.operation) result += '; operation=' + JSON.stringify(this.operation);
+    if (this.action != null) result += '; action=' + this.action;
+    if (this.serial != null) result += '; serial=' + this.serial;
+    if (this.refSerial != null) result += '; refSerial=' + this.refSerial;
+    if (this.refType != null) result += '; refType=' + this.refType;
+    if (this.updatedAt != null) result += '; updatedAt=' + this.updatedAt;
+    if (this.deletedAt != null) result += '; deletedAt=' + this.deletedAt;
+    if (this.operation != null) result += '; operation=' + JSON.stringify(this.operation);

Likely invalid or redundant comment.

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

398-398: LGTM: Message action stringification for msgpack decoding

The addition of stringifyAction: true option ensures proper handling of the new message action field during msgpack deserialization. This change aligns with the PR's objective of supporting message operations and actions.

Also applies to: 443-443

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

1275-1324: LGTM! Well-structured test suite for message action stringification.

The test suite comprehensively covers various scenarios for message action stringification, including:

• Numeric to string conversion
• Already stringified actions
• Edge cases (undefined/unknown actions)
• Both string representation and JSON output validation

The implementation aligns well with the PR objectives of enhancing message actions support.

ably.d.ts (4)

2338-2365: Addition of new properties to 'Message' interface is appropriate

The newly added optional properties action, serial, refSerial, refType, updatedAt, deletedAt, and operation enhance the Message interface effectively.

---

2371-2384: Definition of 'Operation' interface is accurate

The Operation interface correctly defines the properties clientId, description, and metadata.

---

2389-2417: Duplicate Comment: Naming convention inconsistency in 'MessageActions'

---

2435-2435: Definition of 'InboundMessage' type is correct

The InboundMessage type appropriately extends Message with required id, timestamp, serial, and action properties.