Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughMultiple space-related commands were refactored to improve event handling, error management, and output formatting. Changes include adding warning messages to initialization, restructuring cursor output for simulate mode, updating error messages, and converting event listeners from class-level fields to inline handlers with enhanced error handling and deduplication logic. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
Adds an informational warning to ably spaces create to clarify that Spaces are ephemeral (channel-backed) and that create initializes without entering/activating the space.
Changes:
- Import
formatWarningand emit a new post-create warning in non-JSON output. - Emit a
statusJSON record withstatus: "warning"containing the same message in--jsonmode.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sacOO7
force-pushed
the
fix/spaces-commands
branch
from
19a8336 to
d66aace
Compare
sacOO7
force-pushed
the
fix/spaces-commands
branch
from
d66aace to
647d78d
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
test/unit/commands/spaces/subscribe.test.ts (1)
81-106: Replace the wall-clock sleeps with a deterministic subscribe handoff.These two tests depend on a 50 ms timeout winning the race against command setup, which can flake on slower CI. Resolve a local promise when the mocked
space.locations.subscribecaptures the handler, and await that instead of sleeping.🧪 Example pattern
- let locationHandler: ((update: unknown) => void) | undefined; + let locationHandler: ((update: unknown) => void) | undefined; + let resolveSubscribed = () => {}; + const subscribed = new Promise<void>((resolve) => { + resolveSubscribed = resolve; + }); space.locations.subscribe.mockImplementation( (_event: string, handler: (update: unknown) => void) => { locationHandler = handler; + resolveSubscribed(); }, ); @@ - await new Promise((resolve) => setTimeout(resolve, 50)); + await subscribed;Apply the same pattern in the JSON location test below.
Also applies to: 154-179
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/spaces/subscribe.test.ts` around lines 81 - 106, The test currently uses a 50ms setTimeout race to wait for the mocked space.locations.subscribe to capture the handler; replace that fragile sleep with a deterministic promise handshake: create a local promise (e.g., subscribeReady) and its resolver, have the mockImplementation for space.locations.subscribe assign locationHandler and call the resolver immediately, then await subscribeReady before invoking locationHandler and awaiting runCommand; apply the same pattern for the JSON location test (the second occurrence) so both tests wait deterministically for the subscribe handoff instead of sleeping.src/commands/spaces/subscribe.ts (1)
48-52: Expire the dedupe cache after the 500 ms window.This map only helps for 500 ms, but stale keys are kept forever. On a long-lived subscription, every transient connection leaves a permanent entry even though it can no longer affect deduplication.
Also applies to: 78-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/spaces/subscribe.ts` around lines 48 - 52, The dedupe Map lastSeenEvents currently stores entries indefinitely; modify the logic that sets or updates lastSeenEvents (where entries are added in the subscription/event handler) to expire entries after 500ms: either schedule a setTimeout to delete the specific key when you insert/update it or implement a lightweight pruning pass that removes keys whose stored timestamp + 500ms < Date.now() before doing dedupe checks; update both places referencing lastSeenEvents (the declaration and its use in the event handling block) so stale client keys are removed and the map doesn't grow unbounded.
🤖 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/create.ts`:
- Around line 59-65: The JSON branch currently emits a separate status record
via logJsonStatus; instead, include the warning on the one-shot result by
calling this.logJsonResult(...) with the result payload plus a warning field
(use ephemeralSpaceWarning content) when shouldOutputJson(flags) is true and
remove the logJsonStatus call; in the human-readable branch replace the quoted
spaceName in ephemeralSpaceWarning with formatResource(spaceName) and keep using
formatWarning(...) when logging; locate symbols ephemeralSpaceWarning,
shouldOutputJson(flags), logJsonStatus, logJsonResult, formatWarning,
formatResource, spaceName and flags to make these changes.
In `@src/commands/spaces/members/subscribe.ts`:
- Around line 119-127: The member subscribe callback (memberListener) currently
calls this.fail(...) inside the callback and inconsistently accesses
member.lastEvent (both direct and with optional chaining); change the callback
to propagate errors via reject(new Error(...)) instead of calling this.fail
directly so the outer await/catch can call this.fail, and make the lastEvent
access consistent across the handler (either use member.lastEvent?.name and
member.lastEvent?.timestamp everywhere for defensive coding or remove the
optional chaining so both places assume lastEvent is present); update the code
around formatMessageTimestamp/formatTimestamp/formatMemberEventBlock usage and
replace the inner catch's this.fail call with reject(...) so errors bubble to
the outer catch.
In `@src/commands/spaces/subscribe.ts`:
- Around line 73-75: member.lastEvent is treated as optional earlier (action =
member.lastEvent?.name) but later code dereferences it unconditionally causing a
crash (this.fail) when it's missing; update the output path to consistently
guard accesses to lastEvent by using the already-computed action and
clientId/connectionId fallbacks or use optional chaining/defaults (e.g.,
member.lastEvent?.name, member.lastEvent?.actor?.id ?? 'Unknown',
member.lastEvent?.someField ?? 'Unknown') before reading properties, and ensure
any conditional logic that relied on lastEvent checks for its existence first so
the subscribe command no longer throws.
---
Nitpick comments:
In `@src/commands/spaces/subscribe.ts`:
- Around line 48-52: The dedupe Map lastSeenEvents currently stores entries
indefinitely; modify the logic that sets or updates lastSeenEvents (where
entries are added in the subscription/event handler) to expire entries after
500ms: either schedule a setTimeout to delete the specific key when you
insert/update it or implement a lightweight pruning pass that removes keys whose
stored timestamp + 500ms < Date.now() before doing dedupe checks; update both
places referencing lastSeenEvents (the declaration and its use in the event
handling block) so stale client keys are removed and the map doesn't grow
unbounded.
In `@test/unit/commands/spaces/subscribe.test.ts`:
- Around line 81-106: The test currently uses a 50ms setTimeout race to wait for
the mocked space.locations.subscribe to capture the handler; replace that
fragile sleep with a deterministic promise handshake: create a local promise
(e.g., subscribeReady) and its resolver, have the mockImplementation for
space.locations.subscribe assign locationHandler and call the resolver
immediately, then await subscribeReady before invoking locationHandler and
awaiting runCommand; apply the same pattern for the JSON location test (the
second occurrence) so both tests wait deterministically for the subscribe
handoff instead of sleeping.
🪄 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: 4eb0c113-425e-4137-950b-66404a1c4ee7
📒 Files selected for processing (8)
src/commands/spaces/create.tssrc/commands/spaces/cursors/set.tssrc/commands/spaces/get.tssrc/commands/spaces/locations/subscribe.tssrc/commands/spaces/members/subscribe.tssrc/commands/spaces/subscribe.tstest/unit/commands/spaces/cursors/set.test.tstest/unit/commands/spaces/subscribe.test.ts
| const ephemeralSpaceWarning = `Space: ${spaceName} is backed by ably channel '${spaceName}::$space' and is ephemeral — it will become active when at least one member enters. This command initializes the space without entering it. To add a member to the space, use 'ably spaces members enter ${spaceName}'`; | ||
|
|
||
| if (this.shouldOutputJson(flags)) { | ||
| this.logJsonStatus("warning", ephemeralSpaceWarning, flags); | ||
| } else { | ||
| this.log(formatWarning(ephemeralSpaceWarning)); | ||
| } |
There was a problem hiding this comment.
Keep one-shot JSON output as a single result record.
This currently emits two JSON records (logJsonResult then logJsonStatus) for a one-shot command. Prefer a single logJsonResult payload that includes warning metadata, and reserve logJsonStatus for long-running status signals. Also use formatResource(...) for resource names in the human-readable warning branch.
Proposed adjustment
- if (this.shouldOutputJson(flags)) {
- this.logJsonResult({ space: { name: spaceName } }, flags);
- } else {
+ const ephemeralSpaceWarning = `Space ${spaceName} is backed by Ably channel '${spaceName}::$space' and is ephemeral — it will become active when at least one member enters. This command initializes the space without entering it. To add a member to the space, use 'ably spaces members enter ${spaceName}'.`;
+
+ if (this.shouldOutputJson(flags)) {
+ this.logJsonResult(
+ {
+ space: { name: spaceName },
+ warning: ephemeralSpaceWarning,
+ },
+ flags,
+ );
+ } else {
this.log(
formatSuccess(
`Space ${formatResource(spaceName)} initialized. Use "ably spaces members enter" to activate it.`,
),
);
+ this.log(
+ formatWarning(
+ `Space ${formatResource(spaceName)} is backed by Ably channel ${formatResource(`${spaceName}::$space`)} and is ephemeral — it will become active when at least one member enters. This command initializes the space without entering it. To add a member to the space, use 'ably spaces members enter ${spaceName}'.`,
+ ),
+ );
}
-
- const ephemeralSpaceWarning = `Space: ${spaceName} is backed by ably channel '${spaceName}::$space' and is ephemeral — it will become active when at least one member enters. This command initializes the space without entering it. To add a member to the space, use 'ably spaces members enter ${spaceName}'`;
-
- if (this.shouldOutputJson(flags)) {
- this.logJsonStatus("warning", ephemeralSpaceWarning, flags);
- } else {
- this.log(formatWarning(ephemeralSpaceWarning));
- }As per coding guidelines: "Use this.logJsonResult(data, flags) for one-shot results ... this.logJsonStatus(status, message, flags) for hold/status signals in long-running commands" and "Always use formatResource(name) (cyan) for resource names instead of quoted strings."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/spaces/create.ts` around lines 59 - 65, The JSON branch
currently emits a separate status record via logJsonStatus; instead, include the
warning on the one-shot result by calling this.logJsonResult(...) with the
result payload plus a warning field (use ephemeralSpaceWarning content) when
shouldOutputJson(flags) is true and remove the logJsonStatus call; in the
human-readable branch replace the quoted spaceName in ephemeralSpaceWarning with
formatResource(spaceName) and keep using formatWarning(...) when logging; locate
symbols ephemeralSpaceWarning, shouldOutputJson(flags), logJsonStatus,
logJsonResult, formatWarning, formatResource, spaceName and flags to make these
changes.
| formatTimestamp( | ||
| formatMessageTimestamp(member.lastEvent.timestamp), | ||
| ), | ||
| ); | ||
| this.log(formatMemberEventBlock(member, action)); | ||
| this.log(""); | ||
| } | ||
| } catch (error) { | ||
| this.fail(error, flags, "memberSubscribe", { spaceName }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the @ably/spacesTypeScript SDK, what is the declared type ofSpaceMember.lastEvent, and can it be undefined on members.subscribe("update", ...) callbacks?
💡 Result:
In the @ably/spaces TypeScript SDK, SpaceMember.lastEvent is declared as an object with properties { name: string; timestamp: number }, based on consistent examples across official documentation and SDK README. It is always present and populated in member objects received in members.subscribe("update", ...) callbacks, as shown in all payload examples for enter, leave, update events (e.g., {"name": "enter", "timestamp": 1677595689759}). No examples show it as undefined or absent, indicating it cannot be undefined in these callbacks.
Citations:
- 1: https://github.com/ably/spaces
- 2: https://www.npmjs.com/package/@ably/spaces
- 3: https://ably.com/docs/spaces/space
- 4: https://ably.io/docs/spaces/avatar
- 5: https://ably.com/docs/spaces/locations
🏁 Script executed:
cat -n src/commands/spaces/members/subscribe.ts | head -150Repository: ably/ably-cli
Length of output: 5568
🏁 Script executed:
find node_modules/@ably/spaces -name "*.d.ts" -o -name "*.ts" | head -20Repository: ably/ably-cli
Length of output: 117
🏁 Script executed:
cat package.json | grep -A 5 "@ably/spaces"Repository: ably/ably-cli
Length of output: 288
Move error handling from callback to outer catch block and use consistent optional chaining.
According to @ably/spaces SDK documentation, SpaceMember.lastEvent is always present in members.subscribe callbacks with { name: string; timestamp: number }, so the direct access on line 120 is safe. However, line 76 treats it as optional with ?.name, creating inconsistency.
More importantly, calling this.fail() inside the memberListener callback (lines 126-127) violates the coding guidelines. Per the guidelines: "In Promise callbacks (e.g., connection event handlers), use reject(new Error(...)) for errors, which propagates to await where the catch block calls this.fail()". Replace the try-catch with reject(new Error(...)) and let the outer catch block handle the error consistently.
If keeping the try-catch for defensive programming, either make both accesses use optional chaining (member.lastEvent?.timestamp) or remove the optional chaining on line 76 to reflect that the property is always defined.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/spaces/members/subscribe.ts` around lines 119 - 127, The member
subscribe callback (memberListener) currently calls this.fail(...) inside the
callback and inconsistently accesses member.lastEvent (both direct and with
optional chaining); change the callback to propagate errors via reject(new
Error(...)) instead of calling this.fail directly so the outer await/catch can
call this.fail, and make the lastEvent access consistent across the handler
(either use member.lastEvent?.name and member.lastEvent?.timestamp everywhere
for defensive coding or remove the optional chaining so both places assume
lastEvent is present); update the code around
formatMessageTimestamp/formatTimestamp/formatMemberEventBlock usage and replace
the inner catch's this.fail call with reject(...) so errors bubble to the outer
catch.
| const action = member.lastEvent?.name || "unknown"; | ||
| const clientId = member.clientId || "Unknown"; | ||
| const connectionId = member.connectionId || "Unknown"; |
There was a problem hiding this comment.
Guard lastEvent consistently in the text output path.
Line 73 already treats member.lastEvent as optional, but Line 114 dereferences it unconditionally. If lastEvent is absent, this throws and terminates the whole subscribe command via this.fail().
🐛 Proposed fix
- this.log(
- formatTimestamp(
- formatMessageTimestamp(member.lastEvent.timestamp),
- ),
- );
+ this.log(
+ formatTimestamp(
+ formatMessageTimestamp(member.lastEvent?.timestamp),
+ ),
+ );Also applies to: 112-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/spaces/subscribe.ts` around lines 73 - 75, member.lastEvent is
treated as optional earlier (action = member.lastEvent?.name) but later code
dereferences it unconditionally causing a crash (this.fail) when it's missing;
update the output path to consistently guard accesses to lastEvent by using the
already-computed action and clientId/connectionId fallbacks or use optional
chaining/defaults (e.g., member.lastEvent?.name, member.lastEvent?.actor?.id ??
'Unknown', member.lastEvent?.someField ?? 'Unknown') before reading properties,
and ensure any conditional logic that relied on lastEvent checks for its
existence first so the subscribe command no longer throws.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| this.log( | ||
| formatTimestamp( | ||
| formatMessageTimestamp(member.lastEvent.timestamp), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
member.lastEvent?.name is treated as optional above, but here member.lastEvent.timestamp is accessed without guarding. If lastEvent is ever missing this will throw from the listener and terminate the command; if it’s guaranteed to exist, consider removing the optional chain/defaults for consistency. Suggest normalizing via a const lastEvent = member.lastEvent and using lastEvent?.timestamp (and lastEvent?.name) or asserting non-null once.
| // Keep track of the last event we've seen for each client to avoid duplicates | ||
| const lastSeenEvents = new Map< | ||
| string, | ||
| { action: string; timestamp: number } | ||
| >(); |
There was a problem hiding this comment.
lastSeenEvents is keyed per client+connection and never pruned. On long-running subscriptions with many transient connections this can grow without bound. Consider periodically deleting entries older than the dedup window (e.g., when the map exceeds a threshold) or storing only the most recent N keys.
| this.log( | ||
| formatTimestamp( | ||
| formatMessageTimestamp(member.lastEvent.timestamp), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Same as spaces:subscribe: the listener treats member.lastEvent?.name as optional but later reads member.lastEvent.timestamp without guarding. Either treat lastEvent as required consistently (no optional chaining/defaults) or guard both name and timestamp via a single local lastEvent variable to avoid potential runtime errors if the SDK ever emits a member without lastEvent.
pnpm cli spaces subscribe dashboard=>pnpm cli spaces subscribe dashboard --pretty-json=>spaces * get-allcommands are eligible forcollectPaginatedResults. The@ably/spacesSDK does not exposePaginatedResultfor any of itsgetAll()methods. All pagination (where it exists) is handled internally by the SDK, invisible to the consumer.Summary by CodeRabbit
New Features
Bug Fixes