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 PR introduces four new space management commands ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 |
![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 (2)
src/commands/spaces/get.ts (2)
128-158: Consider extracting member formatting to avoid duplication withformatMemberBlock.The inline formatting here closely mirrors
formatMemberBlockfromsrc/utils/spaces-output.ts(context snippet 1). While the input types differ (MemberInfovsSpaceMember), the output structure is identical.You could either:
- Create a shared interface that both types satisfy for formatting purposes, or
- Accept this duplication given the REST-based approach in this command
This is a minor refactor opportunity that can be deferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/spaces/get.ts` around lines 128 - 158, The member-printing loop duplicates the output of formatMemberBlock; refactor by creating a small adapter that converts the REST MemberInfo shape used in the get command to the shape expected by formatMemberBlock (or introduce a shared interface both MemberInfo and SpaceMember implement), then replace the inline logging inside the loop with a single call to formatMemberBlock(memberAdapter) to centralize formatting and remove duplication; reference the loop in the get command, the types MemberInfo and SpaceMember, and the existing formatMemberBlock utility when making the change.
17-17: ExtractSPACE_CHANNEL_TAGto a shared constant file.This constant is defined in three files:
src/commands/spaces/get.ts,src/commands/spaces/occupancy/get.ts, andsrc/commands/spaces/occupancy/subscribe.ts. Extract it tosrc/utils/spaces-constants.tsto prevent divergence if the suffix changes.♻️ Suggested refactor
Create a shared constant file:
// src/utils/spaces-constants.ts export const SPACE_CHANNEL_TAG = "::$space";Then import it in all three files:
-const SPACE_CHANNEL_TAG = "::$space"; +import { SPACE_CHANNEL_TAG } from "../../utils/spaces-constants.js";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/spaces/get.ts` at line 17, Extract the repeated constant SPACE_CHANNEL_TAG into a single shared module (e.g., export const SPACE_CHANNEL_TAG = "::$space" from a new spaces-constants module) and replace the local declarations in each file with an import from that module; update the three places where SPACE_CHANNEL_TAG is currently defined to import the shared constant so the suffix is maintained in one location.
🤖 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/spaces/members/get-all.ts`:
- Around line 35-38: Remove clientIdFlag from the static flags definition on the
get-all command: update the static override flags object (the "static override
flags" declaration in the get-all command class) to only spread productApiFlags
and remove ...clientIdFlag; also search the same class for any references to
clientIdFlag and delete or adapt them so the command remains a read-only query
using only productApiFlags.
In `@src/commands/spaces/subscribe.ts`:
- Around line 42-43: The listener stored in the property "listener" is
subscribed via this.space!.subscribe("update", this.listener) but never removed;
override or extend the class's finally() method (the one in SpacesBaseCommand or
the command class that sets this.listener) to, if this.space and this.listener
are non-null, call this.space.unsubscribe("update", this.listener), then set
this.listener = null to allow GC; ensure the unsubscribe call is guarded (check
this.space and this.listener) and placed before calling super.finally() if
applicable.
---
Nitpick comments:
In `@src/commands/spaces/get.ts`:
- Around line 128-158: The member-printing loop duplicates the output of
formatMemberBlock; refactor by creating a small adapter that converts the REST
MemberInfo shape used in the get command to the shape expected by
formatMemberBlock (or introduce a shared interface both MemberInfo and
SpaceMember implement), then replace the inline logging inside the loop with a
single call to formatMemberBlock(memberAdapter) to centralize formatting and
remove duplication; reference the loop in the get command, the types MemberInfo
and SpaceMember, and the existing formatMemberBlock utility when making the
change.
- Line 17: Extract the repeated constant SPACE_CHANNEL_TAG into a single shared
module (e.g., export const SPACE_CHANNEL_TAG = "::$space" from a new
spaces-constants module) and replace the local declarations in each file with an
import from that module; update the three places where SPACE_CHANNEL_TAG is
currently defined to import the shared constant so the suffix is maintained in
one location.
🪄 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: fabbcfdc-be62-4728-9cfb-dd91c60baabe
📒 Files selected for processing (12)
README.mdsrc/commands/spaces/create.tssrc/commands/spaces/get.tssrc/commands/spaces/index.tssrc/commands/spaces/members/get-all.tssrc/commands/spaces/subscribe.tssrc/utils/spaces-output.tstest/helpers/mock-ably-spaces.tstest/unit/commands/spaces/create.test.tstest/unit/commands/spaces/get.test.tstest/unit/commands/spaces/members/get-all.test.tstest/unit/commands/spaces/subscribe.test.ts
👮 Files not reviewed due to content moderation or server errors (8)
- test/helpers/mock-ably-spaces.ts
- src/commands/spaces/index.ts
- src/utils/spaces-output.ts
- README.md
- test/unit/commands/spaces/subscribe.test.ts
- test/unit/commands/spaces/create.test.ts
- src/commands/spaces/create.ts
- test/unit/commands/spaces/members/get-all.test.ts
There was a problem hiding this comment.
Pull request overview
Adds missing Ably Spaces CLI commands to create/inspect/subscribe to spaces and to retrieve all space members, along with supporting mocks, output tweaks, tests, and README docs.
Changes:
- Introduces new commands:
spaces create,spaces get,spaces subscribe, andspaces members get-all. - Extends the Spaces test mocks and adds unit tests covering JSON and non-JSON output plus error cases.
- Updates human-readable member output to separate “Last Event” from its timestamp, and documents the new commands in the README.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/commands/spaces/subscribe.test.ts | Adds unit coverage for spaces:subscribe output, JSON mode, and error handling. |
| test/unit/commands/spaces/members/get-all.test.ts | Adds unit coverage for spaces:members:get-all including NDJSON envelopes and non-JSON display. |
| test/unit/commands/spaces/get.test.ts | Adds unit coverage for REST-backed spaces:get behavior and parsing. |
| test/unit/commands/spaces/create.test.ts | Adds unit coverage for spaces:create including JSON envelope and failure path. |
| test/helpers/mock-ably-spaces.ts | Extends Spaces SDK mocks with space-level subscribe/unsubscribe/state support. |
| src/utils/spaces-output.ts | Adjusts member block formatting to output event type and timestamp on separate lines. |
| src/commands/spaces/subscribe.ts | Implements spaces subscribe command to stream space update events. |
| src/commands/spaces/members/get-all.ts | Implements spaces members get-all command to retrieve all members without entering. |
| src/commands/spaces/index.ts | Updates topic examples to include new spaces subcommands. |
| src/commands/spaces/get.ts | Implements spaces get via REST presence query against the ::$space channel. |
| src/commands/spaces/create.ts | Implements spaces create command behavior and JSON/human output. |
| README.md | Documents the new spaces 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.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sacOO7
force-pushed
the
feature/add-new-spaces-commands
branch
from
bf293cd to
1d3b7b0
Compare
umair-ably
force-pushed
the
feature/add-new-spaces-commands
branch
from
1d3b7b0 to
09f0316
Compare
…nd create description verbosity
umair-ably
left a comment
umair-ably
left a comment
There was a problem hiding this comment.
LGTM - pushed up small bits that needed addressing
| @@ -0,0 +1,61 @@ | |||
| import { Args } from "@oclif/core"; | |||
There was a problem hiding this comment.
Does this command add any value? Is it misleading?
It essentially just calls spaces.get('foo') which under the hood implicitly attaches the channel. So the channel exists for a very brief period and disappears. Other commands like get will fail because there are no members.
This could be misleading as a create command usually has an air of persistence about it. So I think its better we don't have it.
There was a problem hiding this comment.
That’s true, but earlier when you looked at spaces, there wasn’t any way to tell how they were created. It also felt quite odd that a space only becomes visible when a member joins it using spaces members enter. At the moment, the spaces create command simply creates an ephemeral space. I had added an explicit description to clarify this, but it was overridden by Umair’s commit (dfd9360).
Now, when you look at the commands, it’s straightforward to create a space using spaces create, and you can also see it listed in spaces list in another terminal
There was a problem hiding this comment.
Sure, you can see it via list - but that's misleading no? If I go and make a cup of tea between running the two commands, then suddenly the space I thought I created is no longer there.
There was a problem hiding this comment.
That's why we had explicit description on spaces create, I feel we should revert it there or there should be clear way to communicate how spaces are initialised. From my perspective, initialising spaces using spaces members enter feels super odd.
Other way is spaces create should print warning message that, space created is ephemeral and will cease to exist if no member is entered
| ); | ||
| } | ||
|
|
||
| const members: MemberOutput[] = items.map((item) => { |
There was a problem hiding this comment.
This won't work how we're intending. isConnected will always be true.
This is because the endpoint being called is a instantaneous presence, so no leave or absent will be in there. The way spaces works (broadly) is that when a member leaves the space, it's retained in the spaces client for a short period before being expelled (to give the air of "so and so was here, but is no longer").
If we're intending to create that sort of behaviour here, we should use the presence history REST endpoint, which would give us our leave events and thus we could approximate that.
Alternatively, we just show "who's online right now".
There was a problem hiding this comment.
Maybe we can go with standard method space.getState(), but it will need to create active realtime connection and behave exactly like SDK method
| const { members } = spaceState; | ||
|
|
||
| if (this.shouldOutputJson(flags)) { | ||
| this.logJsonEvent( |
There was a problem hiding this comment.
What would the user expect to see here? A list of all members in the space on every change (which could be large) or just the event that's just happened?
There was a problem hiding this comment.
No spaces subscribe shows both members and location change events as per doc. Command description says
DESCRIPTION
Subscribe to both spaces members and location update events
There was a problem hiding this comment.
My question was about the volume of information rather than what information - should we be showing the member/location that changed on each listener call, or should it be everything in presence?
There was a problem hiding this comment.
Currently, this behaves as per standard space.subscribe method -> https://ably.com/docs/spaces/space#subscribe
There was a problem hiding this comment.
That's fine programatically, but what's the UX like on the CLI?
If I have 100 people in the space, 1 person changing their location means I get 100 space members dumped into my console. I've got no way of telling what happened and it its hundreds of lines of terminal buffer.
I don't think we should be following the APIs directly if the CLI experience is going to be bad.
There was a problem hiding this comment.
As per doc, that seems to be the behaviour. Seems, it should only return the changes. Maybe internally subscribing to member, location, cursor changes etc would make more sense.
I am manually checking all commands, so will cross check again.
There was a problem hiding this comment.
Since, official doc says Subscribe to both spaces members and location update events, we should only be emitting changes to those events combined instead of the full state.
Added following missing commands
Summary by CodeRabbit
New Features
ably spaces create SPACE_NAMEcommand to create new spacesably spaces get SPACE_NAMEcommand to view space state and membersably spaces members get-all SPACE_NAMEcommand to retrieve all members in a spaceably spaces subscribe SPACE_NAMEcommand to monitor real-time space updatesDocumentation