Merged
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:
WalkthroughThis pull request introduces a new "channels annotations" feature for the Ably CLI with four subcommands: delete, get, publish, and subscribe. It includes complete command implementations, test helpers for mocking annotation functionality, comprehensive unit tests, and documentation updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
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 |
umair-ably
force-pushed
the
DX-789-annotations
branch
from
25e6e6a to
97c0915
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Adds support for Ably message annotations and richer message metadata (serial/version/annotations) across channel-related CLI commands, including new channels:annotations:* commands, and updates output formatting/tests accordingly.
Changes:
- Introduces
channels:annotationstopic commands (subscribe/publish/get/delete) with unit tests and README docs. - Updates channel subscribe/history and presence subscribe outputs to use structured formatting helpers, including version + annotations summary display.
- Extends test mocks (REST + Realtime) to support
channel.annotations.*APIs and adjusts JSON log envelope expectations in tests.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/unit/commands/channels/subscribe.test.ts | Updates message fixtures/assertions for serial/version/annotations and new JSON envelope shape. |
| test/unit/commands/channels/presence/subscribe.test.ts | Updates presence output assertions and adds a flags describe for --client-id. |
| test/unit/commands/channels/history.test.ts | Adds fixtures/assertions for serial/version/annotations; restructures describe blocks and updates “no messages” text. |
| test/unit/commands/channels/annotations/subscribe.test.ts | New tests for annotations subscribe (human + JSON + --type). |
| test/unit/commands/channels/annotations/publish.test.ts | New tests for publishing annotations and JSON result output. |
| test/unit/commands/channels/annotations/get.test.ts | New tests for fetching annotations (limit/empty/JSON/human output). |
| test/unit/commands/channels/annotations/delete.test.ts | New tests for deleting annotations and JSON result output. |
| test/helpers/mock-ably-rest.ts | Adds annotations mocks to REST channel mock. |
| test/helpers/mock-ably-realtime.ts | Adds annotations mocks to Realtime channel mock. |
| test/e2e/channels/channels-e2e.test.ts | Removes a brittle history output assertion. |
| src/utils/output.ts | Adds formatMessagesOutput / formatPresenceOutput and related field interfaces; adds annotations/version formatting. |
| src/commands/channels/subscribe.ts | Switches to formatMessagesOutput; includes serial/version/annotations; changes JSON event envelope shape. |
| src/commands/channels/presence/subscribe.ts | Switches to formatPresenceOutput; changes JSON event envelope shape. |
| src/commands/channels/history.ts | Switches to formatMessagesOutput; updates empty-history message. |
| src/commands/channels/annotations/index.ts | Adds channels:annotations topic command. |
| src/commands/channels/annotations/subscribe.ts | New annotations subscribe implementation (rewind/type filter, human + JSON output). |
| src/commands/channels/annotations/publish.ts | New annotations publish implementation (count/name/data/encoding flags, human + JSON output). |
| src/commands/channels/annotations/get.ts | New annotations get implementation (limit flag, human + JSON output). |
| src/commands/channels/annotations/delete.ts | New annotations delete implementation (realtime-based delete, human + JSON output). |
| README.md | Adds generated CLI docs for the new channels:annotations commands. |
💡 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: 2
🧹 Nitpick comments (4)
test/unit/commands/channels/annotations/get.test.ts (1)
101-113: UsecaptureJsonLogs()here too.Directly parsing
stdoutbypasses the NDJSON helper the unit-test suite uses for JSON assertions, so this gets fragile once the command emits more than one JSON line. Based on learnings, "For JSON output assertions, use the captureJsonLogs() helper from test/helpers/ndjson.ts."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/channels/annotations/get.test.ts` around lines 101 - 113, The test is directly parsing stdout which bypasses the NDJSON helper; replace the JSON.parse(stdout) usage with the test/helpers/ndjson.ts helper captureJsonLogs() when invoking the command (the same way other tests do) so you capture NDJSON lines robustly; call runCommand with captureJsonLogs() to retrieve parsed JSON entries for the "channels:annotations:get" invocation, then perform the same assertions against the first parsed object (e.g., result = parsed[0]) instead of parsing stdout.test/unit/commands/channels/annotations/delete.test.ts (1)
106-124: UsecaptureJsonLogs()for the JSON assertion.Parsing raw
stdouthere is brittle if the command later emits multiple NDJSON lines or verbose JSON events. The unit-test pattern in this repo is to assert JSON output through the helper instead ofJSON.parse(stdout). Based on learnings, "For JSON output assertions, use the captureJsonLogs() helper from test/helpers/ndjson.ts."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/channels/annotations/delete.test.ts` around lines 106 - 124, Replace the brittle JSON.parse(stdout) approach in the "should output JSON when --json flag is used" test with the shared helper captureJsonLogs() from test/helpers/ndjson.ts: call captureJsonLogs on the output of runCommand (instead of JSON.parse(stdout)), retrieve the captured JSON event (e.g., first object) and run the same assertions on its properties (type, command, success, channel, serial); update the test around the runCommand/capture point accordingly so it uses captureJsonLogs to parse NDJSON-style output reliably.src/commands/channels/annotations/index.ts (1)
4-5: PluralizecommandGroupfor the generated help heading.
BaseTopicCommandrenders this asAbly ${commandGroup} commands:. With the singular value here, the heading reads awkwardly asAbly Pub/Sub channel annotation commands:.✏️ Suggested fix
- protected commandGroup = "Pub/Sub channel annotation"; + protected commandGroup = "Pub/Sub channel annotations";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/channels/annotations/index.ts` around lines 4 - 5, The commandGroup string is singular and leads to an awkward help heading; update the protected property commandGroup in this class (where topicName = "channels:annotations") to a plural form such as "Pub/Sub channel annotations" so BaseTopicCommand will render the heading correctly; locate and change the protected commandGroup = "Pub/Sub channel annotation" to the pluralized text in the same file.src/commands/channels/annotations/delete.ts (1)
79-91: Includecountin logged and returned data for consistency.The
countflag is used when building the annotation (line 75) but is omitted from bothlogCliEvent(line 84) andlogJsonResult(line 89). For debugging and auditability, includecountalongsidename:Proposed fix
this.logCliEvent( flags, "annotationDelete", "annotationDeleted", `Deleted annotation on message ${serial} in channel ${channelName}`, - { channel: channelName, serial, type, name: flags.name }, + { channel: channelName, serial, type, name: flags.name, count: flags.count }, ); if (this.shouldOutputJson(flags)) { this.logJsonResult( - { channel: channelName, serial, type, name: flags.name }, + { channel: channelName, serial, type, name: flags.name, count: flags.count }, flags, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/channels/annotations/delete.ts` around lines 79 - 91, The logged and returned payloads omit the count used when building the annotation; update the calls to include flags.count so both audit logs and JSON output contain count. Specifically, modify the logCliEvent invocation (the call to this.logCliEvent) to add count: flags.count in the metadata object, and modify the this.logJsonResult call to include count: flags.count in the returned object alongside channel, serial, type, and name; keep using the existing flags variable and existing function signatures (logCliEvent, logJsonResult).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/channels/annotations/subscribe.ts`:
- Around line 112-125: The subscribe callback currently calls
formatMessageTimestamp(annotation.timestamp) unconditionally in function
callback which can produce "Invalid Date" or throw when annotation.timestamp is
missing; update callback to guard the field: compute timestamp only if
annotation.timestamp is present (e.g., const timestamp = annotation.timestamp ?
formatMessageTimestamp(annotation.timestamp) : undefined) and set
annotationEvent.timestamp accordingly (or omit it), so the handler mirrors the
optional behavior used in get.ts and avoids emitting invalid timestamps or
throwing in the stream callback.
In `@test/helpers/mock-ably-realtime.ts`:
- Around line 183-187: The annotations mock's subscribe function doesn't
transition its parent channel to attached like MockRealtimeChannel.subscribe
does; update the factory that creates the annotations object to accept the
parent channel's attach() method and modify annotations.subscribe (the vi.fn at
subscribe) to call that attach() when a subscription is made (before registering
the emitter callback), so channel.annotations.subscribe() will attach the
enclosing channel the same way MockRealtimeChannel.subscribe() does; ensure you
update any initialization sites to pass the parentChannel.attach reference into
the annotations mock.
---
Nitpick comments:
In `@src/commands/channels/annotations/delete.ts`:
- Around line 79-91: The logged and returned payloads omit the count used when
building the annotation; update the calls to include flags.count so both audit
logs and JSON output contain count. Specifically, modify the logCliEvent
invocation (the call to this.logCliEvent) to add count: flags.count in the
metadata object, and modify the this.logJsonResult call to include count:
flags.count in the returned object alongside channel, serial, type, and name;
keep using the existing flags variable and existing function signatures
(logCliEvent, logJsonResult).
In `@src/commands/channels/annotations/index.ts`:
- Around line 4-5: The commandGroup string is singular and leads to an awkward
help heading; update the protected property commandGroup in this class (where
topicName = "channels:annotations") to a plural form such as "Pub/Sub channel
annotations" so BaseTopicCommand will render the heading correctly; locate and
change the protected commandGroup = "Pub/Sub channel annotation" to the
pluralized text in the same file.
In `@test/unit/commands/channels/annotations/delete.test.ts`:
- Around line 106-124: Replace the brittle JSON.parse(stdout) approach in the
"should output JSON when --json flag is used" test with the shared helper
captureJsonLogs() from test/helpers/ndjson.ts: call captureJsonLogs on the
output of runCommand (instead of JSON.parse(stdout)), retrieve the captured JSON
event (e.g., first object) and run the same assertions on its properties (type,
command, success, channel, serial); update the test around the
runCommand/capture point accordingly so it uses captureJsonLogs to parse
NDJSON-style output reliably.
In `@test/unit/commands/channels/annotations/get.test.ts`:
- Around line 101-113: The test is directly parsing stdout which bypasses the
NDJSON helper; replace the JSON.parse(stdout) usage with the
test/helpers/ndjson.ts helper captureJsonLogs() when invoking the command (the
same way other tests do) so you capture NDJSON lines robustly; call runCommand
with captureJsonLogs() to retrieve parsed JSON entries for the
"channels:annotations:get" invocation, then perform the same assertions against
the first parsed object (e.g., result = parsed[0]) instead of parsing stdout.
🪄 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: a1d34e53-6679-42c0-96e1-3dad7ad379fc
📒 Files selected for processing (12)
README.mdsrc/commands/channels/annotations/delete.tssrc/commands/channels/annotations/get.tssrc/commands/channels/annotations/index.tssrc/commands/channels/annotations/publish.tssrc/commands/channels/annotations/subscribe.tstest/helpers/mock-ably-realtime.tstest/helpers/mock-ably-rest.tstest/unit/commands/channels/annotations/delete.test.tstest/unit/commands/channels/annotations/get.test.tstest/unit/commands/channels/annotations/publish.test.tstest/unit/commands/channels/annotations/subscribe.test.ts
umair-ably
changed the base branch from
fix/channel-messages-formatting-with-missing-fields
to
main
umair-ably
force-pushed
the
DX-789-annotations
branch
from
97c0915 to
0bd38fc
Compare
umair-ably
force-pushed
the
DX-789-annotations
branch
from
0bd38fc to
902cdca
Compare
umair-ably
force-pushed
the
DX-789-annotations
branch
from
902cdca to
de2b67a
Compare
umair-ably
force-pushed
the
DX-789-annotations
branch
from
de2b67a to
c654999
Compare
umair-ably
force-pushed
the
DX-789-annotations
branch
from
c654999 to
8735c53
Compare
Contributor
Author
|
LGTM - repo rules prevent me from also approving this @sacOO7 |
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
channels annotations publish— publish annotations (reactions, flags) on channel messageschannels annotations subscribe— real-time subscription to annotation events withANNOTATION_SUBSCRIBEchannel modechannels annotations get— retrieve annotations for a specific messagechannels annotations delete— remove annotations from a messagechannels annotationstopic index commandmock-ably-realtime.tsandmock-ably-rest.tstest helpersReview Strategy
src/commands/channels/annotations/index.tsBaseTopicCommandpatternsrc/commands/channels/annotations/publish.tschannel.annotations.publish(), JSON envelope,--name/--count/--data/--encodingflagssrc/commands/channels/annotations/delete.tsclient.close()in finallysrc/commands/channels/annotations/get.ts--limit,formatCountLabel/formatLimitWarningoutputsrc/commands/channels/annotations/subscribe.tsANNOTATION_SUBSCRIBEmode,--typefilter,--rewind/--durationflags,waitAndTrackCleanuplifecycletest/helpers/mock-ably-{realtime,rest}.tsMockRealtimeAnnotations/MockRestAnnotationsinterfaces, event emitter wiring for subscribetest/unit/commands/channels/annotations/*.test.tsTest Plan
pnpm preparesucceedspnpm exec eslint .— 0 errorspnpm test:unit— all passably channels annotations publish my-channel "serial" "reactions:flag.v1" --name thumbsupworksably channels annotations subscribe my-channelstreams events with--jsonand human outputably channels annotations get my-channel "serial"retrieves annotationsably channels annotations delete my-channel "serial" "reactions:flag.v1"removes annotations