[DX-788] Add channels update, delete, and append commands#165
[DX-788] Add channels update, delete, and append commands#165umair-ably merged 3 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
WalkthroughIntroduces three new Ably channel message mutation commands (append, delete, update) with comprehensive test coverage, adds serial/version/action field logging to existing channel operations, updates the ably dependency to ^2.19.0, enhances CLI documentation, and adds a message preparation utility function. Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI
participant RestClient
participant Channel
participant Ably
User->>CLI: ably channels update CHANNEL SERIAL MESSAGE --clientIdFlag
CLI->>RestClient: Create Ably REST client
RestClient-->>CLI: client ready
CLI->>Channel: Get channel(CHANNEL)
Channel-->>CLI: channel instance
CLI->>CLI: Prepare message with serial,<br/>encoding, name from flags
CLI->>Ably: channel.updateMessage(message, operation?)
Ably-->>Ably: Process mutation<br/>with clientId
Ably-->>CLI: { versionSerial: "..." }
CLI->>CLI: Log channelUpdate event<br/>(channel, serial, versionSerial)
CLI->>User: Output result (JSON or formatted)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
umair-ably
force-pushed
the
DX-788-update-delete-appends
branch
from
d90401f to
f4b2096
Compare
umair-ably
force-pushed
the
DX-788-update-delete-appends
branch
from
f4b2096 to
60afef9
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Adds support for Ably message mutation operations (append/update/delete) in the CLI, along with related output enhancements and SDK updates.
Changes:
- Introduces new
channels:append,channels:update, andchannels:deletecommands (plus unit tests) using Ably REST message mutations. - Enhances message visibility by outputting publish serials and displaying
action/serial/versionin subscribe + history output. - Updates mocks and bumps the
ablySDK dependency to support the new mutation/publish result behaviors.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/commands/channels/append.ts |
New command to append to an existing message by serial (REST mutation). |
src/commands/channels/update.ts |
New command to update an existing message by serial (REST mutation). |
src/commands/channels/delete.ts |
New command to delete an existing message by serial (REST mutation). |
src/utils/message.ts |
Adds prepareMessageFromInput helper to parse user message input for mutation APIs. |
src/commands/channels/publish.ts |
Captures/prints publish serial when available; updates publisher typing to allow publish result return. |
src/commands/channels/subscribe.ts |
Includes action/serial/version in JSON + human-readable subscribe output. |
src/commands/channels/history.ts |
Includes action/serial/version in human-readable history output. |
test/helpers/mock-ably-rest.ts |
Extends REST channel mock with update/delete/append methods and publish serials. |
test/unit/commands/channels/append.test.ts |
New unit tests for channels:append. |
test/unit/commands/channels/update.test.ts |
New unit tests for channels:update. |
test/unit/commands/channels/delete.test.ts |
New unit tests for channels:delete. |
README.md |
Documents the new channels subcommands and usage. |
package.json |
Bumps ably dependency version. |
pnpm-lock.yaml |
Lockfile updates corresponding to dependency bumps. |
AGENTS.md |
Updates documented guidance for when clientIdFlag should be used (includes REST mutations). |
.claude/skills/ably-review/SKILL.md |
Updates review checklist to include mutation commands in clientIdFlag expectations. |
.claude/skills/ably-codebase-review/SKILL.md |
Updates codebase-review checklist similarly for mutation commands. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.claude/skills/ably-codebase-review/SKILL.md (1)
120-126:⚠️ Potential issue | 🟠 MajorKeep the codebase-review skill aligned with the repo's
clientIdFlagrule.Encoding mutations as another class of commands that "must" carry
clientIdFlagcontradicts the current command guideline and will make this audit skill report false deviations.As per coding guidelines, "Include
clientIdFlagonly on commands that create a realtime connection (publish, subscribe, presence enter/subscribe, spaces enter/get/subscribe, locks acquire/get/subscribe, cursors set/get/subscribe, locations set/get/subscribe, or callspace.enter())."Also applies to: 131-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/ably-codebase-review/SKILL.md around lines 120 - 126, Update the SKILL.md rule so the codebase-review skill does not require clientIdFlag for all mutation commands; instead, limit clientIdFlag enforcement to commands that create realtime connections as defined by the coding guidelines (e.g., publish, subscribe, presence enter/subscribe, spaces enter/get/subscribe, locks acquire/get/subscribe, cursors set/get/subscribe, locations set/get/subscribe, and any command that calls space.enter()); modify the cross-reference block (the paragraph that currently says "Commands creating realtime connections or performing mutations (publish, update, delete, append) must have clientIdFlag") to remove mutations and list the exact allowed command types, keeping the identifier clientIdFlag and ensuring ControlBaseCommand.globalFlags and other rules remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/ably-review/SKILL.md:
- Around line 102-106: Update the skill's flag-architecture check to not require
clientIdFlag for REST mutation commands: modify the instructions around the
grep/list of flag spreads (productApiFlags, clientIdFlag, durationFlag,
rewindFlag, timeRangeFlags, ControlBaseCommand.globalFlags) and the rule in step
3 so it only mandates clientIdFlag for commands that create a realtime
connection (e.g., publish, subscribe, presence enter/subscribe, spaces
enter/get/subscribe, locks acquire/get/subscribe, cursors set/get/subscribe,
locations set/get/subscribe, or call space.enter()), and explicitly exclude
REST-only mutations like update/delete/append from requiring clientIdFlag; keep
the LSP goToDefinition guidance for ambiguous imports to verify flags resolve to
src/flags.ts.
In `@README.md`:
- Around line 1395-1428: The fenced code blocks added for the CLI usage/examples
are missing language tags and trigger markdownlint warnings; update each
triple-backtick fence (the usage/help blocks such as the
"USAGE/ARGUMENTS/FLAGS/DESCRIPTION/EXAMPLES" blocks) to include a language tag
like text (e.g., ```text) so the generated help remains readable and the
markdownlint warnings are cleared.
In `@src/commands/channels/append.ts`:
- Around line 41-55: The flags object for this REST-only command incorrectly
includes clientIdFlag; remove clientIdFlag from the static override flags spread
so the class no longer exposes --client-id (leave productApiFlags intact), and
verify there are no references to clientIdFlag elsewhere in this file (e.g., in
run() which creates an Ably REST client) so compilation/type checks still pass.
In `@src/commands/channels/publish.ts`:
- Around line 271-282: The per-message success path in publish flow ignores
publishResult.serials for multi-message publishes: capture
publishResult?.serials[0] into serial (as you already do) but ensure this serial
is attached to the per-message result object and included in the emitted
messagePublished payload for count > 1; update the code around
publisher(message) / publishResult / result (and the emission of
messagePublished) to always include serial when defined (i.e., add serial to the
result and to the event payload rather than omitting it for multi-message
flows).
In `@src/utils/message.ts`:
- Around line 33-43: The parsing logic in the message builder drops or
misassigns an "extras" field when extracting name and data; update the flow
around the existing message/messageData/flags handling so that if
messageData.extras exists you move it into message.extras (and delete it from
messageData) before deciding on message.data, and when deciding message.data use
"data" in messageData first, otherwise if messageData has other keys (after
removing extras and name) assign that object to message.data, but do not treat a
sole extras object as data; ensure you delete moved keys from messageData to
avoid duplication.
In `@test/unit/commands/channels/append.test.ts`:
- Around line 10-25: Remove the top-level describe("channels:append command",
...) wrapper so the file's five required describe suites live at the top level;
keep the existing beforeEach(getMockAblyRest), retain
standardHelpTests("channels:append", import.meta.url),
standardArgValidationTests("channels:append", import.meta.url, { requiredArgs:
[...] }), standardFlagTests("channels:append", import.meta.url, [...]) and the
local describe blocks for "functionality" and "error handling" unchanged,
ensuring the final file contains exactly the five top-level describe blocks
named 'help', 'argument validation', 'functionality', 'flags', and 'error
handling'.
In `@test/unit/commands/channels/delete.test.ts`:
- Around line 10-22: Remove the extra outer describe("channels:delete command")
wrapper and replace it with exactly five top-level describe blocks named "help",
"argument validation", "functionality", "flags", and "error handling"; call
standardHelpTests("channels:delete", import.meta.url) inside the "help" block,
standardArgValidationTests("channels:delete", import.meta.url, { requiredArgs:
["test-channel"] }) inside the "argument validation" block, and
standardFlagTests("channels:delete", import.meta.url,
["--json","--description"]) inside the "flags" block, and move getMockAblyRest()
into a beforeEach that applies to the "functionality" and "error handling"
blocks (or a top-level beforeEach outside the describe blocks) so the mocks are
initialized for those suites; ensure the five describe block names match
exactly.
In `@test/unit/commands/channels/update.test.ts`:
- Around line 10-25: Remove the extra outer suite wrapper
describe("channels:update command") and its matching closing brace so the file
only contains the five required describe blocks provided by the helper calls;
keep the getMockAblyRest() invocation but move it into a top-level beforeEach
(or into the appropriate individual describe blocks) instead of inside the
removed wrapper — adjust the current beforeEach that calls getMockAblyRest()
accordingly and ensure the helper calls standardHelpTests,
standardArgValidationTests, and standardFlagTests remain unchanged.
---
Outside diff comments:
In @.claude/skills/ably-codebase-review/SKILL.md:
- Around line 120-126: Update the SKILL.md rule so the codebase-review skill
does not require clientIdFlag for all mutation commands; instead, limit
clientIdFlag enforcement to commands that create realtime connections as defined
by the coding guidelines (e.g., publish, subscribe, presence enter/subscribe,
spaces enter/get/subscribe, locks acquire/get/subscribe, cursors
set/get/subscribe, locations set/get/subscribe, and any command that calls
space.enter()); modify the cross-reference block (the paragraph that currently
says "Commands creating realtime connections or performing mutations (publish,
update, delete, append) must have clientIdFlag") to remove mutations and list
the exact allowed command types, keeping the identifier clientIdFlag and
ensuring ControlBaseCommand.globalFlags and other rules remain unchanged.
🪄 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: f3eb0405-ef1e-4191-ab9f-c3a70989f883
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
.claude/skills/ably-codebase-review/SKILL.md.claude/skills/ably-review/SKILL.mdAGENTS.mdREADME.mdpackage.jsonsrc/commands/channels/append.tssrc/commands/channels/delete.tssrc/commands/channels/history.tssrc/commands/channels/publish.tssrc/commands/channels/subscribe.tssrc/commands/channels/update.tssrc/utils/message.tstest/helpers/mock-ably-rest.tstest/unit/commands/channels/append.test.tstest/unit/commands/channels/delete.test.tstest/unit/commands/channels/update.test.ts
…mInput, expose serial in multi-message publish output
umair-ably
force-pushed
the
DX-264-rules
branch
from
88e1a67 to
5f14348
Compare
…mInput, expose serial in multi-message publish output
umair-ably
force-pushed
the
DX-788-update-delete-appends
branch
from
368a6ef to
e878183
Compare
umair-ably
force-pushed
the
DX-264-rules
branch
from
cdb4eae to
377a61c
Compare
…mInput, expose serial in multi-message publish output
umair-ably
force-pushed
the
DX-788-update-delete-appends
branch
from
e878183 to
9ce23eb
Compare
…mInput, expose serial in multi-message publish output
umair-ably
force-pushed
the
DX-788-update-delete-appends
branch
from
9ce23eb to
8918f6f
Compare
…mInput, expose serial in multi-message publish output
umair-ably
force-pushed
the
DX-788-update-delete-appends
branch
from
8918f6f to
ea916c2
Compare
sacOO7
left a comment
sacOO7
left a comment
There was a problem hiding this comment.
Running pnpm cli channels --help currently shows the following commands:
ably channels append Append data to a message on an Ably channel
ably channels batch-publish Publish messages to multiple Ably channels with a single request
ably channels delete Delete a message on an Ably channel
ably channels history Retrieve message history for a channel
ably channels inspect Open the Ably dashboard to inspect a specific channel
ably channels list List active channels using the channel enumeration API
ably channels occupancy Get occupancy metrics for a channel
ably channels presence Manage presence on Ably channels
ably channels publish Publish a message to an Ably channel
ably channels subscribe Subscribe to messages published on one or more Ably channels
ably channels update Update a message on an Ably channel
Currently, the following commands are related specifically to message operations:
ably channels append
ably channels batch-publish
ably channels delete
ably channels history
ably channels publish
ably channels subscribe
ably channels update
The remaining commands operate on other channel-level features and behaviors, rather than message operations:
ably channels inspect Open the Ably dashboard to inspect a specific channel
ably channels list List active channels using the channel enumeration API
ably channels occupancy Get occupancy metrics for a channel
ably channels presence Manage presence on Ably channels
Given this separation of responsibilities, it might make sense to introduce a dedicated command group such as ably channels messages (or ably channels message)
For example:
ably channels messages publish
wdyt?
I had exactly this thought but this is a wider product issue and not just a CLI issue... e.g. all of our SDKs have message operations under the channels namespace. Whilst a messages namespace/command group makes sense, it would make a lot less sense for the CLI to deviate from what our SDKs do |
…mInput, expose serial in multi-message publish output
sacOO7
left a comment
sacOO7
left a comment
There was a problem hiding this comment.
Thanks for addressing comments: )
LGTM !
|
@umair-ably Good to see these new commands added. However, I think we're missing an obvious utility command given why we added append functionality in the first place: streaming tokens. If an agent (or a human debugging) wants to use the CLI to simulate token streaming, they're not going to make 20 separate CLI calls to Looking at how AI Transport works, each token is a separate
Did you consider this? I'd suggest we add something like |
|
@umair-ably Adding to Matt's comment above, there was a ticket assigned to @matt423 in the AIT board for mutable messages support in the CLI that included that feature. Can you please chat with Matt A to make sure that the AIT-specific parts are tracked in a ticket and in the list of things to do? |
|
@mattheworiordan @rainbowFi this was missed during some handover and ticket rewrites. I've raised https://ably.atlassian.net/browse/DX-972 which also references another semi-related PR that's open. Will address both shortly |
need #164 in first
Summary
ably channels update,ably channels delete, andably channels append— enabling message versioning operations (edit, delete, append) from the CLIpublish,subscribe,history) to surface message versioning metadata:action,serial, andversionfieldssrc/utils/message.ts(prepareMessageFromInput)updateMessage/deleteMessage/appendMessagemethods andPublishResultreturn typepublishnow returns and displays the message serial, enabling users to chain publish → update/delete/append workflowsReview strategy
src/utils/message.tsprepareMessageFromInputfirst, sinceupdateandappenddepend on it. Check JSON parsing edge cases (primitives, arrays, objects).src/commands/channels/delete.tssrc/commands/channels/update.ts--name/--encoding/--descriptionflags. Compare with delete to verify consistency.src/commands/channels/append.tssrc/commands/channels/publish.tspublishercallback return type toPublishResult, now extracts and displays serial. Check theserials[0]access is safe.src/commands/channels/subscribe.ts,src/commands/channels/history.tsaction/serial/versionto output. Verify JSON envelope includes new fields and human output is guarded by!shouldOutputJson.test/unit/commands/channels/{update,delete,append}.test.ts,test/helpers/mock-ably-rest.tsREADME.md,package.json,pnpm-lock.yaml,.claude/skills/*Test plan
pnpm test:unitpasses (includes new tests for update, delete, append)pnpm exec eslint .has 0 errorsably channels publish test-ch "hello"→ note the serial in outputably channels update test-ch "<serial>" "edited"→ confirm success + version serialably channels append test-ch "<serial>" "more data"→ confirm successably channels delete test-ch "<serial>"→ confirm successably channels history test-ch→ verify action/serial/version shownably channels subscribe test-ch+ publish/update/delete in another terminal → verify versioning fields in real-time output--jsonand--pretty-jsonoutput correct for all new commands