[DX-978] Implemented missing rooms presence commands#179
[DX-978] Implemented missing rooms presence commands#179
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:
WalkthroughAdded two new room-presence CLI commands ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "CLI (get-all)"
participant REST as "Ably REST"
participant Channel as "Chat SDK Channel"
participant Formatter as "Output Formatter"
User->>CLI: ably rooms presence get-all ROOM --limit 100
CLI->>REST: create REST client
REST->>Channel: channels.get("ROOM::$chat")
CLI->>Channel: presence.get({ limit: 100 })
Channel-->>CLI: paginated presence members
CLI->>CLI: collectPaginatedResults()
CLI->>Formatter: emit fetched state / progress (non-JSON)
alt JSON mode
CLI->>Formatter: output JSON result (presenceMembers, hasMore, next, total)
else Human-readable
CLI->>Formatter: print formatted member list (clientId, connectionId, action, timestamp, Data)
end
Formatter-->>User: display results
sequenceDiagram
participant User
participant CLI as "CLI (update)"
participant ChatClient as "Ably Chat Client"
participant Room as "Room"
participant Presence as "Presence API"
participant Formatter as "Output Formatter"
User->>CLI: ably rooms presence update ROOM --data '{"k":"v"}' --duration 30
CLI->>ChatClient: create chat client
ChatClient->>Room: rooms.get("ROOM")
Room->>Room: attach()
CLI->>Presence: presence.enter(parsedData)
Presence-->>CLI: enter confirmed
CLI->>Presence: presence.update(parsedData)
Presence-->>CLI: update confirmed
alt JSON mode
CLI->>Formatter: output JSON result (presenceMessage action: update)
else Human-readable
CLI->>Formatter: print success, clientId, connectionId, Data, "Holding presence"
end
CLI->>Formatter: emit status "holding"
CLI->>CLI: waitAndTrackCleanup(duration)
Formatter-->>User: display result and hold state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
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 |
3659708 to
4b182f3
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
4afc78c to
eecc67a
Compare
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Adds the missing Rooms Presence CLI surface area and aligns rooms presence “enter/update” behavior with the newer JSON/status output patterns used across long-running commands.
Changes:
- Introduces
rooms presence get-all(REST-backed) androoms presence update(Chat SDK-backed) commands. - Refactors
rooms presence enterto emit progress output (human mode) and structured JSON result + “holding” status (JSON mode). - Adds unit tests and updates README command docs for the new rooms presence commands.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/commands/rooms/presence/update.test.ts | Adds unit coverage for the new rooms presence update command (human + JSON + error cases). |
| test/unit/commands/rooms/presence/get-all.test.ts | Adds unit coverage for the new rooms presence get-all command, including pagination and JSON output. |
| test/unit/commands/rooms/presence/enter.test.ts | Updates/refactors tests to match the new progress + JSON result/status behavior for rooms presence enter. |
| src/commands/rooms/presence/update.ts | Implements rooms presence update using Chat SDK, with long-running “hold” semantics and JSON result/status output. |
| src/commands/rooms/presence/get-all.ts | Implements rooms presence get-all by querying the underlying room::$chat channel presence via REST + pagination utilities. |
| src/commands/rooms/presence/enter.ts | Adds progress output and JSON result/status emission; removes prior finally/logging behavior. |
| README.md | Documents the new commands and adds them to the command index. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/commands/rooms/presence/get-all.ts (1)
123-126: Keep the empty-state warning on stdout.This is the only warning path in the command that goes to stderr; the pagination warnings in the same file already use stdout, so
rooms presence get-allends up behaving differently from the rest of the CLI for human-mode warnings.♻️ Suggested change
- } else if (items.length === 0) { - this.logToStderr( - formatWarning("No members currently present in this room."), - ); + } else if (items.length === 0) { + this.log(formatWarning("No members currently present in this room.")); } else {Based on learnings, warnings produced by
formatWarning(...)insrc/commands/**should be emitted viathis.log(...)on stdout when human output is enabled.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/rooms/presence/get-all.ts` around lines 123 - 126, In the empty-state branch inside the rooms presence get-all command, replace the stderr emission with stdout: change the call that uses this.logToStderr(formatWarning("No members currently present in this room.")) to emit via this.log(...) so the formatted warning follows the same human-mode stdout pattern as other pagination warnings; locate the branch that checks items.length === 0 and update the call to use this.log with formatWarning to maintain consistent CLI behavior.test/unit/commands/rooms/presence/enter.test.ts (1)
99-151: Assert onrunCommand()output here instead of spying onconsole.log.
runCommand()already captures oclif output for unit tests, so this adds a second capture path and makes the test more fragile than the rest of the suite.♻️ Suggested change
- const capturedLogs: string[] = []; - - const logSpy = vi.spyOn(console, "log").mockImplementation((msg) => { - capturedLogs.push(String(msg)); - }); - let presenceCallback: ((event: unknown) => void) | null = null; room.presence.subscribe.mockImplementation((callback) => { presenceCallback = callback; return { unsubscribe: vi.fn() }; }); @@ - await commandPromise; - logSpy.mockRestore(); - - const output = capturedLogs.join("\n"); + const { stdout } = await commandPromise; + const output = stdout;Based on learnings, unit tests in this repo should assert
stdoutfromrunCommand()and reserveconsole.logspies for integration/e2e coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/rooms/presence/enter.test.ts` around lines 99 - 151, Remove the console.log spy and assert on the oclif-captured output from runCommand instead: stop creating capturedLogs and vi.spyOn(console,"log"), keep the mock room and presence.subscribe setup and the presenceCallback event simulation, but call and await runCommand(...) to get its stdout (e.g., const output = await runCommand(["rooms:presence:enter","test-room","--show-others"], import.meta.url)), then assert output contains "other-user" and does not contain mock.clientId; also remove logSpy.mockRestore() and any capturedLogs.join usage.
🤖 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`:
- Line 4250: The new fenced code blocks currently lack a language tag which
triggers markdownlint MD040; update the two newly added triple-backtick fences
(the anonymous fenced blocks around the command docs) to include an appropriate
language label such as text or sh-session so CI passes, and if these sections
are generated, change the generator/source that emits those fences instead of
just patching the README.
---
Nitpick comments:
In `@src/commands/rooms/presence/get-all.ts`:
- Around line 123-126: In the empty-state branch inside the rooms presence
get-all command, replace the stderr emission with stdout: change the call that
uses this.logToStderr(formatWarning("No members currently present in this
room.")) to emit via this.log(...) so the formatted warning follows the same
human-mode stdout pattern as other pagination warnings; locate the branch that
checks items.length === 0 and update the call to use this.log with formatWarning
to maintain consistent CLI behavior.
In `@test/unit/commands/rooms/presence/enter.test.ts`:
- Around line 99-151: Remove the console.log spy and assert on the
oclif-captured output from runCommand instead: stop creating capturedLogs and
vi.spyOn(console,"log"), keep the mock room and presence.subscribe setup and the
presenceCallback event simulation, but call and await runCommand(...) to get its
stdout (e.g., const output = await
runCommand(["rooms:presence:enter","test-room","--show-others"],
import.meta.url)), then assert output contains "other-user" and does not contain
mock.clientId; also remove logSpy.mockRestore() and any capturedLogs.join usage.
🪄 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: 135fd8ff-2d03-4243-a03c-8ed5ce4fbe28
📒 Files selected for processing (7)
README.mdsrc/commands/rooms/presence/enter.tssrc/commands/rooms/presence/get-all.tssrc/commands/rooms/presence/update.tstest/unit/commands/rooms/presence/enter.test.tstest/unit/commands/rooms/presence/get-all.test.tstest/unit/commands/rooms/presence/update.test.ts
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
README.md (1)
4252-4276:⚠️ Potential issue | 🟡 MinorAdd language tags to these fenced command blocks (MD040).
Both new fenced blocks are still missing a language identifier, so markdownlint will keep failing.Suggested patch
-``` +```text USAGE $ ably rooms presence get-all ROOM [-v] [--json | --pretty-json] [--limit <value>] @@ $ ably rooms presence get-all my-room --pretty-json@@
-+text
USAGE
$ ably rooms presence update ROOM --data [-v] [--json | --pretty-json] [--client-id ] [-D ]
@@
$ ably rooms presence update my-room --data '{"status":"online"}' --duration 60Also applies to: 4316-4344
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 4252 - 4276, The markdown fenced code blocks for the CLI examples (the blocks showing "USAGE ... ably rooms presence get-all ..." and the block for "ably rooms presence update ...") lack a language identifier and trigger MD040; update each opening fence from ``` to ```text for the two shown examples (and the other occurrences noted around the presence update example and the duplicate region) so the blocks are explicitly marked as plain text and markdownlint stops flagging them.
🧹 Nitpick comments (1)
test/unit/commands/rooms/presence/enter.test.ts (1)
104-151: UsestdoutfromrunCommand()result instead ofconsole.logspying for consistency with other tests.This test manually spies on
console.logto capture presence event output, but the pattern in similar streaming tests (e.g.,rooms:typing:subscribe) shows thatrunCommand()properly captures allthis.log()calls instdout. Simply await the command and use the result'sstdoutproperty, which is cleaner and aligns with repository standards.Suggested refactor
- const capturedLogs: string[] = []; - - const logSpy = vi.spyOn(console, "log").mockImplementation((msg) => { - capturedLogs.push(String(msg)); - }); - let presenceCallback: ((event: unknown) => void) | null = null; room.presence.subscribe.mockImplementation((callback) => { presenceCallback = callback; return { unsubscribe: vi.fn() }; }); - const commandPromise = runCommand( + const result = await runCommand( ["rooms:presence:enter", "test-room", "--show-others"], import.meta.url, ); await vi.waitFor( () => { expect(room.presence.subscribe).toHaveBeenCalled(); }, { timeout: 1000 }, ); // Simulate a self event (should be filtered out) if (presenceCallback) { presenceCallback({ type: "enter", member: { clientId: mock.clientId, data: {}, }, }); // Simulate another user's event (should be shown) presenceCallback({ type: "enter", member: { clientId: "other-user", data: {}, }, }); } - - await commandPromise; - logSpy.mockRestore(); - - const output = capturedLogs.join("\n"); + const output = result.stdout; expect(output).toContain("other-user"); expect(output).not.toContain(mock.clientId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/rooms/presence/enter.test.ts` around lines 104 - 151, Remove the manual console.log spy and capturedLogs logic and instead await the runCommand(...) call to get its result object, use room.presence.subscribe mock to capture presenceCallback as before, invoke presenceCallback with the self and other-user events, then await the runCommand result and assert against result.stdout (expect it to contain "other-user" and not contain mock.clientId); update references to runCommand, room.presence.subscribe, presenceCallback, and mock.clientId accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@README.md`:
- Around line 4252-4276: The markdown fenced code blocks for the CLI examples
(the blocks showing "USAGE ... ably rooms presence get-all ..." and the block
for "ably rooms presence update ...") lack a language identifier and trigger
MD040; update each opening fence from ``` to ```text for the two shown examples
(and the other occurrences noted around the presence update example and the
duplicate region) so the blocks are explicitly marked as plain text and
markdownlint stops flagging them.
---
Nitpick comments:
In `@test/unit/commands/rooms/presence/enter.test.ts`:
- Around line 104-151: Remove the manual console.log spy and capturedLogs logic
and instead await the runCommand(...) call to get its result object, use
room.presence.subscribe mock to capture presenceCallback as before, invoke
presenceCallback with the self and other-user events, then await the runCommand
result and assert against result.stdout (expect it to contain "other-user" and
not contain mock.clientId); update references to runCommand,
room.presence.subscribe, presenceCallback, and mock.clientId accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a3bf8989-db81-4a1d-b372-a98736a158a8
📒 Files selected for processing (7)
README.mdsrc/commands/rooms/presence/enter.tssrc/commands/rooms/presence/get-all.tssrc/commands/rooms/presence/update.tstest/unit/commands/rooms/presence/enter.test.tstest/unit/commands/rooms/presence/get-all.test.tstest/unit/commands/rooms/presence/update.test.ts
✅ Files skipped from review due to trivial changes (2)
- test/unit/commands/rooms/presence/get-all.test.ts
- src/commands/rooms/presence/enter.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/commands/rooms/presence/get-all.ts
- src/commands/rooms/presence/update.ts
- test/unit/commands/rooms/presence/update.test.ts
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/commands/rooms/presence/enter.ts:70
--data "{}"is currently treated as “no data” (rawData !== "{}"), which makesthis.datastay null even though the command will still enter presence with a non-null payload (see thepresence.enter(this.data || {})call below). This can lead to the CLI reportingdata: nullin JSON/human output while the actual presence data is{}. Consider removing the special-case for "{}" and always parsing provided JSON, including an empty object, so output reflects the actual presence payload.
const rawData = flags.data;
if (rawData && rawData !== "{}") {
const parsed = this.parseJsonFlag(rawData, "data", flags);
this.data = parsed as PresenceData;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/commands/rooms/presence/subscribe.ts (1)
115-117: Consider removing the redundant timestamp line.Line 116 outputs a visual timestamp via
formatTimestamp(timestamp), and line 117 immediately outputs the same timestamp with a label. This results in the timestamp appearing twice in the output. If the visual separator is intentional, consider adding a brief comment explaining the purpose.♻️ Proposed fix to remove redundancy
const lines: string[] = [ - formatTimestamp(timestamp), `${formatLabel("Timestamp")} ${timestamp}`, `${formatLabel("Action")} ${formatEventType(event.type)}`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/rooms/presence/subscribe.ts` around lines 115 - 117, The lines array currently contains both formatTimestamp(timestamp) and `${formatLabel("Timestamp")} ${timestamp}`, causing the same timestamp to appear twice; open the const lines definition in subscribe.ts (look for formatTimestamp and formatLabel usage) and remove the redundant entry (prefer keeping the visual formatTimestamp(timestamp) and delete the `${formatLabel("Timestamp")} ${timestamp}` line), or if the visual separator is intentional keep both but add a short comment explaining why both are present.
🤖 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/rooms/presence/get-all.ts`:
- Around line 72-77: The log messages currently interpolate raw roomName when
calling this.logCliEvent in get-all (e.g., the "Fetching presence members for
room ${roomName}" message and the later message around the membership result);
replace those interpolations with formatted output by calling
formatResource(roomName) so the room name is displayed consistently (cyan)
across command output; update both places where roomName is embedded in the
message and any related metadata strings passed to this.logCliEvent to use
formatResource(roomName) instead of the raw variable.
---
Nitpick comments:
In `@src/commands/rooms/presence/subscribe.ts`:
- Around line 115-117: The lines array currently contains both
formatTimestamp(timestamp) and `${formatLabel("Timestamp")} ${timestamp}`,
causing the same timestamp to appear twice; open the const lines definition in
subscribe.ts (look for formatTimestamp and formatLabel usage) and remove the
redundant entry (prefer keeping the visual formatTimestamp(timestamp) and delete
the `${formatLabel("Timestamp")} ${timestamp}` line), or if the visual separator
is intentional keep both but add a short comment explaining why both are
present.
🪄 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: a8134939-1be5-4f2e-9ac7-5b3b5f2b66dd
📒 Files selected for processing (8)
src/commands/rooms/presence/enter.tssrc/commands/rooms/presence/get-all.tssrc/commands/rooms/presence/subscribe.tssrc/commands/rooms/presence/update.tstest/unit/commands/rooms/presence/enter.test.tstest/unit/commands/rooms/presence/get-all.test.tstest/unit/commands/rooms/presence/subscribe.test.tstest/unit/commands/rooms/presence/update.test.ts
✅ Files skipped from review due to trivial changes (1)
- test/unit/commands/rooms/presence/get-all.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- test/unit/commands/rooms/presence/update.test.ts
- src/commands/rooms/presence/update.ts
- src/commands/rooms/presence/enter.ts
4e28b33 to
768467a
Compare
768467a to
432c8d3
Compare
| @@ -0,0 +1,166 @@ | |||
| import { Args, Flags } from "@oclif/core"; | |||
There was a problem hiding this comment.
Chat doesn't have the concept of get-all for presence, it has get (getAll is a spaces thing) - the CLI should probably follow the SDKs patterns unless we have a good reason to deviate
There was a problem hiding this comment.
Got it, but this retrieves presence for all chat members, correct? The term get sounds singular and might not stay consistent with the rest of the commands.
Since we’re not strictly adhering to product-specific command patterns anyway.. wdyt
There was a problem hiding this comment.
Btw, we also use get-all for channels presence
i.e.
Ably channel presence commands:
ably channels presence enter - Enter presence on a channel and listen for presence events
ably channels presence get-all - Get all current presence members on a channel
ably channels presence subscribe - Subscribe to presence events on a channel
ably channels presence update - Update presence data on a channel
There was a problem hiding this comment.
for channels presence get-all fix, created PR -> #186
| ); | ||
|
|
||
| // Show pagination warning early (before main output) | ||
| const paginationWarning = formatPaginationLog( |
There was a problem hiding this comment.
Just a note on the warning message here, its the individual messages that count towards the billable messages, not the pages consumed.
| @@ -0,0 +1,162 @@ | |||
| import { ChatClient, PresenceData, Room } from "@ably/chat"; | |||
There was a problem hiding this comment.
What's the thinking behind this command?
So update will come through as an update event, but you won't be updating an "existing" presence member, you'll just get 2 members (even if you use the same clientId). If that's the intention, fine - but just checking reality vs expectation for users :)
There was a problem hiding this comment.
That's true. If someone wants to test user presence on UI for given client-id where they don't really care about connectionId, this command should make sense.
There was a problem hiding this comment.
I feel things get tricky when we use CLI to achieve same behaviour as SDK methods.
There was a problem hiding this comment.
chat rooms only respect identified clients, updating clientId on behalf of another user with wildcard (*) isn't supported, so we can accordingly add a warning message here.
There was a problem hiding this comment.
- Removed
presence updatecommand since exact same behaviour can be achieved using presence enter twice ( added description for the same ) - Publishing presence on behalf for given clientId also gives exact same output ( new connectionId with same clientId )
presence updatedoesn't seem to serve any meaningful purpose, we can only mark clientId as required and presence update always emits enter as first event, since connectionId is always differentpresence memberis identified based on combination ofclientIdandconnectionId
7ca5070 to
c776aa5
Compare
c776aa5 to
3f6a2f1
Compare
3f6a2f1 to
c524e64
Compare
c524e64 to
3a407f1
Compare
3a407f1 to
1980ecd
Compare
1980ecd to
f3e1e90
Compare
f3e1e90 to
9f516fa
Compare
9f516fa to
54319c4
Compare
- Marked clientId as mandatory while updating a presence, added warning about how enter and update works
54319c4 to
bfb720b
Compare
|
|
||
| static override description = | ||
| "Enter presence in a chat room and remain present until terminated"; | ||
| "Enter presence in a chat room and remain present until terminated. Provide clientId and data to update existing presence member."; |
There was a problem hiding this comment.
Provide clientId and data to update existing presence member
is this entirely true? I thought the combination of connection id and client id basically acts like a new presence for the same user? That's not the same as updating an existing presence member?
There was a problem hiding this comment.
I tested it using wildcard clientId * as per Publishing presence on behalf with channel.presence.update method.
It emitted enter event in subscriber with same clientId but had a different connectionId.
This is same behavior we get when using channels presence update or channels presence enter --client-id=my-id
There was a problem hiding this comment.
yes, but as per the docs, if you had already entered presence with clientID = example on a different client e.g. an iOS app and then tried to "update" the same clientID = example on the cli, this would in fact show as 2 presence enters?
it's not really an update because of how we treat the combination of client ids and connection ids as unique presence events?
There was a problem hiding this comment.
the point i'm making is that I don't think we need a change to the description given you are still just entering presence again with the same member - it's not an update.
There was a problem hiding this comment.
Yes, presence.update method call shows two presence enter if request is coming from different ably clients, because connectionId is always different.
It will only work, if presence.update call is made on the same ably connection.
Different connectionId denotes presence on different devices for same user clientId.
Users can filter by clientId and timestamp to calculate latest presence if needed.
There was a problem hiding this comment.
Yeah, makes sense.
It's tricky to tell how to update their presence for given clientId using presence enter.
Maybe it's better to drop the new description.
rooms presence get, updated description to performpresence updateoperation.presence updatenot needed, since exact same behaviour can be achieved usingpresence entertwice (added description for the same).clientIdalso gives exact same output ( new connectionId with same clientId )rooms enterfor better status update--json/--pretty-jsonflagSummary by CodeRabbit
New Features
rooms presence get-all(fetch members, --limit default 100, JSON/pretty-json)rooms presence update(required --data, optional --duration/--client-id, JSON/pretty-json)Improvements
rooms presence enterandsubscribeoutput and status reporting for both JSON and human modesDocumentation
Tests