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 refactors Ably Spaces commands to use domain-keyed envelope nesting for JSON and structured multi-line formatting for human-readable output. It introduces a new output utilities module, simplifies multiple command flows by removing complex subscription/monitoring logic, and updates documentation and test expectations to align with these new data-structure conventions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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)
📝 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 |
sacOO7
changed the title
sacOO7
force-pushed
the
fix/ably-spaces-command-group-formatting
branch
from
95a65b9 to
cd25291
Compare
sacOO7
force-pushed
the
fix/ably-spaces-command-group-formatting
branch
from
cd25291 to
6a66293
Compare
sacOO7
force-pushed
the
fix/ably-spaces-command-group-formatting
branch
from
2551916 to
362e694
Compare
sacOO7
force-pushed
the
fix/ably-spaces-command-group-formatting
branch
from
362e694 to
e56ad94
Compare
sacOO7
force-pushed
the
fix/ably-spaces-command-group-formatting
branch
from
e56ad94 to
ae2214f
Compare
sacOO7
force-pushed
the
fix/ably-spaces-command-group-formatting
branch
from
ae2214f to
03f1ab8
Compare
![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 (8)
.claude/skills/ably-new-command/references/patterns.md (1)
403-412: Consider adding language identifiers to example output blocks.The static analysis tool flags several fenced code blocks without language specifiers (lines 403, 434, 470, 500). While these are example outputs rather than code, adding
textorplaintextas the language identifier would satisfy markdownlint and maintain consistency.✏️ Suggested fix
-``` +```text [2024-01-15T10:30:00.000Z] ID: msg-123Apply similar changes to the other flagged blocks at lines 434, 470, and 500.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/ably-new-command/references/patterns.md around lines 403 - 412, Add a language identifier (e.g., "text" or "plaintext") to the fenced example output blocks so markdownlint stops flagging them; specifically update the triple-backtick fences around the sample output that begins with "[2024-01-15T10:30:00.000Z] ID: msg-123 ..." and apply the same change to the other similar examples in the file (the blocks starting with the outputs flagged by the reviewer). Ensure you only modify the opening fence to ```text (or ```plaintext) and leave the block contents unchanged.src/commands/spaces/cursors/set.ts (1)
171-188: Consider using singularcursorkey for single-item result.Per the JSON Data Nesting Convention in patterns.md (lines 623-630), single-item results from create/get/set operations should use a singular domain key (e.g.,
{ cursor: {...} }), while collections should use plural keys (e.g.,{ cursors: [...] }).The current implementation returns
{ cursors: [{ ... }] }which is technically a collection with one item, but semantically this is setting a single cursor.♻️ Suggested change for consistency
if (this.shouldOutputJson(flags)) { this.logJsonResult( { - cursors: [ - { - clientId: this.realtimeClient!.auth.clientId, - connectionId: this.realtimeClient!.connection.id, - position: ( - cursorForOutput as { position: { x: number; y: number } } - ).position, - data: - (cursorForOutput as { data?: Record<string, unknown> }) - .data ?? null, - }, - ], + cursor: { + clientId: this.realtimeClient!.auth.clientId, + connectionId: this.realtimeClient!.connection.id, + position: ( + cursorForOutput as { position: { x: number; y: number } } + ).position, + data: + (cursorForOutput as { data?: Record<string, unknown> }) + .data ?? null, + }, }, flags, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/spaces/cursors/set.ts` around lines 171 - 188, The JSON output currently wraps the single set result in a plural array under cursors; change it to emit a singular cursor object when only one item is returned: inside the branch that calls this.logJsonResult (the block using this.shouldOutputJson(flags)), construct and pass { cursor: { clientId: this.realtimeClient!.auth.clientId, connectionId: this.realtimeClient!.connection.id, position: (cursorForOutput as { position: { x: number; y: number } }).position, data: (cursorForOutput as { data?: Record<string, unknown> }).data ?? null } } instead of { cursors: [ ... ] }; keep the rest of the call (flags) unchanged and only adjust the top-level key/name used for single-item responses.src/commands/spaces/locations/get-all.ts (1)
79-82: Usethis.logToStderr()for warning output.Similar to the cursors/get-all command, warning messages should be written to stderr.
Suggested fix
} else if (entries.length === 0) { - this.log( - formatWarning("No locations are currently set in this space."), - ); + this.logToStderr( + formatWarning("No locations are currently set in this space."), + ); } else {Based on learnings: "Always use this.logToStderr(formatWarning('...')) for warning messages."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/spaces/locations/get-all.ts` around lines 79 - 82, Replace the warning that currently uses this.log(formatWarning(...)) with this.logToStderr(formatWarning(...)) in the branch that handles entries.length === 0; locate the else-if that checks entries.length === 0 (where formatWarning("No locations are currently set in this space.") is used) and call this.logToStderr with the same formatWarning output instead of this.log.src/commands/spaces/cursors/get-all.ts (1)
71-72: Usethis.logToStderr()for warning output.Per project conventions, warning messages should be written to stderr rather than stdout to avoid polluting normal output.
Suggested fix
} else if (cursors.length === 0) { - this.log(formatWarning("No active cursors found in space.")); + this.logToStderr(formatWarning("No active cursors found in space.")); } else {Based on learnings: "In the ably/ably-cli TypeScript (oclif) codebase, do not use this.warn() directly in command implementations. Always use this.logToStderr(formatWarning('...')) for warning messages."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/spaces/cursors/get-all.ts` around lines 71 - 72, The warning is currently being logged to stdout using this.log(formatWarning(...)) in the get-all command; change it to write to stderr by replacing that call with this.logToStderr(formatWarning("No active cursors found in space.")) so it follows the project's convention for warnings in the get-all command implementation (file: src/commands/spaces/cursors/get-all.ts, locate the branch where cursors.length === 0 and update the this.log call).test/unit/commands/spaces/locations/get-all.test.ts (1)
64-69: Assert the serialized coordinates here, not just the key.
toHaveProperty("location")still passes fornullor the wrong payload, so this won’t catch regressions in the new map-to-array formatter.🔎 Tighten the assertion
expect(resultRecord!.locations[0]).toHaveProperty( "connectionId", "conn-1", ); - expect(resultRecord!.locations[0]).toHaveProperty("location"); + expect(resultRecord!.locations[0]).toHaveProperty("location", { + x: 100, + y: 200, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/spaces/locations/get-all.test.ts` around lines 64 - 69, The test currently only asserts that resultRecord.locations[0] has a "location" property which allows null/wrong values to slip through; update the assertion to validate the serialized coordinates payload itself (for example assert resultRecord!.locations[0].location equals the expected coordinates object/array produced by the map-to-array formatter). Locate the assertion in the get-all.test.ts where resultRecord!.locations[0] is checked and replace the toHaveProperty("location") check with a strict equality/assertion against the expected serialized coordinates (matching the output shape produced by the map-to-array formatter).test/unit/commands/spaces/locations/subscribe.test.ts (1)
59-73: Avoid wall-clock sleeps in these unit tests.These 50ms waits make the assertions timing-dependent. On a busy runner the handler can still be unset, and on a fast exit the update can be emitted after
runCommand()has already finished. It’s more deterministic to fire the update from thesubscribemock itself or coordinate with fake timers/a controllable promise.Also applies to: 100-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/spaces/locations/subscribe.test.ts` around lines 59 - 73, The test uses a 50ms setTimeout to wait for the subscribe handler to be registered (locationHandler) which makes the test flaky; change the space.locations.subscribe mockImplementation to synchronously emit the mock update (i.e., call handler(mockUpdate) inside the mock) or return a controllable promise that resolves when the handler is set and awaited by the test so runCommand([... "spaces:locations:subscribe" ...]) sees the update deterministically; update the logic around the locationHandler variable and the subscribe mock so the test does not rely on wall-clock sleeps.test/unit/commands/spaces/locks/subscribe.test.ts (2)
124-126: UsecaptureJsonLogs()instead ofparseNdjsonLines()in this unit testFor mocked unit tests, prefer the repo-standard JSON capture helper to keep assertions consistent with other unit suites and reduce parser-coupling drift.
Suggested update
-import { parseNdjsonLines } from "../../../../helpers/ndjson.js"; +import { captureJsonLogs } from "../../../../helpers/ndjson.js"; ... - const records = parseNdjsonLines(stdout); + const records = captureJsonLogs(stdout);Based on learnings: In unit tests for this repo, JSON output assertions should use
captureJsonLogs()fromtest/helpers/ndjson.tsrather than manual/parsing-oriented approaches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/spaces/locks/subscribe.test.ts` around lines 124 - 126, Replace the manual NDJSON parsing with the repo-standard JSON capture helper: remove the use of parseNdjsonLines(stdout) in this test and instead call captureJsonLogs() (imported from test/helpers/ndjson.ts) to obtain the logged records (await if it returns a promise), then locate the event record the same way (const eventRecord = records.find((r) => r.type === "event" && r.lock)); also update imports to remove parseNdjsonLines and add captureJsonLogs so assertions remain consistent with other unit tests.
158-165: Tighten the rejection-path assertionConsider asserting the error message (or command component) in addition to
errorbeing defined; this avoids passing on unrelated failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/spaces/locks/subscribe.test.ts` around lines 158 - 165, The test currently only checks that an error was defined after calling runCommand for the "spaces:locks:subscribe" command, which can mask unrelated failures; update the assertion to inspect the error message or command-specific component returned by runCommand (e.g., check error.message or error.output) to ensure it contains a distinguishing token such as "spaces:locks:subscribe" or the space name "test-space". Locate the failing test using runCommand in this file and replace or add the loose expect(error).toBeDefined() with a tighter check like expect(error.message).toContain('spaces:locks:subscribe') or expect(error.message).toContain('test-space') so the test only passes for the intended rejection path.
🤖 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/locks/acquire.ts`:
- Around line 124-130: The JSON output uses an array under "locks" for a
single-item result; change the payload in the acquire command (the block using
this.shouldOutputJson, this.logJsonResult, and formatLockOutput) to return a
singular domain key "lock" with the single formatted lock (e.g., { lock:
formatLockOutput(lock) }) instead of { locks: [...] }; also scan and align the
other single-item commands mentioned (members/enter, cursors/set, locks/get) to
the same singular-key convention to stay consistent with AGENTS.md.
---
Nitpick comments:
In @.claude/skills/ably-new-command/references/patterns.md:
- Around line 403-412: Add a language identifier (e.g., "text" or "plaintext")
to the fenced example output blocks so markdownlint stops flagging them;
specifically update the triple-backtick fences around the sample output that
begins with "[2024-01-15T10:30:00.000Z] ID: msg-123 ..." and apply the same
change to the other similar examples in the file (the blocks starting with the
outputs flagged by the reviewer). Ensure you only modify the opening fence to
```text (or ```plaintext) and leave the block contents unchanged.
In `@src/commands/spaces/cursors/get-all.ts`:
- Around line 71-72: The warning is currently being logged to stdout using
this.log(formatWarning(...)) in the get-all command; change it to write to
stderr by replacing that call with this.logToStderr(formatWarning("No active
cursors found in space.")) so it follows the project's convention for warnings
in the get-all command implementation (file:
src/commands/spaces/cursors/get-all.ts, locate the branch where cursors.length
=== 0 and update the this.log call).
In `@src/commands/spaces/cursors/set.ts`:
- Around line 171-188: The JSON output currently wraps the single set result in
a plural array under cursors; change it to emit a singular cursor object when
only one item is returned: inside the branch that calls this.logJsonResult (the
block using this.shouldOutputJson(flags)), construct and pass { cursor: {
clientId: this.realtimeClient!.auth.clientId, connectionId:
this.realtimeClient!.connection.id, position: (cursorForOutput as { position: {
x: number; y: number } }).position, data: (cursorForOutput as { data?:
Record<string, unknown> }).data ?? null } } instead of { cursors: [ ... ] };
keep the rest of the call (flags) unchanged and only adjust the top-level
key/name used for single-item responses.
In `@src/commands/spaces/locations/get-all.ts`:
- Around line 79-82: Replace the warning that currently uses
this.log(formatWarning(...)) with this.logToStderr(formatWarning(...)) in the
branch that handles entries.length === 0; locate the else-if that checks
entries.length === 0 (where formatWarning("No locations are currently set in
this space.") is used) and call this.logToStderr with the same formatWarning
output instead of this.log.
In `@test/unit/commands/spaces/locations/get-all.test.ts`:
- Around line 64-69: The test currently only asserts that
resultRecord.locations[0] has a "location" property which allows null/wrong
values to slip through; update the assertion to validate the serialized
coordinates payload itself (for example assert
resultRecord!.locations[0].location equals the expected coordinates object/array
produced by the map-to-array formatter). Locate the assertion in the
get-all.test.ts where resultRecord!.locations[0] is checked and replace the
toHaveProperty("location") check with a strict equality/assertion against the
expected serialized coordinates (matching the output shape produced by the
map-to-array formatter).
In `@test/unit/commands/spaces/locations/subscribe.test.ts`:
- Around line 59-73: The test uses a 50ms setTimeout to wait for the subscribe
handler to be registered (locationHandler) which makes the test flaky; change
the space.locations.subscribe mockImplementation to synchronously emit the mock
update (i.e., call handler(mockUpdate) inside the mock) or return a controllable
promise that resolves when the handler is set and awaited by the test so
runCommand([... "spaces:locations:subscribe" ...]) sees the update
deterministically; update the logic around the locationHandler variable and the
subscribe mock so the test does not rely on wall-clock sleeps.
In `@test/unit/commands/spaces/locks/subscribe.test.ts`:
- Around line 124-126: Replace the manual NDJSON parsing with the repo-standard
JSON capture helper: remove the use of parseNdjsonLines(stdout) in this test and
instead call captureJsonLogs() (imported from test/helpers/ndjson.ts) to obtain
the logged records (await if it returns a promise), then locate the event record
the same way (const eventRecord = records.find((r) => r.type === "event" &&
r.lock)); also update imports to remove parseNdjsonLines and add captureJsonLogs
so assertions remain consistent with other unit tests.
- Around line 158-165: The test currently only checks that an error was defined
after calling runCommand for the "spaces:locks:subscribe" command, which can
mask unrelated failures; update the assertion to inspect the error message or
command-specific component returned by runCommand (e.g., check error.message or
error.output) to ensure it contains a distinguishing token such as
"spaces:locks:subscribe" or the space name "test-space". Locate the failing test
using runCommand in this file and replace or add the loose
expect(error).toBeDefined() with a tighter check like
expect(error.message).toContain('spaces:locks:subscribe') or
expect(error.message).toContain('test-space') so the test only passes for the
intended rejection path.
🪄 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: 262c13ff-538d-44de-91a0-c868d4aa7448
📒 Files selected for processing (35)
.claude/skills/ably-codebase-review/SKILL.md.claude/skills/ably-new-command/SKILL.md.claude/skills/ably-new-command/references/patterns.md.claude/skills/ably-review/SKILL.mdAGENTS.mdsrc/commands/spaces/cursors/get-all.tssrc/commands/spaces/cursors/set.tssrc/commands/spaces/cursors/subscribe.tssrc/commands/spaces/list.tssrc/commands/spaces/locations/get-all.tssrc/commands/spaces/locations/set.tssrc/commands/spaces/locations/subscribe.tssrc/commands/spaces/locks/acquire.tssrc/commands/spaces/locks/get-all.tssrc/commands/spaces/locks/get.tssrc/commands/spaces/locks/subscribe.tssrc/commands/spaces/members/enter.tssrc/commands/spaces/members/subscribe.tssrc/spaces-base-command.tssrc/utils/spaces-output.tstest/e2e/spaces/spaces-e2e.test.tstest/helpers/mock-ably-spaces.tstest/unit/commands/spaces/cursors/get-all.test.tstest/unit/commands/spaces/cursors/set.test.tstest/unit/commands/spaces/cursors/subscribe.test.tstest/unit/commands/spaces/list.test.tstest/unit/commands/spaces/locations/get-all.test.tstest/unit/commands/spaces/locations/set.test.tstest/unit/commands/spaces/locations/subscribe.test.tstest/unit/commands/spaces/locks/acquire.test.tstest/unit/commands/spaces/locks/get-all.test.tstest/unit/commands/spaces/locks/get.test.tstest/unit/commands/spaces/locks/subscribe.test.tstest/unit/commands/spaces/members/enter.test.tstest/unit/commands/spaces/members/subscribe.test.ts
💤 Files with no reviewable changes (1)
- test/unit/commands/spaces/list.test.ts
7 tasks
Contributor
Author
|
Will fix the conflict asap. |
There was a problem hiding this comment.
Pull request overview
This PR fixes and standardizes the Ably Spaces command group behavior and output, aligning commands with the documented “passive subscribe / one-shot get / one-shot set / hold enter-acquire” semantics and introducing shared structured output helpers.
Changes:
- Introduces
src/utils/spaces-output.tsfor consistent JSON shaping and multi-line labeled block output across Spaces commands. - Refactors multiple Spaces commands to avoid unintended side effects (e.g., subscribe/get commands no longer
space.enter()or fetch initial snapshots) and updates cleanup tracking viamarkAsEntered(). - Updates unit/e2e tests and documentation to reflect new argument naming (
SPACE_NAME) and output envelopes.
Reviewed changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/commands/spaces/spaces.test.ts | Updates help/usage assertions for SPACE_NAME. |
| test/unit/commands/spaces/members/subscribe.test.ts | Reworks expectations for event-only subscribe output (block + JSON event envelope). |
| test/unit/commands/spaces/members/enter.test.ts | Updates JSON expectations to return a member object. |
| test/unit/commands/spaces/locks/subscribe.test.ts | Aligns tests with passive subscribe behavior and event envelopes. |
| test/unit/commands/spaces/locks/get.test.ts | Aligns output shape to { lock: ... } and removes enter side effects. |
| test/unit/commands/spaces/locks/get-all.test.ts | Updates lock fixtures/expectations and removes enter side effects. |
| test/unit/commands/spaces/locks/acquire.test.ts | Updates lock shape and JSON expectations to { lock: { id, ... } }. |
| test/unit/commands/spaces/locations/subscribe.test.ts | Updates subscribe tests to event-only output (no initial getAll snapshot). |
| test/unit/commands/spaces/locations/set.test.ts | Removes --duration 0 path expectations and updates JSON structure. |
| test/unit/commands/spaces/locations/get-all.test.ts | Updates to locations map return type and revised JSON envelope. |
| test/unit/commands/spaces/list.test.ts | Refactors tests into standard suites + reorganizes functionality vs flag tests. |
| test/unit/commands/spaces/cursors/subscribe.test.ts | Updates to passive subscribe semantics and event envelope shape. |
| test/unit/commands/spaces/cursors/set.test.ts | Updates human-readable output expectations (Position X/Y) and JSON cursor envelope. |
| test/unit/commands/spaces/cursors/get-all.test.ts | Updates to map-based getAll return, JSON envelope, and cleanup expectations. |
| test/helpers/mock-ably-spaces.ts | Updates mocks to reflect new SDK-shaped returns (e.g., cursors/locations maps, member fields). |
| test/e2e/spaces/spaces-e2e.test.ts | Updates readiness signals/output checks; removes --duration 0 usage for locations set. |
| src/utils/spaces-output.ts | New shared formatters for Spaces JSON output + multi-line labeled blocks. |
| src/spaces-base-command.ts | Adds hasEnteredSpace tracking + gates leave()/member-cleanup waiting behind actual entry. |
| src/commands/spaces/members/subscribe.ts | Makes subscribe passive + switches to block output and domain-key JSON events. |
| src/commands/spaces/members/enter.ts | Refactors enter output + JSON returns member object; removes extra subscriptions. |
| src/commands/spaces/locks/subscribe.ts | Makes subscribe passive + switches to block output and domain-key JSON events. |
| src/commands/spaces/locks/get.ts | Makes get one-shot without entering; outputs lock domain-key JSON + block output. |
| src/commands/spaces/locks/get-all.ts | Makes get-all one-shot; outputs list + block formatting. |
| src/commands/spaces/locks/acquire.ts | Uses shared lock formatting/output and marks entry for cleanup. |
| src/commands/spaces/locations/subscribe.ts | Makes subscribe passive + emits block + JSON event envelope. |
| src/commands/spaces/locations/set.ts | Makes set one-shot (no subscribe/watch); clears location in finally before base cleanup. |
| src/commands/spaces/locations/get-all.ts | Simplifies get-all to map→entries conversion + block output. |
| src/commands/spaces/list.ts | Simplifies list output/JSON payload (removes occupancy metrics + extra metadata). |
| src/commands/spaces/cursors/subscribe.ts | Makes subscribe passive + switches to block output and domain-key JSON events. |
| src/commands/spaces/cursors/set.ts | Removes custom cursor interfaces; improves output and keeps simulate mode behavior. |
| src/commands/spaces/cursors/get-all.ts | Simplifies to map-based getAll + consistent block + JSON output. |
| README.md | Updates generated CLI docs (but currently contains formatting + placeholder drift issues). |
| AGENTS.md | Documents new structured output + JSON nesting and Spaces command semantics. |
| .claude/skills/ably-review/SKILL.md | Extends review checklist for structured output + JSON nesting + side-effect rules. |
| .claude/skills/ably-new-command/references/patterns.md | Updates guidance/examples for block output + JSON domain-key nesting. |
| .claude/skills/ably-new-command/SKILL.md | Updates skill checklist for block output + JSON nesting. |
| .claude/skills/ably-codebase-review/SKILL.md | Adds repo-wide review steps for block formatting + JSON nesting compliance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…l formatting issue
json and non-json output
Fixed few more formatting issues
umair-ably
force-pushed
the
fix/ably-spaces-command-group-formatting
branch
from
7451a66 to
93e0e2a
Compare
Contributor
|
Addressing all CodeRabbit nitpick and actionable comments from the review: Actionable (1):
Nitpicks (8):
Additionally fixed (not flagged by CodeRabbit but flagged by Copilot):
|
Contributor
|
Addressing all Copilot review comments: All 6 inline comments have been replied to and resolved:
|
umair-ably
reviewed
Mar 18, 2026
| } from "../../utils/pagination.js"; | ||
| import { SpacesBaseCommand } from "../../spaces-base-command.js"; | ||
|
|
||
| interface SpaceMetrics { |
Contributor
There was a problem hiding this comment.
i partially understand removing this from the list command but it doesn't seem like we provide a way for a user to observe the metrics on a spaces channel now? I had a skim through your follow up PR and couldn't see anything here either.
A user can of course use ably channels occupancy get "my-space::$space" but:
- This isn't in the spaces command group
- A user would need to know/remember the $space namespace
WDYT about adding a ably spaces occupancy get "my-space"? (and ably spaces occupancy subscribe "my-space"). Basically mirrors ably channels occupancy but tailored for spaces.
Contributor
Author
There was a problem hiding this comment.
Okay, maybe would make sense to add it 👍
Contributor
Author
There was a problem hiding this comment.
Added spaces occupancy get and spaces occupancy subscribe -> 39199d4
…hods operating at presence level
sacOO7
force-pushed
the
fix/ably-spaces-command-group-formatting
branch
from
58782e9 to
0dc59cc
Compare
sacOO7
force-pushed
the
fix/ably-spaces-command-group-formatting
branch
from
5512389 to
326550a
Compare
sacOO7
force-pushed
the
fix/ably-spaces-command-group-formatting
branch
from
326550a to
39199d4
Compare
Contributor
|
Feedback shared via slack:
|
umair-ably
approved these changes
Mar 19, 2026
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.
CLI Issues & Improvements
1. General / Output Formatting
JSON Structure Issues
JSON output is incorrectly structured.
Top-level fields should only include:
typecommandsuccessCurrently:
2. Spaces Commands
pnpm cli spaces list3. Members Commands
cli spaces members subscribeAutomatically enters the current member into the channel.
Expected behavior:
cli spaces members enterIssues:
Dashboard flow:
Response data:
Expected improvements:
Include:
clientIdconnectionId4. Locations Commands
cli spaces locations set dashboard--location='{"slide": 1}' --client-id=...Issue:
Expected behavior:
cli spaces locations subscribe dashboardAutomatically enters the current client into presence.
Expected:
cli spaces locations get-allIssue:
Expected:
5. Cursors Commands
cli spaces cursors set dashboardIssue:
Expected:
cli spaces cursors get-all dashboardSide Effects
Automatically enters the member.
Expected:
Output Issues
Includes unnecessary messages:
Output format is:
Expected Output Improvements
Follow
--pretty-jsonstructureInclude:
{ "clientId": "...", "connectionId": "...", "position": { "x": 0, "y": 0 }, "data": {} }Formatting rules:
clientIdshould appear on a new lineIf time-based or paginated:
cli spaces cursors subscribeBehavior Issue
Automatically enters the member.
Expected:
Output Issues
JSON output:
Non-JSON output:
Missing explicit:
xyfields in position displaySummary of Key Problems
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor