From 95a65b9de90d7c864b70a61facf264e332913cca Mon Sep 17 00:00:00 2001 From: sacOO7 Date: Tue, 10 Mar 2026 18:56:01 +0530 Subject: [PATCH] Created implementation plan for ably spaces formtting fix --- ably_spaces_command_group_fix.md | 365 +++++++++++++++++++++++++++++++ 1 file changed, 365 insertions(+) create mode 100644 ably_spaces_command_group_fix.md diff --git a/ably_spaces_command_group_fix.md b/ably_spaces_command_group_fix.md new file mode 100644 index 00000000..58b8da42 --- /dev/null +++ b/ably_spaces_command_group_fix.md @@ -0,0 +1,365 @@ +# Fix: Spaces locations/cursors commands expect wrong output format + +## Problem Statement + +The `cursors get-all` and `locations get-all` commands (and the `locations subscribe` initial snapshot) process results from the Spaces SDK's `getAll()` methods with **incorrect data format assumptions**. This causes them to display degraded or empty output when reading location/cursor data set by the `set` commands. + +The unit test mocks also use wrong formats (arrays with event-style objects) instead of matching the actual SDK return types, so the bug was never caught. + +--- + +## Root Cause Analysis + +### SDK Method Signatures (from `node_modules/@ably/spaces`) + +| Method | Return Type | Key Structure | +|--------|------------|---------------| +| `cursors.getAll()` | `Promise>` | `{ [connectionId]: CursorUpdate \| null }` | +| `cursors.getOthers()` | `Promise>` | Same as above (excludes self) | +| `locations.getAll()` | `Promise>` | `{ [connectionId]: locationData }` | +| `locations.getOthers()` | `Promise>` | Same as above (excludes self) | +| `cursors.set()` | Accepts `Pick` | `{ position: {x, y}, data? }` | +| `locations.set()` | Accepts `unknown` | Any JSON-serializable data | + +Note: `locks.getAll()` returns `Promise` — an **array**, not a Record — so the locks commands do not have this issue. + +**SDK types from `@ably/spaces`:** +```typescript +interface CursorUpdate { + clientId: string; // required (not optional!) + connectionId: string; // required (not optional!) + position: CursorPosition; + data?: CursorData; // CursorData = Record +} + +interface CursorPosition { x: number; y: number; } + +namespace LocationsEvents { + interface UpdateEvent { + member: SpaceMember; + currentLocation: unknown; + previousLocation: unknown; + } +} + +interface SpaceMember { + clientId: string; + connectionId: string; + isConnected: boolean; + profileData: ProfileData; // Record | null + location: unknown; + lastEvent: { name: PresenceAction; timestamp: number; }; +} +``` + +--- + +### Bug 1: `locations get-all` (`src/commands/spaces/locations/get-all.ts`) — BROKEN + +**Severity: HIGH** — human-readable output is effectively useless; JSON output works by luck. + +**What the SDK returns:** `{ "conn-abc": {"x": 10, "y": 20, "page": "dashboard"} }` + +**What the command expects:** A complex structure with `current.member`, `LocationWithCurrent`, etc. + +**Specific issues:** + +- **Lines 15-44**: Defines interfaces (`Member`, `LocationWithCurrent`, `LocationItem`) that model a format `getAll()` never returns. `LocationWithCurrent` expects `{ current: { member: { clientId, memberId, isCurrentMember } } }` — this is NOT the `getAll()` return format. `LocationItem` has 8+ optional fields (`clientId?`, `connectionId?`, `data?`, `id?`, `location?`, `member?`, `memberId?`, `userId?`) when the actual mapped items only ever have `{ location, memberId }`. + +- **Lines 139-140**: Array handling — dead code, SDK never returns an array. + +- **Lines 142-148**: Object handling maps `Record` entries to `{ location: locationData, memberId: connectionId }`. This mapping IS correct — the location data and connection ID are captured. But the downstream code doesn't use this simple structure properly. + +- **Lines 151-159**: `knownMetaKeys` set includes `current`, `member`, `userId`, `id` — these keys never appear in `getAll()` results. Only `memberId` and `location` exist on the mapped items. + +- **Lines 161-171**: `extractLocationData()` is over-engineered for 5+ format variants. For the actual mapped items, it simply returns `item.location` on the first check (line 162). This works but the other branches are dead code. + +- **Lines 192-207 (JSON output)**: Tries to extract `current.member` (always undefined), then falls back through a chain: `item.member` (undefined) → `item.memberId` → **"conn-abc"**. So JSON output DOES work correctly by luck of the fallback chain, producing `{ memberId: "conn-abc", location: {...}, isCurrentMember: false }`. The `isCurrentMember: false` is meaningless since that info isn't available from `getAll()`. + +- **Lines 232-270 (Human display)**: This is where the command is truly broken: + - Line 234: Checks `"current" in location` — always **false** for mapped items `{ location, memberId }`, so it always falls to the else branch. + - **Line 265**: `formatClientId("Member")` — displays the literal string "Member" in blue instead of the actual connection ID (`location.memberId` is available but unused!). + - **Line 267**: `JSON.stringify(location, null, 2)` — `location` here is the loop variable (the full `LocationItem` object), NOT the extracted location data. This displays the **double-wrapped** output: + ``` + - Member: + Location: { + "location": { + "x": 10, + "y": 20 + }, + "memberId": "conn-abc" + } + ``` + The user sees the internal wrapper keys (`location`, `memberId`) instead of just the location data. The actual location data is buried one level deeper than expected. + +- **Lines 272-297 (Error handling)**: Uses manual `jsonError`/`this.error` in both inner and outer catch blocks instead of `this.handleCommandError()` as required by CLAUDE.md convention. Other spaces commands (`cursors/set.ts`, `cursors/subscribe.ts`, `locations/subscribe.ts`) already use `handleCommandError`. + +--- + +### Bug 2: `locations subscribe` initial snapshot (`src/commands/spaces/locations/subscribe.ts`) — MINOR + +**Severity: LOW** — Display and JSON output work correctly. Issues are dead code and unnecessary complexity. + +**Lines 99-176**: The `getAll()` processing and display for the initial snapshot. + +- **Lines 111-126**: Array handling — dead code, SDK never returns array. The comment even says "Unlikely based on current docs." + +- **Lines 127-140**: Object handling maps `Record` entries to `{ location, member: { clientId: "unknown", connectionId, isConnected: true, profileData: null } }`. This creates an unnecessary synthetic `SpaceMember` object — only `connectionId` is ever used from it. + +- **Lines 168-175**: Human display works correctly — shows `Connection ID: conn-abc` and `Location: {...}`. But uses `chalk.blue()` and `chalk.dim("Location:")` directly instead of `formatClientId()` and `formatLabel()` (inconsistent with project output conventions from CLAUDE.md). + +- **Lines 144-159**: JSON output already uses the correct structure — maps to `{ connectionId, location }`. No change needed to the JSON schema. + +- **Lines 17-32**: Local `SpaceMember`, `LocationData`, and `LocationItem` interfaces are defined. `SpaceMember` shadows the SDK's `SpaceMember` type (imported as `LocationsEvents` at line 1). These are only needed because of the synthetic member construction and could be simplified. + +--- + +### Bug 3: `cursors get-all` (`src/commands/spaces/cursors/get-all.ts`) — MODERATE + +**Severity: MODERATE** — The object handling path works, but type safety is weakened and dead code adds confusion. + +**What the SDK returns:** `{ "conn-abc": { clientId: "user1", connectionId: "conn-abc", position: {x: 100, y: 200}, data: {...} } }` + +**Specific issues:** + +- **Lines 15-18**: Defines local `CursorPosition` interface `{ x: number; y: number }` — redundant with SDK's `CursorPosition`. + +- **Lines 20-25**: Defines local `CursorUpdate` interface with `clientId?: string` and `connectionId?: string` (optional) — the SDK's `CursorUpdate` has these as **required**. This weakens type safety throughout the command (the subscription handler at line 134, the `cursorMap` at line 125, and the display code all use this weaker type). + +- **Lines 201-210**: Dead code for `Array.isArray(allCursors)` — `cursors.getAll()` returns `Record`, never an array. + +- **Lines 211-222**: Object handling works correctly — `Object.values()` extracts `CursorUpdate | null` values, null values are filtered by the `cursor &&` check, and valid cursors are added to `cursorMap`. + +- **Lines 134-155**: Subscription handler types `cursor` parameter as the local `CursorUpdate` (with optional fields). Should use the SDK's `CursorUpdate` type. The `if (cursor.connectionId)` check at line 138 is redundant when using the SDK type (where `connectionId` is required/always present), but harmless as a runtime safety net. + +- **Lines 319-342**: Table display code accesses `cursor.position.x`, `cursor.position.y`, `cursor.clientId`, `cursor.connectionId` — all present in the SDK's `CursorUpdate`. Display works correctly. The `|| "Unknown"` fallbacks (lines 320, 326, 364) become unnecessary with SDK types but are harmless to keep. + +- **Lines 80-91**: When using `--json`, the command emits a **separate JSON connection-status object** (`{ connectionId, spaceName, status: "connected", success: true }`) BEFORE the final cursors JSON. This means `--json` output contains two JSON objects, which may confuse JSON parsers. (Pre-existing issue, not introduced by this bug — note for awareness, not fixing here.) + +- **Lines 369-395 (Error handling)**: Uses manual `jsonError`/`this.error` with custom connection-error detection instead of `handleCommandError`. Since this has non-trivial connection-error-specific logic (checking for error code 80017), converting to `handleCommandError` is optional — note as a future improvement. + +--- + +### Bug 4: Unit test mocks use wrong data formats + +| File | Line(s) | Mock Returns | SDK Actually Returns | +|------|---------|-------------|---------------------| +| `test/helpers/mock-ably-spaces.ts` | 177 | `locations.getAll → []` | `{}` | +| `test/helpers/mock-ably-spaces.ts` | 240 | `cursors.getAll → []` | `{}` | +| `test/unit/.../locations/get-all.test.ts` | 52-57 | Array of `{ member, currentLocation, previousLocation }` | `Record` | +| `test/unit/.../locations/get-all.test.ts` | 37, 73 | `[]` | `{}` | +| `test/unit/.../cursors/get-all.test.ts` | 63-76 | Array of `CursorUpdate` | `Record` | +| `test/unit/.../cursors/get-all.test.ts` | 37, 98, 136 | `[]` | `{}` | +| `test/unit/.../cursors/subscribe.test.ts` | 37, 75, 92, 109, 127 | `[]` | `{}` (note: `cursors subscribe` never calls `getAll()` — these mocks are dead but should still match the SDK type for consistency) | + +--- + +## Implementation Plan + +### Step 1: Fix mock defaults in `test/helpers/mock-ably-spaces.ts` + +Change the default return values for `getAll` mocks to match SDK types: + +```typescript +// locations — createMockSpaceLocations(), line 177 +getAll: vi.fn().mockResolvedValue({}), // was: [] + +// cursors — createMockSpaceCursors(), line 240 +getAll: vi.fn().mockResolvedValue({}), // was: [] +``` + +### Step 2: Fix `locations get-all` command + +**File:** `src/commands/spaces/locations/get-all.ts` + +This is the most broken command and needs the most changes. + +1. **Remove dead interfaces**: Delete `LocationData`, `Member`, `LocationWithCurrent`, and the overly-broad `LocationItem`. Replace with a simple mapped type: + ```typescript + interface LocationEntry { + connectionId: string; + location: unknown; + } + ``` + +2. **Simplify `getAll()` processing**: Only handle `Record` format — remove the `Array.isArray` branch. Remove `knownMetaKeys` and `extractLocationData`. Map and filter entries directly: + ```typescript + const locationsFromSpace = await this.space!.locations.getAll(); + const entries: LocationEntry[] = Object.entries(locationsFromSpace) + .filter(([, loc]) => loc !== null && loc !== undefined && + !(typeof loc === "object" && Object.keys(loc as object).length === 0)) + .map(([connectionId, loc]) => ({ connectionId, location: loc })); + ``` + +3. **Fix display (human-readable)**: Show connection ID as the identifier and location data directly (not double-wrapped): + ``` + Current locations (2): + + - conn-abc: + Location: {"x": 10, "y": 20, "page": "dashboard"} + ``` + Use `formatClientId(connectionId)` for the identifier and `formatLabel("Location")` for the data label. + + > **Note on `formatClientId` for connection IDs**: Per CLAUDE.md, `formatClientId` is "for user/client identifiers in events." A connection ID is technically not a client ID, but there is no `formatConnectionId` helper, and the existing `cursors get-all` command already uses `formatClientId` for connection display (line 333). Using it here maintains visual consistency across spaces commands. + +4. **Fix display (JSON)**: Simplify to `{ connectionId, location }` — remove `isCurrentMember` (not available from `getAll()`). **This is a JSON schema change** from the previous output (`memberId` → `connectionId`, `isCurrentMember` removed), but the previous format was effectively incorrect (it called connection IDs "memberId" and always returned `isCurrentMember: false`): + ```json + { + "locations": [ + { "connectionId": "conn-abc", "location": {"x": 10, "y": 20} } + ], + "spaceName": "my-space", + "success": true, + "timestamp": "..." + } + ``` + +5. **Fix error handling**: Replace the inner catch (line 272-286) and outer catch (line 287-297) with `this.handleCommandError(error, flags, "location", { spaceName })` per CLAUDE.md convention. Other spaces commands already use this pattern (`cursors/set.ts`, `cursors/subscribe.ts`, `locations/subscribe.ts`). + +6. **Clean up imports**: Remove unused imports after interface cleanup. Keep `formatProgress`, `formatResource`, `formatSuccess`, `formatLabel`, `formatClientId`. + +### Step 3: Fix `locations subscribe` initial snapshot + +**File:** `src/commands/spaces/locations/subscribe.ts` + +This command's display already works — changes are dead code cleanup and output helper consistency. + +1. **Remove dead array handling** (lines 111-126): SDK never returns array. Remove the `if (Array.isArray(result))` branch and the associated log event. + +2. **Simplify data model**: Remove the fake `SpaceMember` construction. Instead of `LocationItem` with a synthetic member, use a simpler structure like `{ connectionId: string, location: unknown }`. This lets us remove the local `SpaceMember`, `LocationData`, and `LocationItem` interfaces (lines 17-32). The `LocationsEvents` import at line 1 is still needed for the subscription handler. + +3. **Fix output helper consistency**: In the display section (lines 168-175): + - Replace `chalk.blue(item.member.connectionId || "Unknown")` → `formatClientId(entry.connectionId)` + - Replace `chalk.dim("Location:")` → `formatLabel("Location")` + - Add `formatLabel` to the imports from `../../../utils/output.js` + +4. **JSON snapshot output**: Already uses the correct `{ connectionId, location }` structure (lines 144-159). After simplifying the data model, the mapping simplifies from `item.member.connectionId` to `entry.connectionId`, but the output schema stays identical. No JSON change for consumers. + +5. **Leave the subscription handler (lines 209-278) unchanged**: It correctly uses `LocationsEvents.UpdateEvent` from the SDK and accesses `update.member.clientId`, `update.currentLocation`, etc. This is the live event path, not the `getAll()` path, and it works correctly. + +### Step 4: Fix `cursors get-all` command + +**File:** `src/commands/spaces/cursors/get-all.ts` + +1. **Remove local `CursorPosition` and `CursorUpdate` interfaces** (lines 15-25): Import `CursorUpdate` from `@ably/spaces` instead. This restores proper type safety (`clientId` and `connectionId` become required, not optional). + ```typescript + import { type CursorUpdate } from "@ably/spaces"; + ``` + +2. **Remove dead array handling** (lines 201-210): `cursors.getAll()` never returns array. + +3. **Update the `getAll()` processing** to handle only `Record`: + ```typescript + const allCursors = await this.space!.cursors.getAll(); + for (const cursor of Object.values(allCursors)) { + if (cursor && !cursorMap.has(cursor.connectionId)) { + cursorMap.set(cursor.connectionId, cursor); + } + } + ``` + +4. **Keep the existing display code**: The table rendering (lines 278-367) and the subscription handler (lines 134-155) work correctly with `CursorUpdate` objects. Only the type annotations change (from local optional to SDK required). The `|| "Unknown"` fallbacks in the display code (lines 146, 320, 326, 364) become unnecessary with SDK types but should be kept as harmless runtime safety nets. + +5. **Subscription handler**: The `if (cursor.connectionId)` guard at line 138 becomes redundant with the SDK type (where `connectionId` is always present). Keep it as a runtime safety check — it's harmless and provides defense-in-depth. + +6. **Error handling** (lines 369-395): Currently uses manual `jsonError`/`this.error` with custom connection-error detection (checking for code 80017). Since this has non-trivial connection-error-specific logic, converting to `handleCommandError` is **optional** for this PR. Note as a future improvement. + +### Step 5: Fix unit test for `locations get-all` + +**File:** `test/unit/commands/spaces/locations/get-all.test.ts` + +Update mock data to match SDK format and strengthen assertions: +```typescript +// Was (WRONG — LocationsEvents.UpdateEvent format, as array): +space.locations.getAll.mockResolvedValue([ + { member: { clientId: "user-1", connectionId: "conn-1" }, + currentLocation: { x: 100, y: 200 }, previousLocation: null } +]); + +// Should be (correct — Record): +space.locations.getAll.mockResolvedValue({ + "conn-1": { x: 100, y: 200 }, +}); +``` + +Update assertions to validate: +- JSON output contains `"conn-1"` as connectionId +- JSON output contains location data `{ x: 100, y: 200 }` +- Empty case uses `{}` not `[]` (lines 37, 73) + +### Step 6: Fix unit test for `cursors get-all` + +**File:** `test/unit/commands/spaces/cursors/get-all.test.ts` + +Update mock data to match SDK format: +```typescript +// Was (WRONG — array): +space.cursors.getAll.mockResolvedValue([ + { clientId: "user-1", connectionId: "conn-1", + position: { x: 100, y: 200 }, data: { color: "red" } }, +]); + +// Should be (correct — Record): +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" } }, +}); +``` + +Update empty-case mocks from `[]` to `{}` (lines 37, 98, 136). + +### Step 7: Fix remaining test mocks for consistency + +**Files:** +- `test/unit/commands/spaces/cursors/subscribe.test.ts` (lines 37, 75, 92, 109, 127) — change `cursors.getAll.mockResolvedValue([])` to `{}`. Note: `cursors subscribe` command never calls `getAll()`, so these are dead mock setups. Fixing for consistency with the SDK type. +- `test/unit/commands/spaces/locations/subscribe.test.ts` — already uses `{}` (correct, no changes needed). + +### Step 8: Validate + +1. `pnpm prepare` — build succeeds (TypeScript compilation with SDK types) +2. `pnpm exec eslint .` — 0 errors (no unused imports/variables) +3. `pnpm test:unit` — all tests pass with new mock formats +4. Verify JSON output format is clean and parseable +5. Verify human-readable output shows connection IDs and location/cursor data + +--- + +## Files to Modify + +| File | Changes | Severity | +|------|---------|----------| +| `src/commands/spaces/locations/get-all.ts` | Remove dead interfaces, fix getAll processing, fix human display (double-wrapped → clean), fix JSON output, fix error handling to use `handleCommandError` | HIGH | +| `src/commands/spaces/locations/subscribe.ts` | Remove dead array handling, simplify data model, fix output helper consistency (`formatClientId`, `formatLabel`) | LOW | +| `src/commands/spaces/cursors/get-all.ts` | Import SDK `CursorUpdate`, remove local `CursorPosition`/`CursorUpdate` interfaces, remove dead array handling | MODERATE | +| `test/helpers/mock-ably-spaces.ts` | Fix default getAll returns from `[]` to `{}` | LOW | +| `test/unit/commands/spaces/locations/get-all.test.ts` | Fix mock data format (array→object) and strengthen assertions | LOW | +| `test/unit/commands/spaces/cursors/get-all.test.ts` | Fix mock data format (array→object) and strengthen assertions | LOW | +| `test/unit/commands/spaces/cursors/subscribe.test.ts` | Fix dead mock defaults from `[]` to `{}` for consistency | LOW | + +--- + +## Out-of-Scope Issues Noted + +These pre-existing issues were discovered during analysis but are NOT part of this fix: + +1. **`cursors get-all` emits two JSON objects** (lines 80-91 + 236-252): When using `--json`, the command outputs a connection-status JSON object AND a cursors-data JSON object. This can confuse JSON parsers expecting a single object. Recommend fixing in a separate PR. + +2. **`cursors get-all` error handling** (lines 369-395): Uses manual `jsonError`/`this.error` with custom connection-error detection (code 80017) instead of `handleCommandError`. The custom logic is intentional, so converting requires care. Optional future improvement. + +3. **`locations subscribe` finally block** (line 318): Uses `flags || {}` which is unusual — `flags` should always be defined at that point. Harmless but worth cleaning up separately. + +--- + +## E2E Test Impact + +- **`test/e2e/spaces/spaces-e2e.test.ts`**: The cursor E2E test (lines 419-426) runs `cursors get-all` in human-readable mode (no `--json`) and checks output contains `client2Id` and "TestUser2". Since the real SDK returns objects (not arrays), the **object handling path** (which we keep) is the one that runs in E2E. The table display code is unchanged, so E2E tests should pass without modification. +- **No E2E test exists for `locations get-all`**, so the display format change has no E2E impact. + +## Risk Assessment + +- **Low risk**: Mock default changes, dead code removal, test mock format fixes +- **Medium risk**: `locations get-all` display format change — human-readable output changes significantly (from broken double-wrapped "Member:" to clean connection-ID-based display). No E2E tests depend on the old format. +- **JSON schema change**: `locations get-all` JSON output changes from `{ memberId, location, isCurrentMember }` to `{ connectionId, location }`. The old `memberId` was actually a connection ID mislabeled, and `isCurrentMember` was always `false`. The new schema is accurate. +- **No behavioral change needed**: The `set`, `subscribe` (event processing), and `cursors subscribe` commands are correct. Only `get-all` commands and the `locations subscribe` initial snapshot processing need fixes.