Skip to content

add chat message reactions#19

Merged
mattheworiordan merged 1 commit into
mainfrom
add-chat-message-reactions
May 29, 2025
Merged

add chat message reactions#19
mattheworiordan merged 1 commit into
mainfrom
add-chat-message-reactions

Conversation

@vladvelici

@vladvelici vladvelici commented May 21, 2025

Copy link
Copy Markdown
Contributor
  • upgrade ably-chat-js
  • Add chat message reactions

Summary by CodeRabbit

  • New Features

    • Introduced CLI commands to manage message reactions in chat rooms, including adding, removing, and subscribing to reactions with flexible options and real-time updates.
  • Documentation

    • Expanded documentation to cover new message reaction commands, usage examples, and updated command listings.
  • Chores

    • Updated a dependency to the latest version for improved compatibility.
  • Style

    • Minor improvements to code imports and formatting for consistency.

@coderabbitai

coderabbitai Bot commented May 21, 2025

Copy link
Copy Markdown

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

This update introduces a comprehensive set of new CLI commands for managing message reactions within chat rooms, including adding, removing, and subscribing to reactions. The README and help documentation are updated accordingly. Dependency versions are bumped, and minor improvements are made to typing and reaction-related logic for consistency and clarity.

Changes

File(s) Change Summary
README.md Extended documentation to cover new CLI commands for message reactions under ably rooms messages reactions, detailing usage, arguments, flags, descriptions, and examples for add, remove, subscribe, and list commands. Updated commands list and examples accordingly.
package.json Updated @ably/chat dependency version from ^0.5.1 to ^0.7.0.
src/commands/rooms/messages/index.ts Updated examples and help output to include the new reactions subcommand group for message reactions.
src/commands/rooms/messages/reactions/add.ts
src/commands/rooms/messages/reactions/remove.ts
src/commands/rooms/messages/reactions/subscribe.ts
src/commands/rooms/messages/reactions/index.ts
Added new command classes for adding, removing, subscribing to, and listing message reactions in chat rooms. Each command handles its own CLI arguments, Ably Chat client setup, event subscriptions, output formatting, and resource cleanup.
src/commands/rooms/reactions/send.ts
src/commands/rooms/reactions/subscribe.ts
Removed the explicit empty reactions option object from room retrieval calls, simplifying the interface for getting a room handle.
src/commands/rooms/typing/start.ts
src/commands/rooms/typing/subscribe.ts
Refined typing indicator logic: switched to using keystroke() for typing heartbeat, adjusted intervals, removed unused options and types, and improved cleanup and process exit handling.
test/unit/services/config-manager.test.ts Changed import syntax for Chai: separated expect import from namespace import for clarity. No test logic changes.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant CLI
    participant ChatClient
    participant AblyRealtime
    participant Room

    User->>CLI: ably rooms messages reactions add ROOMID MSGSERIAL REACTION
    CLI->>AblyRealtime: Initialize connection
    CLI->>ChatClient: Initialize chat client
    ChatClient->>Room: Get room handle
    CLI->>Room: Attach and subscribe to status
    CLI->>Room: Add reaction to message
    Room-->>CLI: Reaction add confirmation
    CLI->>User: Output success or error
    CLI->>Room: Release room
    CLI->>AblyRealtime: Close connection
Loading
sequenceDiagram
    participant User
    participant CLI
    participant ChatClient
    participant AblyRealtime
    participant Room

    User->>CLI: ably rooms messages reactions subscribe ROOMID [--raw]
    CLI->>AblyRealtime: Initialize connection
    CLI->>ChatClient: Initialize chat client
    ChatClient->>Room: Get room handle (with raw option if set)
    CLI->>Room: Attach and subscribe to status
    CLI->>Room: Subscribe to reactions (raw or summary)
    Room-->>CLI: Emit reaction events
    CLI->>User: Display reaction updates
    User->>CLI: Ctrl+C (SIGINT)
    CLI->>Room: Unsubscribe and release
    CLI->>AblyRealtime: Close connection
