Open
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
…l formatting issue
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
json and non-json output
sacOO7
force-pushed
the
fix/ably-spaces-command-group-formatting
branch
from
ae2214f to
03f1ab8
Compare
Fixed few more formatting issues
and command behaviour
![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
Comment on lines
124
to
130
| if (this.shouldOutputJson(flags)) { | ||
| this.logJsonResult({ lock: lockDetails }, flags); | ||
| this.logJsonResult({ locks: [formatLockOutput(lock)] }, flags); | ||
| } else { | ||
| this.log(formatSuccess(`Lock acquired: ${formatResource(lockId)}.`)); | ||
| this.log( | ||
| `${formatLabel("Lock details")} ${this.formatJsonOutput(lockDetails, { ...flags, "pretty-json": true })}`, | ||
| ); | ||
| this.log(formatLockBlock(lock)); | ||
| this.log(`\n${formatListening("Holding lock.")}`); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how other single-item spaces commands structure their JSON output
rg -n 'logJsonResult.*{' src/commands/spaces/ -A1 | head -40Repository: ably/ably-cli
Length of output: 152
🏁 Script executed:
#!/bin/bash
# Check how spaces commands structure their JSON output - fixed regex
rg 'logJsonResult' src/commands/spaces/ -A1 -B1Repository: ably/ably-cli
Length of output: 2215
🏁 Script executed:
#!/bin/bash
# Get full context for cursors/set.ts
echo "=== cursors/set.ts ==="
sed -n '1,200p' src/commands/spaces/cursors/set.ts | grep -A5 'logJsonResult'
echo -e "\n=== spaces/list.ts ==="
sed -n '1,200p' src/commands/spaces/list.ts | grep -A5 'logJsonResult'
echo -e "\n=== Check for SKILL.md and AGENTS.md ==="
find . -maxdepth 3 -name 'SKILL.md' -o -name 'AGENTS.md'Repository: ably/ably-cli
Length of output: 539
🏁 Script executed:
#!/bin/bash
# Check AGENTS.md for domain key convention
echo "=== AGENTS.md (first 300 lines) ==="
head -300 AGENTS.md
echo -e "\n=== locations/set.ts full logJsonResult context ==="
rg -B2 -A10 'logJsonResult.*location' src/commands/spaces/locations/set.tsRepository: ably/ably-cli
Length of output: 18966
Implement singular domain key for single-item results.
Per the documented convention in AGENTS.md ("Events and single results: nest under a singular domain key"), the acquire command should return a single lock unwrapped under a singular key:
- this.logJsonResult({ locks: [formatLockOutput(lock)] }, flags);
+ this.logJsonResult({ lock: formatLockOutput(lock) }, flags);Note: This convention is enforced correctly in locations/set.ts but violated in several other single-item commands (members/enter, cursors/set, and locks/get). Consider standardizing across the codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/spaces/locks/acquire.ts` around lines 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.
7 tasks
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor