[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 |
sacOO7
changed the title
umair-ably
force-pushed
the
feature/implement-missing-presence-commands
branch
from
3659708 to
4b182f3
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
sacOO7
force-pushed
the
feature/add-chat-presence-commands
branch
from
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.
| $ ably rooms presence get-all my-room --pretty-json | ||
| ``` | ||
|
|
||
| _See code: [src/commands/rooms/presence/get-all.ts](https://github.com/ably/ably-cli/blob/v0.17.0/src/commands/rooms/presence/get-all.ts)_ |
There was a problem hiding this comment.
The generated “See code” links point to blob/v0.17.0/... but these new command files won’t exist in the v0.17.0 tag, so the links will 404 until a release/tag is cut or the README generation target is updated. Consider bumping the package version before regenerating the README, or adjusting link generation to reference main for unreleased commands.
| _See code: [src/commands/rooms/presence/get-all.ts](https://github.com/ably/ably-cli/blob/v0.17.0/src/commands/rooms/presence/get-all.ts)_ | |
| _See code: [src/commands/rooms/presence/get-all.ts](https://github.com/ably/ably-cli/blob/main/src/commands/rooms/presence/get-all.ts)_ |
| $ ably rooms presence update my-room --data '{"status":"busy"}' --json | ||
|
|
||
| $ ably rooms presence update my-room --data '{"status":"busy"}' --pretty-json | ||
|
|
There was a problem hiding this comment.
Same issue as above: this new command’s “See code” link references blob/v0.17.0, where src/commands/rooms/presence/update.ts does not exist yet, so the link will 404 until the release/tag is updated.
| connectionId: member.connectionId, | ||
| data: member.data ?? null, | ||
| extras: member.extras ?? null, |
There was a problem hiding this comment.
In JSON output, this command returns updatedAt and omits fields (action, timestamp, id) that are included by the analogous channels presence get-all command. This makes the JSON schema inconsistent across presence "get-all" commands and drops information that’s available on the presence message. Consider aligning the presenceMembers item shape (field names and included fields) with src/commands/channels/presence/get-all.ts for parity (e.g., include action, timestamp/updatedAt, and id, or standardize on a single timestamp field name).
| connectionId: member.connectionId, | |
| data: member.data ?? null, | |
| extras: member.extras ?? null, | |
| connectionId: member.connectionId, | |
| id: member.id ?? null, | |
| action: member.action ?? null, | |
| data: member.data ?? null, | |
| extras: member.extras ?? null, | |
| timestamp: formatMessageTimestamp(member.timestamp), | |
| // `updatedAt` retained for backwards compatibility; mirrors `timestamp` |
src/commands/rooms/presence/enter.ts
Outdated
| data: this.data, | ||
| }); | ||
| await currentRoom.presence.enter(this.data || {}); | ||
| this.logCliEvent(flags, "presence", "entered", "Entered presence"); |
There was a problem hiding this comment.
The logCliEvent calls for enter/entered don’t include the room name (and the entered event has no context at all), which makes verbose logs harder to correlate when multiple rooms/commands are running. Consider including { room: this.roomName, clientId: …, data: … } (and similarly for "entered") to match the richer context used by other presence commands.
| data: this.data, | |
| }); | |
| await currentRoom.presence.enter(this.data || {}); | |
| this.logCliEvent(flags, "presence", "entered", "Entered presence"); | |
| room: this.roomName, | |
| clientId: this.chatClient?.clientId, | |
| data: this.data ?? null, | |
| }); | |
| await currentRoom.presence.enter(this.data || {}); | |
| this.logCliEvent(flags, "presence", "entered", "Entered presence", { | |
| room: this.roomName, | |
| clientId: this.chatClient?.clientId, | |
| data: this.data ?? null, | |
| }); |
| private room: Room | null = null; | ||
| private commandFlags: Interfaces.InferredFlags< | ||
| typeof RoomsPresenceUpdate.flags | ||
| > | null = null; |
There was a problem hiding this comment.
commandFlags is assigned in run() but never used in this command. If it’s not needed for cleanup/logging, removing it will reduce dead state and avoid confusion about whether flags are meant to be read later (e.g., in finally()).
sacOO7
changed the base branch from
feature/implement-missing-presence-commands
to
main
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
|
|
||
| Get all current presence members in a chat room | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add a language tag to the new fenced blocks.
markdownlint is flagging both new command docs with MD040. Please annotate these fences with something like text or sh-session so CI stays clean. If this section is generated, fix the generator/source rather than only patching the generated README.
Also applies to: 4314-4314
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 4250-4250: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at 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.
✅ 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.
| this.logCliEvent(flags, "presence", "entering", "Entering presence", { | ||
| data: this.data, | ||
| }); | ||
| await currentRoom.presence.enter(this.data || {}); | ||
| this.logCliEvent(flags, "presence", "entered", "Entered presence"); | ||
|
|
||
| if (!this.shouldOutputJson(flags) && this.roomName) { | ||
| if (this.shouldOutputJson(flags)) { | ||
| this.logJsonResult( | ||
| { | ||
| presenceMessage: { | ||
| action: "enter", | ||
| room: this.roomName, | ||
| clientId: this.chatClient!.clientId, | ||
| connectionId: this.chatClient!.realtime.connection.id, | ||
| data: this.data ?? null, | ||
| timestamp: new Date().toISOString(), |
There was a problem hiding this comment.
When --data is not provided, the command calls presence.enter(this.data || {}), which passes {} to the SDK. However, the JSON result emitted afterwards reports data: this.data ?? null (null) and the human output omits the Data line. This makes the output inconsistent with the actual entered presence data. Prefer calling presence.enter() with no argument (or undefined) when this.data is null/undefined, and keep the emitted presenceMessage.data aligned with what was sent.
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
| this.logCliEvent( | ||
| flags, | ||
| "presence", | ||
| "fetching", | ||
| `Fetching presence members for room ${roomName}`, | ||
| { room: roomName }, |
There was a problem hiding this comment.
Use formatResource(roomName) consistently in room-name output/log text
You already format the room name at Line 67, but Line 76 and Line 131 still interpolate raw roomName. Align these with formatResource(roomName) for consistency with command output conventions.
Suggested patch
this.logCliEvent(
flags,
"presence",
"fetching",
- `Fetching presence members for room ${roomName}`,
+ `Fetching presence members for room ${formatResource(roomName)}`,
{ room: roomName },
);
@@
this.log(
- `\n${formatHeading(`Presence members in room: ${roomName}`)} (${formatCountLabel(items.length, "member")}):\n`,
+ `\n${formatHeading(`Presence members in room: ${formatResource(roomName)}`)} (${formatCountLabel(items.length, "member")}):\n`,
);As per coding guidelines, "Always use formatResource(name) (cyan) for resource names instead of quoted strings, including in logCliEvent messages".
Also applies to: 131-131
🤖 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 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.
4e28b33 to
768467a
Compare
rooms presence get-allandrooms presence updaterooms 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