Loading

Suggested reviewers

  • AndyTWF

Poem

In chat rooms where messages flow,
Now reactions can sparkle and glow!
Add a thumbs-up, remove with a frown,
Or watch them all as they come down.
With code so fresh and docs so neat,
This bunny hops with happy feet!
🐇✨


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🔭 Outside diff range comments (1)
src/commands/rooms/typing/start.ts (1)

342-357: 🛠️ Refactor suggestion

process.exit() prevents Oclif finally() from running

Calling process.exit(0) short-circuits Node’s event-loop and may skip the finally hook you carefully implemented for clean-up. Allow the promise to resolve naturally and let Oclif invoke finally().

- resolve();
- process.exit(0); // Explicitly exit to ensure process terminates
+ resolve(); // Oclif will exit once run() settles and finally() completes
🧹 Nitpick comments (15)
src/commands/rooms/messages/index.ts (2)

11-11: Add matching examples for remove and subscribe for parity

Now that the reactions command-group offers three sub-commands (add, remove, subscribe), consider extending the examples list so that users discover the whole surface area from --help output.

   '$ ably rooms messages reactions add my-room "message-id" "👍"',
+  '$ ably rooms messages reactions remove my-room "message-id" "👍"',
+  "$ ably rooms messages reactions subscribe my-room",

26-28: Minor ordering / grouping nit

All other help lines are sorted alphabetically by sub-command (get, send, subscribe). Moving reactions below subscribe keeps the list alphabetised and easier to scan.

src/commands/rooms/messages/reactions/index.ts (2)

1-12: Expose global flags so users don’t lose functionality

