[DX-790] Add support for Chat message updates and deletes#168
Open
umair-ably wants to merge 1 commit intomainfrom
Open
[DX-790] Add support for Chat message updates and deletes#168umair-ably wants to merge 1 commit intomainfrom
umair-ably wants to merge 1 commit intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Author
|
@coderabbitai review |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe PR adds two new CLI commands for Ably Chat room message operations ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as Delete Command
participant ChatClient
participant Room
participant MessageAPI as Messages API
User->>CLI: Execute: rooms messages delete ROOM SERIAL
CLI->>ChatClient: Initialize chat client
ChatClient-->>CLI: Client ready
CLI->>Room: Get and attach room
Room-->>CLI: Room attached
CLI->>MessageAPI: room.messages.delete(serial, OperationDetails?)
MessageAPI-->>CLI: Deletion confirmed (versionSerial)
CLI->>User: Output success message + versionSerial
sequenceDiagram
participant User
participant CLI as Update Command
participant ChatClient
participant Room
participant MessageAPI as Messages API
User->>CLI: Execute: rooms messages update ROOM SERIAL TEXT
Note over CLI: Parse --metadata, --headers, --description flags
CLI->>ChatClient: Initialize chat client
ChatClient-->>CLI: Client ready
CLI->>Room: Get and attach room
Room-->>CLI: Room attached
CLI->>MessageAPI: room.messages.update(serial, text, {metadata?, headers?, OperationDetails?})
MessageAPI-->>CLI: Update confirmed (versionSerial)
CLI->>User: Output success message + versionSerial
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Adds new CLI commands for updating and deleting Ably Chat room messages, including unit tests and README command documentation, plus a small consistency tweak to ensure execution stops after fail() in several existing room commands.
Changes:
- Add
rooms messages updatecommand with--metadata,--headers, and--descriptionsupport and JSON/human output. - Add
rooms messages deletecommand with optional--descriptionand JSON/human output. - Update mocks, docs, and several existing commands to
return this.fail(...)for consistent early-exit flow.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/commands/rooms/messages/update.test.ts | Unit tests covering update behavior, flag handling, JSON output, and error cases. |
| test/unit/commands/rooms/messages/delete.test.ts | Unit tests covering delete behavior, description handling, JSON output, and API error propagation. |
| test/helpers/mock-ably-chat.ts | Extends the room messages mock to include update and delete. |
| src/commands/rooms/messages/update.ts | Implements the new message update command, including JSON parsing/validation for metadata and headers. |
| src/commands/rooms/messages/delete.ts | Implements the new message delete command, including optional operation details. |
| src/commands/rooms/messages/index.ts | Adds topic examples for the new update and delete subcommands. |
| README.md | Adds generated documentation sections and TOC entries for the new commands. |
| src/commands/rooms/typing/subscribe.ts | Returns after fail() when chat client init fails. |
| src/commands/rooms/typing/keystroke.ts | Returns after fail() when chat client init fails. |
| src/commands/rooms/reactions/subscribe.ts | Returns after fail() when chat client init fails. |
| src/commands/rooms/reactions/send.ts | Returns after fail() when chat client init fails. |
| src/commands/rooms/occupancy/subscribe.ts | Returns after fail() when chat client init fails. |
| src/commands/rooms/occupancy/get.ts | Returns after fail() when chat client init fails. |
| src/commands/rooms/messages/send.ts | Returns after fail() when chat client init fails. |
| src/commands/rooms/messages/reactions/subscribe.ts | Returns after fail() when chat client init fails. |
| src/commands/rooms/messages/reactions/send.ts | Returns after fail() when chat client init fails. |
| src/commands/rooms/messages/reactions/remove.ts | Returns after fail() when chat client init fails. |
| src/commands/rooms/messages/history.ts | Returns after fail() when chat client init fails. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/rooms/messages/reactions/send.ts (1)
62-67:⚠️ Potential issue | 🟠 MajorReturn after invalid
--countvalidation too.Line 62 still calls
this.fail(...)without exiting. That leaves--type multiple --count <= 0able to continue into room attach andmessages.reactions.send(...)after the command has already rejected the input.Suggested fix
if ( flags.type === "multiple" && flags.count !== undefined && flags.count <= 0 ) { - this.fail( + return this.fail( "Count must be a positive integer for Multiple type reactions", flags, "roomMessageReactionSend", { room, count: flags.count }, ); }Also applies to: 74-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/rooms/messages/reactions/send.ts` around lines 62 - 67, Validation branches call this.fail(...) when count is invalid but do not stop execution, allowing the flow to continue into room attach and messages.reactions.send(...); after each validation this.fail(...) (specifically the Multiple-type count checks) add an immediate return to exit the handler early so the method does not proceed after rejecting the input.
🧹 Nitpick comments (1)
test/unit/commands/rooms/messages/delete.test.ts (1)
12-14: Optional cleanup: localbeforeEachseems redundant.
getMockAblyChat()here doesn’t add isolation beyond the global unit-test setup/reset and can likely be removed to reduce noise.Based on learnings: In the ably/ably-cli repository, unit test mocks are centralized in
test/unit/setup.tswith global initialization/reset before each test, and per-file lifecycle setup should avoid duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/rooms/messages/delete.test.ts` around lines 12 - 14, Remove the redundant per-file setup by deleting the local beforeEach that calls getMockAblyChat() in this test file; rely on the global test setup/reset (test/unit/setup.ts) instead, and if any test in this file actually depends on per-file mock state, replace the beforeEach with only the minimal calls required (referencing getMockAblyChat and any specific mocks) and run the unit tests to confirm no regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 2889-2914: Add the missing fenced-code language marker "text" to
the two usage/example blocks for the commands so markdownlint MD040 is
satisfied: update the opening triple-backtick fence for the "ably rooms messages
delete ROOM SERIAL ..." block and the "ably rooms messages update ROOM SERIAL
TEXT ..." block to use ```text (leave the closing ``` unchanged); search for
those exact command-usage lines to locate the blocks and change their opening
fences to include the language identifier.
In `@src/commands/rooms/messages/history.ts`:
- Around line 69-73: The time-range validation in
src/commands/rooms/messages/history.ts currently calls this.fail(...) without
returning, allowing execution to continue into room.messages.history(...) after
reporting invalid --start/--end; update the validation branch so it returns the
result of this.fail(...) (matching the early exit used when client creation
fails) to stop further execution—specifically change the bare this.fail(...)
used for start/end validation to return this.fail(..., flags,
"roomMessageHistory") so the command exits immediately on invalid time-range.
In `@src/commands/rooms/messages/update.ts`:
- Around line 56-133: Move the JSON parsing and validation of flags.metadata and
flags.headers to before calling createChatClient() and
setupConnectionStateLogging(); specifically, in the command handler parse the
flags when flags.metadata !== undefined and flags.headers !== undefined (so
empty string is treated as provided), JSON.parse them inside try/catch, validate
the result is a non-null object and not an array, call this.fail(...) on
parse/shape errors using errorMessage, and only call this.logCliEvent(...) after
the object validation succeeds; keep createChatClient() and
setupConnectionStateLogging(chatClient.realtime, flags) after successful flag
validation.
---
Outside diff comments:
In `@src/commands/rooms/messages/reactions/send.ts`:
- Around line 62-67: Validation branches call this.fail(...) when count is
invalid but do not stop execution, allowing the flow to continue into room
attach and messages.reactions.send(...); after each validation this.fail(...)
(specifically the Multiple-type count checks) add an immediate return to exit
the handler early so the method does not proceed after rejecting the input.
---
Nitpick comments:
In `@test/unit/commands/rooms/messages/delete.test.ts`:
- Around line 12-14: Remove the redundant per-file setup by deleting the local
beforeEach that calls getMockAblyChat() in this test file; rely on the global
test setup/reset (test/unit/setup.ts) instead, and if any test in this file
actually depends on per-file mock state, replace the beforeEach with only the
minimal calls required (referencing getMockAblyChat and any specific mocks) and
run the unit tests to confirm no regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 16a0e997-c1b6-4010-a65a-31160c04a097
📒 Files selected for processing (18)
README.mdsrc/commands/rooms/messages/delete.tssrc/commands/rooms/messages/history.tssrc/commands/rooms/messages/index.tssrc/commands/rooms/messages/reactions/remove.tssrc/commands/rooms/messages/reactions/send.tssrc/commands/rooms/messages/reactions/subscribe.tssrc/commands/rooms/messages/send.tssrc/commands/rooms/messages/update.tssrc/commands/rooms/occupancy/get.tssrc/commands/rooms/occupancy/subscribe.tssrc/commands/rooms/reactions/send.tssrc/commands/rooms/reactions/subscribe.tssrc/commands/rooms/typing/keystroke.tssrc/commands/rooms/typing/subscribe.tstest/helpers/mock-ably-chat.tstest/unit/commands/rooms/messages/delete.test.tstest/unit/commands/rooms/messages/update.test.ts
umair-ably
force-pushed
the
DX-790-chat-update-delete
branch
from
592961f to
2c735b9
Compare
umair-ably
force-pushed
the
DX-790-chat-update-delete
branch
from
2c735b9 to
e4f9e11
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ably rooms messages updateandably rooms messages deletecommands for Chat message lifecycle management--jsonoutput,--descriptionfor operation details, and--client-id--metadataand--headersflags for rich message updatesreturnbeforethis.fail()calls across 9 existing Chat commands to satisfy TypeScript control flow (sincefail()returnsnever, thereturnmakes it explicit to the type checker that execution stops)Changes
src/commands/rooms/messages/update.tssrc/commands/rooms/messages/delete.tssrc/commands/rooms/messages/index.tstest/unit/commands/rooms/messages/update.test.tstest/unit/commands/rooms/messages/delete.test.tstest/helpers/mock-ably-chat.tsupdateanddeletemocks toMockRoomMessagesreturnbeforethis.fail()for correct control flowREADME.mdReview strategy
room.messages.update(serial, params, details)androom.messages.delete(serial, details)match the@ably/chatSDK signaturessrc/commands/rooms/messages/update.ts,delete.ts--metadataand--headersare properly parsed, validated (must be JSON objects, not arrays/primitives), and passed throughupdate.tslines 78-130--descriptiondelete.ts{ description }when--descriptionis set,undefinedotherwise — verify no empty object leaks throughupdate.ts,delete.tslogJsonResultpayloads includeroom,serial,versionSerial(andupdatedTextfor update)formatResource(),formatProgress(),formatSuccess()correctlyreturn this.fail()fixreturnbeforethis.fail()— verify no behavior change, just type safetymock-ably-chat.tsupdate/delete mocks return shapes that match the SDK'sMessagetypetest/helpers/mock-ably-chat.tssendmessages/send.tsupdate.ts,delete.tsvssend.tsTest plan
pnpm test:unitpasses (including new update/delete tests)pnpm exec eslint .shows 0 errorsably rooms messages update <room> <serial> "new text"works against a real Chat roomably rooms messages delete <room> <serial>works against a real Chat room--jsonoutput produces valid NDJSON envelopes for both commands--descriptionis passed through correctly in both commands--metadata/--headers(non-JSON, arrays, primitives) produce clear error messagesSummary by CodeRabbit
New Features
deletecommand to remove messages from Ably Chat rooms.updatecommand to modify existing messages in Ably Chat rooms.Bug Fixes
Documentation
Tests