From 7ac2431699270df9e8ca35eb83a9031d471e5595 Mon Sep 17 00:00:00 2001 From: sacOO7 Date: Tue, 10 Mar 2026 18:56:01 +0530 Subject: [PATCH 1/5] Removed dead interfaces from spaces command group, fixed cursor-getall formatting issue --- src/commands/spaces/cursors/get-all.ts | 71 ++------ src/commands/spaces/locations/get-all.ts | 171 +++--------------- src/commands/spaces/locations/subscribe.ts | 89 +++------ test/helpers/mock-ably-spaces.ts | 4 +- .../commands/spaces/cursors/get-all.test.ts | 18 +- .../commands/spaces/cursors/subscribe.test.ts | 10 +- .../commands/spaces/locations/get-all.test.ts | 30 +-- 7 files changed, 101 insertions(+), 292 deletions(-) diff --git a/src/commands/spaces/cursors/get-all.ts b/src/commands/spaces/cursors/get-all.ts index 0e2d1592..bfab0972 100644 --- a/src/commands/spaces/cursors/get-all.ts +++ b/src/commands/spaces/cursors/get-all.ts @@ -1,3 +1,4 @@ +import { type CursorUpdate } from "@ably/spaces"; import { Args } from "@oclif/core"; import chalk from "chalk"; @@ -5,24 +6,15 @@ import { productApiFlags, clientIdFlag } from "../../../flags.js"; import { SpacesBaseCommand } from "../../../spaces-base-command.js"; import isTestMode from "../../../utils/test-mode.js"; import { + formatClientId, + formatCountLabel, + formatHeading, formatProgress, - formatSuccess, formatResource, - formatClientId, + formatSuccess, + formatWarning, } from "../../../utils/output.js"; -interface CursorPosition { - x: number; - y: number; -} - -interface CursorUpdate { - clientId?: string; - connectionId?: string; - data?: Record; - position: CursorPosition; -} - export default class SpacesCursorsGetAll extends SpacesBaseCommand { static override args = { space: Args.string({ @@ -76,16 +68,7 @@ export default class SpacesCursorsGetAll extends SpacesBaseCommand { // Check realtime client state if (this.realtimeClient!.connection.state === "connected") { clearTimeout(timeout); - if (this.shouldOutputJson(flags)) { - this.logJsonResult( - { - connectionId: this.realtimeClient!.connection.id, - spaceName, - status: "connected", - }, - flags, - ); - } else { + if (!this.shouldOutputJson(flags)) { this.log( formatSuccess(`Entered space: ${formatResource(spaceName)}.`), ); @@ -194,34 +177,17 @@ export default class SpacesCursorsGetAll extends SpacesBaseCommand { const allCursors = await this.space!.cursors.getAll(); // Add any cached cursors that we didn't see in live updates - if (Array.isArray(allCursors)) { - allCursors.forEach((cursor) => { - if ( - cursor && - cursor.connectionId && - !cursorMap.has(cursor.connectionId) - ) { - cursorMap.set(cursor.connectionId, cursor as CursorUpdate); - } - }); - } else if (allCursors && typeof allCursors === "object") { - // Handle object return type - Object.values(allCursors).forEach((cursor) => { - if ( - cursor && - cursor.connectionId && - !cursorMap.has(cursor.connectionId) - ) { - cursorMap.set(cursor.connectionId, cursor as CursorUpdate); - } - }); + for (const cursor of Object.values(allCursors)) { + if (cursor && !cursorMap.has(cursor.connectionId)) { + cursorMap.set(cursor.connectionId, cursor); + } } } catch { // If getAll fails due to connection issues, use only the live updates we collected if (!this.shouldOutputJson(flags)) { this.log( - chalk.yellow( - "Warning: Could not fetch all cursors, showing only live updates", + formatWarning( + "Could not fetch all cursors, showing only live updates.", ), ); } @@ -232,6 +198,7 @@ export default class SpacesCursorsGetAll extends SpacesBaseCommand { if (this.shouldOutputJson(flags)) { this.logJsonResult( { + connectionId: this.realtimeClient!.connection.id, cursors: cursors.map((cursor: CursorUpdate) => ({ clientId: cursor.clientId, connectionId: cursor.connectionId, @@ -247,7 +214,7 @@ export default class SpacesCursorsGetAll extends SpacesBaseCommand { if (!cursorUpdateReceived && cursors.length === 0) { this.log(chalk.dim("─".repeat(60))); this.log( - chalk.yellow( + formatWarning( "No cursor updates are being sent in this space. Make sure other clients are actively setting cursor positions.", ), ); @@ -256,16 +223,14 @@ export default class SpacesCursorsGetAll extends SpacesBaseCommand { if (cursors.length === 0) { this.log(chalk.dim("─".repeat(60))); - this.log(chalk.yellow("No active cursors found in space.")); + this.log(formatWarning("No active cursors found in space.")); return; } // Show summary table this.log(chalk.dim("─".repeat(60))); this.log( - chalk.bold( - `\nCursor Summary - ${cursors.length} cursor${cursors.length === 1 ? "" : "s"} found:\n`, - ), + `\n${formatHeading("Cursor Summary")} - ${formatCountLabel(cursors.length, "cursor")} found:\n`, ); // Table header @@ -351,7 +316,7 @@ export default class SpacesCursorsGetAll extends SpacesBaseCommand { // Show additional data if any cursor has it const cursorsWithData = cursors.filter((c) => c.data); if (cursorsWithData.length > 0) { - this.log(`\n${chalk.bold("Additional Data:")}`); + this.log(`\n${formatHeading("Additional Data")}:`); cursorsWithData.forEach((cursor: CursorUpdate) => { this.log( ` ${formatClientId(cursor.clientId || "Unknown")}: ${JSON.stringify(cursor.data)}`, diff --git a/src/commands/spaces/locations/get-all.ts b/src/commands/spaces/locations/get-all.ts index 4149dbe1..3fa0fe58 100644 --- a/src/commands/spaces/locations/get-all.ts +++ b/src/commands/spaces/locations/get-all.ts @@ -1,47 +1,21 @@ import { Args } from "@oclif/core"; -import chalk from "chalk"; -import { errorMessage } from "../../../utils/errors.js"; import { productApiFlags, clientIdFlag } from "../../../flags.js"; import { SpacesBaseCommand } from "../../../spaces-base-command.js"; import { formatClientId, + formatCountLabel, formatHeading, formatLabel, formatProgress, formatResource, formatSuccess, + formatWarning, } from "../../../utils/output.js"; -interface LocationData { - [key: string]: unknown; -} - -interface Member { - clientId?: string; - memberId?: string; - isCurrentMember?: boolean; -} - -interface LocationWithCurrent { - current: { - member: Member; - }; - location?: LocationData; - data?: LocationData; - [key: string]: unknown; -} - -interface LocationItem { - [key: string]: unknown; - clientId?: string; - connectionId?: string; - data?: LocationData; - id?: string; - location?: LocationData; - member?: Member; - memberId?: string; - userId?: string; +interface LocationEntry { + connectionId: string; + location: unknown; } export default class SpacesLocationsGetAll extends SpacesBaseCommand { @@ -132,139 +106,46 @@ export default class SpacesLocationsGetAll extends SpacesBaseCommand { ); } - let locations: LocationItem[] = []; try { const locationsFromSpace = await this.space!.locations.getAll(); - if (locationsFromSpace && typeof locationsFromSpace === "object") { - if (Array.isArray(locationsFromSpace)) { - locations = locationsFromSpace as LocationItem[]; - } else if (Object.keys(locationsFromSpace).length > 0) { - locations = Object.entries(locationsFromSpace).map( - ([memberId, locationData]) => ({ - location: locationData, - memberId, - }), - ) as LocationItem[]; - } - } - - const knownMetaKeys = new Set([ - "clientId", - "connectionId", - "current", - "id", - "member", - "memberId", - "userId", - ]); - - const extractLocationData = (item: LocationItem): unknown => { - if (item.location !== undefined) return item.location; - if (item.data !== undefined) return item.data; - const rest: Record = {}; - for (const [key, value] of Object.entries(item)) { - if (!knownMetaKeys.has(key)) { - rest[key] = value; - } - } - return Object.keys(rest).length > 0 ? rest : null; - }; - - const validLocations = locations.filter((item: LocationItem) => { - if (item === null || item === undefined) return false; - - const locationData = extractLocationData(item); - - if (locationData === null || locationData === undefined) return false; - if ( - typeof locationData === "object" && - Object.keys(locationData as object).length === 0 + const entries: LocationEntry[] = Object.entries(locationsFromSpace) + .filter( + ([, loc]) => + loc != null && + !( + typeof loc === "object" && + Object.keys(loc as object).length === 0 + ), ) - return false; - - return true; - }); + .map(([connectionId, loc]) => ({ connectionId, location: loc })); if (this.shouldOutputJson(flags)) { this.logJsonResult( { - locations: validLocations.map((item: LocationItem) => { - const currentMember = - "current" in item && - item.current && - typeof item.current === "object" - ? (item.current as LocationWithCurrent["current"]).member - : undefined; - const member = item.member || currentMember; - const memberId = - item.memberId || - member?.memberId || - member?.clientId || - item.clientId || - item.id || - item.userId || - "Unknown"; - const locationData = extractLocationData(item); - return { - isCurrentMember: member?.isCurrentMember || false, - location: locationData, - memberId, - }; - }), + locations: entries.map((entry) => ({ + connectionId: entry.connectionId, + location: entry.location, + })), spaceName, timestamp: new Date().toISOString(), }, flags, ); - } else if (!validLocations || validLocations.length === 0) { + } else if (entries.length === 0) { this.log( - chalk.yellow("No locations are currently set in this space."), + formatWarning("No locations are currently set in this space."), ); } else { - const locationsCount = validLocations.length; this.log( - `\n${formatHeading("Current locations")} (${chalk.bold(String(locationsCount))}):\n`, + `\n${formatHeading("Current locations")} (${formatCountLabel(entries.length, "location")}):\n`, ); - for (const location of validLocations) { - // Check if location has 'current' property with expected structure - if ( - "current" in location && - typeof location.current === "object" && - location.current !== null && - "member" in location.current - ) { - const locationWithCurrent = location as LocationWithCurrent; - const { member } = locationWithCurrent.current; - this.log( - `Member ID: ${formatResource(member.memberId || member.clientId || "Unknown")}`, - ); - try { - const locationData = extractLocationData(location); - - this.log( - `- ${formatClientId(member.memberId || member.clientId || "Unknown")}:`, - ); - this.log( - ` ${formatLabel("Location")} ${JSON.stringify(locationData, null, 2)}`, - ); - - if (member.isCurrentMember) { - this.log(` ${chalk.dim("(Current member)")}`); - } - } catch (error) { - this.log( - `- ${chalk.red("Error displaying location item")}: ${errorMessage(error)}`, - ); - } - } else { - // Simpler display if location doesn't have expected structure - this.log(`- ${formatClientId("Member")}:`); - this.log( - ` ${formatLabel("Location")} ${JSON.stringify(location, null, 2)}`, - ); - } + for (const entry of entries) { + this.log(`- ${formatClientId(entry.connectionId)}:`); + this.log( + ` ${formatLabel("Location")} ${JSON.stringify(entry.location, null, 2)}`, + ); } } } catch (error) { diff --git a/src/commands/spaces/locations/subscribe.ts b/src/commands/spaces/locations/subscribe.ts index 9ff8b31e..5ff534bd 100644 --- a/src/commands/spaces/locations/subscribe.ts +++ b/src/commands/spaces/locations/subscribe.ts @@ -1,40 +1,27 @@ import type { LocationsEvents } from "@ably/spaces"; import { Args } from "@oclif/core"; -import chalk from "chalk"; import { productApiFlags, clientIdFlag, durationFlag } from "../../../flags.js"; import { SpacesBaseCommand } from "../../../spaces-base-command.js"; import { formatClientId, + formatCountLabel, formatEventType, formatHeading, + formatLabel, formatListening, formatProgress, formatResource, formatSuccess, formatTimestamp, - formatLabel, + formatWarning, } from "../../../utils/output.js"; -// Define interfaces for location types -interface SpaceMember { - clientId: string; +interface LocationEntry { connectionId: string; - isConnected: boolean; - profileData: Record | null; + location: unknown; } -interface LocationData { - [key: string]: unknown; -} - -interface LocationItem { - location: LocationData; - member: SpaceMember; -} - -// Define type for subscription - export default class SpacesLocationsSubscribe extends SpacesBaseCommand { static override args = { space: Args.string({ @@ -98,7 +85,7 @@ export default class SpacesLocationsSubscribe extends SpacesBaseCommand { ); } - let locations: LocationItem[] = []; + let locations: LocationEntry[] = []; try { const result = await this.space!.locations.getAll(); this.logCliEvent( @@ -109,47 +96,25 @@ export default class SpacesLocationsSubscribe extends SpacesBaseCommand { { locations: result }, ); - if (result && typeof result === "object") { - if (Array.isArray(result)) { - // Unlikely based on current docs, but handle if API changes - // Need to map Array result to LocationItem[] if structure differs - this.logCliEvent( - flags, - "location", - "initialFormatWarning", - "Received array format for initial locations, expected object", - ); - // Assuming array elements match expected structure for now: - locations = result.map( - (item: { location: LocationData; member: SpaceMember }) => ({ - location: item.location, - member: item.member, - }), - ); - } else if (Object.keys(result).length > 0) { - // Standard case: result is an object { connectionId: locationData } - locations = Object.entries(result).map( - ([connectionId, locationData]) => ({ - location: locationData as LocationData, - member: { - // Construct a partial SpaceMember as SDK doesn't provide full details here - clientId: "unknown", // clientId not directly available in getAll response - connectionId, - isConnected: true, // Assume connected for initial state - profileData: null, - }, - }), - ); - } + if ( + result && + typeof result === "object" && + Object.keys(result).length > 0 + ) { + locations = Object.entries(result) + .filter(([, loc]) => loc != null) + .map(([connectionId, locationData]) => ({ + connectionId, + location: locationData, + })); } if (this.shouldOutputJson(flags)) { this.logJsonResult( { - locations: locations.map((item) => ({ - // Map to a simpler structure for output if needed - connectionId: item.member.connectionId, - location: item.location, + locations: locations.map((entry) => ({ + connectionId: entry.connectionId, + location: entry.location, })), spaceName, eventType: "locations_snapshot", @@ -158,18 +123,16 @@ export default class SpacesLocationsSubscribe extends SpacesBaseCommand { ); } else if (locations.length === 0) { this.log( - chalk.yellow("No locations are currently set in this space."), + formatWarning("No locations are currently set in this space."), ); } else { this.log( - `\n${formatHeading("Current locations")} (${chalk.bold(locations.length.toString())}):\n`, + `\n${formatHeading("Current locations")} (${formatCountLabel(locations.length, "location")}):\n`, ); - for (const item of locations) { + for (const entry of locations) { + this.log(`- ${formatClientId(entry.connectionId)}:`); this.log( - `- Connection ID: ${chalk.blue(item.member.connectionId || "Unknown")}`, - ); // Use connectionId as key - this.log( - ` ${formatLabel("Location")} ${JSON.stringify(item.location)}`, + ` ${formatLabel("Location")} ${JSON.stringify(entry.location)}`, ); } } @@ -273,7 +236,7 @@ export default class SpacesLocationsSubscribe extends SpacesBaseCommand { this.fail(error, flags, "locationSubscribe", { spaceName }); } finally { // Wrap all cleanup in a timeout to prevent hanging - if (!this.shouldOutputJson(flags || {})) { + if (!this.shouldOutputJson(flags)) { if (this.cleanupInProgress) { this.log(formatSuccess("Graceful shutdown complete.")); } else { diff --git a/test/helpers/mock-ably-spaces.ts b/test/helpers/mock-ably-spaces.ts index 22641ca1..62b621e2 100644 --- a/test/helpers/mock-ably-spaces.ts +++ b/test/helpers/mock-ably-spaces.ts @@ -174,7 +174,7 @@ function createMockSpaceLocations(): MockSpaceLocations { return { set: vi.fn().mockImplementation(async () => {}), - getAll: vi.fn().mockResolvedValue([]), + getAll: vi.fn().mockResolvedValue({}), getSelf: vi.fn().mockResolvedValue(null), subscribe: vi.fn((eventOrCallback, callback?) => { const cb = callback ?? eventOrCallback; @@ -237,7 +237,7 @@ function createMockSpaceCursors(): MockSpaceCursors { return { set: vi.fn().mockImplementation(async () => {}), - getAll: vi.fn().mockResolvedValue([]), + getAll: vi.fn().mockResolvedValue({}), subscribe: vi.fn((eventOrCallback, callback?) => { const cb = callback ?? eventOrCallback; const event = callback ? eventOrCallback : null; diff --git a/test/unit/commands/spaces/cursors/get-all.test.ts b/test/unit/commands/spaces/cursors/get-all.test.ts index f5774460..c246e885 100644 --- a/test/unit/commands/spaces/cursors/get-all.test.ts +++ b/test/unit/commands/spaces/cursors/get-all.test.ts @@ -26,20 +26,20 @@ describe("spaces:cursors:get-all command", () => { it("should get all cursors from a space", async () => { const spacesMock = getMockAblySpaces(); const space = spacesMock._getSpace("test-space"); - space.cursors.getAll.mockResolvedValue([ - { + space.cursors.getAll.mockResolvedValue({ + "conn-1": { clientId: "user-1", connectionId: "conn-1", position: { x: 100, y: 200 }, data: { color: "red" }, }, - { + "conn-2": { clientId: "user-2", connectionId: "conn-2", position: { x: 300, y: 400 }, data: { color: "blue" }, }, - ]); + }); const { stdout } = await runCommand( ["spaces:cursors:get-all", "test-space", "--json"], @@ -61,7 +61,7 @@ describe("spaces:cursors:get-all command", () => { it("should handle no cursors found", async () => { const spacesMock = getMockAblySpaces(); const space = spacesMock._getSpace("test-space"); - space.cursors.getAll.mockResolvedValue([]); + space.cursors.getAll.mockResolvedValue({}); const { stdout } = await runCommand( ["spaces:cursors:get-all", "test-space", "--json"], @@ -98,14 +98,14 @@ describe("spaces:cursors:get-all command", () => { it("should output JSON envelope with type and command for cursor results", async () => { const spacesMock = getMockAblySpaces(); const space = spacesMock._getSpace("test-space"); - space.cursors.getAll.mockResolvedValue([ - { + space.cursors.getAll.mockResolvedValue({ + "conn-1": { clientId: "user-1", connectionId: "conn-1", position: { x: 10, y: 20 }, data: null, }, - ]); + }); const { stdout } = await runCommand( ["spaces:cursors:get-all", "test-space", "--json"], @@ -130,7 +130,7 @@ describe("spaces:cursors:get-all command", () => { const realtimeMock = getMockAblyRealtime(); const spacesMock = getMockAblySpaces(); const space = spacesMock._getSpace("test-space"); - space.cursors.getAll.mockResolvedValue([]); + space.cursors.getAll.mockResolvedValue({}); await runCommand( ["spaces:cursors:get-all", "test-space", "--json"], diff --git a/test/unit/commands/spaces/cursors/subscribe.test.ts b/test/unit/commands/spaces/cursors/subscribe.test.ts index c53854c0..7baed505 100644 --- a/test/unit/commands/spaces/cursors/subscribe.test.ts +++ b/test/unit/commands/spaces/cursors/subscribe.test.ts @@ -26,7 +26,7 @@ describe("spaces:cursors:subscribe command", () => { it("should subscribe to cursor updates in a space", async () => { const spacesMock = getMockAblySpaces(); const space = spacesMock._getSpace("test-space"); - space.cursors.getAll.mockResolvedValue([]); + space.cursors.getAll.mockResolvedValue({}); await runCommand( ["spaces:cursors:subscribe", "test-space"], @@ -43,7 +43,7 @@ describe("spaces:cursors:subscribe command", () => { it("should display initial subscription message", async () => { const spacesMock = getMockAblySpaces(); const space = spacesMock._getSpace("test-space"); - space.cursors.getAll.mockResolvedValue([]); + space.cursors.getAll.mockResolvedValue({}); const { stdout } = await runCommand( ["spaces:cursors:subscribe", "test-space"], @@ -60,7 +60,7 @@ describe("spaces:cursors:subscribe command", () => { const realtimeMock = getMockAblyRealtime(); const spacesMock = getMockAblySpaces(); const space = spacesMock._getSpace("test-space"); - space.cursors.getAll.mockResolvedValue([]); + space.cursors.getAll.mockResolvedValue({}); // Use SIGINT to exit @@ -78,7 +78,7 @@ describe("spaces:cursors:subscribe command", () => { it("should output JSON event with envelope when cursor update is received", async () => { const spacesMock = getMockAblySpaces(); const space = spacesMock._getSpace("test-space"); - space.cursors.getAll.mockResolvedValue([]); + space.cursors.getAll.mockResolvedValue({}); // Fire a cursor event synchronously when subscribe is called space.cursors.subscribe.mockImplementation( @@ -114,7 +114,7 @@ describe("spaces:cursors:subscribe command", () => { it("should wait for cursors channel to attach if not already attached", async () => { const spacesMock = getMockAblySpaces(); const space = spacesMock._getSpace("test-space"); - space.cursors.getAll.mockResolvedValue([]); + space.cursors.getAll.mockResolvedValue({}); // Mock channel as attaching space.cursors.channel.state = "attaching"; diff --git a/test/unit/commands/spaces/locations/get-all.test.ts b/test/unit/commands/spaces/locations/get-all.test.ts index 7856093f..8891811a 100644 --- a/test/unit/commands/spaces/locations/get-all.test.ts +++ b/test/unit/commands/spaces/locations/get-all.test.ts @@ -26,13 +26,9 @@ describe("spaces:locations:get-all command", () => { it("should get all locations from a space", async () => { const spacesMock = getMockAblySpaces(); const space = spacesMock._getSpace("test-space"); - space.locations.getAll.mockResolvedValue([ - { - member: { clientId: "user-1", connectionId: "conn-1" }, - currentLocation: { x: 100, y: 200 }, - previousLocation: null, - }, - ]); + space.locations.getAll.mockResolvedValue({ + "conn-1": { x: 100, y: 200 }, + }); const { stdout } = await runCommand( ["spaces:locations:get-all", "test-space", "--json"], @@ -47,13 +43,9 @@ describe("spaces:locations:get-all command", () => { it("should output JSON envelope with type and command for location results", async () => { const spacesMock = getMockAblySpaces(); const space = spacesMock._getSpace("test-space"); - space.locations.getAll.mockResolvedValue([ - { - member: { clientId: "user-1", connectionId: "conn-1" }, - currentLocation: { x: 100, y: 200 }, - previousLocation: null, - }, - ]); + space.locations.getAll.mockResolvedValue({ + "conn-1": { x: 100, y: 200 }, + }); const { stdout } = await runCommand( ["spaces:locations:get-all", "test-space", "--json"], @@ -70,12 +62,20 @@ describe("spaces:locations:get-all command", () => { expect(resultRecord).toHaveProperty("success", true); expect(resultRecord).toHaveProperty("spaceName", "test-space"); expect(resultRecord!.locations).toBeInstanceOf(Array); + expect(resultRecord!.locations.length).toBe(1); + expect(resultRecord!.locations[0]).toHaveProperty( + "connectionId", + "conn-1", + ); + expect(resultRecord!.locations[0]).toHaveProperty("location"); + expect(resultRecord!.locations[0]).not.toHaveProperty("memberId"); + expect(resultRecord!.locations[0]).not.toHaveProperty("isCurrentMember"); }); it("should handle no locations found", async () => { const spacesMock = getMockAblySpaces(); const space = spacesMock._getSpace("test-space"); - space.locations.getAll.mockResolvedValue([]); + space.locations.getAll.mockResolvedValue({}); const { stdout } = await runCommand( ["spaces:locations:get-all", "test-space", "--json"], From 03f1ab87a37a0e650fc6ab3905e1658033eb8222 Mon Sep 17 00:00:00 2001 From: sacOO7 Date: Mon, 16 Mar 2026 19:36:37 +0530 Subject: [PATCH 2/5] Refactored and updated spaces/cursors/locations formatting for both json and non-json output --- src/commands/spaces/cursors/get-all.ts | 113 +-------- src/commands/spaces/cursors/set.ts | 39 +-- src/commands/spaces/cursors/subscribe.ts | 38 ++- src/commands/spaces/list.ts | 6 +- src/commands/spaces/locations/get-all.ts | 20 +- src/commands/spaces/locations/set.ts | 52 ++-- src/commands/spaces/locations/subscribe.ts | 126 ++-------- src/commands/spaces/locks/acquire.ts | 25 +- src/commands/spaces/locks/get-all.ts | 64 ++--- src/commands/spaces/locks/get.ts | 19 +- src/commands/spaces/locks/subscribe.ts | 139 +---------- src/commands/spaces/members/enter.ts | 90 ++----- src/commands/spaces/members/subscribe.ts | 118 +-------- src/utils/spaces-output.ts | 226 ++++++++++++++++++ test/e2e/spaces/spaces-e2e.test.ts | 4 +- test/helpers/mock-ably-spaces.ts | 18 +- .../commands/spaces/cursors/get-all.test.ts | 5 +- test/unit/commands/spaces/cursors/set.test.ts | 10 +- .../commands/spaces/cursors/subscribe.test.ts | 9 +- test/unit/commands/spaces/list.test.ts | 3 - .../commands/spaces/locations/get-all.test.ts | 5 +- .../commands/spaces/locations/set.test.ts | 1 - .../spaces/locations/subscribe.test.ts | 109 +++++---- .../commands/spaces/locks/acquire.test.ts | 46 +++- .../commands/spaces/locks/get-all.test.ts | 33 ++- test/unit/commands/spaces/locks/get.test.ts | 40 +++- .../commands/spaces/locks/subscribe.test.ts | 124 +++++----- .../commands/spaces/members/enter.test.ts | 12 +- .../commands/spaces/members/subscribe.test.ts | 104 ++++---- 29 files changed, 718 insertions(+), 880 deletions(-) create mode 100644 src/utils/spaces-output.ts diff --git a/src/commands/spaces/cursors/get-all.ts b/src/commands/spaces/cursors/get-all.ts index bfab0972..1f535eae 100644 --- a/src/commands/spaces/cursors/get-all.ts +++ b/src/commands/spaces/cursors/get-all.ts @@ -9,11 +9,16 @@ import { formatClientId, formatCountLabel, formatHeading, + formatIndex, formatProgress, formatResource, formatSuccess, formatWarning, } from "../../../utils/output.js"; +import { + formatCursorBlock, + formatCursorOutput, +} from "../../../utils/spaces-output.js"; export default class SpacesCursorsGetAll extends SpacesBaseCommand { static override args = { @@ -198,21 +203,14 @@ export default class SpacesCursorsGetAll extends SpacesBaseCommand { if (this.shouldOutputJson(flags)) { this.logJsonResult( { - connectionId: this.realtimeClient!.connection.id, - cursors: cursors.map((cursor: CursorUpdate) => ({ - clientId: cursor.clientId, - connectionId: cursor.connectionId, - data: cursor.data, - position: cursor.position, - })), - spaceName, - cursorUpdateReceived, + cursors: cursors.map((cursor: CursorUpdate) => + formatCursorOutput(cursor), + ), }, flags, ); } else { if (!cursorUpdateReceived && cursors.length === 0) { - this.log(chalk.dim("─".repeat(60))); this.log( formatWarning( "No cursor updates are being sent in this space. Make sure other clients are actively setting cursor positions.", @@ -222,107 +220,20 @@ export default class SpacesCursorsGetAll extends SpacesBaseCommand { } if (cursors.length === 0) { - this.log(chalk.dim("─".repeat(60))); this.log(formatWarning("No active cursors found in space.")); return; } - // Show summary table - this.log(chalk.dim("─".repeat(60))); this.log( - `\n${formatHeading("Cursor Summary")} - ${formatCountLabel(cursors.length, "cursor")} found:\n`, + `\n${formatHeading("Current cursors")} (${formatCountLabel(cursors.length, "cursor")}):\n`, ); - // Table header - const colWidths = { client: 20, x: 8, y: 8, connection: 20 }; - this.log( - chalk.gray( - "┌" + - "─".repeat(colWidths.client + 2) + - "┬" + - "─".repeat(colWidths.x + 2) + - "┬" + - "─".repeat(colWidths.y + 2) + - "┬" + - "─".repeat(colWidths.connection + 2) + - "┐", - ), - ); - this.log( - chalk.gray("│ ") + - chalk.bold("Client ID".padEnd(colWidths.client)) + - chalk.gray(" │ ") + - chalk.bold("X".padEnd(colWidths.x)) + - chalk.gray(" │ ") + - chalk.bold("Y".padEnd(colWidths.y)) + - chalk.gray(" │ ") + - chalk.bold("connection".padEnd(colWidths.connection)) + - chalk.gray(" │"), - ); - this.log( - chalk.gray( - "├" + - "─".repeat(colWidths.client + 2) + - "┼" + - "─".repeat(colWidths.x + 2) + - "┼" + - "─".repeat(colWidths.y + 2) + - "┼" + - "─".repeat(colWidths.connection + 2) + - "┤", - ), - ); - - // Table rows - cursors.forEach((cursor: CursorUpdate) => { - const clientId = (cursor.clientId || "Unknown").slice( - 0, - colWidths.client, - ); - const x = cursor.position.x.toString().slice(0, colWidths.x); - const y = cursor.position.y.toString().slice(0, colWidths.y); - const connectionId = (cursor.connectionId || "Unknown").slice( - 0, - colWidths.connection, - ); - + cursors.forEach((cursor: CursorUpdate, index: number) => { this.log( - chalk.gray("│ ") + - formatClientId(clientId.padEnd(colWidths.client)) + - chalk.gray(" │ ") + - chalk.yellow(x.padEnd(colWidths.x)) + - chalk.gray(" │ ") + - chalk.yellow(y.padEnd(colWidths.y)) + - chalk.gray(" │ ") + - chalk.dim(connectionId.padEnd(colWidths.connection)) + - chalk.gray(" │"), + `${formatIndex(index + 1)} ${formatCursorBlock(cursor, { indent: " " })}`, ); + this.log(""); }); - - this.log( - chalk.gray( - "└" + - "─".repeat(colWidths.client + 2) + - "┴" + - "─".repeat(colWidths.x + 2) + - "┴" + - "─".repeat(colWidths.y + 2) + - "┴" + - "─".repeat(colWidths.connection + 2) + - "┘", - ), - ); - - // Show additional data if any cursor has it - const cursorsWithData = cursors.filter((c) => c.data); - if (cursorsWithData.length > 0) { - this.log(`\n${formatHeading("Additional Data")}:`); - cursorsWithData.forEach((cursor: CursorUpdate) => { - this.log( - ` ${formatClientId(cursor.clientId || "Unknown")}: ${JSON.stringify(cursor.data)}`, - ); - }); - } } } catch (error) { this.fail(error, flags, "cursorGetAll", { spaceName }); diff --git a/src/commands/spaces/cursors/set.ts b/src/commands/spaces/cursors/set.ts index f8bcf3fc..e3ea8ad7 100644 --- a/src/commands/spaces/cursors/set.ts +++ b/src/commands/spaces/cursors/set.ts @@ -1,6 +1,5 @@ +import type { CursorData, CursorPosition } from "@ably/spaces"; import { Args, Flags } from "@oclif/core"; -import chalk from "chalk"; - import { errorMessage } from "../../../utils/errors.js"; import { productApiFlags, clientIdFlag, durationFlag } from "../../../flags.js"; import { SpacesBaseCommand } from "../../../spaces-base-command.js"; @@ -12,18 +11,6 @@ import { formatLabel, } from "../../../utils/output.js"; -// Define cursor types based on Ably documentation -interface CursorPosition { - x: number; - y: number; -} - -interface CursorData { - [key: string]: unknown; -} - -// CursorUpdate interface no longer required in this file - export default class SpacesCursorsSet extends SpacesBaseCommand { static override args = { space: Args.string({ @@ -191,17 +178,31 @@ export default class SpacesCursorsSet extends SpacesBaseCommand { if (this.shouldOutputJson(flags)) { this.logJsonResult( { - cursor: cursorForOutput, - spaceName, + 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 }) + .data ?? null, + }, + ], }, flags, ); } else { this.log( - formatSuccess( - `Set cursor in space ${formatResource(spaceName)} with data: ${chalk.blue(JSON.stringify(cursorForOutput))}.`, - ), + formatSuccess(`Set cursor in space ${formatResource(spaceName)}.`), ); + const lines: string[] = []; + lines.push(`${formatLabel("Position")} (${position.x}, ${position.y})`); + if (data) { + lines.push(`${formatLabel("Data")} ${JSON.stringify(data)}`); + } + this.log(lines.join("\n")); } // Decide how long to remain connected diff --git a/src/commands/spaces/cursors/subscribe.ts b/src/commands/spaces/cursors/subscribe.ts index b0aba265..ccd77f6f 100644 --- a/src/commands/spaces/cursors/subscribe.ts +++ b/src/commands/spaces/cursors/subscribe.ts @@ -7,9 +7,11 @@ import { formatResource, formatSuccess, formatTimestamp, - formatClientId, - formatLabel, } from "../../../utils/output.js"; +import { + formatCursorBlock, + formatCursorOutput, +} from "../../../utils/spaces-output.js"; export default class SpacesCursorsSubscribe extends SpacesBaseCommand { static override args = { @@ -56,35 +58,27 @@ export default class SpacesCursorsSubscribe extends SpacesBaseCommand { this.listener = (cursorUpdate: CursorUpdate) => { try { const timestamp = new Date().toISOString(); - const eventData = { - member: { - clientId: cursorUpdate.clientId, - connectionId: cursorUpdate.connectionId, - }, - position: cursorUpdate.position, - data: cursorUpdate.data, - spaceName, - timestamp, - eventType: "cursor_update", - }; this.logCliEvent( flags, "cursor", "updateReceived", "Cursor update received", - eventData, + { + clientId: cursorUpdate.clientId, + position: cursorUpdate.position, + timestamp, + }, ); if (this.shouldOutputJson(flags)) { - this.logJsonEvent(eventData, flags); - } else { - // Include data field in the output if present - const dataString = cursorUpdate.data - ? ` data: ${JSON.stringify(cursorUpdate.data)}` - : ""; - this.log( - `${formatTimestamp(timestamp)} ${formatClientId(cursorUpdate.clientId)} ${formatLabel("position")} ${JSON.stringify(cursorUpdate.position)}${dataString}`, + this.logJsonEvent( + { cursor: formatCursorOutput(cursorUpdate) }, + flags, ); + } else { + this.log(formatTimestamp(timestamp)); + this.log(formatCursorBlock(cursorUpdate)); + this.log(""); } } catch (error) { this.fail(error, flags, "cursorSubscribe", { diff --git a/src/commands/spaces/list.ts b/src/commands/spaces/list.ts index 3c1b2e3a..3aab7489 100644 --- a/src/commands/spaces/list.ts +++ b/src/commands/spaces/list.ts @@ -132,14 +132,10 @@ export default class SpacesList extends SpacesBaseCommand { if (this.shouldOutputJson(flags)) { this.logJsonResult( { - hasMore: spacesList.length > flags.limit, - shown: limitedSpaces.length, spaces: limitedSpaces.map((space: SpaceItem) => ({ - metrics: space.status?.occupancy?.metrics || {}, spaceName: space.spaceName, + metrics: space.status?.occupancy?.metrics || {}, })), - timestamp: new Date().toISOString(), - total: spacesList.length, }, flags, ); diff --git a/src/commands/spaces/locations/get-all.ts b/src/commands/spaces/locations/get-all.ts index 3fa0fe58..175b25e1 100644 --- a/src/commands/spaces/locations/get-all.ts +++ b/src/commands/spaces/locations/get-all.ts @@ -3,20 +3,16 @@ import { Args } from "@oclif/core"; import { productApiFlags, clientIdFlag } from "../../../flags.js"; import { SpacesBaseCommand } from "../../../spaces-base-command.js"; import { - formatClientId, formatCountLabel, formatHeading, + formatIndex, formatLabel, formatProgress, formatResource, formatSuccess, formatWarning, } from "../../../utils/output.js"; - -interface LocationEntry { - connectionId: string; - location: unknown; -} +import type { LocationEntry } from "../../../utils/spaces-output.js"; export default class SpacesLocationsGetAll extends SpacesBaseCommand { static override args = { @@ -127,8 +123,6 @@ export default class SpacesLocationsGetAll extends SpacesBaseCommand { connectionId: entry.connectionId, location: entry.location, })), - spaceName, - timestamp: new Date().toISOString(), }, flags, ); @@ -141,11 +135,15 @@ export default class SpacesLocationsGetAll extends SpacesBaseCommand { `\n${formatHeading("Current locations")} (${formatCountLabel(entries.length, "location")}):\n`, ); - for (const entry of entries) { - this.log(`- ${formatClientId(entry.connectionId)}:`); + for (let i = 0; i < entries.length; i++) { + const entry = entries[i]; + this.log( + `${formatIndex(i + 1)} ${formatLabel("Connection ID")} ${entry.connectionId}`, + ); this.log( - ` ${formatLabel("Location")} ${JSON.stringify(entry.location, null, 2)}`, + ` ${formatLabel("Location")} ${JSON.stringify(entry.location)}`, ); + this.log(""); } } } catch (error) { diff --git a/src/commands/spaces/locations/set.ts b/src/commands/spaces/locations/set.ts index caaa7c9e..1b4e201a 100644 --- a/src/commands/spaces/locations/set.ts +++ b/src/commands/spaces/locations/set.ts @@ -1,6 +1,5 @@ import type { LocationsEvents } from "@ably/spaces"; import { Args, Flags } from "@oclif/core"; -import chalk from "chalk"; import { productApiFlags, clientIdFlag, durationFlag } from "../../../flags.js"; import { SpacesBaseCommand } from "../../../spaces-base-command.js"; @@ -9,9 +8,8 @@ import { formatListening, formatResource, formatTimestamp, - formatClientId, - formatLabel, } from "../../../utils/output.js"; +import { formatLocationUpdateBlock } from "../../../utils/spaces-output.js"; // Define the type for location subscription interface LocationSubscription { @@ -133,7 +131,7 @@ export default class SpacesLocationsSet extends SpacesBaseCommand { }); if (this.shouldOutputJson(flags)) { - this.logJsonResult({ location, spaceName }, flags); + this.logJsonResult({ location }, flags); } else { this.log( formatSuccess( @@ -144,7 +142,7 @@ export default class SpacesLocationsSet extends SpacesBaseCommand { } catch { // If an error occurs in E2E mode, just exit cleanly after showing what we can if (this.shouldOutputJson(flags)) { - this.logJsonResult({ location, spaceName }, flags); + this.logJsonResult({ location }, flags); } // Don't call this.error() in E2E mode as it sets exit code to 1 } @@ -188,7 +186,6 @@ export default class SpacesLocationsSet extends SpacesBaseCommand { this.locationHandler = (locationUpdate: LocationsEvents.UpdateEvent) => { const timestamp = new Date().toISOString(); const { member } = locationUpdate; - const { currentLocation } = locationUpdate; // Use current location const { connectionId } = member; // Skip self events - check connection ID @@ -197,36 +194,37 @@ export default class SpacesLocationsSet extends SpacesBaseCommand { return; } - const eventData = { - action: "update", - location: currentLocation, - member: { - clientId: member.clientId, - connectionId: member.connectionId, - }, - timestamp, - }; this.logCliEvent( flags, "location", "updateReceived", "Location update received", - eventData, + { + clientId: member.clientId, + connectionId: member.connectionId, + timestamp, + }, ); if (this.shouldOutputJson(flags)) { - this.logJsonEvent(eventData, flags); - } else { - // For locations, use yellow for updates - const actionColor = chalk.yellow; - const action = "update"; - - this.log( - `${formatTimestamp(timestamp)} ${formatClientId(member.clientId || "Unknown")} ${actionColor(action)}d location:`, - ); - this.log( - ` ${formatLabel("Location")} ${JSON.stringify(currentLocation, null, 2)}`, + this.logJsonEvent( + { + location: { + member: { + clientId: member.clientId, + connectionId: member.connectionId, + }, + currentLocation: locationUpdate.currentLocation, + previousLocation: locationUpdate.previousLocation, + timestamp, + }, + }, + flags, ); + } else { + this.log(formatTimestamp(timestamp)); + this.log(formatLocationUpdateBlock(locationUpdate)); + this.log(""); } }; diff --git a/src/commands/spaces/locations/subscribe.ts b/src/commands/spaces/locations/subscribe.ts index 5ff534bd..fc1abe18 100644 --- a/src/commands/spaces/locations/subscribe.ts +++ b/src/commands/spaces/locations/subscribe.ts @@ -4,23 +4,11 @@ import { Args } from "@oclif/core"; import { productApiFlags, clientIdFlag, durationFlag } from "../../../flags.js"; import { SpacesBaseCommand } from "../../../spaces-base-command.js"; import { - formatClientId, - formatCountLabel, - formatEventType, - formatHeading, - formatLabel, formatListening, - formatProgress, - formatResource, formatSuccess, formatTimestamp, - formatWarning, } from "../../../utils/output.js"; - -interface LocationEntry { - connectionId: string; - location: unknown; -} +import { formatLocationUpdateBlock } from "../../../utils/spaces-output.js"; export default class SpacesLocationsSubscribe extends SpacesBaseCommand { static override args = { @@ -70,78 +58,6 @@ export default class SpacesLocationsSubscribe extends SpacesBaseCommand { await this.initializeSpace(flags, spaceName, { enterSpace: true }); - // Get current locations - this.logCliEvent( - flags, - "location", - "gettingInitial", - `Fetching initial locations for space ${spaceName}`, - ); - if (!this.shouldOutputJson(flags)) { - this.log( - formatProgress( - `Fetching current locations for space ${formatResource(spaceName)}`, - ), - ); - } - - let locations: LocationEntry[] = []; - try { - const result = await this.space!.locations.getAll(); - this.logCliEvent( - flags, - "location", - "gotInitial", - `Fetched initial locations`, - { locations: result }, - ); - - if ( - result && - typeof result === "object" && - Object.keys(result).length > 0 - ) { - locations = Object.entries(result) - .filter(([, loc]) => loc != null) - .map(([connectionId, locationData]) => ({ - connectionId, - location: locationData, - })); - } - - if (this.shouldOutputJson(flags)) { - this.logJsonResult( - { - locations: locations.map((entry) => ({ - connectionId: entry.connectionId, - location: entry.location, - })), - spaceName, - eventType: "locations_snapshot", - }, - flags, - ); - } else if (locations.length === 0) { - this.log( - formatWarning("No locations are currently set in this space."), - ); - } else { - this.log( - `\n${formatHeading("Current locations")} (${formatCountLabel(locations.length, "location")}):\n`, - ); - for (const entry of locations) { - this.log(`- ${formatClientId(entry.connectionId)}:`); - this.log( - ` ${formatLabel("Location")} ${JSON.stringify(entry.location)}`, - ); - } - } - } catch (error) { - this.fail(error, flags, "locationSubscribe", { - spaceName, - }); - } - this.logCliEvent( flags, "location", @@ -163,43 +79,37 @@ export default class SpacesLocationsSubscribe extends SpacesBaseCommand { const locationHandler = (update: LocationsEvents.UpdateEvent) => { try { const timestamp = new Date().toISOString(); - const eventData = { - action: "update", - location: update.currentLocation, - member: { - clientId: update.member.clientId, - connectionId: update.member.connectionId, - }, - previousLocation: update.previousLocation, - timestamp, - }; this.logCliEvent( flags, "location", "updateReceived", "Location update received", - { spaceName, ...eventData }, + { + clientId: update.member.clientId, + connectionId: update.member.connectionId, + timestamp, + }, ); if (this.shouldOutputJson(flags)) { this.logJsonEvent( { - spaceName, - eventType: "location_update", - ...eventData, + location: { + member: { + clientId: update.member.clientId, + connectionId: update.member.connectionId, + }, + currentLocation: update.currentLocation, + previousLocation: update.previousLocation, + timestamp, + }, }, flags, ); } else { - this.log( - `${formatTimestamp(timestamp)} ${formatClientId(update.member.clientId)} ${formatEventType("updated")} location:`, - ); - this.log( - ` ${formatLabel("Current")} ${JSON.stringify(update.currentLocation)}`, - ); - this.log( - ` ${formatLabel("Previous")} ${JSON.stringify(update.previousLocation)}`, - ); + this.log(formatTimestamp(timestamp)); + this.log(formatLocationUpdateBlock(update)); + this.log(""); } } catch (error) { this.fail(error, flags, "locationSubscribe", { diff --git a/src/commands/spaces/locks/acquire.ts b/src/commands/spaces/locks/acquire.ts index 146b9775..f75354b3 100644 --- a/src/commands/spaces/locks/acquire.ts +++ b/src/commands/spaces/locks/acquire.ts @@ -8,8 +8,11 @@ import { formatSuccess, formatListening, formatResource, - formatLabel, } from "../../../utils/output.js"; +import { + formatLockBlock, + formatLockOutput, +} from "../../../utils/spaces-output.js"; export default class SpacesLocksAcquire extends SpacesBaseCommand { static override args = { @@ -109,33 +112,19 @@ export default class SpacesLocksAcquire extends SpacesBaseCommand { lockId, lockData as LockOptions, ); - const lockDetails = { - lockId: lock.id, - member: lock.member - ? { - clientId: lock.member.clientId, - connectionId: lock.member.connectionId, - } - : null, - reason: lock.reason, - status: lock.status, - timestamp: lock.timestamp, - }; this.logCliEvent( flags, "lock", "acquired", `Lock acquired: ${lockId}`, - lockDetails, + { lockId: lock.id, status: lock.status }, ); 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.")}`); } } catch (error) { diff --git a/src/commands/spaces/locks/get-all.ts b/src/commands/spaces/locks/get-all.ts index 1f0ccd09..9465728f 100644 --- a/src/commands/spaces/locks/get-all.ts +++ b/src/commands/spaces/locks/get-all.ts @@ -1,25 +1,21 @@ +import type { Lock } from "@ably/spaces"; import { Args } from "@oclif/core"; import chalk from "chalk"; -import { errorMessage } from "../../../utils/errors.js"; import { productApiFlags, clientIdFlag } from "../../../flags.js"; import { SpacesBaseCommand } from "../../../spaces-base-command.js"; import { + formatCountLabel, formatHeading, - formatLabel, + formatIndex, formatProgress, formatResource, formatSuccess, } from "../../../utils/output.js"; - -interface LockItem { - attributes?: Record; - id: string; - member?: { - clientId?: string; - }; - status?: string; -} +import { + formatLockBlock, + formatLockOutput, +} from "../../../utils/spaces-output.js"; export default class SpacesLocksGetAll extends SpacesBaseCommand { static override args = { @@ -112,56 +108,26 @@ export default class SpacesLocksGetAll extends SpacesBaseCommand { ); } - let locks: LockItem[] = []; - const result = await this.space!.locks.getAll(); - locks = Array.isArray(result) ? result : []; - - const validLocks = locks.filter((lock: LockItem) => { - if (!lock || !lock.id) return false; - return true; - }); + const locks: Lock[] = await this.space!.locks.getAll(); if (this.shouldOutputJson(flags)) { this.logJsonResult( { - locks: validLocks.map((lock) => ({ - attributes: lock.attributes || {}, - holder: lock.member?.clientId || null, - id: lock.id, - status: lock.status || "unknown", - })), - spaceName, - timestamp: new Date().toISOString(), + locks: locks.map((lock) => formatLockOutput(lock)), }, flags, ); - } else if (!validLocks || validLocks.length === 0) { + } else if (locks.length === 0) { this.log(chalk.yellow("No locks are currently active in this space.")); } else { - const lockCount = validLocks.length; this.log( - `\n${formatHeading("Current locks")} (${chalk.bold(String(lockCount))}):\n`, + `\n${formatHeading("Current locks")} (${formatCountLabel(locks.length, "lock")}):\n`, ); - validLocks.forEach((lock: LockItem) => { - try { - this.log(`- ${formatResource(lock.id)}:`); - this.log(` ${formatLabel("Status")} ${lock.status || "unknown"}`); - this.log( - ` ${formatLabel("Holder")} ${lock.member?.clientId || "None"}`, - ); - - if (lock.attributes && Object.keys(lock.attributes).length > 0) { - this.log( - ` ${formatLabel("Attributes")} ${JSON.stringify(lock.attributes, null, 2)}`, - ); - } - } catch (error) { - this.log( - `- ${chalk.red("Error displaying lock item")}: ${errorMessage(error)}`, - ); - } - }); + for (let i = 0; i < locks.length; i++) { + this.log(`${formatIndex(i + 1)} ${formatLockBlock(locks[i])}`); + this.log(""); + } } } catch (error) { this.fail(error, flags, "lockGetAll", { spaceName }); diff --git a/src/commands/spaces/locks/get.ts b/src/commands/spaces/locks/get.ts index 5148ba18..d2d0cd64 100644 --- a/src/commands/spaces/locks/get.ts +++ b/src/commands/spaces/locks/get.ts @@ -3,11 +3,11 @@ import chalk from "chalk"; import { productApiFlags, clientIdFlag } from "../../../flags.js"; import { SpacesBaseCommand } from "../../../spaces-base-command.js"; +import { formatResource, formatSuccess } from "../../../utils/output.js"; import { - formatLabel, - formatResource, - formatSuccess, -} from "../../../utils/output.js"; + formatLockBlock, + formatLockOutput, +} from "../../../utils/spaces-output.js"; export default class SpacesLocksGet extends SpacesBaseCommand { static override args = { @@ -55,7 +55,7 @@ export default class SpacesLocksGet extends SpacesBaseCommand { if (!lock) { if (this.shouldOutputJson(flags)) { - this.logJsonResult({ found: false, lockId }, flags); + this.logJsonResult({ locks: [] }, flags); } else { this.log( chalk.yellow( @@ -68,14 +68,9 @@ export default class SpacesLocksGet extends SpacesBaseCommand { } if (this.shouldOutputJson(flags)) { - this.logJsonResult( - structuredClone(lock) as Record, - flags, - ); + this.logJsonResult({ locks: [formatLockOutput(lock)] }, flags); } else { - this.log( - `${formatLabel("Lock details")} ${this.formatJsonOutput(structuredClone(lock), flags)}`, - ); + this.log(formatLockBlock(lock)); } } catch (error) { this.fail(error, flags, "lockGet"); diff --git a/src/commands/spaces/locks/subscribe.ts b/src/commands/spaces/locks/subscribe.ts index ef3035b5..ee154f7b 100644 --- a/src/commands/spaces/locks/subscribe.ts +++ b/src/commands/spaces/locks/subscribe.ts @@ -1,17 +1,13 @@ import { type Lock } from "@ably/spaces"; import { Args } from "@oclif/core"; -import chalk from "chalk"; import { productApiFlags, clientIdFlag, durationFlag } from "../../../flags.js"; import { SpacesBaseCommand } from "../../../spaces-base-command.js"; +import { formatListening, formatTimestamp } from "../../../utils/output.js"; import { - formatHeading, - formatLabel, - formatListening, - formatProgress, - formatResource, - formatTimestamp, -} from "../../../utils/output.js"; + formatLockBlock, + formatLockOutput, +} from "../../../utils/spaces-output.js"; export default class SpacesLocksSubscribe extends SpacesBaseCommand { static override args = { @@ -38,35 +34,6 @@ export default class SpacesLocksSubscribe extends SpacesBaseCommand { private listener: ((lock: Lock) => void) | null = null; - private displayLockDetails(lock: Lock): void { - this.log(` ${formatLabel("Status")} ${lock.status}`); - this.log( - ` ${formatLabel("Member")} ${lock.member?.clientId || "Unknown"}`, - ); - - if (lock.member?.connectionId) { - this.log(` ${formatLabel("Connection ID")} ${lock.member.connectionId}`); - } - - if (lock.timestamp) { - this.log( - ` ${formatLabel("Timestamp")} ${new Date(lock.timestamp).toISOString()}`, - ); - } - - if (lock.attributes) { - this.log( - ` ${formatLabel("Attributes")} ${JSON.stringify(lock.attributes)}`, - ); - } - - if (lock.reason) { - this.log( - ` ${formatLabel("Reason")} ${lock.reason.message || lock.reason.toString()}`, - ); - } - } - async run(): Promise { const { args, flags } = await this.parse(SpacesLocksSubscribe); const { space: spaceName } = args; @@ -91,70 +58,6 @@ export default class SpacesLocksSubscribe extends SpacesBaseCommand { await this.initializeSpace(flags, spaceName, { enterSpace: true }); - if (!this.shouldOutputJson(flags)) { - this.log( - formatProgress(`Connecting to space: ${formatResource(spaceName)}`), - ); - } - - // Get current locks - this.logCliEvent( - flags, - "lock", - "gettingInitial", - "Fetching initial locks", - ); - if (!this.shouldOutputJson(flags)) { - this.log( - formatProgress( - `Fetching current locks for space ${formatResource(spaceName)}`, - ), - ); - } - - const locks = await this.space!.locks.getAll(); - this.logCliEvent( - flags, - "lock", - "gotInitial", - `Fetched ${locks.length} initial locks`, - { count: locks.length, locks }, - ); - - // Output current locks - if (locks.length === 0) { - if (!this.shouldOutputJson(flags)) { - this.log( - chalk.yellow("No locks are currently active in this space."), - ); - } - } else if (this.shouldOutputJson(flags)) { - this.logJsonResult( - { - locks: locks.map((lock) => ({ - id: lock.id, - member: lock.member, - status: lock.status, - timestamp: lock.timestamp, - ...(lock.attributes && { attributes: lock.attributes }), - ...(lock.reason && { reason: lock.reason }), - })), - spaceName, - status: "connected", - }, - flags, - ); - } else { - this.log( - `\n${formatHeading("Current locks")} (${chalk.bold(locks.length.toString())}):\n`, - ); - - for (const lock of locks) { - this.log(`- Lock ID: ${formatResource(lock.id)}`); - this.displayLockDetails(lock); - } - } - // Subscribe to lock events this.logCliEvent( flags, @@ -176,35 +79,17 @@ export default class SpacesLocksSubscribe extends SpacesBaseCommand { this.listener = (lock: Lock) => { const timestamp = new Date().toISOString(); - const eventData = { - lock: { - id: lock.id, - member: lock.member, - status: lock.status, - timestamp: lock.timestamp, - ...(lock.attributes && { attributes: lock.attributes }), - ...(lock.reason && { reason: lock.reason }), - }, - spaceName, - timestamp, - eventType: "lock_event", - }; - - this.logCliEvent( - flags, - "lock", - "event-update", - "Lock event received", - eventData, - ); + this.logCliEvent(flags, "lock", "event-update", "Lock event received", { + lockId: lock.id, + status: lock.status, + }); if (this.shouldOutputJson(flags)) { - this.logJsonEvent(eventData, flags); + this.logJsonEvent({ lock: formatLockOutput(lock) }, flags); } else { - this.log( - `${formatTimestamp(timestamp)} Lock ${formatResource(lock.id)} updated`, - ); - this.displayLockDetails(lock); + this.log(formatTimestamp(timestamp)); + this.log(formatLockBlock(lock)); + this.log(""); } }; diff --git a/src/commands/spaces/members/enter.ts b/src/commands/spaces/members/enter.ts index bdaaf44f..e3d25e6f 100644 --- a/src/commands/spaces/members/enter.ts +++ b/src/commands/spaces/members/enter.ts @@ -8,10 +8,12 @@ import { formatListening, formatResource, formatTimestamp, - formatPresenceAction, - formatClientId, formatLabel, } from "../../../utils/output.js"; +import { + formatMemberEventBlock, + formatMemberOutput, +} from "../../../utils/spaces-output.js"; export default class SpacesMembersEnter extends SpacesBaseCommand { static override args = { @@ -83,22 +85,14 @@ export default class SpacesMembersEnter extends SpacesBaseCommand { { profileData }, ); await this.space!.enter(profileData); - const enteredEventData = { + this.logCliEvent(flags, "member", "enteredSpace", "Entered space", { connectionId: this.realtimeClient!.connection.id, - profile: profileData, - spaceName, - status: "connected", - }; - this.logCliEvent( - flags, - "member", - "enteredSpace", - "Entered space", - enteredEventData, - ); + profileData, + }); if (this.shouldOutputJson(flags)) { - this.logJsonResult(enteredEventData, flags); + const self = await this.space!.members.getSelf(); + this.logJsonResult({ members: [formatMemberOutput(self!)] }, flags); } else { this.log(formatSuccess(`Entered space: ${formatResource(spaceName)}.`)); if (profileData) { @@ -171,76 +165,20 @@ export default class SpacesMembersEnter extends SpacesBaseCommand { timestamp: now, }); - const memberEventData = { - action, - member: { - clientId: member.clientId, - connectionId: member.connectionId, - isConnected: member.isConnected, - profileData: member.profileData, - }, - spaceName, - timestamp, - eventType: "member_update", - }; this.logCliEvent( flags, "member", `update-${action}`, `Member event '${action}' received`, - memberEventData, + { action, clientId, connectionId }, ); if (this.shouldOutputJson(flags)) { - this.logJsonEvent(memberEventData, flags); + this.logJsonEvent({ member: formatMemberOutput(member) }, flags); } else { - const { symbol: actionSymbol, color: actionColor } = - formatPresenceAction(action); - - this.log( - `${formatTimestamp(timestamp)} ${actionColor(actionSymbol)} ${formatClientId(clientId)} ${actionColor(action)}`, - ); - - const hasProfileData = - member.profileData && Object.keys(member.profileData).length > 0; - - if (hasProfileData) { - this.log( - ` ${formatLabel("Profile")} ${JSON.stringify(member.profileData, null, 2)}`, - ); - } else { - // No profile data available - this.logCliEvent( - flags, - "member", - "noProfileDataForMember", - "No profile data available for member", - ); - } - - if (connectionId === "Unknown") { - // Connection ID is unknown - this.logCliEvent( - flags, - "member", - "unknownConnectionId", - "Connection ID is unknown for member", - ); - } else { - this.log(` ${formatLabel("Connection ID")} ${connectionId}`); - } - - if (member.isConnected === false) { - this.log(` ${formatLabel("Status")} Not connected`); - } else { - // Member is connected - this.logCliEvent( - flags, - "member", - "memberConnected", - "Member is connected", - ); - } + this.log(formatTimestamp(timestamp)); + this.log(formatMemberEventBlock(member, action)); + this.log(""); } }; diff --git a/src/commands/spaces/members/subscribe.ts b/src/commands/spaces/members/subscribe.ts index 03ec3608..290dcbb1 100644 --- a/src/commands/spaces/members/subscribe.ts +++ b/src/commands/spaces/members/subscribe.ts @@ -1,18 +1,17 @@ import type { SpaceMember } from "@ably/spaces"; import { Args } from "@oclif/core"; -import chalk from "chalk"; import { productApiFlags, clientIdFlag, durationFlag } from "../../../flags.js"; import { SpacesBaseCommand } from "../../../spaces-base-command.js"; import { - formatClientId, - formatHeading, formatListening, - formatPresenceAction, formatProgress, formatTimestamp, - formatLabel, } from "../../../utils/output.js"; +import { + formatMemberEventBlock, + formatMemberOutput, +} from "../../../utils/spaces-output.js"; export default class SpacesMembersSubscribe extends SpacesBaseCommand { static override args = { @@ -58,73 +57,6 @@ export default class SpacesMembersSubscribe extends SpacesBaseCommand { await this.initializeSpace(flags, spaceName, { enterSpace: true }); - // Get current members - this.logCliEvent( - flags, - "member", - "gettingInitial", - "Fetching initial members", - ); - const members = await this.space!.members.getAll(); - const initialMembers = members.map((member) => ({ - clientId: member.clientId, - connectionId: member.connectionId, - isConnected: member.isConnected, - profileData: member.profileData, - })); - this.logCliEvent( - flags, - "member", - "gotInitial", - `Fetched ${members.length} initial members`, - { count: members.length, members: initialMembers }, - ); - - // Output current members - if (members.length === 0) { - if (!this.shouldOutputJson(flags)) { - this.log( - chalk.yellow("No members are currently present in this space."), - ); - } - } else if (this.shouldOutputJson(flags)) { - this.logJsonResult( - { - members: initialMembers, - spaceName, - status: "connected", - }, - flags, - ); - } else { - this.log( - `\n${formatHeading("Current members")} (${chalk.bold(members.length.toString())}):\n`, - ); - - for (const member of members) { - this.log(`- ${formatClientId(member.clientId || "Unknown")}`); - - if ( - member.profileData && - Object.keys(member.profileData).length > 0 - ) { - this.log( - ` ${formatLabel("Profile")} ${JSON.stringify(member.profileData, null, 2)}`, - ); - } - - if (member.connectionId) { - this.log( - ` ${formatLabel("Connection ID")} ${member.connectionId}`, - ); - } - - if (member.isConnected === false) { - this.log(` ${formatLabel("Status")} Not connected`); - } - } - } - if (!this.shouldOutputJson(flags)) { this.log(`\n${formatListening("Listening for member events.")}\n`); } @@ -179,52 +111,20 @@ export default class SpacesMembersSubscribe extends SpacesBaseCommand { timestamp: now, }); - const memberEventData = { - action, - member: { - clientId: member.clientId, - connectionId: member.connectionId, - isConnected: member.isConnected, - profileData: member.profileData, - }, - spaceName, - timestamp, - eventType: "member_update", - }; this.logCliEvent( flags, "member", `update-${action}`, `Member event '${action}' received`, - memberEventData, + { action, clientId, connectionId }, ); if (this.shouldOutputJson(flags)) { - this.logJsonEvent(memberEventData, flags); + this.logJsonEvent({ member: formatMemberOutput(member) }, flags); } else { - const { symbol: actionSymbol, color: actionColor } = - formatPresenceAction(action); - - this.log( - `${formatTimestamp(timestamp)} ${actionColor(actionSymbol)} ${formatClientId(clientId)} ${actionColor(action)}`, - ); - - if ( - member.profileData && - Object.keys(member.profileData).length > 0 - ) { - this.log( - ` ${formatLabel("Profile")} ${JSON.stringify(member.profileData, null, 2)}`, - ); - } - - if (connectionId !== "Unknown") { - this.log(` ${formatLabel("Connection ID")} ${connectionId}`); - } - - if (member.isConnected === false) { - this.log(` ${formatLabel("Status")} Not connected`); - } + this.log(formatTimestamp(timestamp)); + this.log(formatMemberEventBlock(member, action)); + this.log(""); } }; diff --git a/src/utils/spaces-output.ts b/src/utils/spaces-output.ts new file mode 100644 index 00000000..80409a1d --- /dev/null +++ b/src/utils/spaces-output.ts @@ -0,0 +1,226 @@ +import type { CursorUpdate, Lock, SpaceMember } from "@ably/spaces"; + +import { + formatClientId, + formatEventType, + formatLabel, + formatMessageTimestamp, + formatResource, +} from "./output.js"; + +// --- JSON display interfaces (used by logJsonResult / logJsonEvent) --- + +export interface MemberOutput { + clientId: string; + connectionId: string; + isConnected: boolean; + profileData: Record | null; + location: unknown | null; + lastEvent: { name: string; timestamp: number }; +} + +export interface CursorOutput { + clientId: string; + connectionId: string; + position: { x: number; y: number }; + data: Record | null; +} + +export interface LockOutput { + id: string; + status: string; + member: MemberOutput; + timestamp: number; + attributes: Record | null; + reason: { message?: string; code?: number; statusCode?: number } | null; +} + +export interface LocationEntry { + connectionId: string; + location: unknown; +} + +// --- JSON formatters (SDK type → display interface) --- + +export function formatMemberOutput(member: SpaceMember): MemberOutput { + return { + clientId: member.clientId, + connectionId: member.connectionId, + isConnected: member.isConnected, + profileData: member.profileData ?? null, + location: member.location ?? null, + lastEvent: { + name: member.lastEvent.name, + timestamp: member.lastEvent.timestamp, + }, + }; +} + +export function formatCursorOutput(cursor: CursorUpdate): CursorOutput { + return { + clientId: cursor.clientId, + connectionId: cursor.connectionId, + position: cursor.position, + data: (cursor.data as Record) ?? null, + }; +} + +export function formatLockOutput(lock: Lock): LockOutput { + return { + id: lock.id, + status: lock.status, + member: formatMemberOutput(lock.member), + timestamp: lock.timestamp, + attributes: (lock.attributes as Record) ?? null, + reason: lock.reason + ? { + message: lock.reason.message, + code: lock.reason.code, + statusCode: lock.reason.statusCode, + } + : null, + }; +} + +// --- Human-readable block formatters (for non-JSON output) --- + +/** + * Format a SpaceMember as a multi-line labeled block. + * Used in members enter, members subscribe, and as nested output in locks. + */ +export function formatMemberBlock( + member: SpaceMember, + options?: { indent?: string }, +): string { + const indent = options?.indent ?? ""; + const lines: string[] = [ + `${indent}${formatLabel("Client ID")} ${formatClientId(member.clientId)}`, + `${indent}${formatLabel("Connection ID")} ${member.connectionId}`, + `${indent}${formatLabel("Connected")} ${member.isConnected}`, + ]; + + if (member.profileData && Object.keys(member.profileData).length > 0) { + lines.push( + `${indent}${formatLabel("Profile")} ${JSON.stringify(member.profileData)}`, + ); + } + + if (member.location != null) { + lines.push( + `${indent}${formatLabel("Location")} ${JSON.stringify(member.location)}`, + ); + } + + lines.push( + `${indent}${formatLabel("Last Event")} ${member.lastEvent.name} at ${formatMessageTimestamp(member.lastEvent.timestamp)}`, + ); + + return lines.join("\n"); +} + +/** + * Format a SpaceMember event as a multi-line labeled block with action header. + * Used in members subscribe and members enter for streaming events. + */ +export function formatMemberEventBlock( + member: SpaceMember, + action: string, +): string { + const lines: string[] = [ + `${formatLabel("Action")} ${formatEventType(action)}`, + `${formatLabel("Client ID")} ${formatClientId(member.clientId)}`, + `${formatLabel("Connection ID")} ${member.connectionId}`, + `${formatLabel("Connected")} ${member.isConnected}`, + ]; + + if (member.profileData && Object.keys(member.profileData).length > 0) { + lines.push( + `${formatLabel("Profile")} ${JSON.stringify(member.profileData)}`, + ); + } + + if (member.location != null) { + lines.push(`${formatLabel("Location")} ${JSON.stringify(member.location)}`); + } + + return lines.join("\n"); +} + +/** + * Format a CursorUpdate as a multi-line labeled block. + */ +export function formatCursorBlock( + cursor: CursorUpdate, + options?: { indent?: string }, +): string { + const indent = options?.indent ?? ""; + const lines: string[] = [ + `${indent}${formatLabel("Client ID")} ${formatClientId(cursor.clientId)}`, + `${indent}${formatLabel("Connection ID")} ${cursor.connectionId}`, + `${indent}${formatLabel("Position")} (${cursor.position.x}, ${cursor.position.y})`, + ]; + + if ( + cursor.data && + Object.keys(cursor.data as Record).length > 0 + ) { + lines.push( + `${indent}${formatLabel("Data")} ${JSON.stringify(cursor.data)}`, + ); + } + + return lines.join("\n"); +} + +/** + * Format a Lock as a multi-line labeled block. + */ +export function formatLockBlock(lock: Lock): string { + const lines: string[] = [ + `${formatLabel("Lock ID")} ${formatResource(lock.id)}`, + `${formatLabel("Status")} ${formatEventType(lock.status)}`, + `${formatLabel("Timestamp")} ${formatMessageTimestamp(lock.timestamp)}`, + `${formatLabel("Member")}`, + formatMemberBlock(lock.member, { indent: " " }), + ]; + + if ( + lock.attributes && + Object.keys(lock.attributes as Record).length > 0 + ) { + lines.push( + `${formatLabel("Attributes")} ${JSON.stringify(lock.attributes)}`, + ); + } + + if (lock.reason) { + lines.push( + `${formatLabel("Reason")} ${lock.reason.message || lock.reason.toString()}`, + ); + } + + return lines.join("\n"); +} + +/** + * Format a location update event as a multi-line labeled block. + */ +export function formatLocationUpdateBlock(update: { + member: SpaceMember; + currentLocation: unknown; + previousLocation: unknown; +}): string { + const lines: string[] = [ + `${formatLabel("Client ID")} ${formatClientId(update.member.clientId)}`, + `${formatLabel("Connection ID")} ${update.member.connectionId}`, + `${formatLabel("Current Location")} ${JSON.stringify(update.currentLocation)}`, + ]; + + if (update.previousLocation != null) { + lines.push( + `${formatLabel("Previous Location")} ${JSON.stringify(update.previousLocation)}`, + ); + } + + return lines.join("\n"); +} diff --git a/test/e2e/spaces/spaces-e2e.test.ts b/test/e2e/spaces/spaces-e2e.test.ts index d48a0ae8..f30ca118 100644 --- a/test/e2e/spaces/spaces-e2e.test.ts +++ b/test/e2e/spaces/spaces-e2e.test.ts @@ -191,7 +191,7 @@ describe("Spaces E2E Tests", () => { `bin/run.js spaces locations subscribe ${testSpaceId} --client-id ${client1Id} --duration 20`, outputPath, { - readySignal: "Fetching current locations for space", + readySignal: "Subscribing to location updates", timeoutMs: process.env.CI ? 40000 : 30000, // Increased timeout retryCount: 2, }, @@ -400,7 +400,7 @@ describe("Spaces E2E Tests", () => { if ( output.includes(client2Id) && - output.includes("position:") && + output.includes("Position:") && output.includes("TestUser2") && output.includes("#ff0000") ) { diff --git a/test/helpers/mock-ably-spaces.ts b/test/helpers/mock-ably-spaces.ts index 62b621e2..661d0a3a 100644 --- a/test/helpers/mock-ably-spaces.ts +++ b/test/helpers/mock-ably-spaces.ts @@ -158,6 +158,8 @@ function createMockSpaceMembers(): MockSpaceMembers { connectionId: "mock-connection-id", isConnected: true, profileData: {}, + location: null, + lastEvent: { name: "enter", timestamp: Date.now() }, }), _emitter: emitter, _emit: (member: SpaceMember) => { @@ -204,7 +206,21 @@ function createMockSpaceLocks(): MockSpaceLocks { const emitter = new EventEmitter(); return { - acquire: vi.fn().mockResolvedValue({ id: "mock-lock-id" }), + acquire: vi.fn().mockResolvedValue({ + id: "mock-lock-id", + status: "locked", + member: { + clientId: "mock-client-id", + connectionId: "mock-connection-id", + isConnected: true, + profileData: null, + location: null, + lastEvent: { name: "enter", timestamp: Date.now() }, + }, + timestamp: Date.now(), + attributes: undefined, + reason: undefined, + }), release: vi.fn().mockImplementation(async () => {}), get: vi.fn().mockResolvedValue(null), getAll: vi.fn().mockResolvedValue([]), diff --git a/test/unit/commands/spaces/cursors/get-all.test.ts b/test/unit/commands/spaces/cursors/get-all.test.ts index c246e885..7c6362b7 100644 --- a/test/unit/commands/spaces/cursors/get-all.test.ts +++ b/test/unit/commands/spaces/cursors/get-all.test.ts @@ -53,8 +53,8 @@ describe("spaces:cursors:get-all command", () => { ); expect(space.cursors.getAll).toHaveBeenCalled(); - // The command outputs multiple JSON lines - check the content contains expected data - expect(stdout).toContain("test-space"); + // The command outputs JSON with cursors array + expect(stdout).toContain("cursors"); expect(stdout).toContain("success"); }); @@ -120,7 +120,6 @@ describe("spaces:cursors:get-all command", () => { expect(resultRecord).toHaveProperty("type", "result"); expect(resultRecord).toHaveProperty("command"); expect(resultRecord).toHaveProperty("success", true); - expect(resultRecord).toHaveProperty("spaceName", "test-space"); expect(resultRecord!.cursors).toBeInstanceOf(Array); }); }); diff --git a/test/unit/commands/spaces/cursors/set.test.ts b/test/unit/commands/spaces/cursors/set.test.ts index 82b6863b..13410be0 100644 --- a/test/unit/commands/spaces/cursors/set.test.ts +++ b/test/unit/commands/spaces/cursors/set.test.ts @@ -91,6 +91,8 @@ describe("spaces:cursors:set command", () => { ); expect(stdout).toContain("Set cursor"); expect(stdout).toContain("test-space"); + expect(stdout).toContain("Position:"); + expect(stdout).toContain("(100, 200)"); }); it("should set cursor from --data with position object", async () => { @@ -165,8 +167,12 @@ describe("spaces:cursors:set command", () => { expect(result).toHaveProperty("type", "result"); expect(result).toHaveProperty("command", "spaces:cursors:set"); expect(result).toHaveProperty("success", true); - expect(result).toHaveProperty("spaceName", "test-space"); - expect(result!.cursor.position).toEqual({ x: 100, y: 200 }); + expect(result).toHaveProperty("cursors"); + expect(result!.cursors).toHaveLength(1); + expect(result!.cursors[0]).toHaveProperty("position"); + expect(result!.cursors[0].position).toEqual({ x: 100, y: 200 }); + expect(result!.cursors[0]).toHaveProperty("clientId"); + expect(result!.cursors[0]).toHaveProperty("connectionId"); }); }); diff --git a/test/unit/commands/spaces/cursors/subscribe.test.ts b/test/unit/commands/spaces/cursors/subscribe.test.ts index 7baed505..9be955bf 100644 --- a/test/unit/commands/spaces/cursors/subscribe.test.ts +++ b/test/unit/commands/spaces/cursors/subscribe.test.ts @@ -100,13 +100,16 @@ describe("spaces:cursors:subscribe command", () => { const records = parseNdjsonLines(stdout); const eventRecords = records.filter( - (r) => r.type === "event" && r.eventType === "cursor_update", + (r) => r.type === "event" && r.cursor, ); expect(eventRecords.length).toBeGreaterThan(0); const event = eventRecords[0]; expect(event).toHaveProperty("command"); - expect(event).toHaveProperty("spaceName", "test-space"); - expect(event).toHaveProperty("position"); + expect(event).toHaveProperty("cursor"); + expect(event.cursor).toHaveProperty("clientId", "user-1"); + expect(event.cursor).toHaveProperty("connectionId", "conn-1"); + expect(event.cursor).toHaveProperty("position"); + expect(event.cursor.position).toEqual({ x: 50, y: 75 }); }); }); diff --git a/test/unit/commands/spaces/list.test.ts b/test/unit/commands/spaces/list.test.ts index 60919eb2..da253e5c 100644 --- a/test/unit/commands/spaces/list.test.ts +++ b/test/unit/commands/spaces/list.test.ts @@ -107,9 +107,6 @@ describe("spaces:list command", () => { const json = JSON.parse(stdout); expect(json).toHaveProperty("spaces"); - expect(json).toHaveProperty("total"); - expect(json).toHaveProperty("shown"); - expect(json).toHaveProperty("hasMore"); expect(json).toHaveProperty("success", true); expect(json.spaces).toBeInstanceOf(Array); expect(json.spaces.length).toBe(2); diff --git a/test/unit/commands/spaces/locations/get-all.test.ts b/test/unit/commands/spaces/locations/get-all.test.ts index 8891811a..16c1331e 100644 --- a/test/unit/commands/spaces/locations/get-all.test.ts +++ b/test/unit/commands/spaces/locations/get-all.test.ts @@ -37,7 +37,7 @@ describe("spaces:locations:get-all command", () => { expect(space.enter).toHaveBeenCalled(); expect(space.locations.getAll).toHaveBeenCalled(); - expect(stdout).toContain("test-space"); + expect(stdout).toContain("locations"); }); it("should output JSON envelope with type and command for location results", async () => { @@ -60,7 +60,6 @@ describe("spaces:locations:get-all command", () => { expect(resultRecord).toHaveProperty("type", "result"); expect(resultRecord).toHaveProperty("command"); expect(resultRecord).toHaveProperty("success", true); - expect(resultRecord).toHaveProperty("spaceName", "test-space"); expect(resultRecord!.locations).toBeInstanceOf(Array); expect(resultRecord!.locations.length).toBe(1); expect(resultRecord!.locations[0]).toHaveProperty( @@ -68,8 +67,6 @@ describe("spaces:locations:get-all command", () => { "conn-1", ); expect(resultRecord!.locations[0]).toHaveProperty("location"); - expect(resultRecord!.locations[0]).not.toHaveProperty("memberId"); - expect(resultRecord!.locations[0]).not.toHaveProperty("isCurrentMember"); }); it("should handle no locations found", async () => { diff --git a/test/unit/commands/spaces/locations/set.test.ts b/test/unit/commands/spaces/locations/set.test.ts index 47148a6e..1215f476 100644 --- a/test/unit/commands/spaces/locations/set.test.ts +++ b/test/unit/commands/spaces/locations/set.test.ts @@ -112,7 +112,6 @@ describe("spaces:locations:set command", () => { const result = JSON.parse(stdout); expect(result.success).toBe(true); expect(result.location).toEqual(location); - expect(result.spaceName).toBe("test-space"); }); it("should output JSON error on invalid location", async () => { diff --git a/test/unit/commands/spaces/locations/subscribe.test.ts b/test/unit/commands/spaces/locations/subscribe.test.ts index 10167d06..314a3b07 100644 --- a/test/unit/commands/spaces/locations/subscribe.test.ts +++ b/test/unit/commands/spaces/locations/subscribe.test.ts @@ -26,7 +26,6 @@ describe("spaces:locations:subscribe command", () => { it("should subscribe to location updates in a space", async () => { const spacesMock = getMockAblySpaces(); const space = spacesMock._getSpace("test-space"); - space.locations.getAll.mockResolvedValue({}); await runCommand( ["spaces:locations:subscribe", "test-space"], @@ -40,10 +39,9 @@ describe("spaces:locations:subscribe command", () => { ); }); - it("should display initial subscription message", async () => { + it("should display initial subscription message without fetching current locations", async () => { const spacesMock = getMockAblySpaces(); const space = spacesMock._getSpace("test-space"); - space.locations.getAll.mockResolvedValue({}); const { stdout } = await runCommand( ["spaces:locations:subscribe", "test-space"], @@ -51,61 +49,96 @@ describe("spaces:locations:subscribe command", () => { ); expect(stdout).toContain("Subscribing to location updates"); - expect(stdout).toContain("test-space"); + expect(space.locations.getAll).not.toHaveBeenCalled(); }); - it("should fetch and display current locations", async () => { + it("should output location updates in block format", async () => { const spacesMock = getMockAblySpaces(); const space = spacesMock._getSpace("test-space"); - space.locations.getAll.mockResolvedValue({ - "conn-1": { room: "lobby", x: 100 }, - "conn-2": { room: "chat", x: 200 }, - }); - const { stdout } = await runCommand( + // Capture the subscribe handler and invoke it with a mock update + let locationHandler: ((update: unknown) => void) | undefined; + space.locations.subscribe.mockImplementation( + (_event: string, handler: (update: unknown) => void) => { + locationHandler = handler; + }, + ); + + const runPromise = runCommand( ["spaces:locations:subscribe", "test-space"], import.meta.url, ); - expect(space.locations.getAll).toHaveBeenCalled(); - expect(stdout).toContain("Current locations"); + // Wait a tick for the subscribe to be set up + await new Promise((resolve) => setTimeout(resolve, 50)); + + if (locationHandler) { + locationHandler({ + member: { + clientId: "user-1", + connectionId: "conn-1", + }, + currentLocation: { room: "lobby" }, + previousLocation: { room: "entrance" }, + }); + } + + const { stdout } = await runPromise; + + expect(stdout).toContain("Client ID:"); + expect(stdout).toContain("Connection ID:"); + expect(stdout).toContain("Current Location:"); + expect(stdout).toContain("Previous Location:"); }); }); describe("JSON output", () => { - it("should output JSON envelope with initial locations snapshot", async () => { + it("should output JSON event envelope for location updates", async () => { const spacesMock = getMockAblySpaces(); const space = spacesMock._getSpace("test-space"); - space.locations.getAll.mockResolvedValue({ - "conn-1": { room: "lobby", x: 100 }, - }); - const { stdout } = await runCommand( + let locationHandler: ((update: unknown) => void) | undefined; + space.locations.subscribe.mockImplementation( + (_event: string, handler: (update: unknown) => void) => { + locationHandler = handler; + }, + ); + + const runPromise = runCommand( ["spaces:locations:subscribe", "test-space", "--json"], import.meta.url, ); + await new Promise((resolve) => setTimeout(resolve, 50)); + + if (locationHandler) { + locationHandler({ + member: { + clientId: "user-1", + connectionId: "conn-1", + }, + currentLocation: { room: "lobby" }, + previousLocation: null, + }); + } + + const { stdout } = await runPromise; + const records = parseNdjsonLines(stdout); - const resultRecord = records.find( - (r) => - r.type === "result" && - r.eventType === "locations_snapshot" && - Array.isArray(r.locations), - ); - expect(resultRecord).toBeDefined(); - expect(resultRecord).toHaveProperty("command"); - expect(resultRecord).toHaveProperty("success", true); - expect(resultRecord).toHaveProperty("spaceName", "test-space"); - expect(resultRecord!.locations).toBeInstanceOf(Array); + const eventRecord = records.find((r) => r.type === "event" && r.location); + expect(eventRecord).toBeDefined(); + expect(eventRecord).toHaveProperty("command"); + expect(eventRecord!.location).toHaveProperty("member"); + expect(eventRecord!.location.member).toHaveProperty("clientId", "user-1"); + expect(eventRecord!.location).toHaveProperty("currentLocation"); + expect(eventRecord!.location).toHaveProperty("previousLocation"); }); }); describe("cleanup behavior", () => { it("should close client on completion", async () => { const realtimeMock = getMockAblyRealtime(); - const spacesMock = getMockAblySpaces(); - const space = spacesMock._getSpace("test-space"); - space.locations.getAll.mockResolvedValue({}); + getMockAblySpaces(); // Use SIGINT to exit @@ -120,24 +153,20 @@ describe("spaces:locations:subscribe command", () => { }); describe("error handling", () => { - it("should handle getAll rejection gracefully", async () => { + it("should handle subscribe error gracefully", async () => { const spacesMock = getMockAblySpaces(); const space = spacesMock._getSpace("test-space"); - space.locations.getAll.mockRejectedValue( - new Error("Failed to get locations"), - ); + space.locations.subscribe.mockImplementation(() => { + throw new Error("Failed to subscribe to locations"); + }); - // The command handles the error via fail and exits const { error } = await runCommand( ["spaces:locations:subscribe", "test-space"], import.meta.url, ); - // Command should report the error expect(error).toBeDefined(); - expect(error?.message).toContain("Failed to get locations"); - // Command should NOT continue to subscribe after getAll fails - expect(space.locations.subscribe).not.toHaveBeenCalled(); + expect(error?.message).toContain("Failed to subscribe to locations"); }); }); }); diff --git a/test/unit/commands/spaces/locks/acquire.test.ts b/test/unit/commands/spaces/locks/acquire.test.ts index 9be20aa3..89470542 100644 --- a/test/unit/commands/spaces/locks/acquire.test.ts +++ b/test/unit/commands/spaces/locks/acquire.test.ts @@ -27,8 +27,16 @@ describe("spaces:locks:acquire command", () => { space.locks.acquire.mockResolvedValue({ id: "my-lock", status: "locked", - member: { clientId: "mock-client-id", connectionId: "conn-1" }, + member: { + clientId: "mock-client-id", + connectionId: "conn-1", + isConnected: true, + profileData: null, + location: null, + lastEvent: { name: "enter", timestamp: Date.now() }, + }, timestamp: Date.now(), + attributes: undefined, reason: undefined, }); @@ -49,8 +57,17 @@ describe("spaces:locks:acquire command", () => { space.locks.acquire.mockResolvedValue({ id: "my-lock", status: "locked", - member: { clientId: "mock-client-id", connectionId: "conn-1" }, + member: { + clientId: "mock-client-id", + connectionId: "conn-1", + isConnected: true, + profileData: null, + location: null, + lastEvent: { name: "enter", timestamp: Date.now() }, + }, timestamp: Date.now(), + attributes: undefined, + reason: undefined, }); const { stdout } = await runCommand( @@ -100,8 +117,17 @@ describe("spaces:locks:acquire command", () => { space.locks.acquire.mockResolvedValue({ id: "my-lock", status: "locked", - member: { clientId: "mock-client-id", connectionId: "conn-1" }, + member: { + clientId: "mock-client-id", + connectionId: "conn-1", + isConnected: true, + profileData: null, + location: null, + lastEvent: { name: "enter", timestamp: 1700000000000 }, + }, timestamp: 1700000000000, + attributes: undefined, + reason: undefined, }); const { stdout } = await runCommand( @@ -111,9 +137,17 @@ describe("spaces:locks:acquire command", () => { const result = JSON.parse(stdout); expect(result).toHaveProperty("success", true); - expect(result).toHaveProperty("lock"); - expect(result.lock).toHaveProperty("lockId", "my-lock"); - expect(result.lock).toHaveProperty("status", "locked"); + expect(result).toHaveProperty("locks"); + expect(result.locks).toHaveLength(1); + expect(result.locks[0]).toHaveProperty("id", "my-lock"); + expect(result.locks[0]).toHaveProperty("status", "locked"); + expect(result.locks[0]).toHaveProperty("member"); + expect(result.locks[0].member).toHaveProperty( + "clientId", + "mock-client-id", + ); + expect(result.locks[0]).toHaveProperty("attributes", null); + expect(result.locks[0]).toHaveProperty("reason", null); }); }); diff --git a/test/unit/commands/spaces/locks/get-all.test.ts b/test/unit/commands/spaces/locks/get-all.test.ts index ea3d8391..b8c25e53 100644 --- a/test/unit/commands/spaces/locks/get-all.test.ts +++ b/test/unit/commands/spaces/locks/get-all.test.ts @@ -29,8 +29,18 @@ describe("spaces:locks:get-all command", () => { space.locks.getAll.mockResolvedValue([ { id: "lock-1", - member: { clientId: "user-1", connectionId: "conn-1" }, + member: { + clientId: "user-1", + connectionId: "conn-1", + isConnected: true, + profileData: null, + location: null, + lastEvent: { name: "enter", timestamp: Date.now() }, + }, status: "locked", + timestamp: Date.now(), + attributes: undefined, + reason: undefined, }, ]); @@ -41,7 +51,7 @@ describe("spaces:locks:get-all command", () => { expect(space.enter).toHaveBeenCalled(); expect(space.locks.getAll).toHaveBeenCalled(); - expect(stdout).toContain("test-space"); + expect(stdout).toContain("locks"); }); it("should output JSON envelope with type and command for lock results", async () => { @@ -50,8 +60,18 @@ describe("spaces:locks:get-all command", () => { space.locks.getAll.mockResolvedValue([ { id: "lock-1", - member: { clientId: "user-1", connectionId: "conn-1" }, + member: { + clientId: "user-1", + connectionId: "conn-1", + isConnected: true, + profileData: null, + location: null, + lastEvent: { name: "enter", timestamp: Date.now() }, + }, status: "locked", + timestamp: Date.now(), + attributes: undefined, + reason: undefined, }, ]); @@ -68,8 +88,13 @@ describe("spaces:locks:get-all command", () => { expect(resultRecord).toHaveProperty("type", "result"); expect(resultRecord).toHaveProperty("command"); expect(resultRecord).toHaveProperty("success", true); - expect(resultRecord).toHaveProperty("spaceName", "test-space"); expect(resultRecord!.locks).toBeInstanceOf(Array); + expect(resultRecord!.locks[0]).toHaveProperty("id", "lock-1"); + expect(resultRecord!.locks[0]).toHaveProperty("member"); + expect(resultRecord!.locks[0].member).toHaveProperty( + "clientId", + "user-1", + ); }); it("should handle no locks found", async () => { diff --git a/test/unit/commands/spaces/locks/get.test.ts b/test/unit/commands/spaces/locks/get.test.ts index c8054e5d..fdfdda84 100644 --- a/test/unit/commands/spaces/locks/get.test.ts +++ b/test/unit/commands/spaces/locks/get.test.ts @@ -53,8 +53,18 @@ describe("spaces:locks:get command", () => { const space = spacesMock._getSpace("test-space"); space.locks.get.mockResolvedValue({ id: "my-lock", - member: { clientId: "user-1", connectionId: "conn-1" }, + member: { + clientId: "user-1", + connectionId: "conn-1", + isConnected: true, + profileData: null, + location: null, + lastEvent: { name: "enter", timestamp: Date.now() }, + }, status: "locked", + timestamp: Date.now(), + attributes: undefined, + reason: undefined, }); const { stdout } = await runCommand( @@ -72,8 +82,18 @@ describe("spaces:locks:get command", () => { const space = spacesMock._getSpace("test-space"); space.locks.get.mockResolvedValue({ id: "my-lock", - member: { clientId: "user-1", connectionId: "conn-1" }, + member: { + clientId: "user-1", + connectionId: "conn-1", + isConnected: true, + profileData: null, + location: null, + lastEvent: { name: "enter", timestamp: Date.now() }, + }, status: "locked", + timestamp: Date.now(), + attributes: undefined, + reason: undefined, }); const { stdout } = await runCommand( @@ -83,13 +103,18 @@ describe("spaces:locks:get command", () => { const records = parseNdjsonLines(stdout); const resultRecord = records.find( - (r) => r.type === "result" && r.id === "my-lock", + (r) => r.type === "result" && Array.isArray(r.locks), ); expect(resultRecord).toBeDefined(); expect(resultRecord).toHaveProperty("type", "result"); expect(resultRecord).toHaveProperty("command", "spaces:locks:get"); expect(resultRecord).toHaveProperty("success", true); - expect(resultRecord).toHaveProperty("status", "locked"); + expect(resultRecord!.locks).toHaveLength(1); + expect(resultRecord!.locks[0]).toHaveProperty("id", "my-lock"); + expect(resultRecord!.locks[0]).toHaveProperty("status", "locked"); + expect(resultRecord!.locks[0]).toHaveProperty("member"); + expect(resultRecord!.locks[0]).toHaveProperty("attributes", null); + expect(resultRecord!.locks[0]).toHaveProperty("reason", null); }); it("should handle lock not found", async () => { @@ -103,7 +128,12 @@ describe("spaces:locks:get command", () => { ); expect(space.locks.get).toHaveBeenCalledWith("nonexistent-lock"); - expect(stdout).toBeDefined(); + const records = parseNdjsonLines(stdout); + const resultRecord = records.find( + (r) => r.type === "result" && Array.isArray(r.locks), + ); + expect(resultRecord).toBeDefined(); + expect(resultRecord!.locks).toEqual([]); }); }); diff --git a/test/unit/commands/spaces/locks/subscribe.test.ts b/test/unit/commands/spaces/locks/subscribe.test.ts index 77476dc5..7ccd3f7d 100644 --- a/test/unit/commands/spaces/locks/subscribe.test.ts +++ b/test/unit/commands/spaces/locks/subscribe.test.ts @@ -26,7 +26,6 @@ describe("spaces:locks:subscribe command", () => { it("should subscribe to lock events in a space", async () => { const spacesMock = getMockAblySpaces(); const space = spacesMock._getSpace("test-space"); - space.locks.getAll.mockResolvedValue([]); await runCommand( ["spaces:locks:subscribe", "test-space"], @@ -37,10 +36,9 @@ describe("spaces:locks:subscribe command", () => { expect(space.locks.subscribe).toHaveBeenCalledWith(expect.any(Function)); }); - it("should display initial subscription message", async () => { + it("should display listening message without fetching initial locks", async () => { const spacesMock = getMockAblySpaces(); const space = spacesMock._getSpace("test-space"); - space.locks.getAll.mockResolvedValue([]); const { stdout } = await runCommand( ["spaces:locks:subscribe", "test-space"], @@ -48,60 +46,75 @@ describe("spaces:locks:subscribe command", () => { ); expect(stdout).toContain("Subscribing to lock events"); - expect(stdout).toContain("test-space"); + expect(space.locks.getAll).not.toHaveBeenCalled(); }); - it("should fetch and display current locks", async () => { + it("should output lock events using block format", async () => { const spacesMock = getMockAblySpaces(); const space = spacesMock._getSpace("test-space"); - space.locks.getAll.mockResolvedValue([ - { - id: "lock-1", - status: "locked", - member: { clientId: "user-1", connectionId: "conn-1" }, - }, - { - id: "lock-2", - status: "pending", - member: { clientId: "user-2", connectionId: "conn-2" }, - }, - ]); - const { stdout } = await runCommand( - ["spaces:locks:subscribe", "test-space"], - import.meta.url, + // Capture the subscribe callback and invoke it with a lock event + space.locks.subscribe.mockImplementation( + (callback: (lock: unknown) => void) => { + callback({ + id: "lock-1", + status: "locked", + member: { + clientId: "user-1", + connectionId: "conn-1", + isConnected: true, + profileData: null, + location: null, + lastEvent: { name: "enter", timestamp: Date.now() }, + }, + timestamp: Date.now(), + attributes: undefined, + reason: undefined, + }); + return Promise.resolve(); + }, ); - expect(space.locks.getAll).toHaveBeenCalled(); - expect(stdout).toContain("Current locks"); - expect(stdout).toContain("lock-1"); - }); - - it("should show message when no locks exist", async () => { - const spacesMock = getMockAblySpaces(); - const space = spacesMock._getSpace("test-space"); - space.locks.getAll.mockResolvedValue([]); - const { stdout } = await runCommand( ["spaces:locks:subscribe", "test-space"], import.meta.url, ); - expect(stdout).toContain("No locks"); + expect(stdout).toContain("Lock ID:"); + expect(stdout).toContain("lock-1"); + expect(stdout).toContain("Status:"); + expect(stdout).toContain("locked"); + expect(stdout).toContain("Member:"); + expect(stdout).toContain("user-1"); }); }); describe("JSON output", () => { - it("should output JSON envelope with initial locks snapshot", async () => { + it("should output JSON event envelope for lock events", async () => { const spacesMock = getMockAblySpaces(); const space = spacesMock._getSpace("test-space"); - space.locks.getAll.mockResolvedValue([ - { - id: "lock-1", - status: "locked", - member: { clientId: "user-1", connectionId: "conn-1" }, + + // Capture the subscribe callback and invoke it with a lock event + space.locks.subscribe.mockImplementation( + (callback: (lock: unknown) => void) => { + callback({ + id: "lock-1", + status: "locked", + member: { + clientId: "user-1", + connectionId: "conn-1", + isConnected: true, + profileData: null, + location: null, + lastEvent: { name: "enter", timestamp: Date.now() }, + }, + timestamp: Date.now(), + attributes: undefined, + reason: undefined, + }); + return Promise.resolve(); }, - ]); + ); const { stdout } = await runCommand( ["spaces:locks:subscribe", "test-space", "--json"], @@ -109,26 +122,20 @@ describe("spaces:locks:subscribe command", () => { ); const records = parseNdjsonLines(stdout); - const resultRecord = records.find( - (r) => r.type === "result" && Array.isArray(r.locks), - ); - expect(resultRecord).toBeDefined(); - expect(resultRecord).toHaveProperty("type", "result"); - expect(resultRecord).toHaveProperty("command"); - expect(resultRecord).toHaveProperty("success", true); - expect(resultRecord).toHaveProperty("spaceName", "test-space"); - expect(resultRecord!.locks).toBeInstanceOf(Array); + const eventRecord = records.find((r) => r.type === "event" && r.lock); + expect(eventRecord).toBeDefined(); + expect(eventRecord).toHaveProperty("type", "event"); + expect(eventRecord).toHaveProperty("command"); + expect(eventRecord!.lock).toHaveProperty("id", "lock-1"); + expect(eventRecord!.lock).toHaveProperty("status", "locked"); + expect(eventRecord!.lock).toHaveProperty("member"); }); }); describe("cleanup behavior", () => { it("should close client on completion", async () => { const realtimeMock = getMockAblyRealtime(); - const spacesMock = getMockAblySpaces(); - const space = spacesMock._getSpace("test-space"); - space.locks.getAll.mockResolvedValue([]); - - // Use SIGINT to exit + getMockAblySpaces(); await runCommand( ["spaces:locks:subscribe", "test-space"], @@ -141,19 +148,20 @@ describe("spaces:locks:subscribe command", () => { }); describe("error handling", () => { - it("should handle getAll rejection gracefully", async () => { + it("should handle subscribe rejection gracefully", async () => { const spacesMock = getMockAblySpaces(); const space = spacesMock._getSpace("test-space"); - space.locks.getAll.mockRejectedValue(new Error("Failed to get locks")); + space.locks.subscribe.mockRejectedValue( + new Error("Failed to subscribe to locks"), + ); - // The command catches errors and continues - const { stdout } = await runCommand( + const { error } = await runCommand( ["spaces:locks:subscribe", "test-space"], import.meta.url, ); - // Command should have run (output should be present) - expect(stdout).toBeDefined(); + // Command should have attempted to run and reported the error + expect(error).toBeDefined(); }); }); }); diff --git a/test/unit/commands/spaces/members/enter.test.ts b/test/unit/commands/spaces/members/enter.test.ts index eccb28cd..702bcdee 100644 --- a/test/unit/commands/spaces/members/enter.test.ts +++ b/test/unit/commands/spaces/members/enter.test.ts @@ -88,8 +88,16 @@ describe("spaces:members:enter command", () => { const result = JSON.parse(stdout); expect(result.success).toBe(true); - expect(result.spaceName).toBe("test-space"); - expect(result.status).toBe("connected"); + expect(result.members).toBeDefined(); + expect(result.members).toHaveLength(1); + expect(result.members[0]).toHaveProperty("clientId", "mock-client-id"); + expect(result.members[0]).toHaveProperty( + "connectionId", + "mock-connection-id", + ); + expect(result.members[0]).toHaveProperty("isConnected", true); + expect(result.members[0]).toHaveProperty("location", null); + expect(result.members[0]).toHaveProperty("lastEvent"); }); it("should output JSON error on invalid profile", async () => { diff --git a/test/unit/commands/spaces/members/subscribe.test.ts b/test/unit/commands/spaces/members/subscribe.test.ts index 11ccbbf5..30b08495 100644 --- a/test/unit/commands/spaces/members/subscribe.test.ts +++ b/test/unit/commands/spaces/members/subscribe.test.ts @@ -21,67 +21,40 @@ describe("spaces:members:subscribe command", () => { standardFlagTests("spaces:members:subscribe", import.meta.url, ["--json"]); describe("functionality", () => { - it("should display current members from getAll()", async () => { + it("should subscribe to member events and output in block format", async () => { const spacesMock = getMockAblySpaces(); const space = spacesMock._getSpace("test-space"); - space.members.getAll.mockResolvedValue([ - { - clientId: "user-1", - connectionId: "conn-1", - isConnected: true, - profileData: {}, - }, - { - clientId: "user-2", - connectionId: "conn-2", - isConnected: true, - profileData: {}, - }, - ]); - - const { stdout } = await runCommand( - ["spaces:members:subscribe", "test-space"], - import.meta.url, - ); - - expect(space.members.getAll).toHaveBeenCalled(); - expect(stdout).toContain("Current members"); - expect(stdout).toContain("user-1"); - expect(stdout).toContain("user-2"); - }); - it("should show profile data for members", async () => { - const spacesMock = getMockAblySpaces(); - const space = spacesMock._getSpace("test-space"); - space.members.getAll.mockResolvedValue([ - { - clientId: "user-1", - connectionId: "conn-1", - isConnected: true, - profileData: { name: "Alice", role: "admin" }, + // Emit a member event after subscription is set up + space.members.subscribe.mockImplementation( + (event: string, cb: (member: unknown) => void) => { + // Fire the callback asynchronously to simulate an incoming event + setTimeout(() => { + cb({ + clientId: "user-1", + connectionId: "other-conn-1", + isConnected: true, + profileData: { name: "Alice" }, + location: null, + lastEvent: { name: "update", timestamp: Date.now() }, + }); + }, 10); + return Promise.resolve(); }, - ]); - - const { stdout } = await runCommand( - ["spaces:members:subscribe", "test-space"], - import.meta.url, ); - expect(stdout).toContain("Alice"); - expect(stdout).toContain("admin"); - }); - - it("should show message when no members are present", async () => { - const spacesMock = getMockAblySpaces(); - const space = spacesMock._getSpace("test-space"); - space.members.getAll.mockResolvedValue([]); - const { stdout } = await runCommand( ["spaces:members:subscribe", "test-space"], import.meta.url, ); - expect(stdout).toContain("No members are currently present"); + expect(stdout).toContain("Action:"); + expect(stdout).toContain("update"); + expect(stdout).toContain("Client ID:"); + expect(stdout).toContain("user-1"); + expect(stdout).toContain("Connection ID:"); + expect(stdout).toContain("other-conn-1"); + expect(stdout).toContain("Connected:"); }); }); @@ -89,7 +62,6 @@ describe("spaces:members:subscribe command", () => { it("should subscribe to member update events", async () => { const spacesMock = getMockAblySpaces(); const space = spacesMock._getSpace("test-space"); - space.members.getAll.mockResolvedValue([]); await runCommand( ["spaces:members:subscribe", "test-space"], @@ -105,17 +77,25 @@ describe("spaces:members:subscribe command", () => { }); describe("JSON output", () => { - it("should output JSON for initial members", async () => { + it("should output JSON event for member updates", async () => { const spacesMock = getMockAblySpaces(); const space = spacesMock._getSpace("test-space"); - space.members.getAll.mockResolvedValue([ - { - clientId: "user-1", - connectionId: "conn-1", - isConnected: true, - profileData: { name: "Alice" }, + + space.members.subscribe.mockImplementation( + (event: string, cb: (member: unknown) => void) => { + setTimeout(() => { + cb({ + clientId: "user-1", + connectionId: "other-conn-1", + isConnected: true, + profileData: { name: "Alice" }, + location: null, + lastEvent: { name: "update", timestamp: Date.now() }, + }); + }, 10); + return Promise.resolve(); }, - ]); + ); const { stdout } = await runCommand( ["spaces:members:subscribe", "test-space", "--json"], @@ -123,9 +103,9 @@ describe("spaces:members:subscribe command", () => { ); const result = JSON.parse(stdout); - expect(result.success).toBe(true); - expect(result.members).toHaveLength(1); - expect(result.members[0].clientId).toBe("user-1"); + expect(result.type).toBe("event"); + expect(result.member).toBeDefined(); + expect(result.member.clientId).toBe("user-1"); }); }); From b19aa4e3f1e06e9fcd34ddcfcb10985c7ab4ef43 Mon Sep 17 00:00:00 2001 From: sacOO7 Date: Tue, 17 Mar 2026 14:08:16 +0530 Subject: [PATCH 3/5] Fixed unintended command execution when spaces subscribe, get-all called Fixed few more formatting issues --- src/commands/spaces/cursors/get-all.ts | 178 ++-------------- src/commands/spaces/cursors/set.ts | 50 +---- src/commands/spaces/cursors/subscribe.ts | 35 +--- src/commands/spaces/list.ts | 70 +------ src/commands/spaces/locations/get-all.ts | 57 +----- src/commands/spaces/locations/set.ts | 193 +----------------- src/commands/spaces/locations/subscribe.ts | 53 +---- src/commands/spaces/locks/acquire.ts | 1 + src/commands/spaces/locks/get-all.ts | 60 +----- src/commands/spaces/locks/get.ts | 12 +- src/commands/spaces/locks/subscribe.ts | 47 +---- src/commands/spaces/members/enter.ts | 139 ++----------- src/commands/spaces/members/subscribe.ts | 2 +- src/spaces-base-command.ts | 94 ++++----- src/utils/spaces-output.ts | 3 +- .../commands/spaces/cursors/get-all.test.ts | 7 +- test/unit/commands/spaces/cursors/set.test.ts | 6 +- .../commands/spaces/cursors/subscribe.test.ts | 10 +- .../commands/spaces/locations/get-all.test.ts | 2 +- .../commands/spaces/locations/set.test.ts | 4 +- .../spaces/locations/subscribe.test.ts | 2 +- .../commands/spaces/locks/get-all.test.ts | 2 +- test/unit/commands/spaces/locks/get.test.ts | 2 +- .../commands/spaces/locks/subscribe.test.ts | 2 +- .../commands/spaces/members/enter.test.ts | 14 -- .../commands/spaces/members/subscribe.test.ts | 4 +- 26 files changed, 151 insertions(+), 898 deletions(-) diff --git a/src/commands/spaces/cursors/get-all.ts b/src/commands/spaces/cursors/get-all.ts index 1f535eae..41988580 100644 --- a/src/commands/spaces/cursors/get-all.ts +++ b/src/commands/spaces/cursors/get-all.ts @@ -1,18 +1,14 @@ import { type CursorUpdate } from "@ably/spaces"; import { Args } from "@oclif/core"; -import chalk from "chalk"; import { productApiFlags, clientIdFlag } from "../../../flags.js"; import { SpacesBaseCommand } from "../../../spaces-base-command.js"; -import isTestMode from "../../../utils/test-mode.js"; import { - formatClientId, formatCountLabel, formatHeading, formatIndex, formatProgress, formatResource, - formatSuccess, formatWarning, } from "../../../utils/output.js"; import { @@ -51,187 +47,37 @@ export default class SpacesCursorsGetAll extends SpacesBaseCommand { setupConnectionLogging: false, }); - // Get the space if (!this.shouldOutputJson(flags)) { this.log( - formatProgress(`Connecting to space: ${formatResource(spaceName)}`), + formatProgress( + `Fetching cursors for space ${formatResource(spaceName)}`, + ), ); } - // Enter the space - await this.space!.enter(); + const allCursors = await this.space!.cursors.getAll(); - // Wait for space to be properly entered before fetching cursors - await new Promise((resolve, reject) => { - // Set a reasonable timeout to avoid hanging indefinitely - const timeout = setTimeout(() => { - reject(new Error("Timed out waiting for space connection")); - }, 5000); - - const checkSpaceStatus = () => { - try { - // Check realtime client state - if (this.realtimeClient!.connection.state === "connected") { - clearTimeout(timeout); - if (!this.shouldOutputJson(flags)) { - this.log( - formatSuccess(`Entered space: ${formatResource(spaceName)}.`), - ); - } - - resolve(); - } else if ( - this.realtimeClient!.connection.state === "failed" || - this.realtimeClient!.connection.state === "closed" || - this.realtimeClient!.connection.state === "suspended" - ) { - clearTimeout(timeout); - reject( - new Error( - `Space connection failed with state: ${this.realtimeClient!.connection.state}`, - ), - ); - } else { - // Still connecting, check again shortly - setTimeout(checkSpaceStatus, 100); - } - } catch (error) { - clearTimeout(timeout); - reject(error); - } - }; - - checkSpaceStatus(); - }); - - // Subscribe to cursor updates to ensure we receive remote cursors - let cursorUpdateReceived = false; - const cursorMap = new Map(); - - // Show initial message - if (!this.shouldOutputJson(flags)) { - const waitSeconds = isTestMode() ? "0.5" : "5"; - this.log(`Collecting cursor positions for ${waitSeconds} seconds...`); - this.log(chalk.dim("─".repeat(60))); - } - - const cursorUpdateHandler = (cursor: CursorUpdate) => { - cursorUpdateReceived = true; - - // Update the cursor map - if (cursor.connectionId) { - cursorMap.set(cursor.connectionId, cursor); - - // Show live cursor position updates - if ( - !this.shouldOutputJson(flags) && - this.shouldUseTerminalUpdates() - ) { - const clientDisplay = cursor.clientId || "Unknown"; - const x = cursor.position.x; - const y = cursor.position.y; - - this.log( - `${chalk.gray("►")} ${formatClientId(clientDisplay)}: (${chalk.yellow(x)}, ${chalk.yellow(y)})`, - ); - } - } - }; - - try { - await this.space!.cursors.subscribe("update", cursorUpdateHandler); - } catch (error) { - // If subscription fails, continue anyway - if (!this.shouldOutputJson(flags)) { - this.debug(`Cursor subscription error: ${error}`); - } - } - - // Wait for 5 seconds (or shorter in test mode) - const waitTime = isTestMode() ? 500 : 5000; - await new Promise((resolve) => { - setTimeout(() => { - resolve(); - }, waitTime); - }); - - // Unsubscribe from cursor updates - this.space!.cursors.unsubscribe("update", cursorUpdateHandler); - - // Ensure connection is stable before calling getAll() - if (this.realtimeClient!.connection.state !== "connected") { - await new Promise((resolve, reject) => { - const timeout = setTimeout(() => { - reject(new Error("Timed out waiting for connection to stabilize")); - }, 5000); - - this.realtimeClient!.connection.once("connected", () => { - clearTimeout(timeout); - resolve(); - }); - - if (this.realtimeClient!.connection.state === "connected") { - clearTimeout(timeout); - resolve(); - } - }); - } - - // Now get all cursors (including locally cached ones) and merge with live updates - try { - const allCursors = await this.space!.cursors.getAll(); - - // Add any cached cursors that we didn't see in live updates - for (const cursor of Object.values(allCursors)) { - if (cursor && !cursorMap.has(cursor.connectionId)) { - cursorMap.set(cursor.connectionId, cursor); - } - } - } catch { - // If getAll fails due to connection issues, use only the live updates we collected - if (!this.shouldOutputJson(flags)) { - this.log( - formatWarning( - "Could not fetch all cursors, showing only live updates.", - ), - ); - } - } - - const cursors = [...cursorMap.values()]; + const cursors: CursorUpdate[] = Object.values(allCursors).filter( + (cursor): cursor is CursorUpdate => cursor != null, + ); if (this.shouldOutputJson(flags)) { this.logJsonResult( { - cursors: cursors.map((cursor: CursorUpdate) => - formatCursorOutput(cursor), - ), + cursors: cursors.map((cursor) => formatCursorOutput(cursor)), }, flags, ); + } else if (cursors.length === 0) { + this.log(formatWarning("No active cursors found in space.")); } else { - if (!cursorUpdateReceived && cursors.length === 0) { - this.log( - formatWarning( - "No cursor updates are being sent in this space. Make sure other clients are actively setting cursor positions.", - ), - ); - return; - } - - if (cursors.length === 0) { - this.log(formatWarning("No active cursors found in space.")); - return; - } - this.log( `\n${formatHeading("Current cursors")} (${formatCountLabel(cursors.length, "cursor")}):\n`, ); cursors.forEach((cursor: CursorUpdate, index: number) => { - this.log( - `${formatIndex(index + 1)} ${formatCursorBlock(cursor, { indent: " " })}`, - ); + this.log(`${formatIndex(index + 1)}`); + this.log(formatCursorBlock(cursor, { indent: " " })); this.log(""); }); } diff --git a/src/commands/spaces/cursors/set.ts b/src/commands/spaces/cursors/set.ts index e3ea8ad7..ce60f122 100644 --- a/src/commands/spaces/cursors/set.ts +++ b/src/commands/spaces/cursors/set.ts @@ -57,7 +57,6 @@ export default class SpacesCursorsSet extends SpacesBaseCommand { private simulationIntervalId: NodeJS.Timeout | null = null; - // Override finally to ensure resources are cleaned up async finally(err: Error | undefined): Promise { if (this.simulationIntervalId) { clearInterval(this.simulationIntervalId); @@ -76,14 +75,12 @@ export default class SpacesCursorsSet extends SpacesBaseCommand { let cursorData: Record; if (flags.simulate) { - // For simulate mode, use provided x/y or generate random starting position const startX = flags.x ?? Math.floor(Math.random() * 1000); const startY = flags.y ?? Math.floor(Math.random() * 1000); cursorData = { position: { x: startX, y: startY }, }; - // If --data is also provided with simulate, treat it as additional cursor data if (flags.data) { try { const additionalData = JSON.parse(flags.data); @@ -98,12 +95,10 @@ export default class SpacesCursorsSet extends SpacesBaseCommand { } } } else if (flags.x !== undefined && flags.y !== undefined) { - // Use x & y flags cursorData = { position: { x: flags.x, y: flags.y }, }; - // If --data is also provided with x/y flags, treat it as additional cursor data if (flags.data) { try { const additionalData = JSON.parse(flags.data); @@ -118,7 +113,6 @@ export default class SpacesCursorsSet extends SpacesBaseCommand { } } } else if (flags.data) { - // Use --data JSON format try { cursorData = JSON.parse(flags.data); } catch { @@ -130,7 +124,6 @@ export default class SpacesCursorsSet extends SpacesBaseCommand { ); } - // Validate position when using --data if ( !cursorData.position || typeof (cursorData.position as Record).x !== @@ -197,25 +190,17 @@ export default class SpacesCursorsSet extends SpacesBaseCommand { this.log( formatSuccess(`Set cursor in space ${formatResource(spaceName)}.`), ); - const lines: string[] = []; - lines.push(`${formatLabel("Position")} (${position.x}, ${position.y})`); + const lines: string[] = [ + `${formatLabel("Position X")} ${position.x}`, + `${formatLabel("Position Y")} ${position.y}`, + ]; if (data) { lines.push(`${formatLabel("Data")} ${JSON.stringify(data)}`); } this.log(lines.join("\n")); } - // Decide how long to remain connected - if (flags.duration === 0) { - // Give Ably a moment to propagate the cursor update before exiting so that - // subscribers in automated tests have a chance to receive the event. - await new Promise((resolve) => setTimeout(resolve, 600)); - - // In immediate exit mode, we don't keep the process alive beyond this. - this.exit(0); - } - - // Start simulation if requested + // In simulate mode, keep running with periodic cursor updates if (flags.simulate) { this.logCliEvent( flags, @@ -232,7 +217,6 @@ export default class SpacesCursorsSet extends SpacesBaseCommand { this.simulationIntervalId = setInterval(async () => { try { - // Generate random position within reasonable bounds const simulatedX = Math.floor(Math.random() * 1000); const simulatedY = Math.floor(Math.random() * 800); @@ -267,29 +251,15 @@ export default class SpacesCursorsSet extends SpacesBaseCommand { ); } }, 250); - } - // Inform the user and wait until interrupted or timeout (if provided) - this.logCliEvent( - flags, - "cursor", - "waiting", - "Cursor set – waiting for further instructions", - { duration: flags.duration ?? "indefinite" }, - ); + if (!this.shouldOutputJson(flags)) { + this.log(formatListening("Simulating cursor movement.")); + } - if (!this.shouldOutputJson(flags)) { - this.log( - flags.duration - ? `Waiting ${flags.duration}s before exiting… Press Ctrl+C to exit sooner.` - : formatListening("Cursor set."), - ); + await this.waitAndTrackCleanup(flags, "cursor", flags.duration); } - await this.waitAndTrackCleanup(flags, "cursor", flags.duration); - - // After cleanup (handled in finally), ensure the process exits so user doesn't need multiple Ctrl-C - this.exit(0); + // Non-simulate mode: run() completes, finally() handles cleanup } catch (error) { this.fail(error, flags, "cursorSet", { spaceName }); } diff --git a/src/commands/spaces/cursors/subscribe.ts b/src/commands/spaces/cursors/subscribe.ts index ccd77f6f..46a0be0a 100644 --- a/src/commands/spaces/cursors/subscribe.ts +++ b/src/commands/spaces/cursors/subscribe.ts @@ -4,8 +4,7 @@ import { productApiFlags, clientIdFlag, durationFlag } from "../../../flags.js"; import { SpacesBaseCommand } from "../../../spaces-base-command.js"; import { formatListening, - formatResource, - formatSuccess, + formatProgress, formatTimestamp, } from "../../../utils/output.js"; import { @@ -43,9 +42,12 @@ export default class SpacesCursorsSubscribe extends SpacesBaseCommand { const { space: spaceName } = args; try { - await this.initializeSpace(flags, spaceName, { enterSpace: true }); + if (!this.shouldOutputJson(flags)) { + this.log(formatProgress("Subscribing to cursor updates")); + } + + await this.initializeSpace(flags, spaceName, { enterSpace: false }); - // Subscribe to cursor updates this.logCliEvent( flags, "cursor", @@ -54,7 +56,6 @@ export default class SpacesCursorsSubscribe extends SpacesBaseCommand { ); try { - // Define the listener function this.listener = (cursorUpdate: CursorUpdate) => { try { const timestamp = new Date().toISOString(); @@ -87,10 +88,7 @@ export default class SpacesCursorsSubscribe extends SpacesBaseCommand { } }; - // Workaround for known SDK issue: cursors.subscribe() fails if the underlying ::$cursors channel is not attached await this.waitForCursorsChannelAttachment(flags); - - // Subscribe using the listener await this.space!.cursors.subscribe("update", this.listener); this.logCliEvent( @@ -105,32 +103,13 @@ export default class SpacesCursorsSubscribe extends SpacesBaseCommand { }); } - this.logCliEvent( - flags, - "cursor", - "listening", - "Listening for cursor updates...", - ); - - if (!this.shouldOutputJson(flags)) { - // Log the ready signal for E2E tests - this.log("Subscribing to cursor movements"); - } - - // Print success message if (!this.shouldOutputJson(flags)) { - this.log( - formatSuccess(`Subscribed to space: ${formatResource(spaceName)}.`), - ); - this.log(formatListening("Listening for cursor movements.")); + this.log(`\n${formatListening("Listening for cursor movements.")}\n`); } - // Wait until the user interrupts or the optional duration elapses await this.waitAndTrackCleanup(flags, "cursor", flags.duration); } catch (error) { this.fail(error, flags, "cursorSubscribe", { spaceName }); - } finally { - // Cleanup is now handled by base class finally() method } } } diff --git a/src/commands/spaces/list.ts b/src/commands/spaces/list.ts index 3aab7489..cf620111 100644 --- a/src/commands/spaces/list.ts +++ b/src/commands/spaces/list.ts @@ -2,30 +2,14 @@ import { Flags } from "@oclif/core"; import { productApiFlags } from "../../flags.js"; import { - formatLabel, formatCountLabel, formatLimitWarning, formatResource, } from "../../utils/output.js"; import { SpacesBaseCommand } from "../../spaces-base-command.js"; -interface SpaceMetrics { - connections?: number; - presenceConnections?: number; - presenceMembers?: number; - publishers?: number; - subscribers?: number; -} - -interface SpaceStatus { - occupancy?: { - metrics?: SpaceMetrics; - }; -} - interface SpaceItem { spaceName: string; - status?: SpaceStatus; channelId?: string; [key: string]: unknown; } @@ -61,8 +45,6 @@ export default class SpacesList extends SpacesBaseCommand { const rest = await this.createAblyRestClient(flags); if (!rest) return; - // Build params for channel listing - // We request more channels than the limit to account for filtering interface ChannelParams { limit: number; prefix?: string; @@ -76,7 +58,6 @@ export default class SpacesList extends SpacesBaseCommand { params.prefix = flags.prefix; } - // Fetch channels const channelsResponse = await rest.request( "get", "/channels", @@ -92,41 +73,27 @@ export default class SpacesList extends SpacesBaseCommand { ); } - // Filter to only include space channels const allChannels = channelsResponse.items || []; - - // Map to store deduplicated spaces const spaces = new Map(); - // Filter for space channels and deduplicate for (const channel of allChannels) { const { channelId } = channel; - // Check if this is a space channel (has ::$space suffix) if (channelId.includes("::$space")) { - // Extract the base space name (everything before the first ::$space) - // We need to escape the $ in the regex pattern since it's a special character const spaceNameMatch = channelId.match(/^(.+?)::\$space.*$/); if (spaceNameMatch && spaceNameMatch[1]) { const spaceName = spaceNameMatch[1]; - // Only add if we haven't seen this space before if (!spaces.has(spaceName)) { - // Store the original channel data but with the simple space name - const spaceData: SpaceItem = { - ...channel, + spaces.set(spaceName, { channelId: spaceName, spaceName, - }; - spaces.set(spaceName, spaceData); + }); } } } } - // Convert map to array const spacesList = [...spaces.values()]; - - // Limit the results to the requested number const limitedSpaces = spacesList.slice(0, flags.limit); if (this.shouldOutputJson(flags)) { @@ -134,7 +101,6 @@ export default class SpacesList extends SpacesBaseCommand { { spaces: limitedSpaces.map((space: SpaceItem) => ({ spaceName: space.spaceName, - metrics: space.status?.occupancy?.metrics || {}, })), }, flags, @@ -146,39 +112,11 @@ export default class SpacesList extends SpacesBaseCommand { } this.log( - `Found ${formatCountLabel(limitedSpaces.length, "active space")}:`, + `Found ${formatCountLabel(limitedSpaces.length, "active space")}:\n`, ); limitedSpaces.forEach((space: SpaceItem) => { this.log(`${formatResource(space.spaceName)}`); - - // Show occupancy if available - if (space.status?.occupancy?.metrics) { - const { metrics } = space.status.occupancy; - this.log( - ` ${formatLabel("Connections")} ${metrics.connections || 0}`, - ); - this.log( - ` ${formatLabel("Publishers")} ${metrics.publishers || 0}`, - ); - this.log( - ` ${formatLabel("Subscribers")} ${metrics.subscribers || 0}`, - ); - - if (metrics.presenceConnections !== undefined) { - this.log( - ` ${formatLabel("Presence Connections")} ${metrics.presenceConnections}`, - ); - } - - if (metrics.presenceMembers !== undefined) { - this.log( - ` ${formatLabel("Presence Members")} ${metrics.presenceMembers}`, - ); - } - } - - this.log(""); // Add a line break between spaces }); const warning = formatLimitWarning( @@ -186,7 +124,7 @@ export default class SpacesList extends SpacesBaseCommand { flags.limit, "spaces", ); - if (warning) this.log(warning); + if (warning) this.log(`\n${warning}`); } } catch (error) { this.fail(error, flags, "spaceList"); diff --git a/src/commands/spaces/locations/get-all.ts b/src/commands/spaces/locations/get-all.ts index 175b25e1..4b74007e 100644 --- a/src/commands/spaces/locations/get-all.ts +++ b/src/commands/spaces/locations/get-all.ts @@ -9,7 +9,6 @@ import { formatLabel, formatProgress, formatResource, - formatSuccess, formatWarning, } from "../../../utils/output.js"; import type { LocationEntry } from "../../../utils/spaces-output.js"; @@ -45,55 +44,6 @@ export default class SpacesLocationsGetAll extends SpacesBaseCommand { setupConnectionLogging: false, }); - if (!this.shouldOutputJson(flags)) { - this.log( - formatProgress(`Connecting to space: ${formatResource(spaceName)}`), - ); - } - - await this.space!.enter(); - - await new Promise((resolve, reject) => { - const timeout = setTimeout(() => { - reject(new Error("Timed out waiting for space connection")); - }, 5000); - - const checkSpaceStatus = () => { - try { - if (this.realtimeClient!.connection.state === "connected") { - clearTimeout(timeout); - if (!this.shouldOutputJson(flags)) { - this.log( - formatSuccess( - `Connected to space: ${formatResource(spaceName)}.`, - ), - ); - } - - resolve(); - } else if ( - this.realtimeClient!.connection.state === "failed" || - this.realtimeClient!.connection.state === "closed" || - this.realtimeClient!.connection.state === "suspended" - ) { - clearTimeout(timeout); - reject( - new Error( - `Space connection failed with connection state: ${this.realtimeClient!.connection.state}`, - ), - ); - } else { - setTimeout(checkSpaceStatus, 100); - } - } catch (error) { - clearTimeout(timeout); - reject(error); - } - }; - - checkSpaceStatus(); - }); - if (!this.shouldOutputJson(flags)) { this.log( formatProgress( @@ -137,11 +87,10 @@ export default class SpacesLocationsGetAll extends SpacesBaseCommand { for (let i = 0; i < entries.length; i++) { const entry = entries[i]; + this.log(`${formatIndex(i + 1)}`); + this.log(` ${formatLabel("Connection ID")} ${entry.connectionId}`); this.log( - `${formatIndex(i + 1)} ${formatLabel("Connection ID")} ${entry.connectionId}`, - ); - this.log( - ` ${formatLabel("Location")} ${JSON.stringify(entry.location)}`, + ` ${formatLabel("Location")} ${JSON.stringify(entry.location)}`, ); this.log(""); } diff --git a/src/commands/spaces/locations/set.ts b/src/commands/spaces/locations/set.ts index 1b4e201a..680fd341 100644 --- a/src/commands/spaces/locations/set.ts +++ b/src/commands/spaces/locations/set.ts @@ -1,20 +1,8 @@ -import type { LocationsEvents } from "@ably/spaces"; import { Args, Flags } from "@oclif/core"; -import { productApiFlags, clientIdFlag, durationFlag } from "../../../flags.js"; +import { productApiFlags, clientIdFlag } from "../../../flags.js"; import { SpacesBaseCommand } from "../../../spaces-base-command.js"; -import { - formatSuccess, - formatListening, - formatResource, - formatTimestamp, -} from "../../../utils/output.js"; -import { formatLocationUpdateBlock } from "../../../utils/spaces-output.js"; - -// Define the type for location subscription -interface LocationSubscription { - unsubscribe: () => void; -} +import { formatSuccess, formatResource } from "../../../utils/output.js"; export default class SpacesLocationsSet extends SpacesBaseCommand { static override args = { @@ -39,24 +27,11 @@ export default class SpacesLocationsSet extends SpacesBaseCommand { description: "Location data to set (JSON format)", required: true, }), - ...durationFlag, }; - private subscription: LocationSubscription | null = null; - private locationHandler: - | ((locationUpdate: LocationsEvents.UpdateEvent) => void) - | null = null; - private isE2EMode = false; // Track if we're in E2E mode to skip cleanup - - // Override finally to ensure resources are cleaned up async finally(err: Error | undefined): Promise { - // For E2E tests with duration=0, skip all cleanup to avoid hanging - if (this.isE2EMode) { - return; - } - // Clear location before leaving space - if (this.space) { + if (this.space && this.hasEnteredSpace) { try { await Promise.race([ this.space.locations.set(null), @@ -84,74 +59,6 @@ export default class SpacesLocationsSet extends SpacesBaseCommand { { location }, ); - // Check if we should exit immediately (optimized path for E2E tests) - const shouldExitImmediately = - typeof flags.duration === "number" && flags.duration === 0; - - if (shouldExitImmediately) { - // Set E2E mode flag to skip cleanup in finally block - this.isE2EMode = true; - - // For E2E mode, suppress unhandled promise rejections from Ably SDK cleanup - const originalHandler = process.listeners("unhandledRejection"); - process.removeAllListeners("unhandledRejection"); - process.on("unhandledRejection", (reason, promise) => { - // Ignore connection-related errors during E2E test cleanup - const reasonStr = String(reason); - if ( - reasonStr.includes("Connection closed") || - reasonStr.includes("80017") - ) { - // Silently ignore these errors in E2E mode - return; - } - // Re-emit other errors to original handlers - originalHandler.forEach((handler) => { - if (typeof handler === "function") { - handler(reason, promise); - } - }); - }); - - // Optimized path for E2E tests - minimal setup and cleanup - try { - const setupResult = await this.setupSpacesClient(flags, spaceName); - this.realtimeClient = setupResult.realtimeClient; - this.space = setupResult.space; - - // Enter the space and set location - await this.space.enter(); - this.logCliEvent(flags, "spaces", "entered", "Entered space", { - clientId: this.realtimeClient.auth.clientId, - }); - - await this.space.locations.set(location); - this.logCliEvent(flags, "location", "setSuccess", "Set location", { - location, - }); - - if (this.shouldOutputJson(flags)) { - this.logJsonResult({ location }, flags); - } else { - this.log( - formatSuccess( - `Location set in space: ${formatResource(spaceName)}.`, - ), - ); - } - } catch { - // If an error occurs in E2E mode, just exit cleanly after showing what we can - if (this.shouldOutputJson(flags)) { - this.logJsonResult({ location }, flags); - } - // Don't call this.error() in E2E mode as it sets exit code to 1 - } - - // For E2E tests, force immediate exit regardless of any errors - this.exit(0); - } - - // Original path for interactive use try { await this.initializeSpace(flags, spaceName, { enterSpace: true }); @@ -163,102 +70,16 @@ export default class SpacesLocationsSet extends SpacesBaseCommand { this.logCliEvent(flags, "location", "setSuccess", "Set location", { location, }); - if (!this.shouldOutputJson(flags)) { - this.log( - formatSuccess(`Location set in space: ${formatResource(spaceName)}.`), - ); - } - // Subscribe to location updates from other users - this.logCliEvent( - flags, - "location", - "subscribing", - "Watching for other location changes...", - ); - if (!this.shouldOutputJson(flags)) { + if (this.shouldOutputJson(flags)) { + this.logJsonResult({ location }, flags); + } else { this.log( - `\n${formatListening("Watching for other location changes.")}\n`, + formatSuccess(`Location set in space: ${formatResource(spaceName)}.`), ); } - - // Store subscription handlers - this.locationHandler = (locationUpdate: LocationsEvents.UpdateEvent) => { - const timestamp = new Date().toISOString(); - const { member } = locationUpdate; - const { connectionId } = member; - - // Skip self events - check connection ID - const selfConnectionId = this.realtimeClient!.connection.id; - if (connectionId === selfConnectionId) { - return; - } - - this.logCliEvent( - flags, - "location", - "updateReceived", - "Location update received", - { - clientId: member.clientId, - connectionId: member.connectionId, - timestamp, - }, - ); - - if (this.shouldOutputJson(flags)) { - this.logJsonEvent( - { - location: { - member: { - clientId: member.clientId, - connectionId: member.connectionId, - }, - currentLocation: locationUpdate.currentLocation, - previousLocation: locationUpdate.previousLocation, - timestamp, - }, - }, - flags, - ); - } else { - this.log(formatTimestamp(timestamp)); - this.log(formatLocationUpdateBlock(locationUpdate)); - this.log(""); - } - }; - - // Subscribe to updates - this.space!.locations.subscribe("update", this.locationHandler); - this.subscription = { - unsubscribe: () => { - if (this.locationHandler && this.space) { - this.space.locations.unsubscribe("update", this.locationHandler); - this.locationHandler = null; - } - }, - }; - - this.logCliEvent( - flags, - "location", - "subscribed", - "Subscribed to location updates", - ); - - this.logCliEvent( - flags, - "location", - "listening", - "Listening for location updates...", - ); - - // Wait until the user interrupts or the optional duration elapses - await this.waitAndTrackCleanup(flags, "location", flags.duration); } catch (error) { this.fail(error, flags, "locationSet"); - } finally { - // Cleanup is now handled by base class finally() method } } } diff --git a/src/commands/spaces/locations/subscribe.ts b/src/commands/spaces/locations/subscribe.ts index fc1abe18..5dd7f874 100644 --- a/src/commands/spaces/locations/subscribe.ts +++ b/src/commands/spaces/locations/subscribe.ts @@ -5,7 +5,7 @@ import { productApiFlags, clientIdFlag, durationFlag } from "../../../flags.js"; import { SpacesBaseCommand } from "../../../spaces-base-command.js"; import { formatListening, - formatSuccess, + formatProgress, formatTimestamp, } from "../../../utils/output.js"; import { formatLocationUpdateBlock } from "../../../utils/spaces-output.js"; @@ -37,26 +37,17 @@ export default class SpacesLocationsSubscribe extends SpacesBaseCommand { async run(): Promise { const { args, flags } = await this.parse(SpacesLocationsSubscribe); const { space: spaceName } = args; - this.logCliEvent( - flags, - "subscribe.run", - "start", - `Starting spaces locations subscribe for space: ${spaceName}`, - ); try { - // Always show the readiness signal first, before attempting auth if (!this.shouldOutputJson(flags)) { - this.log("Subscribing to location updates"); + this.log(formatProgress("Subscribing to location updates")); } - this.logCliEvent( - flags, - "subscribe.run", - "initialSignalLogged", - "Initial readiness signal logged.", - ); - await this.initializeSpace(flags, spaceName, { enterSpace: true }); + await this.initializeSpace(flags, spaceName, { enterSpace: false }); + + if (!this.shouldOutputJson(flags)) { + this.log(`\n${formatListening("Listening for location updates.")}\n`); + } this.logCliEvent( flags, @@ -64,18 +55,8 @@ export default class SpacesLocationsSubscribe extends SpacesBaseCommand { "subscribing", "Subscribing to location updates", ); - if (!this.shouldOutputJson(flags)) { - this.log(formatListening("Subscribing to location updates.")); - } - this.logCliEvent( - flags, - "location.subscribe", - "readySignalLogged", - "Final readiness signal 'Subscribing to location updates' logged.", - ); try { - // Define the location update handler const locationHandler = (update: LocationsEvents.UpdateEvent) => { try { const timestamp = new Date().toISOString(); @@ -118,7 +99,6 @@ export default class SpacesLocationsSubscribe extends SpacesBaseCommand { } }; - // Subscribe to location updates this.space!.locations.subscribe("update", locationHandler); this.logCliEvent( @@ -133,28 +113,9 @@ export default class SpacesLocationsSubscribe extends SpacesBaseCommand { }); } - this.logCliEvent( - flags, - "location", - "listening", - "Listening for location updates...", - ); - - // Wait until the user interrupts or the optional duration elapses await this.waitAndTrackCleanup(flags, "location", flags.duration); } catch (error) { this.fail(error, flags, "locationSubscribe", { spaceName }); - } finally { - // Wrap all cleanup in a timeout to prevent hanging - if (!this.shouldOutputJson(flags)) { - if (this.cleanupInProgress) { - this.log(formatSuccess("Graceful shutdown complete.")); - } else { - this.log( - formatSuccess("Duration elapsed, command finished cleanly."), - ); - } - } } } } diff --git a/src/commands/spaces/locks/acquire.ts b/src/commands/spaces/locks/acquire.ts index f75354b3..f03040d4 100644 --- a/src/commands/spaces/locks/acquire.ts +++ b/src/commands/spaces/locks/acquire.ts @@ -95,6 +95,7 @@ export default class SpacesLocksAcquire extends SpacesBaseCommand { // Enter the space first this.logCliEvent(flags, "spaces", "entering", "Entering space..."); await this.space!.enter(); + this.markAsEntered(); this.logCliEvent(flags, "spaces", "entered", "Entered space", { clientId: this.realtimeClient!.auth.clientId, }); diff --git a/src/commands/spaces/locks/get-all.ts b/src/commands/spaces/locks/get-all.ts index 9465728f..c7e3e87d 100644 --- a/src/commands/spaces/locks/get-all.ts +++ b/src/commands/spaces/locks/get-all.ts @@ -1,6 +1,5 @@ import type { Lock } from "@ably/spaces"; import { Args } from "@oclif/core"; -import chalk from "chalk"; import { productApiFlags, clientIdFlag } from "../../../flags.js"; import { SpacesBaseCommand } from "../../../spaces-base-command.js"; @@ -10,7 +9,7 @@ import { formatIndex, formatProgress, formatResource, - formatSuccess, + formatWarning, } from "../../../utils/output.js"; import { formatLockBlock, @@ -48,58 +47,6 @@ export default class SpacesLocksGetAll extends SpacesBaseCommand { setupConnectionLogging: false, }); - // Get the space - if (!this.shouldOutputJson(flags)) { - this.log( - formatProgress(`Connecting to space: ${formatResource(spaceName)}`), - ); - } - - await this.space!.enter(); - - // Wait for space to be properly entered before fetching locks - await new Promise((resolve, reject) => { - const timeout = setTimeout(() => { - reject(new Error("Timed out waiting for space connection")); - }, 5000); - - const checkSpaceStatus = () => { - try { - if (this.realtimeClient!.connection.state === "connected") { - clearTimeout(timeout); - if (!this.shouldOutputJson(flags)) { - this.log( - formatSuccess( - `Connected to space: ${formatResource(spaceName)}.`, - ), - ); - } - - resolve(); - } else if ( - this.realtimeClient!.connection.state === "failed" || - this.realtimeClient!.connection.state === "closed" || - this.realtimeClient!.connection.state === "suspended" - ) { - clearTimeout(timeout); - reject( - new Error( - `Space connection failed with connection state: ${this.realtimeClient!.connection.state}`, - ), - ); - } else { - setTimeout(checkSpaceStatus, 100); - } - } catch (error) { - clearTimeout(timeout); - reject(error); - } - }; - - checkSpaceStatus(); - }); - - // Get all locks if (!this.shouldOutputJson(flags)) { this.log( formatProgress( @@ -118,14 +65,15 @@ export default class SpacesLocksGetAll extends SpacesBaseCommand { flags, ); } else if (locks.length === 0) { - this.log(chalk.yellow("No locks are currently active in this space.")); + this.log(formatWarning("No locks are currently active in this space.")); } else { this.log( `\n${formatHeading("Current locks")} (${formatCountLabel(locks.length, "lock")}):\n`, ); for (let i = 0; i < locks.length; i++) { - this.log(`${formatIndex(i + 1)} ${formatLockBlock(locks[i])}`); + this.log(`${formatIndex(i + 1)}`); + this.log(formatLockBlock(locks[i])); this.log(""); } } diff --git a/src/commands/spaces/locks/get.ts b/src/commands/spaces/locks/get.ts index d2d0cd64..3c6bd9ed 100644 --- a/src/commands/spaces/locks/get.ts +++ b/src/commands/spaces/locks/get.ts @@ -1,9 +1,8 @@ import { Args } from "@oclif/core"; -import chalk from "chalk"; import { productApiFlags, clientIdFlag } from "../../../flags.js"; import { SpacesBaseCommand } from "../../../spaces-base-command.js"; -import { formatResource, formatSuccess } from "../../../utils/output.js"; +import { formatResource, formatWarning } from "../../../utils/output.js"; import { formatLockBlock, formatLockOutput, @@ -45,11 +44,6 @@ export default class SpacesLocksGet extends SpacesBaseCommand { setupConnectionLogging: false, }); - await this.space!.enter(); - if (!this.shouldOutputJson(flags)) { - this.log(formatSuccess(`Entered space: ${formatResource(spaceName)}.`)); - } - try { const lock = await this.space!.locks.get(lockId); @@ -58,8 +52,8 @@ export default class SpacesLocksGet extends SpacesBaseCommand { this.logJsonResult({ locks: [] }, flags); } else { this.log( - chalk.yellow( - `Lock ${formatResource(lockId)} not found in space ${formatResource(spaceName)}`, + formatWarning( + `Lock ${formatResource(lockId)} not found in space ${formatResource(spaceName)}.`, ), ); } diff --git a/src/commands/spaces/locks/subscribe.ts b/src/commands/spaces/locks/subscribe.ts index ee154f7b..fbcbbb4c 100644 --- a/src/commands/spaces/locks/subscribe.ts +++ b/src/commands/spaces/locks/subscribe.ts @@ -3,7 +3,11 @@ import { Args } from "@oclif/core"; import { productApiFlags, clientIdFlag, durationFlag } from "../../../flags.js"; import { SpacesBaseCommand } from "../../../spaces-base-command.js"; -import { formatListening, formatTimestamp } from "../../../utils/output.js"; +import { + formatListening, + formatProgress, + formatTimestamp, +} from "../../../utils/output.js"; import { formatLockBlock, formatLockOutput, @@ -37,45 +41,21 @@ export default class SpacesLocksSubscribe extends SpacesBaseCommand { async run(): Promise { const { args, flags } = await this.parse(SpacesLocksSubscribe); const { space: spaceName } = args; - this.logCliEvent( - flags, - "subscribe.run", - "start", - `Starting spaces locks subscribe for space: ${spaceName}`, - ); try { - // Always show the readiness signal first, before attempting auth if (!this.shouldOutputJson(flags)) { - this.log("Subscribing to lock events"); + this.log(formatProgress("Subscribing to lock events")); } - this.logCliEvent( - flags, - "subscribe.run", - "initialSignalLogged", - "Initial readiness signal logged.", - ); - await this.initializeSpace(flags, spaceName, { enterSpace: true }); + await this.initializeSpace(flags, spaceName, { enterSpace: false }); - // Subscribe to lock events this.logCliEvent( flags, "lock", "subscribing", "Subscribing to lock events", ); - if (!this.shouldOutputJson(flags)) { - this.log(formatListening("Subscribing to lock events.")); - } - this.logCliEvent( - flags, - "lock.subscribe", - "readySignalLogged", - "Final readiness signal 'Subscribing to lock events' logged.", - ); - // Define the listener function this.listener = (lock: Lock) => { const timestamp = new Date().toISOString(); @@ -93,7 +73,6 @@ export default class SpacesLocksSubscribe extends SpacesBaseCommand { } }; - // Subscribe using the stored listener await this.space!.locks.subscribe(this.listener); this.logCliEvent( @@ -103,19 +82,13 @@ export default class SpacesLocksSubscribe extends SpacesBaseCommand { "Successfully subscribed to lock events", ); - this.logCliEvent( - flags, - "lock", - "listening", - "Listening for lock events...", - ); + if (!this.shouldOutputJson(flags)) { + this.log(`\n${formatListening("Listening for lock events.")}\n`); + } - // Wait until the user interrupts or the optional duration elapses await this.waitAndTrackCleanup(flags, "lock", flags.duration); } catch (error) { this.fail(error, flags, "lockSubscribe"); - } finally { - // Cleanup is now handled by base class finally() method } } } diff --git a/src/commands/spaces/members/enter.ts b/src/commands/spaces/members/enter.ts index e3d25e6f..a80d71b9 100644 --- a/src/commands/spaces/members/enter.ts +++ b/src/commands/spaces/members/enter.ts @@ -1,4 +1,4 @@ -import type { ProfileData, SpaceMember } from "@ably/spaces"; +import type { ProfileData } from "@ably/spaces"; import { Args, Flags } from "@oclif/core"; import { productApiFlags, clientIdFlag, durationFlag } from "../../../flags.js"; @@ -6,14 +6,12 @@ import { SpacesBaseCommand } from "../../../spaces-base-command.js"; import { formatSuccess, formatListening, + formatProgress, formatResource, - formatTimestamp, formatLabel, + formatClientId, } from "../../../utils/output.js"; -import { - formatMemberEventBlock, - formatMemberOutput, -} from "../../../utils/spaces-output.js"; +import { formatMemberOutput } from "../../../utils/spaces-output.js"; export default class SpacesMembersEnter extends SpacesBaseCommand { static override args = { @@ -48,16 +46,9 @@ export default class SpacesMembersEnter extends SpacesBaseCommand { const { args, flags } = await this.parse(SpacesMembersEnter); const { space: spaceName } = args; - // Keep track of the last event we've seen for each client to avoid duplicates - const lastSeenEvents = new Map< - string, - { action: string; timestamp: number } - >(); - try { - // Always show the readiness signal first, before attempting auth if (!this.shouldOutputJson(flags)) { - this.log(formatListening("Entering space.")); + this.log(formatProgress("Entering space")); } await this.initializeSpace(flags, spaceName, { enterSpace: false }); @@ -85,6 +76,7 @@ export default class SpacesMembersEnter extends SpacesBaseCommand { { profileData }, ); await this.space!.enter(profileData); + this.markAsEntered(); this.logCliEvent(flags, "member", "enteredSpace", "Entered space", { connectionId: this.realtimeClient!.connection.id, profileData, @@ -95,128 +87,25 @@ export default class SpacesMembersEnter extends SpacesBaseCommand { this.logJsonResult({ members: [formatMemberOutput(self!)] }, flags); } else { this.log(formatSuccess(`Entered space: ${formatResource(spaceName)}.`)); + this.log( + `${formatLabel("Client ID")} ${formatClientId(this.realtimeClient!.auth.clientId)}`, + ); + this.log( + `${formatLabel("Connection ID")} ${this.realtimeClient!.connection.id}`, + ); if (profileData) { - this.log( - `${formatLabel("Profile")} ${JSON.stringify(profileData, null, 2)}`, - ); - } else { - // No profile data provided - this.logCliEvent( - flags, - "member", - "noProfileData", - "No profile data provided", - ); + this.log(`${formatLabel("Profile")} ${JSON.stringify(profileData)}`); } } - // Subscribe to member presence events to show other members' activities - this.logCliEvent( - flags, - "member", - "subscribing", - "Subscribing to member updates", - ); if (!this.shouldOutputJson(flags)) { - this.log(`\n${formatListening("Watching for other members.")}\n`); + this.log(`\n${formatListening("Holding presence.")}\n`); } - // Define the listener function - const listener = (member: SpaceMember) => { - const timestamp = new Date().toISOString(); - const now = Date.now(); - - // Determine the action from the member's lastEvent - const action = member.lastEvent?.name || "unknown"; - const clientId = member.clientId || "Unknown"; - const connectionId = member.connectionId || "Unknown"; - - // Skip self events - check connection ID - const selfConnectionId = this.realtimeClient!.connection.id; - if (member.connectionId === selfConnectionId) { - return; - } - - // Create a unique key for this client+connection combination - const clientKey = `${clientId}:${connectionId}`; - - // Check if we've seen this exact event recently (within 500ms) - // This helps avoid duplicate enter/leave events that might come through - const lastEvent = lastSeenEvents.get(clientKey); - - if ( - lastEvent && - lastEvent.action === action && - now - lastEvent.timestamp < 500 - ) { - this.logCliEvent( - flags, - "member", - "duplicateEventSkipped", - `Skipping duplicate event '${action}' for ${clientId}`, - { action, clientId }, - ); - return; // Skip duplicate events within 500ms window - } - - // Update the last seen event for this client+connection - lastSeenEvents.set(clientKey, { - action, - timestamp: now, - }); - - this.logCliEvent( - flags, - "member", - `update-${action}`, - `Member event '${action}' received`, - { action, clientId, connectionId }, - ); - - if (this.shouldOutputJson(flags)) { - this.logJsonEvent({ member: formatMemberOutput(member) }, flags); - } else { - this.log(formatTimestamp(timestamp)); - this.log(formatMemberEventBlock(member, action)); - this.log(""); - } - }; - - // Subscribe using the stored listener - await this.space!.members.subscribe("update", listener); - - this.logCliEvent( - flags, - "member", - "subscribed", - "Subscribed to member updates", - ); - - this.logCliEvent( - flags, - "member", - "listening", - "Listening for member updates...", - ); - // Wait until the user interrupts or the optional duration elapses await this.waitAndTrackCleanup(flags, "member", flags.duration); } catch (error) { this.fail(error, flags, "memberEnter"); - } finally { - if (!this.shouldOutputJson(flags || {})) { - if (this.cleanupInProgress) { - this.log(formatSuccess("Graceful shutdown complete.")); - } else { - // Normal completion without user interrupt - this.logCliEvent( - flags || {}, - "member", - "completedNormally", - "Command completed normally", - ); - } - } } } } diff --git a/src/commands/spaces/members/subscribe.ts b/src/commands/spaces/members/subscribe.ts index 290dcbb1..16017bd9 100644 --- a/src/commands/spaces/members/subscribe.ts +++ b/src/commands/spaces/members/subscribe.ts @@ -55,7 +55,7 @@ export default class SpacesMembersSubscribe extends SpacesBaseCommand { this.log(formatProgress("Subscribing to member updates")); } - await this.initializeSpace(flags, spaceName, { enterSpace: true }); + await this.initializeSpace(flags, spaceName, { enterSpace: false }); if (!this.shouldOutputJson(flags)) { this.log(`\n${formatListening("Listening for member events.")}\n`); diff --git a/src/spaces-base-command.ts b/src/spaces-base-command.ts index dbaf0689..5a264420 100644 --- a/src/spaces-base-command.ts +++ b/src/spaces-base-command.ts @@ -31,66 +31,67 @@ export abstract class SpacesBaseCommand extends AblyBaseCommand { protected spaces: Spaces | null = null; protected realtimeClient: Ably.Realtime | null = null; protected parsedFlags: BaseFlags = {}; + protected hasEnteredSpace = false; + + protected markAsEntered(): void { + this.hasEnteredSpace = true; + } async finally(error: Error | undefined): Promise { - // Always clean up connections try { - // Unsubscribe from all namespace listeners if (this.space !== null) { + // Unsubscribe from all namespace listeners try { await this.space.members.unsubscribe(); await this.space.locks.unsubscribe(); this.space.locations.unsubscribe(); this.space.cursors.unsubscribe(); } catch (error) { - // Log but don't throw unsubscribe errors - if (!this.shouldOutputJson(this.parsedFlags)) { - this.debug(`Namespace unsubscribe error: ${error}`); - } + this.debug(`Namespace unsubscribe error: ${error}`); } - await this.space!.leave(); - // Wait a bit after leaving space - await new Promise((resolve) => setTimeout(resolve, 200)); - - // Spaces maintains an internal map of members which have timeouts. This keeps node alive. - // This is a workaround to hold off until those timeouts are cleared by the client, as otherwise - // we'll get unhandled presence rejections as the connection closes. - await new Promise((resolve) => { - let intervalId: ReturnType; - const maxWaitMs = 10000; // 10 second timeout - const startTime = Date.now(); - const getAll = async () => { - // Avoid waiting forever - if (Date.now() - startTime > maxWaitMs) { - clearInterval(intervalId); - this.debug("Timed out waiting for space members to clear"); - resolve(); - return; - } - - const members = await this.space!.members.getAll(); - if (members.filter((member) => !member.isConnected).length === 0) { - clearInterval(intervalId); - this.debug("space members cleared"); - resolve(); - } else { - this.debug( - `waiting for spaces members to clear, ${members.length} remaining`, - ); - } - }; - - intervalId = setInterval(() => { - getAll(); - }, 1000); - }); + // Only leave and wait for member cleanup if we actually entered the space + if (this.hasEnteredSpace) { + await this.space!.leave(); + await new Promise((resolve) => setTimeout(resolve, 200)); + + // Spaces maintains an internal map of members which have timeouts. This keeps node alive. + // This is a workaround to hold off until those timeouts are cleared by the client, as otherwise + // we'll get unhandled presence rejections as the connection closes. + await new Promise((resolve) => { + let intervalId: ReturnType; + const maxWaitMs = 10000; + const startTime = Date.now(); + const getAll = async () => { + if (Date.now() - startTime > maxWaitMs) { + clearInterval(intervalId); + this.debug("Timed out waiting for space members to clear"); + resolve(); + return; + } + + const members = await this.space!.members.getAll(); + if ( + members.filter((member) => !member.isConnected).length === 0 + ) { + clearInterval(intervalId); + this.debug("space members cleared"); + resolve(); + } else { + this.debug( + `waiting for spaces members to clear, ${members.length} remaining`, + ); + } + }; + + intervalId = setInterval(() => { + getAll(); + }, 1000); + }); + } } } catch (error) { - // Log but don't throw cleanup errors - if (!this.shouldOutputJson(this.parsedFlags)) { - this.debug(`Space leave error: ${error}`); - } + this.debug(`Space cleanup error: ${error}`); } await super.finally(error); @@ -247,6 +248,7 @@ export abstract class SpacesBaseCommand extends AblyBaseCommand { if (enterSpace) { this.logCliEvent(flags, "spaces", "entering", "Entering space..."); await this.space!.enter(); + this.markAsEntered(); this.logCliEvent(flags, "spaces", "entered", "Entered space", { clientId: this.realtimeClient!.auth.clientId, }); diff --git a/src/utils/spaces-output.ts b/src/utils/spaces-output.ts index 80409a1d..c54a698d 100644 --- a/src/utils/spaces-output.ts +++ b/src/utils/spaces-output.ts @@ -157,7 +157,8 @@ export function formatCursorBlock( const lines: string[] = [ `${indent}${formatLabel("Client ID")} ${formatClientId(cursor.clientId)}`, `${indent}${formatLabel("Connection ID")} ${cursor.connectionId}`, - `${indent}${formatLabel("Position")} (${cursor.position.x}, ${cursor.position.y})`, + `${indent}${formatLabel("Position X")} ${cursor.position.x}`, + `${indent}${formatLabel("Position Y")} ${cursor.position.y}`, ]; if ( diff --git a/test/unit/commands/spaces/cursors/get-all.test.ts b/test/unit/commands/spaces/cursors/get-all.test.ts index 7c6362b7..919a6b88 100644 --- a/test/unit/commands/spaces/cursors/get-all.test.ts +++ b/test/unit/commands/spaces/cursors/get-all.test.ts @@ -46,11 +46,7 @@ describe("spaces:cursors:get-all command", () => { import.meta.url, ); - expect(space.enter).toHaveBeenCalled(); - expect(space.cursors.subscribe).toHaveBeenCalledWith( - "update", - expect.any(Function), - ); + expect(space.enter).not.toHaveBeenCalled(); expect(space.cursors.getAll).toHaveBeenCalled(); // The command outputs JSON with cursors array @@ -137,7 +133,6 @@ describe("spaces:cursors:get-all command", () => { ); // Verify cleanup was performed - expect(space.leave).toHaveBeenCalled(); expect(realtimeMock.close).toHaveBeenCalled(); }); }); diff --git a/test/unit/commands/spaces/cursors/set.test.ts b/test/unit/commands/spaces/cursors/set.test.ts index 13410be0..5076f7c7 100644 --- a/test/unit/commands/spaces/cursors/set.test.ts +++ b/test/unit/commands/spaces/cursors/set.test.ts @@ -91,8 +91,10 @@ describe("spaces:cursors:set command", () => { ); expect(stdout).toContain("Set cursor"); expect(stdout).toContain("test-space"); - expect(stdout).toContain("Position:"); - expect(stdout).toContain("(100, 200)"); + expect(stdout).toContain("Position X:"); + expect(stdout).toContain("100"); + expect(stdout).toContain("Position Y:"); + expect(stdout).toContain("200"); }); it("should set cursor from --data with position object", async () => { diff --git a/test/unit/commands/spaces/cursors/subscribe.test.ts b/test/unit/commands/spaces/cursors/subscribe.test.ts index 9be955bf..16a6a05a 100644 --- a/test/unit/commands/spaces/cursors/subscribe.test.ts +++ b/test/unit/commands/spaces/cursors/subscribe.test.ts @@ -33,7 +33,7 @@ describe("spaces:cursors:subscribe command", () => { import.meta.url, ); - expect(space.enter).toHaveBeenCalled(); + expect(space.enter).not.toHaveBeenCalled(); expect(space.cursors.subscribe).toHaveBeenCalledWith( "update", expect.any(Function), @@ -50,8 +50,8 @@ describe("spaces:cursors:subscribe command", () => { import.meta.url, ); - expect(stdout).toContain("Subscribing"); - expect(stdout).toContain("test-space"); + expect(stdout).toContain("Subscribing to cursor updates"); + expect(stdout).toContain("Listening for cursor movements"); }); }); @@ -144,10 +144,10 @@ describe("spaces:cursors:subscribe command", () => { }); describe("error handling", () => { - it("should handle space entry failure", async () => { + it("should handle subscribe failure", async () => { const spacesMock = getMockAblySpaces(); const space = spacesMock._getSpace("test-space"); - space.enter.mockRejectedValue(new Error("Connection failed")); + space.cursors.subscribe.mockRejectedValue(new Error("Connection failed")); const { error } = await runCommand( ["spaces:cursors:subscribe", "test-space"], diff --git a/test/unit/commands/spaces/locations/get-all.test.ts b/test/unit/commands/spaces/locations/get-all.test.ts index 16c1331e..5b4fd191 100644 --- a/test/unit/commands/spaces/locations/get-all.test.ts +++ b/test/unit/commands/spaces/locations/get-all.test.ts @@ -35,7 +35,7 @@ describe("spaces:locations:get-all command", () => { import.meta.url, ); - expect(space.enter).toHaveBeenCalled(); + expect(space.enter).not.toHaveBeenCalled(); expect(space.locations.getAll).toHaveBeenCalled(); expect(stdout).toContain("locations"); }); diff --git a/test/unit/commands/spaces/locations/set.test.ts b/test/unit/commands/spaces/locations/set.test.ts index 1215f476..6afd85a7 100644 --- a/test/unit/commands/spaces/locations/set.test.ts +++ b/test/unit/commands/spaces/locations/set.test.ts @@ -90,7 +90,7 @@ describe("spaces:locations:set command", () => { }); describe("JSON output", () => { - it("should output JSON on success with --duration 0", async () => { + it("should output JSON on success", async () => { const spacesMock = getMockAblySpaces(); spacesMock._getSpace("test-space"); @@ -103,8 +103,6 @@ describe("spaces:locations:set command", () => { "--location", JSON.stringify(location), "--json", - "--duration", - "0", ], import.meta.url, ); diff --git a/test/unit/commands/spaces/locations/subscribe.test.ts b/test/unit/commands/spaces/locations/subscribe.test.ts index 314a3b07..20f93199 100644 --- a/test/unit/commands/spaces/locations/subscribe.test.ts +++ b/test/unit/commands/spaces/locations/subscribe.test.ts @@ -32,7 +32,7 @@ describe("spaces:locations:subscribe command", () => { import.meta.url, ); - expect(space.enter).toHaveBeenCalled(); + expect(space.enter).not.toHaveBeenCalled(); expect(space.locations.subscribe).toHaveBeenCalledWith( "update", expect.any(Function), diff --git a/test/unit/commands/spaces/locks/get-all.test.ts b/test/unit/commands/spaces/locks/get-all.test.ts index b8c25e53..966cbe0c 100644 --- a/test/unit/commands/spaces/locks/get-all.test.ts +++ b/test/unit/commands/spaces/locks/get-all.test.ts @@ -49,7 +49,7 @@ describe("spaces:locks:get-all command", () => { import.meta.url, ); - expect(space.enter).toHaveBeenCalled(); + expect(space.enter).not.toHaveBeenCalled(); expect(space.locks.getAll).toHaveBeenCalled(); expect(stdout).toContain("locks"); }); diff --git a/test/unit/commands/spaces/locks/get.test.ts b/test/unit/commands/spaces/locks/get.test.ts index fdfdda84..ab059218 100644 --- a/test/unit/commands/spaces/locks/get.test.ts +++ b/test/unit/commands/spaces/locks/get.test.ts @@ -72,7 +72,7 @@ describe("spaces:locks:get command", () => { import.meta.url, ); - expect(space.enter).toHaveBeenCalled(); + expect(space.enter).not.toHaveBeenCalled(); expect(space.locks.get).toHaveBeenCalledWith("my-lock"); expect(stdout).toContain("my-lock"); }); diff --git a/test/unit/commands/spaces/locks/subscribe.test.ts b/test/unit/commands/spaces/locks/subscribe.test.ts index 7ccd3f7d..fcd22308 100644 --- a/test/unit/commands/spaces/locks/subscribe.test.ts +++ b/test/unit/commands/spaces/locks/subscribe.test.ts @@ -32,7 +32,7 @@ describe("spaces:locks:subscribe command", () => { import.meta.url, ); - expect(space.enter).toHaveBeenCalled(); + expect(space.enter).not.toHaveBeenCalled(); expect(space.locks.subscribe).toHaveBeenCalledWith(expect.any(Function)); }); diff --git a/test/unit/commands/spaces/members/enter.test.ts b/test/unit/commands/spaces/members/enter.test.ts index 702bcdee..c90d229f 100644 --- a/test/unit/commands/spaces/members/enter.test.ts +++ b/test/unit/commands/spaces/members/enter.test.ts @@ -62,20 +62,6 @@ describe("spaces:members:enter command", () => { }); }); - describe("member event handling", () => { - it("should subscribe to member update events", async () => { - const spacesMock = getMockAblySpaces(); - const space = spacesMock._getSpace("test-space"); - - await runCommand(["spaces:members:enter", "test-space"], import.meta.url); - - expect(space.members.subscribe).toHaveBeenCalledWith( - "update", - expect.any(Function), - ); - }); - }); - describe("JSON output", () => { it("should output JSON on success", async () => { const spacesMock = getMockAblySpaces(); diff --git a/test/unit/commands/spaces/members/subscribe.test.ts b/test/unit/commands/spaces/members/subscribe.test.ts index 30b08495..602ace38 100644 --- a/test/unit/commands/spaces/members/subscribe.test.ts +++ b/test/unit/commands/spaces/members/subscribe.test.ts @@ -68,7 +68,7 @@ describe("spaces:members:subscribe command", () => { import.meta.url, ); - expect(space.enter).toHaveBeenCalled(); + expect(space.enter).not.toHaveBeenCalled(); expect(space.members.subscribe).toHaveBeenCalledWith( "update", expect.any(Function), @@ -113,7 +113,7 @@ describe("spaces:members:subscribe command", () => { it("should handle errors gracefully", async () => { const spacesMock = getMockAblySpaces(); const space = spacesMock._getSpace("test-space"); - space.enter.mockRejectedValue(new Error("Connection failed")); + space.members.subscribe.mockRejectedValue(new Error("Connection failed")); const { error } = await runCommand( ["spaces:members:subscribe", "test-space"], From 563d18702a47f73a92816c593881a8e11660aee2 Mon Sep 17 00:00:00 2001 From: sacOO7 Date: Tue, 17 Mar 2026 14:11:32 +0530 Subject: [PATCH 4/5] Updated CLAUDE.md and claude skills with recommendations for formatting and command behaviour --- .claude/skills/ably-codebase-review/SKILL.md | 15 +- .claude/skills/ably-new-command/SKILL.md | 23 +- .../ably-new-command/references/patterns.md | 309 +++++++++++++++++- .claude/skills/ably-review/SKILL.md | 9 + AGENTS.md | 30 +- 5 files changed, 366 insertions(+), 20 deletions(-) diff --git a/.claude/skills/ably-codebase-review/SKILL.md b/.claude/skills/ably-codebase-review/SKILL.md index fc3603e8..35821348 100644 --- a/.claude/skills/ably-codebase-review/SKILL.md +++ b/.claude/skills/ably-codebase-review/SKILL.md @@ -103,13 +103,23 @@ Launch these agents **in parallel**. Each agent gets a focused mandate and uses 5. **Read** command files and look for unguarded `this.log()` calls (not inside `if (!this.shouldOutputJson(flags))`) 6. **Grep** for quoted resource names patterns like `"${` or `'${` near `channel`, `name`, `app` variables — should use `formatResource()` +**Method (grep — structured output format):** +7. **Grep** for box-drawing characters (`┌`, `┬`, `├`, `└`, `│`) in command files — non-JSON output must use multi-line labeled blocks, not ASCII tables or grids +8. **Grep** for subscribe commands that call `getAll()` or equivalent before subscribing — subscribe commands must NOT fetch initial state (they should only listen for new events) +9. For data-outputting commands, **read** both the JSON and non-JSON output paths and compare fields — non-JSON should expose the same fields as JSON mode (omitting only null/empty values) +10. **Grep** for local `interface` definitions in `src/commands/` that duplicate SDK types (e.g., `interface CursorPosition`, `interface CursorData`, `interface PresenceMessage`) — these should import from `ably`, `@ably/spaces`, or `@ably/chat` instead. Display/output interfaces in `src/utils/` are intentional and fine. + **Method (LSP — for completeness mapping):** -7. Use `LSP findReferences` on `shouldOutputJson` to get the definitive list of all commands that check for JSON mode — cross-reference against the full command list to find commands missing JSON guards +11. Use `LSP findReferences` on `shouldOutputJson` to get the definitive list of all commands that check for JSON mode — cross-reference against the full command list to find commands missing JSON guards **Reasoning guidance:** - List commands don't use `formatSuccess()` (no action to confirm) — this is correct, not a deviation - `chalk.red("✗")` / `chalk.green("✓")` as visual indicators in test/bench output is acceptable - `chalk.yellow("Warning: ...")` should use `formatWarning()` instead — `formatWarning` adds the `⚠` symbol automatically and "Warning:" prefix is unnecessary +- ASCII tables/grids in non-JSON output are deviations — use multi-line labeled blocks with `formatLabel()` instead +- Subscribe commands fetching initial state (via `getAll()`, `getSelf()`, etc.) before subscribing are deviations — subscribe should only listen for new events +- Non-JSON output that hides fields available in JSON mode is a deviation — both modes should expose the same data +- Local `interface` definitions in command files that duplicate SDK types are deviations — import from the SDK package instead. Display/output interfaces in `src/utils/` (e.g., `MemberOutput`, `MessageDisplayFields`) are intentional transformations, not duplicates. ### Agent 4: Flag Architecture Sweep @@ -143,11 +153,14 @@ Launch these agents **in parallel**. Each agent gets a focused mandate and uses 3. **Grep** for `shouldOutputJson` in command files to find all JSON-aware commands 4. Cross-reference: every leaf command should appear in both the `logJsonResult`/`logJsonEvent` list and the `shouldOutputJson` list 5. **Read** streaming commands to verify they use `logJsonEvent`, one-shot commands use `logJsonResult` +6. **Read** each `logJsonResult`/`logJsonEvent` call and verify data is nested under a domain key — singular for events/single items (e.g., `{message: ...}`, `{cursor: ...}`), plural for collections (e.g., `{cursors: [...]}`, `{rules: [...]}`). Top-level envelope fields are `type`, `command`, `success` only. Metadata like `total`, `timestamp`, `appId` may sit alongside the domain key. **Reasoning guidance:** - Commands that ONLY have human output (no JSON path) are deviations - Direct `formatJsonRecord` usage in command files should use `logJsonResult`/`logJsonEvent` instead - Topic index commands (showing help) don't need JSON output +- Data spread at the top level without a domain key is a deviation — nest under a singular or plural domain noun +- Metadata fields (`total`, `timestamp`, `hasMore`, `appId`) alongside the domain key are acceptable — they describe the result, not the domain objects ### Agent 6: Test Pattern Sweep diff --git a/.claude/skills/ably-new-command/SKILL.md b/.claude/skills/ably-new-command/SKILL.md index 5c40bcbe..88bcb094 100644 --- a/.claude/skills/ably-new-command/SKILL.md +++ b/.claude/skills/ably-new-command/SKILL.md @@ -176,14 +176,23 @@ if (!this.shouldOutputJson(flags)) { this.log(formatListening("Listening for messages.")); } -// JSON output — use logJsonResult for one-shot results: +// JSON output — nest data under a domain key, not spread at top level. +// Envelope provides top-level: type, command, success. +// One-shot single result — singular domain key: if (this.shouldOutputJson(flags)) { - this.logJsonResult({ channel: args.channel, message }, flags); + this.logJsonResult({ message: messageData }, flags); + // → {"type":"result","command":"...","success":true,"message":{...}} } -// Streaming events — use logJsonEvent: +// One-shot collection result — plural domain key + optional metadata: if (this.shouldOutputJson(flags)) { - this.logJsonEvent({ eventType: event.type, message, channel: channelName }, flags); + this.logJsonResult({ messages: items, total: items.length }, flags); +} + +// Streaming events — singular domain key: +if (this.shouldOutputJson(flags)) { + this.logJsonEvent({ message: messageData }, flags); + // → {"type":"event","command":"...","message":{...}} } ``` @@ -389,8 +398,14 @@ See the "Keeping Skills Up to Date" section in `CLAUDE.md` for the full list of - [ ] All human output wrapped in `if (!this.shouldOutputJson(flags))` - [ ] Output helpers used correctly (`formatProgress`, `formatSuccess`, `formatWarning`, `formatListening`, `formatResource`, `formatTimestamp`, `formatClientId`, `formatEventType`, `formatLabel`, `formatHeading`, `formatIndex`) - [ ] `formatSuccess()` messages end with `.` (period) +- [ ] Non-JSON data output uses multi-line labeled blocks (see `patterns.md` "Human-Readable Output Format"), not tables or custom grids +- [ ] Non-JSON output exposes all available SDK fields (same data as JSON mode, omitting only null/empty values) +- [ ] SDK types imported directly (`import type { CursorUpdate } from "@ably/spaces"`) — no local interface redefinitions of SDK types (display interfaces in `src/utils/` are fine) +- [ ] Field coverage checked against SDK type definitions (`node_modules/ably/ably.d.ts`, `node_modules/@ably/spaces/dist/mjs/types.d.ts`) +- [ ] Subscribe commands do NOT fetch initial state — they only listen for new events (use `get-all` for current state) - [ ] Resource names use `formatResource(name)`, never quoted - [ ] JSON output uses `logJsonResult()` (one-shot) or `logJsonEvent()` (streaming), not direct `formatJsonRecord()` +- [ ] JSON data nested under a domain key (singular for events/single items, plural for collections) — not spread at top level (see `patterns.md` "JSON Data Nesting Convention") - [ ] Subscribe/enter commands use `this.waitAndTrackCleanup(flags, component, flags.duration)` (not `waitUntilInterruptedOrTimeout`) - [ ] Error handling uses `this.fail()` exclusively, not `this.error()` or `this.log(chalk.red(...))` - [ ] Component strings are camelCase: single-word lowercase (`"room"`, `"auth"`), multi-word camelCase (`"channelPublish"`, `"roomPresenceSubscribe"`) diff --git a/.claude/skills/ably-new-command/references/patterns.md b/.claude/skills/ably-new-command/references/patterns.md index c7dd25dd..cb81af49 100644 --- a/.claude/skills/ably-new-command/references/patterns.md +++ b/.claude/skills/ably-new-command/references/patterns.md @@ -59,17 +59,32 @@ async run(): Promise { sequenceCounter++; // Format and output the message if (this.shouldOutputJson(flags)) { - // Use "event" type for streaming records. IMPORTANT: don't use "type" as a + // Nest data under a singular domain key. IMPORTANT: don't use "type" as a // data key — it's reserved by the envelope. Use "eventType" instead. this.logJsonEvent({ - eventType: "message", // not "type" — that's reserved by the envelope - channel: args.channel, - data: message.data, - name: message.name, - timestamp: message.timestamp, + message: { + id: message.id, + timestamp: message.timestamp, + channel: args.channel, + event: message.name, + clientId: message.clientId, + serial: message.serial, + data: message.data, + }, }, flags); } else { - // Human-readable output with formatTimestamp, formatResource, chalk colors + // Human-readable output — multi-line labeled block per event + // Use shared formatters where available (e.g., formatMessagesOutput for channels) + const timestamp = formatMessageTimestamp(message.timestamp); + this.log(formatTimestamp(timestamp)); + if (message.id) this.log(`${formatLabel("ID")} ${message.id}`); + this.log(`${formatLabel("Timestamp")} ${timestamp}`); + this.log(`${formatLabel("Channel")} ${formatResource(args.channel)}`); + this.log(`${formatLabel("Event")} ${formatEventType(message.name || "(none)")}`); + if (message.clientId) this.log(`${formatLabel("Client ID")} ${formatClientId(message.clientId)}`); + if (message.serial) this.log(`${formatLabel("Serial")} ${message.serial}`); + this.log(`${formatLabel("Data")} ${String(message.data)}`); + this.log(""); // blank line between events } }); @@ -112,9 +127,9 @@ async run(): Promise { await channel.publish(message as Ably.Message); if (this.shouldOutputJson(flags)) { - // Use "result" type for one-shot results. Don't use "success" as a data key + // Nest data under a domain key. Don't use "success" as a data key // for batch summaries — it overrides the envelope's success field. Use "allSucceeded". - this.logJsonResult({ channel: args.channel }, flags); + this.logJsonResult({ message: { channel: args.channel, name: message.name, data: message.data } }, flags); } else { this.log(formatSuccess("Message published to channel: " + formatResource(args.channel) + ".")); } @@ -158,10 +173,11 @@ async run(): Promise { const messages = history.items; if (this.shouldOutputJson(flags)) { - this.logJsonResult({ messages }, flags); + // Plural domain key for collections, optional metadata alongside + this.logJsonResult({ messages, total: messages.length }, flags); } else { this.log(formatSuccess(`Found ${messages.length} messages.`)); - // Display each message + // Display each message using multi-line labeled blocks } } catch (error) { this.fail(error, flags, "history", { channel: args.channel }); @@ -195,11 +211,15 @@ async run(): Promise { const data = result.items?.[0] || {}; if (this.shouldOutputJson(flags)) { - this.logJsonResult({ resource: args.id, ...data }, flags); + // Singular domain key for single-item results + this.logJsonResult({ resource: data }, flags); } else { + // Multi-line labeled block — one field per line, using formatLabel for all labels this.log(`Details for ${formatResource(args.id)}:\n`); this.log(`${formatLabel("Field")} ${data.field}`); this.log(`${formatLabel("Status")} ${data.status}`); + if (data.clientId) this.log(`${formatLabel("Client ID")} ${formatClientId(data.clientId)}`); + // Omit null/undefined fields, show everything else } } catch (error) { this.fail(error, flags, "resourceGet", { resource: args.id }); @@ -318,6 +338,7 @@ async run(): Promise { const limited = flags.limit ? items.slice(0, flags.limit) : items; if (this.shouldOutputJson(flags)) { + // Plural domain key for collections, metadata alongside this.logJsonResult({ items: limited, total: limited.length, appId }, flags); } else { this.log(`Found ${limited.length} item${limited.length !== 1 ? "s" : ""}:\n`); @@ -355,13 +376,275 @@ async run(): Promise { const result = await controlApi.someMethod(appId, data); if (this.shouldOutputJson(flags)) { + // Singular domain key for single-item results this.logJsonResult({ resource: result }, flags); } else { this.log(formatSuccess("Resource created: " + formatResource(result.id) + ".")); - // Display additional fields + // Display additional fields using formatLabel + this.log(`${formatLabel("Status")} ${result.status}`); + this.log(`${formatLabel("Created")} ${new Date(result.createdAt).toISOString()}`); } } catch (error) { this.fail(error, flags, "createResource"); } } ``` + +--- + +## Human-Readable Output Format + +All non-JSON output for data records uses **multi-line labeled blocks**. Never use ASCII tables, box-drawing characters (`┌─┬─┐`, `│`), or custom grid layouts. + +### Streaming events (subscribe commands) + +Each event is a multi-line block with a timestamp header, then labeled fields. Separate events with a blank line. + +``` +[2024-01-15T10:30:00.000Z] +ID: msg-123 +Timestamp: 2024-01-15T10:30:00.000Z +Channel: my-channel +Event: message.created +Client ID: user-123 +Serial: 01H... +Data: {"key": "value"} +``` + +Code pattern: +```typescript +// In the event handler callback +const timestamp = formatMessageTimestamp(message.timestamp); +this.log(formatTimestamp(timestamp)); // dim [timestamp] header +if (message.id) this.log(`${formatLabel("ID")} ${message.id}`); +this.log(`${formatLabel("Timestamp")} ${timestamp}`); +this.log(`${formatLabel("Channel")} ${formatResource(channelName)}`); +this.log(`${formatLabel("Event")} ${formatEventType(message.name)}`); +if (message.clientId) this.log(`${formatLabel("Client ID")} ${formatClientId(message.clientId)}`); +this.log(`${formatLabel("Data")} ${String(message.data)}`); +this.log(""); // blank line separator +``` + +For domain-specific events, create shared formatting functions in the appropriate utils file (e.g., `src/utils/spaces-output.ts` for spaces, `src/utils/output.ts` for channels). + +### One-shot results with multiple records (get-all) + +Use `formatIndex()` for numbering and `formatCountLabel()` for the heading. Index goes on its own line, fields are indented below it. + +``` +Current cursors (3 cursors): + +[1] + Client ID: user-123 + Connection ID: conn-abc + Position X: 150 + Position Y: 300 + Data: {"color": "red"} + +[2] + Client ID: user-456 + Connection ID: conn-def + Position X: 200 + Position Y: 400 +``` + +Code pattern: +```typescript +this.log(`${formatHeading("Current cursors")} (${formatCountLabel(items.length, "cursor")}):\n`); +for (let i = 0; i < items.length; i++) { + const item = items[i]; + this.log(`${formatIndex(i + 1)}`); + this.log(` ${formatLabel("Client ID")} ${formatClientId(item.clientId)}`); + this.log(` ${formatLabel("Connection ID")} ${item.connectionId}`); + this.log(` ${formatLabel("Position X")} ${item.position.x}`); + this.log(` ${formatLabel("Position Y")} ${item.position.y}`); + if (item.data) this.log(` ${formatLabel("Data")} ${JSON.stringify(item.data)}`); + this.log(""); // blank line between records +} +``` + +### History results (time-ordered records) + +History commands combine `formatIndex()` and `formatTimestamp()` on the same line as a heading, since records are time-ordered. This is distinct from get-all which uses index alone. + +``` +[1] [2024-01-15T10:30:00.000Z] + Event: message.created + Channel: my-channel + Data: {"key": "value"} + +[2] [2024-01-15T10:29:55.000Z] + Event: message.created + Channel: my-channel + Data: {"other": "data"} +``` + +Code pattern: +```typescript +for (let i = 0; i < messages.length; i++) { + const msg = messages[i]; + const ts = formatMessageTimestamp(msg.timestamp); + this.log(`${formatIndex(i + 1)} ${formatTimestamp(ts)}`); // index + timestamp on same line + this.log(` ${formatLabel("Event")} ${formatEventType(msg.name || "(none)")}`); + this.log(` ${formatLabel("Channel")} ${formatResource(channelName)}`); + if (msg.clientId) this.log(` ${formatLabel("Client ID")} ${formatClientId(msg.clientId)}`); + this.log(` ${formatLabel("Data")} ${String(msg.data)}`); + this.log(""); +} +``` + +### Single record results (get, acquire, set) + +Same labeled format, no index needed: + +``` +Lock ID: my-lock +Status: locked +Timestamp: 2024-01-15T10:30:00.000Z +Member: + Client ID: user-123 + Connection ID: conn-abc +Attributes: {"priority": "high"} +``` + +### Field display rules + +**Use SDK type definitions as the source of truth** for which fields exist. Before implementing output for a command, read the relevant SDK type definition to ensure all important fields are covered: + +| SDK | Type file | Key types | +|-----|-----------|-----------| +| `ably` | `node_modules/ably/ably.d.ts` | `Message`, `PresenceMessage`, `ChannelStateChange`, `ConnectionStateChange` | +| `@ably/spaces` | `node_modules/@ably/spaces/dist/mjs/types.d.ts` | `SpaceMember`, `CursorUpdate`, `CursorPosition`, `CursorData`, `Lock`, `ProfileData` | +| `@ably/chat` | `node_modules/@ably/chat/dist/chat/core/*.d.ts` | `Message` (chat), `PresenceMember`, `OccupancyEvent`, `Reaction` | + +**Use SDK source code as the source of truth** for method behavior — whether a method requires prior state (e.g., `space.enter()`), what side effects it has, what it actually returns. When in doubt, read the implementation: + +| SDK | Source directory | Key files | +|-----|-----------------|-----------| +| `ably` | `node_modules/ably/` | Realtime, REST, channels, presence | +| `@ably/spaces` | `node_modules/@ably/spaces/dist/mjs/` | `Space.js`, `Members.js`, `Locations.js`, `Cursors.js`, `Locks.js` | +| `@ably/chat` | `node_modules/@ably/chat/dist/chat/core/` | Rooms, messages, presence, reactions | + +**Import SDK types directly** — never redefine SDK interfaces locally. If `@ably/spaces` exports `CursorPosition`, import it: +```typescript +// WRONG — local redefinition duplicates SDK type +interface CursorPosition { x: number; y: number; } + +// CORRECT — import from SDK +import type { CursorPosition, CursorData, CursorUpdate } from "@ably/spaces"; +``` + +**Display interfaces are fine** — types like `MemberOutput`, `MessageDisplayFields`, `CursorOutput` in `src/utils/` that transform SDK types for output are intentional. They're the right place to decide field naming, null handling, and which fields to include. The SDK type defines what's *available*; the display interface defines what to *present*. + +**Field coverage:** +- **Show all available fields** — non-JSON output should expose the same data as JSON mode +- **Omit null/undefined/empty fields** — skip fields with no value (don't show "Profile: null") + +**Formatting:** +- **Use `formatLabel("Name")`** for all field labels — it appends `:` and applies dim styling +- **Use type-appropriate formatters**: `formatClientId()` for client IDs, `formatResource()` for resource names, `formatEventType()` for actions, `formatTimestamp()` for timestamp headers +- **Nested objects**: display as `JSON.stringify(data)` on the same line, or indent with `formatMessageData()` for large/multi-line JSON +- **Nested records** (e.g., member inside lock): use 2-space indent for the sub-fields + +### Command behavior semantics + +Commands must behave strictly according to their documented purpose — no unintended side effects. + +**Subscribe commands** — passive observers: +- **Only** listen for new events — must NOT fetch initial state (use `get-all` for that) +- **NOT enter presence/space** — use `enterSpace: false`. The Spaces SDK's `subscribe()` methods do NOT require `space.enter()` +- Use the message pattern: `formatProgress("Subscribing to X")` → `formatListening("Listening for X.")` + +**Get-all / get commands** — one-shot queries: +- **NOT enter presence/space** — `getAll()`, `get()` do NOT require `space.enter()` +- **NOT subscribe** to events or poll — fetch once, output, exit + +**Set commands** — one-shot mutations: +- Enter space (required by SDK), set value, output, **exit** +- **NOT subscribe** after setting — that is what subscribe commands are for + +**Enter / acquire commands** — hold state until Ctrl+C / `--duration`: +- Enter space, output confirmation with all relevant fields, then `waitAndTrackCleanup` +- **NOT subscribe** to other events + +**Side-effect rules:** +- `space.enter()` only when SDK requires it (set, enter, acquire) +- Call `this.markAsEntered()` after every `space.enter()` (enables cleanup) +- `initializeSpace(enterSpace: true)` calls `markAsEntered()` automatically + +```typescript +// WRONG — subscribe enters the space +await this.initializeSpace(flags, spaceName, { enterSpace: true }); +await this.space!.members.subscribe("update", handler); + +// CORRECT — subscribe is passive +await this.initializeSpace(flags, spaceName, { enterSpace: false }); +await this.space!.members.subscribe("update", handler); + +// WRONG — get-all enters the space +await this.space!.enter(); +const data = await this.space!.locations.getAll(); + +// CORRECT — get-all just fetches +const data = await this.space!.locations.getAll(); + +// WRONG — set command subscribes after setting +await this.space!.locations.set(location); +this.space!.locations.subscribe("update", handler); // NO +await this.waitAndTrackCleanup(flags, "location"); // NO + +// CORRECT — set command exits after setting +await this.space!.locations.set(location); +// run() completes, finally() handles cleanup +``` + +--- + +## JSON Data Nesting Convention + +The JSON envelope provides three top-level fields (`type`, `command`, `success`). Domain data must be nested under a **domain key**, not spread at the top level. + +### Streaming events (logJsonEvent) — singular domain key + +```typescript +// CORRECT — nest event payload under a singular domain key +this.logJsonEvent({ message: messageData }, flags); // → {"type":"event","command":"channels:subscribe","message":{...}} +this.logJsonEvent({ cursor: cursorData }, flags); // spaces cursors +this.logJsonEvent({ member: memberData }, flags); // spaces members +this.logJsonEvent({ lock: lockData }, flags); // spaces locks +this.logJsonEvent({ location: locationData }, flags); // spaces locations +this.logJsonEvent({ annotation: annotationData }, flags); // channels annotations +this.logJsonEvent({ reaction: reactionData }, flags); // rooms reactions + +// WRONG — spreading fields loses the domain boundary +this.logJsonEvent({ clientId, position, data }, flags); +``` + +### One-shot single results (logJsonResult) — singular domain key + +```typescript +this.logJsonResult({ lock: lockData }, flags); // → {"type":"result","command":"spaces:locks:get","success":true,"lock":{...}} +this.logJsonResult({ key: keyData }, flags); // auth keys create +this.logJsonResult({ app: appData }, flags); // apps create +this.logJsonResult({ rule: ruleData }, flags); // apps rules create +``` + +### One-shot collection results (logJsonResult) — plural domain key + metadata + +```typescript +this.logJsonResult({ cursors: items }, flags); // → {"type":"result","command":"spaces:cursors:get-all","success":true,"cursors":[...]} +this.logJsonResult({ rules: items, appId, total }, flags); // rules list — metadata alongside collection +this.logJsonResult({ channels: items, total, hasMore }, flags); // channels list +``` + +Metadata fields (`total`, `timestamp`, `hasMore`, `appId`) may sit alongside the collection key since they describe the result, not the domain objects. + +### Choosing the domain key name + +| Scenario | Key | Example | +|----------|-----|---------| +| Single event | Singular noun matching the SDK type | `message`, `cursor`, `member`, `lock` | +| Single result (create/get) | Singular noun | `key`, `app`, `rule`, `lock` | +| Collection result (list/get-all) | Plural noun | `keys`, `apps`, `rules`, `cursors` | + +The key name should match the SDK/domain terminology, not be generic. Use `message` not `data`, `cursor` not `item`. diff --git a/.claude/skills/ably-review/SKILL.md b/.claude/skills/ably-review/SKILL.md index 3826fb17..d7ba5f34 100644 --- a/.claude/skills/ably-review/SKILL.md +++ b/.claude/skills/ably-review/SKILL.md @@ -98,6 +98,14 @@ For each changed command file, run the relevant checks. Spawn agents for paralle 4. **Grep** for `formatSuccess(` and check lines end with `.` 5. **Read** the file and look for unguarded `this.log()` calls (not inside `if (!this.shouldOutputJson(flags))`) 6. Look for quoted resource names instead of `formatResource(name)` +7. **Grep** for box-drawing characters (`┌`, `┬`, `├`, `└`, `─.*─`, `│`) — non-JSON output must use multi-line labeled blocks, not ASCII tables or grids +8. **Read** the file and check that non-JSON data output uses `formatLabel()` for field labels in multi-line blocks, not inline or single-line formatting +9. **Check** subscribe commands do NOT fetch initial state (no `getAll()` or equivalent before subscribing) — subscribe should only listen for new events + +**Field completeness check (read — for data-outputting commands):** +1. **Read** the JSON output path and compare fields emitted vs the non-JSON output path — non-JSON should expose the same fields as JSON mode (omitting only null/empty values) +2. **Check** that available SDK fields (e.g., `connectionId`, `clientId`, `isConnected`, `profileData`, `lastEvent`) are shown in non-JSON output, not just in JSON mode +3. **Grep** for local `interface` definitions in command files that duplicate SDK types (e.g., `interface CursorPosition`, `interface CursorData`) — these should import from `ably`, `@ably/spaces`, or `@ably/chat` instead. Display/output interfaces in `src/utils/` are fine. **Flag architecture check (grep, with LSP for ambiguous cases):** 1. **Grep** for flag spreads (`productApiFlags`, `clientIdFlag`, `durationFlag`, `rewindFlag`, `timeRangeFlags`, `ControlBaseCommand.globalFlags`) @@ -110,6 +118,7 @@ For each changed command file, run the relevant checks. Spawn agents for paralle 2. **Grep** for `formatJsonRecord` — direct usage should be flagged as needing migration 3. **Grep** for `shouldOutputJson` — verify human output is guarded 4. **Read** the file to verify streaming commands use `logJsonEvent` and one-shot commands use `logJsonResult` +5. **Read** `logJsonResult`/`logJsonEvent` call sites and check data is nested under a domain key (singular for events/single items, plural for collections) — not spread at top level. Top-level envelope fields are `type`, `command`, `success` only. Metadata like `total`, `timestamp`, `appId` may sit alongside the domain key. **Control API helper check (grep — for Control API commands only):** 1. **Grep** for `resolveAppId` — should use `requireAppId` instead (encapsulates null check and `fail()`) diff --git a/AGENTS.md b/AGENTS.md index 7d3feedf..608f65fa 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -219,9 +219,34 @@ All output helpers use the `format` prefix and are exported from `src/utils/outp - **Count labels**: `formatCountLabel(n, "message")` — cyan count + pluralized label. - **Limit warnings**: `formatLimitWarning(count, limit, "items")` — yellow warning if results truncated. - **JSON guard**: All human-readable output (progress, success, listening messages) must be wrapped in `if (!this.shouldOutputJson(flags))` so it doesn't pollute `--json` output. Only JSON payloads should be emitted when `--json` is active. -- **JSON envelope**: Use `this.logJsonResult(data, flags)` for one-shot results and `this.logJsonEvent(data, flags)` for streaming events. These are shorthand for `this.log(this.formatJsonRecord("result"|"event", data, flags))`. The envelope wraps data in `{type, command, success?, ...data}`. Do NOT add ad-hoc `success: true/false` — the envelope handles it. `--json` produces compact single-line output (NDJSON for streaming). `--pretty-json` is unchanged. +- **JSON envelope**: Use `this.logJsonResult(data, flags)` for one-shot results and `this.logJsonEvent(data, flags)` for streaming events. The envelope adds three top-level fields (`type`, `command`, `success?`). Nest domain data under a **domain key** (see "JSON data nesting convention" below). Do NOT add ad-hoc `success: true/false` — the envelope handles it. `--json` produces compact single-line output (NDJSON for streaming). `--pretty-json` is unchanged. - **JSON errors**: Use `this.fail(error, flags, component, context?)` as the single error funnel in command `run()` methods. It logs the CLI event, preserves structured error data (Ably codes, HTTP status), emits JSON error envelope when `--json` is active, and calls `this.error()` for human-readable output. Returns `never` — no `return;` needed after calling it. Do NOT call `this.error()` directly — it is an internal implementation detail of `fail`. -- **History output**: Use `[index] timestamp` ordering: `` `${formatIndex(index + 1)} ${formatTimestamp(timestamp)}` ``. Consistent across all history commands (channels, logs, connection-lifecycle, push). +- **History output**: Use `[index] [timestamp]` on the same line as a heading: `` `${formatIndex(index + 1)} ${formatTimestamp(timestamp)}` ``, then fields indented below. This is distinct from **get-all output** which uses `[index]` alone on its own line. See `references/patterns.md` "History results" and "One-shot results" for both patterns. + +### Structured output format (non-JSON) + +All non-JSON output for data records must use **multi-line labeled blocks** — one block per record, separated by blank lines. Never use ASCII tables (`┌─┬─┐`, `│`, box-drawing characters) or custom grid layouts. Non-JSON output must expose the same fields as JSON output (omit only null/undefined/empty values). Use `formatLabel()` for field names, type-appropriate formatters for values (`formatClientId`, `formatResource`, `formatEventType`, `formatTimestamp`). Check SDK type definitions (see "Ably Knowledge" below) as the source of truth for available fields — import SDK types directly, never redefine them locally. See `references/patterns.md` "Human-Readable Output Format" in the `ably-new-command` skill for detailed examples. + +### JSON data nesting convention + +The envelope provides three top-level fields: `type`, `command`, and `success`. All domain data must be nested under a **domain key** — never spread raw data fields at the top level alongside envelope fields. + +- **Events and single results**: nest under a **singular** domain key (`message`, `cursor`, `lock`) +- **Collection results**: nest under a **plural** domain key (`cursors`, `rules`, `keys`) +- **Metadata** (`total`, `timestamp`, `hasMore`, `appId`) may sit alongside the domain key + +See `references/patterns.md` "JSON Data Nesting Convention" in the `ably-new-command` skill for detailed examples and domain key naming. + +### Command behavior semantics + +Each command type has strict rules about what side effects it may have. Remove unintended side effects (e.g., auto-entering presence) and support passive ("dumb") operations where applicable. Key principles: +- **Subscribe** = passive observer (no `space.enter()`, no fetching initial state) +- **Get-all / get** = one-shot query (no `space.enter()`, no subscribing) +- **Set** = one-shot mutation (enter, set, exit — no subscribing after) +- **Enter / acquire** = hold state until Ctrl+C / `--duration` +- Call `space.enter()` only when SDK requires it; always call `this.markAsEntered()` after + +See `references/patterns.md` "Command behavior semantics" in the `ably-new-command` skill for full rules, side-effect table, and code examples. ### Error handling architecture @@ -288,6 +313,7 @@ When adding COMMANDS sections in `src/help.ts`, use `chalk.bold()` for headers, - Platform: https://ably.com/docs/platform - The CLI uses Ably SDKs for all data plane commands. When an API exists in the data plane REST API but has no corresponding SDK method, use the Pub/Sub SDK's request method. - The Control API has no official SDK, so raw HTTP requests are used. +- **SDK packages (`node_modules/ably/`, `node_modules/@ably/spaces/`, `node_modules/@ably/chat/`) are the local source of truth** for types and method behavior. Type definitions (e.g., `ably.d.ts`, `types.d.ts`) tell you what fields exist; source code (e.g., `Space.js`, `Members.js`) tells you how methods behave (side effects, prerequisites like `space.enter()`). When in doubt, read the implementation — not just the types. See `references/patterns.md` "Field display rules" in the `ably-new-command` skill for the full path table and import conventions. ## Development Standards From 02b7c9c2d7a8fd452c8471d36ccc2d95431b145f Mon Sep 17 00:00:00 2001 From: sacOO7 Date: Tue, 17 Mar 2026 14:24:08 +0530 Subject: [PATCH 5/5] Fixed failing e2e tests --- test/e2e/spaces/spaces-e2e.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/e2e/spaces/spaces-e2e.test.ts b/test/e2e/spaces/spaces-e2e.test.ts index f30ca118..e1ae7015 100644 --- a/test/e2e/spaces/spaces-e2e.test.ts +++ b/test/e2e/spaces/spaces-e2e.test.ts @@ -212,7 +212,7 @@ describe("Spaces E2E Tests", () => { }; const setLocationResult = await runBackgroundProcessAndGetOutput( - `bin/run.js spaces locations set ${testSpaceId} --location '${JSON.stringify(locationData)}' --client-id ${client2Id} --duration 0`, + `bin/run.js spaces locations set ${testSpaceId} --location '${JSON.stringify(locationData)}' --client-id ${client2Id}`, process.env.CI ? 15000 : 15000, // Timeout for the command ); @@ -252,7 +252,7 @@ describe("Spaces E2E Tests", () => { }; const updateLocationResult = await runBackgroundProcessAndGetOutput( - `bin/run.js spaces locations set ${testSpaceId} --location '${JSON.stringify(newLocationData)}' --client-id ${client2Id} --duration 0`, + `bin/run.js spaces locations set ${testSpaceId} --location '${JSON.stringify(newLocationData)}' --client-id ${client2Id}`, process.env.CI ? 15000 : 15000, // Increased local timeout ); @@ -349,7 +349,7 @@ describe("Spaces E2E Tests", () => { `bin/run.js spaces cursors subscribe ${testSpaceId} --client-id ${client1Id} --duration 20`, outputPath, { - readySignal: "Subscribing to cursor movements", + readySignal: "Subscribing to cursor updates", timeoutMs: 60000, // Increased timeout significantly retryCount: 3, }, @@ -370,7 +370,7 @@ describe("Spaces E2E Tests", () => { let currentOutput = await readProcessOutput(outputPath); if ( !currentOutput.includes("Entered space:") && - !currentOutput.includes("Subscribing to cursor movements") + !currentOutput.includes("Subscribing to cursor updates") ) { // The cursor subscribe process might have failed, let's skip this test shouldSkipCursorTest = true;