Other top-level commands inherit the shared globalFlags (API key, JSON output, etc.). Adding the same here will keep UX consistent:

 import { Command } from "@oclif/core";

 export default class MessagesReactionsIndex extends Command {
+  static override flags = {
+    ...require("../../../chat-base-command.js").ChatBaseCommand.globalFlags,
+  };

13-25: Let oclif generate sub-command help automatically

Instead of maintaining a hard-coded list that can drift, you can rely on oclif’s built-in help by throwing new Help({ config: this.config }).showHelp(this, []); or simply doing nothing and allowing oclif to fall back to its default behaviour.

This reduces maintenance and eliminates duplication with the examples array.

src/commands/rooms/typing/start.ts (1)

142-151: Rename log tag to match keystroke() semantics

The surrounding log messages still refer to “start typing”, but we now use keystroke(). Consider renaming the tag ("keystrokeAttempt", "keystroked") so logs remain semantically accurate.

README.md (5)

3513-3516: Expand parent command examples for full reactions lifecycle
You’ve added the reactions add example under the ably rooms messages examples. For consistency and to demonstrate the full lifecycle in the parent group, consider including remove and subscribe examples here as well.

Example diff:

 ## EXAMPLES
   $ ably rooms messages send my-room "Hello world!"
   $ ably rooms messages subscribe my-room
   $ ably rooms messages get my-room
-  $ ably rooms messages reactions add my-room "message-id" "👍"
+  $ ably rooms messages reactions add my-room "message-id" "👍"
+  $ ably rooms messages reactions remove my-room "message-id" "👍"
+  $ ably rooms messages reactions subscribe my-room

3571-3577: Specify language for fenced code blocks
The usage block for ably rooms messages reactions is fenced without a language spec. For improved markdown linting and syntax highlighting, add a language (e.g., sh or sh-session).

- ```
+ ```sh
 USAGE
   $ ably rooms messages reactions

<details>
<summary>🧰 Tools</summary>

<details>
<summary>🪛 markdownlint-cli2 (0.17.2)</summary>

3571-3571: Fenced code blocks should have a language specified
null

(MD040, fenced-code-language)

</details>

</details>

---

`3592-3600`: **Add language spec to `add` command usage block**  
The `reactions add` usage block is missing a language identifier on its fenced code. Use `sh` for consistency with other CLI examples.  
 
```diff
- ```
+ ```sh
USAGE
  $ ably rooms messages reactions add ROOMID MESSAGESERIAL REACTION [--access-token <value>] ...

<details>
<summary>🧰 Tools</summary>

<details>
<summary>🪛 markdownlint-cli2 (0.17.2)</summary>

3592-3592: Fenced code blocks should have a language specified
null

(MD040, fenced-code-language)

</details>

</details>

---

`3643-3652`: **Add language spec to `remove` command usage block**  
The `reactions remove` code block should also include a language spec.  
 
```diff
- ```
+ ```sh
USAGE
  $ ably rooms messages reactions remove ROOMID MESSAGESERIAL REACTION [--access-token <value>] ...

<details>
<summary>🧰 Tools</summary>

<details>
<summary>🪛 markdownlint-cli2 (0.17.2)</summary>

3643-3643: Fenced code blocks should have a language specified
null

(MD040, fenced-code-language)

</details>

</details>

---

`3683-3690`: **Add language spec to `subscribe` command usage block**  
Similarly, add a language to the `reactions subscribe` usage code fence for proper linting.  
 
```diff
- ```
+ ```sh
USAGE
  $ ably rooms messages reactions subscribe ROOMID [--access-token <value>] ...

<details>
<summary>🧰 Tools</summary>

<details>
<summary>🪛 markdownlint-cli2 (0.17.2)</summary>

3683-3683: Fenced code blocks should have a language specified
null

(MD040, fenced-code-language)

</details>

</details>

</blockquote></details>
<details>
<summary>src/commands/rooms/messages/reactions/remove.ts (1)</summary><blockquote>

`171-185`: **No validation for unknown message serial format**

If `messageSerial` is malformed the SDK will throw an opaque error.  
Adding a quick format/emptiness check gives earlier feedback and a clearer message to the user.

```ts
if (!/^[\w-]+:\d+$/.test(messageSerial)) {
 this.error(`Invalid messageSerial "${messageSerial}". Expected format "<id>:<index>".`);
}
src/commands/rooms/messages/reactions/add.ts (2)

241-256: Default reaction parameters can be simplified

Building reactionParams piecemeal leads to three conditional blocks and repeated casts.
A flatter expression reduces complexity:

-const reactionParams: { … } = { name: reaction };
-
-if (flags.type) {
-  reactionParams.type = flags.type as MessageReactionType;
-}
-if (flags.type === "multiple" && flags.count) {
-  reactionParams.count = flags.count;
-}
-if (this.metadataObj) {
-  reactionParams.metadata = this.metadataObj;
-}
+const reactionParams = {
+  name: reaction,
+  ...(flags.type && { type: flags.type as MessageReactionType }),
+  ...(flags.type === "multiple" && flags.count && { count: flags.count }),
+  ...(this.metadataObj && { metadata: this.metadataObj }),
+};

278-288: resultData omits metadata for empty objects

...(this.metadataObj && { metadata: this.metadataObj }) filters out {}.
If callers intentionally pass an empty object it disappears from the output.
Use metadataObj !== null to preserve intent.

src/commands/rooms/messages/reactions/subscribe.ts (2)

224-258: Guard console output against undefined fields

event.reaction.clientId and .name may be undefined; interpolating them directly yields "undefined" strings.

-  `${chalk.blue(event.reaction.clientId || "Unknown")}`
+  `${chalk.blue(event.reaction.clientId ?? "anonymous")}`

Apply similar guards for name & messageSerial to keep output tidy.


48-75: Duplicated cleanup logic across commands

finally blocks in all three reaction commands repeat nearly identical resource-release code. Extracting a helper (e.g., withCleanup(async () => { … })) or adding a dispose() method to ChatBaseCommand would cut ~40 lines per file and reduce future maintenance.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0dd8a68 and 0dcef6f.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (12)
  • README.md (3 hunks)
  • package.json (1 hunks)
  • src/commands/rooms/messages/index.ts (2 hunks)
  • src/commands/rooms/messages/reactions/add.ts (1 hunks)
  • src/commands/rooms/messages/reactions/index.ts (1 hunks)
  • src/commands/rooms/messages/reactions/remove.ts (1 hunks)
  • src/commands/rooms/messages/reactions/subscribe.ts (1 hunks)
  • src/commands/rooms/reactions/send.ts (1 hunks)
  • src/commands/rooms/reactions/subscribe.ts (1 hunks)
  • src/commands/rooms/typing/start.ts (7 hunks)
  • src/commands/rooms/typing/subscribe.ts (2 hunks)
  • test/unit/services/config-manager.test.ts (1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md

3571-3571: Fenced code blocks should have a language specified
null

(MD040, fenced-code-language)


3592-3592: Fenced code blocks should have a language specified
null

(MD040, fenced-code-language)


3643-3643: Fenced code blocks should have a language specified
null

(MD040, fenced-code-language)


3683-3683: Fenced code blocks should have a language specified
null

(MD040, fenced-code-language)

⏰ Context from checks skipped due to timeout of 90000ms (4)
  • GitHub Check: e2e-tests
  • GitHub Check: test
  • GitHub Check: test
  • GitHub Check: e2e-tests
🔇 Additional comments (11)
src/commands/rooms/reactions/send.ts (1)

133-133: Update aligned with API simplification.

The removal of the explicit empty reactions option from the room retrieval call reflects the dependency upgrade to @ably/chat v0.7.0, which no longer requires these empty options.

test/unit/services/config-manager.test.ts (1)

11-12: Import statement refactoring improves clarity.

Separating the named import expect from the namespace import chai follows better import organization practices while maintaining the same functionality.

package.json (1)

70-70:

Details

❓ Verification inconclusive

Dependency version upgrade to support new chat reaction features.

Upgrading @ably/chat from v0.5.1 to v0.7.0 is necessary to support the new chat message reaction functionality being implemented in this PR.


🏁 Script executed:

#!/bin/bash
# Check for breaking changes or notable features in the new version

echo "Checking for new features in @ably/chat v0.7.0 compared to v0.5.1"
gh api graphql -f query='
{
  repository(owner: "ably", name: "ably-chat-js") {
    releases(first: 10) {
      nodes {
        name
        tagName
        description
      }
    }
  }
}' | jq '.data.repository.releases.nodes[] | select(.tagName == "v0.7.0" or .tagName == "v0.6.0")'

Length of output: 428


🏁 Script executed:

#!/bin/bash
set -e

# List published releases for @ably/chat
echo "Releases (via GitHub API):"
gh api repos/ably/ably-chat-js/releases --jq '.[].tag_name'

# List all tags in the repo
echo ""
echo "Tags (via GitHub API):"
gh api repos/ably/ably-chat-js/tags --jq '.[].name'

# Locate the changelog file in the repo
changelog=$(git ls-files | grep -i CHANGELOG.md || true)
if [ -n "$changelog" ]; then
  echo ""
  echo "Found changelog at: $changelog"
  echo ""
  echo "Changelog entries around v0.7.0:"
  # Print lines from v0.7.0 section to the next version heading
  sed -n '/## *\[?0\.7\.0\]*/,/## *\[?[0-9]\+\.[0-9]\+\.[0-9]\]*/p' "$changelog" || echo "No entry for v0.7.0 found"
else
  echo ""
  echo "No CHANGELOG.md file found in repository"
fi

Length of output: 717


🏁 Script executed:

#!/bin/bash
set -e

echo "Fetching release notes for @ably/chat v0.7.0 from GitHub:"
gh api repos/ably/ably-chat-js/releases/tags/0.7.0 --jq '.body'

echo ""
echo "Filtering for mentions of 'reaction' in v0.7.0 release notes:"
gh api repos/ably/ably-chat-js/releases/tags/0.7.0 --jq '.body' | grep -i reaction -C2 || echo "No 'reaction' mentions found"

Length of output: 1738


Upgrade @ably/chat to v0.7.0 for experimental message reactions

  • package.json (line 70):
    -  "@ably/chat": "^0.5.1",
    +  "@ably/chat": "^0.7.0",
  • v0.7.0 adds experimental message reactions (PR #473), which your new feature relies on.
  • Note: v0.7.0 also includes minor breaking changes:
    • Single-channel architecture
    • Room options restructuring
    • Typing API changes (typing.keystroke() replaces typing.start())
    onDiscontinuity now scoped at the room level
  • Please verify that all affected code paths have been updated to accommodate these changes.
src/commands/rooms/reactions/subscribe.ts (1)

112-112: Update aligned with API simplification.

The removal of explicit options from this.chatClient.rooms.get(roomId) is consistent with similar changes in other files and reflects the updated API in @ably/chat v0.7.0.

src/commands/rooms/typing/subscribe.ts (2)

101-101: Update aligned with API simplification.

Removal of the explicit typing options from the room retrieval call aligns with similar changes across the codebase and the updated API patterns in @ably/chat v0.7.0.


167-167: Type annotation simplification.

Removing the explicit type annotation for the typing event parameter allows the TypeScript compiler to infer the type automatically, which is now possible with the updated library version.

src/commands/rooms/typing/start.ts (2)

8-16: Comments and constants contradict each other

The block comment states heartbeats are throttled to once every 10 s, yet the KEYSTROKE_INTERVAL is set to 450 ms. Either the comment is stale or we are sending a redundant 20× flood of keystrokes.

Update the documentation or increase the interval (e.g. 9 000 ms) to match the library’s throttle period and save unnecessary network traffic.


165-177: 🛠️ Refactor suggestion

High-frequency setInterval can leak rejected promises

At 450 ms we create ~133 Promises per minute; unhandled rejections could accumulate if the connection drops. Consider:

• Moving the interval to ≥9 000 ms (see first comment)
• Using an async IIFE inside the timer with await + try/catch to avoid silent rejections.

- this.typingIntervalId = setInterval(() => {
-   room.typing.keystroke().catch((error: Error) => { … });
- }, KEYSTROKE_INTERVAL);
+ this.typingIntervalId = setInterval(async () => {
+   try {
+     await room.typing.keystroke();
+   } catch (error) {
+     this.logCliEvent(flags, "typing", "keystrokeError", …);
+   }
+ }, KEYSTROKE_INTERVAL);

Likely an incorrect or invalid review comment.

README.md (2)

153-156: New reaction commands listed correctly in commands overview
The rooms messages reactions subcommands are inserted in the commands list following the existing pattern (get, reactions group, send, subscribe). The indentation and link anchors follow the established style.


3567-3570: Document new message-reactions command group
The ## \ably rooms messages reactions`` section correctly introduces the new command group. The heading level and style match existing sections.

src/commands/rooms/messages/reactions/remove.ts (1)

183-185: Confirm delete is the correct API call

In SDK versions prior to @ably/chat@0.7.0 the method was named remove, not delete.
If users install the CLI with an older lock-file they will face a runtime failure (room.messages.reactions.delete is not a function).

Ask npm/yarn to respect the ^0.7.0 minimum or guard the call:

if (typeof room.messages.reactions.delete !== "function") {
  this.error("Reaction deletion requires @ably/chat >= 0.7.0");
}

Comment on lines +88 to +112
this.ablyClient.connection.on(
(stateChange: Ably.ConnectionStateChange) => {
this.logCliEvent(
flags,
"connection",
stateChange.current,
`Realtime connection state changed to ${stateChange.current}`,
{ reason: stateChange.reason },
);
},
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Listener should be detached to avoid memory leaks

You attach a permanent connection.on listener to the underlying ablyClient but never remove it.
After repeated CLI invocations in the same long–lived process (tests, REPL, or in-process usage), listeners will accumulate and trigger the Node.js MaxListenersExceededWarning.

 this.ablyClient.connection.on(
   (stateChange: Ably.ConnectionStateChange) => {
     /* … */
   },
 );
+//   ⬆ keep the unsubscribe handle and call `off` once finished

Consider capturing the returned listener function (or call once) and explicitly remove it in your cleanup / finally block, similar to how you handle room status subscriptions.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In src/commands/rooms/messages/reactions/remove.ts around lines 88 to 99, the
connection.on listener on ablyClient is attached permanently without removal,
causing potential memory leaks and MaxListenersExceededWarning. Modify the code
to capture the listener function returned by connection.on or use
connection.once instead, and ensure this listener is explicitly removed or
cleaned up in the appropriate cleanup or finally block, similar to how room
status subscriptions are handled.

Comment on lines +122 to +110
// Validate count for Multiple type
if (flags.type === "multiple" && flags.count !== undefined && flags.count <= 0) {
const errorMsg = "Count must be a positive integer for Multiple type reactions";
this.logCliEvent(flags, "reaction", "invalidCount", errorMsg, {
error: errorMsg,
count: flags.count,
});
if (this.shouldOutputJson(flags)) {
this.log(
this.formatJsonOutput(
{ error: errorMsg, roomId, success: false },
flags,
),
);
} else {
this.error(errorMsg);
}
return;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

Enforce count when type === "multiple"

The current validation only rejects non-positive counts but allows a missing count, resulting in undefined being sent which the SDK rejects.

-if (flags.type === "multiple" && flags.count !== undefined && flags.count <= 0) {
+if (flags.type === "multiple" && (flags.count === undefined || flags.count <= 0)) {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Validate count for Multiple type
if (flags.type === "multiple" && flags.count !== undefined && flags.count <= 0) {
const errorMsg = "Count must be a positive integer for Multiple type reactions";
this.logCliEvent(flags, "reaction", "invalidCount", errorMsg, {
error: errorMsg,
count: flags.count,
});
if (this.shouldOutputJson(flags)) {
this.log(
this.formatJsonOutput(
{ error: errorMsg, roomId, success: false },
flags,
),
);
} else {
this.error(errorMsg);
}
return;
}
// Validate count for Multiple type
if (flags.type === "multiple" && (flags.count === undefined || flags.count <= 0)) {
const errorMsg = "Count must be a positive integer for Multiple type reactions";
this.logCliEvent(flags, "reaction", "invalidCount", errorMsg, {
error: errorMsg,
count: flags.count,
});
if (this.shouldOutputJson(flags)) {
this.log(
this.formatJsonOutput(
{ error: errorMsg, roomId, success: false },
flags,
),
);
} else {
this.error(errorMsg);
}
return;
}
🤖 Prompt for AI Agents
In src/commands/rooms/messages/reactions/add.ts around lines 122 to 140, the
validation for the "multiple" type reactions only rejects counts that are
non-positive but does not reject missing counts, allowing undefined to be sent.
Update the condition to also check if the count is undefined or not a positive
integer, and reject the input if count is missing or invalid by logging the
error and stopping execution as currently done.

Comment on lines +90 to +121
try {
// Parse metadata if provided
if (flags.metadata) {
try {
this.metadataObj = JSON.parse(flags.metadata);
this.logCliEvent(
flags,
"reaction",
"metadataParsed",
"Metadata parsed successfully",
{ metadata: this.metadataObj },
);
} catch (error) {
const errorMsg = `Invalid metadata JSON: ${error instanceof Error ? error.message : String(error)}`;
this.logCliEvent(flags, "reaction", "metadataParseError", errorMsg, {
error: errorMsg,
roomId,
});
if (this.shouldOutputJson(flags)) {
this.log(
this.formatJsonOutput(
{ error: errorMsg, roomId, success: false },
flags,
),
);
} else {
this.error(errorMsg);
}
return;
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

Parsed metadata never reaches the server

metadataObj is parsed and logged but later not attached to the reaction sent to Ably; users will believe the data was stored although it’s silently dropped.

 const reactionParams: {
   name: string;
   type?: MessageReactionType;
   count?: number;
+  metadata?: Record<string, unknown>;
 } = {
   name: reaction,
 };
@@
 if (flags.type === "multiple" && flags.count) {
   reactionParams.count = flags.count;
 }
+if (this.metadataObj) {
+  reactionParams.metadata = this.metadataObj;
+}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
try {
// Parse metadata if provided
if (flags.metadata) {
try {
this.metadataObj = JSON.parse(flags.metadata);
this.logCliEvent(
flags,
"reaction",
"metadataParsed",
"Metadata parsed successfully",
{ metadata: this.metadataObj },
);
} catch (error) {
const errorMsg = `Invalid metadata JSON: ${error instanceof Error ? error.message : String(error)}`;
this.logCliEvent(flags, "reaction", "metadataParseError", errorMsg, {
error: errorMsg,
roomId,
});
if (this.shouldOutputJson(flags)) {
this.log(
this.formatJsonOutput(
{ error: errorMsg, roomId, success: false },
flags,
),
);
} else {
this.error(errorMsg);
}
return;
}
}
const reactionParams: {
name: string;
type?: MessageReactionType;
count?: number;
metadata?: Record<string, unknown>;
} = {
name: reaction,
};
if (flags.type === "multiple" && flags.count) {
reactionParams.count = flags.count;
}
if (this.metadataObj) {
reactionParams.metadata = this.metadataObj;
}
🤖 Prompt for AI Agents
In src/commands/rooms/messages/reactions/add.ts between lines 90 and 121, the
parsed metadata is stored in this.metadataObj and logged but never included in
the reaction payload sent to the server. To fix this, ensure that the parsed
metadata is attached to the reaction object or request payload before sending it
to Ably, so the metadata is actually transmitted and stored as intended.

Comment on lines +522 to +515
process.exit(0);
};

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Avoid forcing process.exit(0) inside an oclif command

oclif handles process termination; calling process.exit bypasses its error handling, can cut off pending I/O (e.g. flushed logs) and complicates test harnesses.

Simply resolving the promise is sufficient:

-          resolve();
-
-          process.exit(0);
+          resolve(); // let oclif regain control
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
process.exit(0);
};
resolve(); // let oclif regain control
};
🤖 Prompt for AI Agents
In src/commands/rooms/messages/reactions/subscribe.ts around lines 522 to 523,
remove the call to process.exit(0) inside the oclif command. Instead of forcing
process termination, allow the command to complete naturally by resolving the
promise or returning from the function, so that oclif can handle process exit
and cleanup properly.

@vladvelici vladvelici changed the base branch from main to upgrade-chat-js May 21, 2025 15:23
@splindsay-92 splindsay-92 force-pushed the upgrade-chat-js branch 2 times, most recently from f4814a9 to 8f810ff Compare May 22, 2025 16:15
Base automatically changed from upgrade-chat-js to main May 22, 2025 20:54
@mattheworiordan

Copy link
Copy Markdown
Member

Tests are failing and this needs a rebase @vladvelici given there were build issues on the commit you based off.

@vladvelici vladvelici force-pushed the add-chat-message-reactions branch from 0dcef6f to d630cf8 Compare May 28, 2025 07:19
@vladvelici vladvelici force-pushed the add-chat-message-reactions branch from d630cf8 to 883e807 Compare May 28, 2025 08:13
@mattheworiordan mattheworiordan merged commit 92a29f1 into main May 29, 2025
5 of 8 checks passed
@mattheworiordan

Copy link
Copy Markdown
Member

Thanks @vladvelici

@mattheworiordan mattheworiordan deleted the add-chat-message-reactions branch May 29, 2025 16:20
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.

2 participants