diff --git a/.claude/skills/ably-codebase-review/SKILL.md b/.claude/skills/ably-codebase-review/SKILL.md index fc3603e8..703ffe32 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 @@ -184,8 +197,9 @@ Launch these agents **in parallel**. Each agent gets a focused mandate and uses **Method (grep/read — text patterns):** 1. **Grep** for `waitUntilInterruptedOrTimeout` in command files — should use `this.waitAndTrackCleanup()` instead 2. **Grep** for `setupChannelStateLogging` in subscribe commands (rooms/*, spaces/*) — flag those that don't call it -3. **Read** command files and check `static examples` arrays for `--json` or `--pretty-json` examples — flag leaf commands that have examples but no JSON variant -4. **Compare** skill templates (`patterns.md`, `SKILL.md`, `testing.md` — already read in Step 1) against actual codebase method names/imports — flag any outdated patterns +3. **Grep** for `room.attach()` or `space.enter()` in all rooms/* and spaces/* commands — verify it's only called for commands that need a realtime connection. In the Chat SDK, methods using `this._chatApi.*` are REST (no attach needed), while methods using `this._channel.publish()` or `this._channel.presence.*` need realtime attachment. REST-only: messages send/update/delete/history, occupancy get. Needs attach: presence enter/get/subscribe, typing keystroke/stop, reactions send/subscribe, occupancy subscribe, messages subscribe. Unnecessary attachment adds latency and creates an unneeded realtime connection. +4. **Read** command files and check `static examples` arrays for `--json` or `--pretty-json` examples — flag leaf commands that have examples but no JSON variant +5. **Compare** skill templates (`patterns.md`, `SKILL.md`, `testing.md` — already read in Step 1) against actual codebase method names/imports — flag any outdated patterns **Method (LSP — for base class verification):** 5. If a subscribe command doesn't call `setupChannelStateLogging` directly, use `LSP goToDefinition` on the base class to check if it's handled there @@ -193,6 +207,7 @@ Launch these agents **in parallel**. Each agent gets a focused mandate and uses **Reasoning guidance:** - `waitUntilInterruptedOrTimeout` in bench commands may be acceptable (different lifecycle) - Missing `setupChannelStateLogging` in rooms/spaces may be handled by `ChatBaseCommand`/`SpacesBaseCommand` — check the base class +- `room.attach()` in REST-based commands is a deviation. Chat SDK methods using `this._chatApi.*` (messages send/update/delete/history, occupancy get) are pure REST calls. Methods using `this._channel.publish()` or `this._channel.presence.*` (reactions send, typing keystroke, presence enter/get/subscribe, occupancy subscribe, messages subscribe) DO need attachment. - Topic index commands and `help.ts` don't need `--json` examples - Skill template accuracy issues are low-effort, high-value fixes diff --git a/.claude/skills/ably-new-command/SKILL.md b/.claude/skills/ably-new-command/SKILL.md index 5c40bcbe..c638cdfa 100644 --- a/.claude/skills/ably-new-command/SKILL.md +++ b/.claude/skills/ably-new-command/SKILL.md @@ -19,6 +19,7 @@ Every command in this CLI falls into one of these patterns. Pick the right one b | **Get** | One-shot query for current state | `AblyBaseCommand` | REST | `channels occupancy get`, `rooms occupancy get` | | **List** | Enumerate resources via REST API | `AblyBaseCommand` | REST | `channels list`, `rooms list` | | **Enter** | Join presence/space then optionally listen | `AblyBaseCommand` | Realtime | `channels presence enter`, `spaces members enter` | +| **REST Mutation** | One-shot REST mutation (no subscription) | `AblyBaseCommand` | REST | `rooms messages update`, `rooms messages delete` | | **CRUD** | Create/read/update/delete via Control API | `ControlBaseCommand` | Control API (HTTP) | `integrations create`, `queues delete` | **Specialized base classes** — some command groups have dedicated base classes that handle common setup (client lifecycle, cleanup, shared flags): @@ -31,6 +32,25 @@ Every command in this CLI falls into one of these patterns. Pick the right one b If your command falls into one of these groups, extend the specialized base class instead of `AblyBaseCommand` or `ControlBaseCommand` directly. **Exception:** if your command only needs a REST client (e.g., history queries that don't enter a space or join a room), you may use `AblyBaseCommand` directly — the specialized base class is most valuable when the command needs realtime connections, cleanup lifecycle, or shared setup like `room.attach()` / `space.enter()`. +### When to call `room.attach()` / `space.enter()` + +**Not every Chat/Spaces command needs `room.attach()` or `space.enter()`.** Before adding attachment, check whether the SDK method you're calling requires an active realtime connection or is a pure REST call: + +| Needs `room.attach()` | Does NOT need `room.attach()` | +|------------------------|-------------------------------| +| Subscribe (realtime listener) | Send (REST via `chatApi.sendMessage`) | +| Presence enter/get/update/leave | Update (REST via `chatApi.updateMessage`) | +| Occupancy subscribe | Delete (REST via `chatApi.deleteMessage`) | +| Typing keystroke/stop | Annotate/append (REST mutation) | +| Reactions send (realtime publish) | History queries (REST via `chatApi.history`) | +| Reactions subscribe | Occupancy get (REST via `chatApi.getOccupancy`) | + +**How it works in the SDK:** Methods that go through `this._chatApi.*` are REST calls and don't need attachment. Methods that use `this._channel.publish()`, `this._channel.presence.*`, or subscribe to channel events require the realtime channel to be attached. Room-level reactions use `this._channel.publish()` (realtime), but messages send/update/delete use `this._chatApi.*` (REST). + +**Rule of thumb:** If the SDK method is a one-shot REST call (returns a Promise with a result, no callback/listener), you do NOT need `room.attach()`. Just call `chatClient.rooms.get(roomId)` to get the room handle and invoke the method directly. Attaching unnecessarily adds latency and creates a realtime connection that isn't needed. + +**How to verify:** Check the Chat SDK source or typedoc — methods that are REST-based don't require the room to be in an `attached` state. When in doubt, test without `room.attach()` — if the SDK method works, attachment isn't needed. + ## Step 2: Create the command file ### File location @@ -176,14 +196,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({ message: messageData }, flags); + // → {"type":"result","command":"...","success":true,"message":{...}} +} + +// One-shot collection result — plural domain key + optional metadata: if (this.shouldOutputJson(flags)) { - this.logJsonResult({ channel: args.channel, message }, flags); + this.logJsonResult({ messages: items, total: items.length }, flags); } -// Streaming events — use logJsonEvent: +// Streaming events — singular domain key: if (this.shouldOutputJson(flags)) { - this.logJsonEvent({ eventType: event.type, message, channel: channelName }, flags); + this.logJsonEvent({ message: messageData }, flags); + // → {"type":"event","command":"...","message":{...}} } ``` @@ -389,8 +418,15 @@ 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") +- [ ] `room.attach()` / `space.enter()` only called when the SDK method requires a realtime connection (subscribe, send, presence) — NOT for REST mutations (update, delete, annotate, history) - [ ] 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..6ff9d0bc 100644 --- a/.claude/skills/ably-new-command/references/patterns.md +++ b/.claude/skills/ably-new-command/references/patterns.md @@ -5,6 +5,7 @@ Pick the pattern that matches your command from Step 1 of the skill, then follow ## Table of Contents - [Subscribe Pattern](#subscribe-pattern) - [Publish/Send Pattern](#publishsend-pattern) +- [REST Mutation Pattern](#rest-mutation-pattern) - [History Pattern](#history-pattern) - [Get Pattern](#get-pattern) - [Enter/Presence Pattern](#enterpresence-pattern) @@ -59,17 +60,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 +128,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) + ".")); } @@ -135,6 +151,57 @@ For single-shot publish, REST is preferred (simpler, no connection overhead). Se --- +## REST Mutation Pattern + +For one-shot SDK operations that are pure REST calls (send, update, delete, annotate, history, occupancy get). These do **NOT** need `room.attach()` or `space.enter()` — they only need a room/space handle. In the Chat SDK, methods that go through `this._chatApi.*` are REST-based, while methods that use `this._channel.publish()` or `this._channel.presence.*` require realtime attachment. + +Flags for REST mutation commands: +```typescript +static override flags = { + ...productApiFlags, + ...clientIdFlag, // Users may want to test "can client B update client A's message?" + // command-specific flags here +}; +``` + +```typescript +async run(): Promise { + const { args, flags } = await this.parse(MyMutationCommand); + + try { + const chatClient = await this.createChatClient(flags); + if (!chatClient) { + return this.fail("Failed to create Chat client", flags, "roomMessageUpdate"); + } + + this.setupConnectionStateLogging(chatClient.realtime, flags); + + // NO room.attach() — update/delete/annotate are REST calls + const room = await chatClient.rooms.get(args.room); + + if (!this.shouldOutputJson(flags)) { + this.log(formatProgress("Updating message " + formatResource(args.serial) + " in room " + formatResource(args.room))); + } + + const result = await room.messages.update(args.serial, updateParams, details); + + if (this.shouldOutputJson(flags)) { + this.logJsonResult({ room: args.room, serial: args.serial, versionSerial: result.version.serial }, flags); + } else { + this.log(formatSuccess(`Message ${formatResource(args.serial)} updated in room ${formatResource(args.room)}.`)); + } + } catch (error) { + this.fail(error, flags, "roomMessageUpdate", { room: args.room, serial: args.serial }); + } +} +``` + +**Key difference from Subscribe/Send:** No `room.attach()`, no `durationFlag`, no `rewindFlag`, no `waitAndTrackCleanup`. The command creates the client, gets the room handle, calls the REST method, and exits. + +See `src/commands/rooms/messages/update.ts` and `src/commands/rooms/messages/delete.ts` as references. + +--- + ## History Pattern ```typescript @@ -158,10 +225,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 +263,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 +390,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 +428,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..530b33d6 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()`) @@ -117,8 +126,9 @@ For each changed command file, run the relevant checks. Spawn agents for paralle **Lifecycle check (grep/read):** 1. **Grep** for `waitUntilInterruptedOrTimeout` — should use `this.waitAndTrackCleanup()` instead -2. **Read** `static examples` and check for `--json` or `--pretty-json` variant -3. **Read** the command description — verify imperative mood, sentence case, no trailing period +2. **Grep** for `room.attach()` or `space.enter()` — verify it's only called for commands that need a realtime connection. In the Chat SDK, methods using `this._chatApi.*` are REST (no attach needed), while methods using `this._channel.publish()` or `this._channel.presence.*` need realtime attachment. REST-only: messages send/update/delete/history, occupancy get. Needs attach: presence enter/get/subscribe, typing keystroke/stop, reactions send/subscribe, occupancy subscribe, messages subscribe. Unnecessary attachment adds latency and creates an unneeded realtime connection. +3. **Read** `static examples` and check for `--json` or `--pretty-json` variant +4. **Read** the command description — verify imperative mood, sentence case, no trailing period ### For changed test files (`test/unit/commands/**/*.ts`) 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 diff --git a/README.md b/README.md index 8afb4c46..ec5b71f7 100644 --- a/README.md +++ b/README.md @@ -177,6 +177,7 @@ $ ably-interactive * [`ably rooms`](#ably-rooms) * [`ably rooms list`](#ably-rooms-list) * [`ably rooms messages`](#ably-rooms-messages) +* [`ably rooms messages delete ROOM SERIAL`](#ably-rooms-messages-delete-room-serial) * [`ably rooms messages history ROOM`](#ably-rooms-messages-history-room) * [`ably rooms messages reactions`](#ably-rooms-messages-reactions) * [`ably rooms messages reactions remove ROOM MESSAGESERIAL REACTION`](#ably-rooms-messages-reactions-remove-room-messageserial-reaction) @@ -184,6 +185,7 @@ $ ably-interactive * [`ably rooms messages reactions subscribe ROOM`](#ably-rooms-messages-reactions-subscribe-room) * [`ably rooms messages send ROOM TEXT`](#ably-rooms-messages-send-room-text) * [`ably rooms messages subscribe ROOMS`](#ably-rooms-messages-subscribe-rooms) +* [`ably rooms messages update ROOM SERIAL TEXT`](#ably-rooms-messages-update-room-serial-text) * [`ably rooms occupancy`](#ably-rooms-occupancy) * [`ably rooms occupancy get ROOM`](#ably-rooms-occupancy-get-room) * [`ably rooms occupancy subscribe ROOM`](#ably-rooms-occupancy-subscribe-room) @@ -3667,10 +3669,47 @@ EXAMPLES $ ably rooms messages subscribe my-room $ ably rooms messages history my-room + + $ ably rooms messages update my-room "serial" "Updated text" + + $ ably rooms messages delete my-room "serial" ``` _See code: [src/commands/rooms/messages/index.ts](https://github.com/ably/ably-cli/blob/v0.17.0/src/commands/rooms/messages/index.ts)_ +## `ably rooms messages delete ROOM SERIAL` + +Delete a message in an Ably Chat room + +``` +USAGE + $ ably rooms messages delete ROOM SERIAL [-v] [--json | --pretty-json] [--client-id ] [--description ] + +ARGUMENTS + ROOM The room containing the message to delete + SERIAL The serial of the message to delete + +FLAGS + -v, --verbose Output verbose logs + --client-id= Overrides any default client ID when using API authentication. Use "none" to explicitly set + no client ID. Not applicable when using token authentication. + --description= Description of the delete operation + --json Output in JSON format + --pretty-json Output in colorized JSON format + +DESCRIPTION + Delete a message in an Ably Chat room + +EXAMPLES + $ ably rooms messages delete my-room "serial-001" + + $ ably rooms messages delete my-room "serial-001" --description "spam removal" + + $ ably rooms messages delete my-room "serial-001" --json +``` + +_See code: [src/commands/rooms/messages/delete.ts](https://github.com/ably/ably-cli/blob/v0.17.0/src/commands/rooms/messages/delete.ts)_ + ## `ably rooms messages history ROOM` Get historical messages from an Ably Chat room @@ -3942,6 +3981,47 @@ EXAMPLES _See code: [src/commands/rooms/messages/subscribe.ts](https://github.com/ably/ably-cli/blob/v0.17.0/src/commands/rooms/messages/subscribe.ts)_ +## `ably rooms messages update ROOM SERIAL TEXT` + +Update a message in an Ably Chat room + +``` +USAGE + $ ably rooms messages update ROOM SERIAL TEXT [-v] [--json | --pretty-json] [--client-id ] [--description ] + [--headers ] [--metadata ] + +ARGUMENTS + ROOM The room containing the message to update + SERIAL The serial of the message to update + TEXT The new message text + +FLAGS + -v, --verbose Output verbose logs + --client-id= Overrides any default client ID when using API authentication. Use "none" to explicitly set + no client ID. Not applicable when using token authentication. + --description= Description of the update operation + --headers= Additional headers for the message (JSON format) + --json Output in JSON format + --metadata= Additional metadata for the message (JSON format) + --pretty-json Output in colorized JSON format + +DESCRIPTION + Update a message in an Ably Chat room + +EXAMPLES + $ ably rooms messages update my-room "serial-001" "Updated text" + + $ ably rooms messages update my-room "serial-001" "Updated text" --description "typo fix" + + $ ably rooms messages update my-room "serial-001" "Updated text" --metadata '{"edited":true}' + + $ ably rooms messages update my-room "serial-001" "Updated text" --headers '{"source":"cli"}' + + $ ably rooms messages update my-room "serial-001" "Updated text" --json +``` + +_See code: [src/commands/rooms/messages/update.ts](https://github.com/ably/ably-cli/blob/v0.17.0/src/commands/rooms/messages/update.ts)_ + ## `ably rooms occupancy` Commands for monitoring room occupancy diff --git a/src/chat-base-command.ts b/src/chat-base-command.ts index 177f7bac..b422dd05 100644 --- a/src/chat-base-command.ts +++ b/src/chat-base-command.ts @@ -1,3 +1,4 @@ +import type * as Ably from "ably"; import { ChatClient, Room, RoomStatus } from "@ably/chat"; import { AblyBaseCommand } from "./base-command.js"; @@ -16,6 +17,17 @@ export abstract class ChatBaseCommand extends AblyBaseCommand { private _chatClient: ChatClient | null = null; private _cleanupTimeout: NodeJS.Timeout | undefined; + /** + * Override getClientOptions to disable binary protocol for Chat commands. + * The Chat API uses realtime.request() for REST calls, and binary (MsgPack) + * encoding is not supported by the Chat API endpoints. + */ + protected override getClientOptions(flags: BaseFlags): Ably.ClientOptions { + const options = super.getClientOptions(flags); + options.useBinaryProtocol = false; + return options; + } + /** * finally disposes of the chat client, if there is one, which includes cleaning up any subscriptions. * diff --git a/src/commands/rooms/messages/delete.ts b/src/commands/rooms/messages/delete.ts new file mode 100644 index 00000000..9dda89ef --- /dev/null +++ b/src/commands/rooms/messages/delete.ts @@ -0,0 +1,116 @@ +import { Args, Flags } from "@oclif/core"; +import type { OperationDetails } from "@ably/chat"; + +import { productApiFlags, clientIdFlag } from "../../../flags.js"; +import { ChatBaseCommand } from "../../../chat-base-command.js"; +import { + formatProgress, + formatSuccess, + formatResource, +} from "../../../utils/output.js"; + +export default class MessagesDelete extends ChatBaseCommand { + static override args = { + room: Args.string({ + description: "The room containing the message to delete", + required: true, + }), + serial: Args.string({ + description: "The serial of the message to delete", + required: true, + }), + }; + + static override description = "Delete a message in an Ably Chat room"; + + static override examples = [ + '$ ably rooms messages delete my-room "serial-001"', + '$ ably rooms messages delete my-room "serial-001" --description "spam removal"', + '$ ably rooms messages delete my-room "serial-001" --json', + ]; + + static override flags = { + ...productApiFlags, + ...clientIdFlag, + description: Flags.string({ + description: "Description of the delete operation", + }), + }; + + async run(): Promise { + const { args, flags } = await this.parse(MessagesDelete); + + try { + const chatClient = await this.createChatClient(flags); + + if (!chatClient) { + return this.fail( + "Failed to create Chat client", + flags, + "roomMessageDelete", + ); + } + + this.setupConnectionStateLogging(chatClient.realtime, flags); + + const room = await chatClient.rooms.get(args.room); + + if (!this.shouldOutputJson(flags)) { + this.log( + formatProgress( + "Deleting message " + + formatResource(args.serial) + + " in room " + + formatResource(args.room), + ), + ); + } + + // Build operation details + const details: OperationDetails | undefined = flags.description + ? { description: flags.description } + : undefined; + + this.logCliEvent( + flags, + "roomMessageDelete", + "deleting", + `Deleting message ${args.serial} from room ${args.room}`, + { room: args.room, serial: args.serial }, + ); + + const result = await room.messages.delete(args.serial, details); + + this.logCliEvent( + flags, + "roomMessageDelete", + "messageDeleted", + `Message ${args.serial} deleted from room ${args.room}`, + { room: args.room, serial: args.serial }, + ); + + if (this.shouldOutputJson(flags)) { + this.logJsonResult( + { + room: args.room, + serial: args.serial, + versionSerial: result.version.serial, + }, + flags, + ); + } else { + this.log( + formatSuccess( + `Message ${formatResource(args.serial)} deleted from room ${formatResource(args.room)}.`, + ), + ); + this.log(` Version serial: ${formatResource(result.version.serial)}`); + } + } catch (error) { + this.fail(error, flags, "roomMessageDelete", { + room: args.room, + serial: args.serial, + }); + } + } +} diff --git a/src/commands/rooms/messages/history.ts b/src/commands/rooms/messages/history.ts index 13f9c7cf..d0483151 100644 --- a/src/commands/rooms/messages/history.ts +++ b/src/commands/rooms/messages/history.ts @@ -11,6 +11,8 @@ import { formatResource, formatTimestamp, formatMessageTimestamp, + formatEventType, + formatClientId, } from "../../../utils/output.js"; import { parseTimestamp } from "../../../utils/time.js"; @@ -66,15 +68,15 @@ export default class MessagesHistory extends ChatBaseCommand { const chatClient = await this.createChatClient(flags); if (!chatClient) { - this.fail("Failed to create Chat client", flags, "roomMessageHistory"); + return this.fail( + "Failed to create Chat client", + flags, + "roomMessageHistory", + ); } - // Get the room const room = await chatClient.rooms.get(args.room); - // Attach to the room - await room.attach(); - if (!this.shouldSuppressOutput(flags)) { if (this.shouldOutputJson(flags)) { this.logJsonEvent( @@ -122,7 +124,7 @@ export default class MessagesHistory extends ChatBaseCommand { historyParams.end !== undefined && historyParams.start > historyParams.end ) { - this.fail( + return this.fail( "--start must be earlier than or equal to --end", flags, "roomMessageHistory", @@ -141,9 +143,9 @@ export default class MessagesHistory extends ChatBaseCommand { clientId: message.clientId, text: message.text, timestamp: message.timestamp, - ...(flags["show-metadata"] && message.metadata - ? { metadata: message.metadata } - : {}), + serial: message.serial, + action: String(message.action), + ...(message.metadata ? { metadata: message.metadata } : {}), })), room: args.room, }, @@ -165,8 +167,14 @@ export default class MessagesHistory extends ChatBaseCommand { const timestamp = formatMessageTimestamp(message.timestamp); const author = message.clientId || "Unknown"; + this.log(formatTimestamp(timestamp)); + this.log( + ` ${formatLabel("Action")} ${formatEventType(String(message.action))}`, + ); + this.log(` ${formatLabel("Client ID")} ${formatClientId(author)}`); + this.log(` ${formatLabel("Text")} ${message.text}`); this.log( - `${formatTimestamp(timestamp)} ${chalk.blue(`${author}:`)} ${message.text}`, + ` ${formatLabel("Serial")} ${formatResource(message.serial)}`, ); // Show metadata if enabled and available diff --git a/src/commands/rooms/messages/index.ts b/src/commands/rooms/messages/index.ts index 90511770..fd985db8 100644 --- a/src/commands/rooms/messages/index.ts +++ b/src/commands/rooms/messages/index.ts @@ -11,5 +11,7 @@ export default class MessagesIndex extends BaseTopicCommand { '<%= config.bin %> <%= command.id %> send my-room "Hello world!"', "<%= config.bin %> <%= command.id %> subscribe my-room", "<%= config.bin %> <%= command.id %> history my-room", + '<%= config.bin %> <%= command.id %> update my-room "serial" "Updated text"', + '<%= config.bin %> <%= command.id %> delete my-room "serial"', ]; } diff --git a/src/commands/rooms/messages/reactions/remove.ts b/src/commands/rooms/messages/reactions/remove.ts index 53fb358f..8b119ad0 100644 --- a/src/commands/rooms/messages/reactions/remove.ts +++ b/src/commands/rooms/messages/reactions/remove.ts @@ -50,7 +50,7 @@ export default class MessagesReactionsRemove extends ChatBaseCommand { const chatClient = await this.createChatClient(flags); if (!chatClient) { - this.fail( + return this.fail( "Failed to create Chat client", flags, "roomMessageReactionRemove", diff --git a/src/commands/rooms/messages/reactions/send.ts b/src/commands/rooms/messages/reactions/send.ts index fe3af2da..73cb05d1 100644 --- a/src/commands/rooms/messages/reactions/send.ts +++ b/src/commands/rooms/messages/reactions/send.ts @@ -59,7 +59,7 @@ export default class MessagesReactionsSend extends ChatBaseCommand { flags.count !== undefined && flags.count <= 0 ) { - this.fail( + return this.fail( "Count must be a positive integer for Multiple type reactions", flags, "roomMessageReactionSend", @@ -71,7 +71,7 @@ export default class MessagesReactionsSend extends ChatBaseCommand { this.chatClient = await this.createChatClient(flags); if (!this.chatClient) { - this.fail( + return this.fail( "Failed to create Chat client", flags, "roomMessageReactionSend", diff --git a/src/commands/rooms/messages/reactions/subscribe.ts b/src/commands/rooms/messages/reactions/subscribe.ts index e79d3173..c487f341 100644 --- a/src/commands/rooms/messages/reactions/subscribe.ts +++ b/src/commands/rooms/messages/reactions/subscribe.ts @@ -60,7 +60,7 @@ export default class MessagesReactionsSubscribe extends ChatBaseCommand { this.chatClient = await this.createChatClient(flags); if (!this.chatClient) { - this.fail( + return this.fail( "Failed to initialize clients", flags, "roomMessageReactionSubscribe", diff --git a/src/commands/rooms/messages/send.ts b/src/commands/rooms/messages/send.ts index c68514e6..829a3afc 100644 --- a/src/commands/rooms/messages/send.ts +++ b/src/commands/rooms/messages/send.ts @@ -22,6 +22,7 @@ interface MessageResult { index?: number; message?: MessageToSend; room: string; + serial?: string; success: boolean; error?: string; [key: string]: unknown; @@ -100,62 +101,44 @@ export default class MessagesSend extends ChatBaseCommand { this.chatClient = await this.createChatClient(flags); if (!this.chatClient) { - this.fail("Failed to create Chat client", flags, "roomMessageSend"); + return this.fail( + "Failed to create Chat client", + flags, + "roomMessageSend", + ); } // Set up connection state logging this.setupConnectionStateLogging(this.chatClient.realtime, flags); // Parse metadata if provided - let metadata; + let metadata: JsonObject | undefined; if (flags.metadata) { - try { - metadata = JSON.parse(flags.metadata); - this.logCliEvent( - flags, - "message", - "metadataParsed", - "Message metadata parsed", - { metadata }, - ); - } catch (error) { - this.fail( - `Invalid metadata JSON: ${errorMessage(error)}`, - flags, - "roomMessageSend", - ); + const parsedMetadata = this.parseJsonFlag( + flags.metadata, + "metadata", + flags, + ); + if ( + typeof parsedMetadata !== "object" || + parsedMetadata === null || + Array.isArray(parsedMetadata) + ) { + this.fail("Metadata must be a JSON object", flags, "roomMessageSend"); } + + metadata = parsedMetadata as JsonObject; + + this.logCliEvent( + flags, + "message", + "metadataParsed", + "Message metadata parsed", + { metadata }, + ); } - // Get the room with default options - this.logCliEvent( - flags, - "room", - "gettingRoom", - `Getting room handle for ${args.room}`, - ); const room = await this.chatClient.rooms.get(args.room); - this.logCliEvent( - flags, - "room", - "gotRoom", - `Got room handle for ${args.room}`, - ); - - // Attach to the room - this.logCliEvent( - flags, - "room", - "attaching", - `Attaching to room ${args.room}`, - ); - await room.attach(); - this.logCliEvent( - flags, - "room", - "attached", - `Attached to room ${args.room}`, - ); // Validate count and delay const count = Math.max(1, flags.count); @@ -358,10 +341,11 @@ export default class MessagesSend extends ChatBaseCommand { ); // Send the message - await room.messages.send(messageToSend); + const sentMessage = await room.messages.send(messageToSend); const result: MessageResult = { message: messageToSend, room: args.room, + serial: sentMessage.serial, success: true, }; this.logCliEvent( @@ -375,7 +359,11 @@ export default class MessagesSend extends ChatBaseCommand { if (!this.shouldSuppressOutput(flags)) { if (this.shouldOutputJson(flags)) { this.logJsonResult( - { message: messageToSend, room: args.room }, + { + message: messageToSend, + room: args.room, + serial: sentMessage.serial, + }, flags, ); } else { @@ -384,6 +372,7 @@ export default class MessagesSend extends ChatBaseCommand { `Message sent to room ${formatResource(args.room)}.`, ), ); + this.log(` Serial: ${formatResource(sentMessage.serial)}`); } } } catch (error) { diff --git a/src/commands/rooms/messages/subscribe.ts b/src/commands/rooms/messages/subscribe.ts index 44f7f3c7..ef6d6eaf 100644 --- a/src/commands/rooms/messages/subscribe.ts +++ b/src/commands/rooms/messages/subscribe.ts @@ -11,6 +11,8 @@ import { formatTimestamp, formatMessageTimestamp, formatIndex, + formatEventType, + formatClientId, } from "../../../utils/output.js"; // Define message interface @@ -18,6 +20,8 @@ interface ChatMessage { clientId: string; text: string; timestamp: number | Date; // Support both timestamp types + serial: string; + action: string; metadata?: Record; [key: string]: unknown; } @@ -97,10 +101,9 @@ export default class MessagesSubscribe extends ChatBaseCommand { clientId: message.clientId, text: message.text, timestamp: message.timestamp, + serial: message.serial, + action: String(messageEvent.type), ...(message.metadata ? { metadata: message.metadata } : {}), - ...(flags["sequence-numbers"] - ? { sequence: this.sequenceCounter } - : {}), }; this.logCliEvent(flags, "message", "received", "Message received", { message: messageLog, @@ -132,9 +135,17 @@ export default class MessagesSubscribe extends ChatBaseCommand { ? `${formatIndex(this.sequenceCounter)}` : ""; - // Message content with consistent formatting + // Message content with multi-line labeled block + this.log(`${roomPrefix}${formatTimestamp(timestamp)}${sequencePrefix}`); this.log( - `${roomPrefix}${formatTimestamp(timestamp)}${sequencePrefix} ${chalk.blue(`${author}:`)} ${message.text}`, + `${roomPrefix} ${formatLabel("Action")} ${formatEventType(String(messageEvent.type))}`, + ); + this.log( + `${roomPrefix} ${formatLabel("Client ID")} ${formatClientId(author)}`, + ); + this.log(`${roomPrefix} ${formatLabel("Text")} ${message.text}`); + this.log( + `${roomPrefix} ${formatLabel("Serial")} ${formatResource(message.serial)}`, ); // Show metadata if enabled and available diff --git a/src/commands/rooms/messages/update.ts b/src/commands/rooms/messages/update.ts new file mode 100644 index 00000000..b5882daf --- /dev/null +++ b/src/commands/rooms/messages/update.ts @@ -0,0 +1,207 @@ +import { Args, Flags } from "@oclif/core"; +import type { + Headers, + JsonObject, + OperationDetails, + UpdateMessageParams, +} from "@ably/chat"; + +import { productApiFlags, clientIdFlag } from "../../../flags.js"; +import { ChatBaseCommand } from "../../../chat-base-command.js"; +import { + formatProgress, + formatSuccess, + formatResource, +} from "../../../utils/output.js"; + +export default class MessagesUpdate extends ChatBaseCommand { + static override args = { + room: Args.string({ + description: "The room containing the message to update", + required: true, + }), + serial: Args.string({ + description: "The serial of the message to update", + required: true, + }), + text: Args.string({ + description: "The new message text", + required: true, + }), + }; + + static override description = "Update a message in an Ably Chat room"; + + static override examples = [ + '$ ably rooms messages update my-room "serial-001" "Updated text"', + '$ ably rooms messages update my-room "serial-001" "Updated text" --description "typo fix"', + '$ ably rooms messages update my-room "serial-001" "Updated text" --metadata \'{"edited":true}\'', + '$ ably rooms messages update my-room "serial-001" "Updated text" --headers \'{"source":"cli"}\'', + '$ ably rooms messages update my-room "serial-001" "Updated text" --json', + ]; + + static override flags = { + ...productApiFlags, + ...clientIdFlag, + description: Flags.string({ + description: "Description of the update operation", + }), + headers: Flags.string({ + description: "Additional headers for the message (JSON format)", + }), + metadata: Flags.string({ + description: "Additional metadata for the message (JSON format)", + }), + }; + + async run(): Promise { + const { args, flags } = await this.parse(MessagesUpdate); + + try { + // Parse and validate metadata before any client setup + let metadata: JsonObject | undefined; + if (flags.metadata !== undefined) { + const parsedMetadata = this.parseJsonFlag( + flags.metadata, + "metadata", + flags, + ); + if ( + typeof parsedMetadata !== "object" || + parsedMetadata === null || + Array.isArray(parsedMetadata) + ) { + this.fail( + "Metadata must be a JSON object", + flags, + "roomMessageUpdate", + ); + } + + metadata = parsedMetadata as JsonObject; + + this.logCliEvent( + flags, + "roomMessageUpdate", + "metadataParsed", + "Message metadata parsed", + { metadata }, + ); + } + + // Parse and validate headers before any client setup + let headers: Headers | undefined; + if (flags.headers !== undefined) { + const parsedHeaders = this.parseJsonFlag( + flags.headers, + "headers", + flags, + ); + if ( + typeof parsedHeaders !== "object" || + parsedHeaders === null || + Array.isArray(parsedHeaders) + ) { + this.fail( + "Headers must be a JSON object", + flags, + "roomMessageUpdate", + ); + } + + headers = parsedHeaders as Headers; + + this.logCliEvent( + flags, + "roomMessageUpdate", + "headersParsed", + "Message headers parsed", + { headers }, + ); + } + + const chatClient = await this.createChatClient(flags); + + if (!chatClient) { + return this.fail( + "Failed to create Chat client", + flags, + "roomMessageUpdate", + ); + } + + this.setupConnectionStateLogging(chatClient.realtime, flags); + + const room = await chatClient.rooms.get(args.room); + + if (!this.shouldOutputJson(flags)) { + this.log( + formatProgress( + "Updating message " + + formatResource(args.serial) + + " in room " + + formatResource(args.room), + ), + ); + } + + // Build update params + const updateParams: UpdateMessageParams = { + text: args.text, + ...(metadata && { metadata }), + ...(headers && { headers }), + }; + + // Build operation details + const details: OperationDetails | undefined = flags.description + ? { description: flags.description } + : undefined; + + this.logCliEvent( + flags, + "roomMessageUpdate", + "updating", + `Updating message ${args.serial} in room ${args.room}`, + { room: args.room, serial: args.serial }, + ); + + const result = await room.messages.update( + args.serial, + updateParams, + details, + ); + + this.logCliEvent( + flags, + "roomMessageUpdate", + "messageUpdated", + `Message ${args.serial} updated in room ${args.room}`, + { room: args.room, serial: args.serial }, + ); + + if (this.shouldOutputJson(flags)) { + this.logJsonResult( + { + room: args.room, + serial: args.serial, + updatedText: result.text, + versionSerial: result.version.serial, + }, + flags, + ); + } else { + this.log( + formatSuccess( + `Message ${formatResource(args.serial)} updated in room ${formatResource(args.room)}.`, + ), + ); + this.log(` Version serial: ${formatResource(result.version.serial)}`); + } + } catch (error) { + this.fail(error, flags, "roomMessageUpdate", { + room: args.room, + serial: args.serial, + }); + } + } +} diff --git a/src/commands/rooms/occupancy/get.ts b/src/commands/rooms/occupancy/get.ts index 4f1ff210..3f39a8ec 100644 --- a/src/commands/rooms/occupancy/get.ts +++ b/src/commands/rooms/occupancy/get.ts @@ -1,5 +1,5 @@ import { Args } from "@oclif/core"; -import { ChatClient, Room, OccupancyData } from "@ably/chat"; +import { ChatClient, Room } from "@ably/chat"; import { ChatBaseCommand } from "../../../chat-base-command.js"; import { clientIdFlag, productApiFlags } from "../../../flags.js"; import { formatResource } from "../../../utils/output.js"; @@ -37,40 +37,18 @@ export default class RoomsOccupancyGet extends ChatBaseCommand { this.chatClient = await this.createChatClient(flags); if (!this.chatClient) { - this.fail("Failed to create Chat client", flags, "roomOccupancyGet"); + return this.fail( + "Failed to create Chat client", + flags, + "roomOccupancyGet", + ); } const { room: roomName } = args; - // Get the room with occupancy enabled this.room = await this.chatClient.rooms.get(roomName); - // Attach to the room to access occupancy with timeout - let attachTimeout; - await Promise.race([ - this.room.attach(), - new Promise((_, reject) => { - attachTimeout = setTimeout( - () => reject(new Error("Room attach timeout")), - 10000, - ); - }), - ]); - - clearTimeout(attachTimeout); - - // Get occupancy metrics using the Chat SDK's occupancy API - let occupancyTimeout; - const occupancyMetrics = await Promise.race([ - this.room.occupancy.get(), - new Promise((_, reject) => { - occupancyTimeout = setTimeout( - () => reject(new Error("Occupancy get timeout")), - 5000, - ); - }), - ]); - clearTimeout(occupancyTimeout); + const occupancyMetrics = await this.room.occupancy.get(); // Output the occupancy metrics based on format if (this.shouldOutputJson(flags)) { diff --git a/src/commands/rooms/occupancy/subscribe.ts b/src/commands/rooms/occupancy/subscribe.ts index 4c65b315..3cbc77e4 100644 --- a/src/commands/rooms/occupancy/subscribe.ts +++ b/src/commands/rooms/occupancy/subscribe.ts @@ -61,7 +61,7 @@ export default class RoomsOccupancySubscribe extends ChatBaseCommand { this.chatClient = await this.createChatClient(flags); if (!this.chatClient) { - this.fail( + return this.fail( "Failed to create Chat client", flags, "roomOccupancySubscribe", diff --git a/src/commands/rooms/reactions/send.ts b/src/commands/rooms/reactions/send.ts index 16a43b32..6878b756 100644 --- a/src/commands/rooms/reactions/send.ts +++ b/src/commands/rooms/reactions/send.ts @@ -70,9 +70,14 @@ export default class RoomsReactionsSend extends ChatBaseCommand { this.chatClient = await this.createChatClient(flags); if (!this.chatClient) { - this.fail("Failed to create Chat client", flags, "roomReactionSend", { - room: roomName, - }); + return this.fail( + "Failed to create Chat client", + flags, + "roomReactionSend", + { + room: roomName, + }, + ); } // Set up connection state logging diff --git a/src/commands/rooms/reactions/subscribe.ts b/src/commands/rooms/reactions/subscribe.ts index e2f5bbe1..97f5d709 100644 --- a/src/commands/rooms/reactions/subscribe.ts +++ b/src/commands/rooms/reactions/subscribe.ts @@ -44,7 +44,7 @@ export default class RoomsReactionsSubscribe extends ChatBaseCommand { this.chatClient = await this.createChatClient(flags); if (!this.chatClient) { - this.fail( + return this.fail( "Failed to initialize clients", flags, "roomReactionSubscribe", diff --git a/src/commands/rooms/typing/keystroke.ts b/src/commands/rooms/typing/keystroke.ts index dba4ada5..d297c4d3 100644 --- a/src/commands/rooms/typing/keystroke.ts +++ b/src/commands/rooms/typing/keystroke.ts @@ -67,7 +67,11 @@ export default class TypingKeystroke extends ChatBaseCommand { // Create Chat client this.chatClient = await this.createChatClient(flags); if (!this.chatClient) { - this.fail("Failed to initialize clients", flags, "roomTypingKeystroke"); + return this.fail( + "Failed to initialize clients", + flags, + "roomTypingKeystroke", + ); } const { room: roomName } = args; diff --git a/src/commands/rooms/typing/subscribe.ts b/src/commands/rooms/typing/subscribe.ts index 82c49168..9d9a66c1 100644 --- a/src/commands/rooms/typing/subscribe.ts +++ b/src/commands/rooms/typing/subscribe.ts @@ -39,7 +39,11 @@ export default class TypingSubscribe extends ChatBaseCommand { // Create Chat client this.chatClient = await this.createChatClient(flags); if (!this.chatClient) { - this.fail("Failed to initialize clients", flags, "roomTypingSubscribe"); + return this.fail( + "Failed to initialize clients", + flags, + "roomTypingSubscribe", + ); } const { room: roomName } = args; diff --git a/test/helpers/mock-ably-chat.ts b/test/helpers/mock-ably-chat.ts index 803b753a..b4993f2f 100644 --- a/test/helpers/mock-ably-chat.ts +++ b/test/helpers/mock-ably-chat.ts @@ -55,6 +55,8 @@ export interface MockRoomMessages { subscribe: Mock; send: Mock; get: Mock; + update: Mock; + delete: Mock; reactions: MockMessageReactions; // Internal emitter for simulating events _emitter: AblyEventEmitter; @@ -236,6 +238,20 @@ function createMockRoomMessages(): MockRoomMessages { createdAt: Date.now(), }), get: vi.fn().mockResolvedValue({ items: [] }), + update: vi.fn().mockResolvedValue({ + serial: "mock-serial", + clientId: "mock-client-id", + text: "updated-text", + timestamp: new Date(), + version: { serial: "mock-version-serial", timestamp: new Date() }, + }), + delete: vi.fn().mockResolvedValue({ + serial: "mock-serial", + clientId: "mock-client-id", + text: "", + timestamp: new Date(), + version: { serial: "mock-version-serial", timestamp: new Date() }, + }), reactions: createMockMessageReactions(), _emitter: emitter, _emit: (message: Message) => { diff --git a/test/unit/commands/rooms/features.test.ts b/test/unit/commands/rooms/features.test.ts index 54aee47b..bc390651 100644 --- a/test/unit/commands/rooms/features.test.ts +++ b/test/unit/commands/rooms/features.test.ts @@ -22,7 +22,6 @@ describe("rooms feature commands", function () { import.meta.url, ); - expect(room.attach).toHaveBeenCalled(); expect(room.occupancy.get).toHaveBeenCalled(); expect(stdout).toContain("5"); }); @@ -187,7 +186,7 @@ describe("rooms feature commands", function () { const chatMock = getMockAblyChat(); const room = chatMock.rooms._getRoom("test-room"); - room.attach.mockRejectedValue(new Error("Connection failed")); + room.occupancy.get.mockRejectedValue(new Error("Connection failed")); const { error } = await runCommand( ["rooms:occupancy:get", "test-room"], diff --git a/test/unit/commands/rooms/messages.test.ts b/test/unit/commands/rooms/messages.test.ts index 6445aff3..9f23079e 100644 --- a/test/unit/commands/rooms/messages.test.ts +++ b/test/unit/commands/rooms/messages.test.ts @@ -23,7 +23,6 @@ describe("rooms messages commands", function () { import.meta.url, ); - expect(room.attach).toHaveBeenCalled(); expect(room.messages.send).toHaveBeenCalledWith({ text: "HelloWorld", }); @@ -382,7 +381,6 @@ describe("rooms messages commands", function () { import.meta.url, ); - expect(room.attach).toHaveBeenCalled(); expect(room.messages.history).toHaveBeenCalled(); expect(stdout).toContain("Historical message 1"); expect(stdout).toContain("Historical message 2"); diff --git a/test/unit/commands/rooms/messages/delete.test.ts b/test/unit/commands/rooms/messages/delete.test.ts new file mode 100644 index 00000000..e010a576 --- /dev/null +++ b/test/unit/commands/rooms/messages/delete.test.ts @@ -0,0 +1,171 @@ +import { describe, it, expect, beforeEach } from "vitest"; +import { runCommand } from "@oclif/test"; +import { getMockAblyChat } from "../../../../helpers/mock-ably-chat.js"; +import { captureJsonLogs } from "../../../../helpers/ndjson.js"; +import { + standardHelpTests, + standardArgValidationTests, + standardFlagTests, +} from "../../../../helpers/standard-tests.js"; + +describe("rooms:messages:delete command", () => { + beforeEach(() => { + getMockAblyChat(); + }); + + standardHelpTests("rooms:messages:delete", import.meta.url); + standardArgValidationTests("rooms:messages:delete", import.meta.url, { + requiredArgs: ["test-room", "serial-001"], + }); + standardFlagTests("rooms:messages:delete", import.meta.url, [ + "--json", + "--description", + ]); + + describe("functionality", () => { + it("should delete a message successfully", async () => { + const chatMock = getMockAblyChat(); + const room = chatMock.rooms._getRoom("test-room"); + + room.messages.delete.mockResolvedValue({ + serial: "serial-001", + clientId: "mock-client-id", + text: "", + timestamp: new Date(), + version: { serial: "version-serial-001", timestamp: new Date() }, + }); + + const { stdout } = await runCommand( + ["rooms:messages:delete", "test-room", "serial-001"], + import.meta.url, + ); + + expect(room.messages.delete).toHaveBeenCalledWith( + "serial-001", + undefined, + ); + expect(stdout).toContain("deleted"); + expect(stdout).toContain("test-room"); + }); + + it("should pass description as OperationDetails", async () => { + const chatMock = getMockAblyChat(); + const room = chatMock.rooms._getRoom("test-room"); + + room.messages.delete.mockResolvedValue({ + serial: "serial-001", + clientId: "mock-client-id", + text: "", + timestamp: new Date(), + version: { serial: "version-serial-001", timestamp: new Date() }, + }); + + await runCommand( + [ + "rooms:messages:delete", + "test-room", + "serial-001", + "--description", + "spam-removal", + ], + import.meta.url, + ); + + expect(room.messages.delete).toHaveBeenCalledWith("serial-001", { + description: "spam-removal", + }); + }); + + it("should not pass details when no description", async () => { + const chatMock = getMockAblyChat(); + const room = chatMock.rooms._getRoom("test-room"); + + room.messages.delete.mockResolvedValue({ + serial: "serial-001", + clientId: "mock-client-id", + text: "", + timestamp: new Date(), + version: { serial: "version-serial-001", timestamp: new Date() }, + }); + + await runCommand( + ["rooms:messages:delete", "test-room", "serial-001"], + import.meta.url, + ); + + expect(room.messages.delete).toHaveBeenCalledWith( + "serial-001", + undefined, + ); + }); + + it("should emit JSON envelope with --json", async () => { + const chatMock = getMockAblyChat(); + const room = chatMock.rooms._getRoom("test-room"); + + room.messages.delete.mockResolvedValue({ + serial: "serial-001", + clientId: "mock-client-id", + text: "", + timestamp: new Date(), + version: { serial: "version-serial-001", timestamp: new Date() }, + }); + + const records = await captureJsonLogs(async () => { + await runCommand( + ["rooms:messages:delete", "test-room", "serial-001", "--json"], + import.meta.url, + ); + }); + + const results = records.filter( + (r) => r.type === "result" && r.room === "test-room", + ); + expect(results.length).toBeGreaterThan(0); + const record = results[0]; + expect(record).toHaveProperty("type", "result"); + expect(record).toHaveProperty("command", "rooms:messages:delete"); + expect(record).toHaveProperty("success", true); + expect(record).toHaveProperty("room", "test-room"); + expect(record).toHaveProperty("serial", "serial-001"); + expect(record).toHaveProperty("versionSerial", "version-serial-001"); + }); + + it("should display version serial in human output", async () => { + const chatMock = getMockAblyChat(); + const room = chatMock.rooms._getRoom("test-room"); + + room.messages.delete.mockResolvedValue({ + serial: "serial-001", + clientId: "mock-client-id", + text: "", + timestamp: new Date(), + version: { serial: "version-serial-001", timestamp: new Date() }, + }); + + const { stdout } = await runCommand( + ["rooms:messages:delete", "test-room", "serial-001"], + import.meta.url, + ); + + expect(stdout).toContain("version-serial-001"); + }); + }); + + describe("error handling", () => { + it("should handle API error", async () => { + const chatMock = getMockAblyChat(); + const room = chatMock.rooms._getRoom("test-room"); + + room.messages.delete.mockRejectedValue(new Error("Delete failed")); + + const { error } = await runCommand( + ["rooms:messages:delete", "test-room", "serial-001"], + import.meta.url, + ); + + expect(error).toBeDefined(); + expect(error?.message).toContain("Delete failed"); + }); + }); +}); diff --git a/test/unit/commands/rooms/messages/history.test.ts b/test/unit/commands/rooms/messages/history.test.ts index b4e61d81..cc1b5542 100644 --- a/test/unit/commands/rooms/messages/history.test.ts +++ b/test/unit/commands/rooms/messages/history.test.ts @@ -27,7 +27,7 @@ describe("rooms:messages:history command", () => { ]); describe("functionality", () => { - it("should retrieve room message history", async () => { + it("should retrieve room message history with action and serial", async () => { const chatMock = getMockAblyChat(); const room = chatMock.rooms._getRoom("test-room"); @@ -38,12 +38,14 @@ describe("rooms:messages:history command", () => { clientId: "client1", timestamp: new Date(Date.now() - 10_000), serial: "msg-1", + action: "message.created", }, { text: "Historical message 2", clientId: "client2", timestamp: new Date(Date.now() - 5000), serial: "msg-2", + action: "message.updated", }, ], }); @@ -53,10 +55,14 @@ describe("rooms:messages:history command", () => { import.meta.url, ); - expect(room.attach).toHaveBeenCalled(); expect(room.messages.history).toHaveBeenCalled(); expect(stdout).toContain("Historical message 1"); expect(stdout).toContain("Historical message 2"); + expect(stdout).toContain("message.created"); + expect(stdout).toContain("message.updated"); + expect(stdout).toContain("Serial"); + expect(stdout).toContain("msg-1"); + expect(stdout).toContain("msg-2"); }); it("should display no messages found when history is empty", async () => { @@ -86,6 +92,7 @@ describe("rooms:messages:history command", () => { clientId: "client1", timestamp: new Date(), serial: "msg-m1", + action: "message.created", metadata: { priority: "high" }, }, ], @@ -100,7 +107,7 @@ describe("rooms:messages:history command", () => { expect(stdout).toContain("priority"); }); - it("should emit JSON envelope with type result for --json", async () => { + it("should emit JSON envelope with serial, action, and metadata", async () => { const chatMock = getMockAblyChat(); const room = chatMock.rooms._getRoom("test-room"); @@ -111,6 +118,8 @@ describe("rooms:messages:history command", () => { clientId: "client1", timestamp: new Date(Date.now() - 5000), serial: "msg-h1", + action: "message.created", + metadata: { key: "value" }, }, ], }); @@ -132,6 +141,10 @@ describe("rooms:messages:history command", () => { expect(record).toHaveProperty("success", true); expect(record).toHaveProperty("room", "test-room"); expect(record).toHaveProperty("messages"); + const messages = record.messages as Array>; + expect(messages[0]).toHaveProperty("serial", "msg-h1"); + expect(messages[0]).toHaveProperty("action", "message.created"); + expect(messages[0]).toHaveProperty("metadata", { key: "value" }); }); }); diff --git a/test/unit/commands/rooms/messages/send.test.ts b/test/unit/commands/rooms/messages/send.test.ts index e27edb85..bb288e36 100644 --- a/test/unit/commands/rooms/messages/send.test.ts +++ b/test/unit/commands/rooms/messages/send.test.ts @@ -39,7 +39,6 @@ describe("rooms:messages:send command", () => { import.meta.url, ); - expect(room.attach).toHaveBeenCalled(); expect(room.messages.send).toHaveBeenCalledWith({ text: "HelloWorld", }); diff --git a/test/unit/commands/rooms/messages/subscribe.test.ts b/test/unit/commands/rooms/messages/subscribe.test.ts index 9951ee33..691937b3 100644 --- a/test/unit/commands/rooms/messages/subscribe.test.ts +++ b/test/unit/commands/rooms/messages/subscribe.test.ts @@ -43,7 +43,7 @@ describe("rooms:messages:subscribe command", () => { expect(stdout).toContain("Subscribed to room"); }); - it("should display received messages", async () => { + it("should display received messages with action and serial", async () => { const chatMock = getMockAblyChat(); const room = chatMock.rooms._getRoom("test-room"); @@ -51,6 +51,7 @@ describe("rooms:messages:subscribe command", () => { (callback: (event: unknown) => void) => { setTimeout(() => { callback({ + type: "message.created", message: { text: "Hello from chat", clientId: "sender-client", @@ -70,6 +71,9 @@ describe("rooms:messages:subscribe command", () => { expect(stdout).toContain("sender-client"); expect(stdout).toContain("Hello from chat"); + expect(stdout).toContain("message.created"); + expect(stdout).toContain("Serial"); + expect(stdout).toContain("msg-123"); }); it("should display metadata when --show-metadata is passed", async () => { @@ -80,6 +84,7 @@ describe("rooms:messages:subscribe command", () => { (callback: (event: unknown) => void) => { setTimeout(() => { callback({ + type: "message.created", message: { text: "Msg with meta", clientId: "user1", @@ -145,6 +150,10 @@ describe("rooms:messages:subscribe command", () => { expect(record).toHaveProperty("type", "event"); expect(record).toHaveProperty("command", "rooms:messages:subscribe"); expect(record).toHaveProperty("room", "test-room"); + expect(record).toHaveProperty("eventType", "message.created"); + const msg = record.message as Record; + expect(msg).toHaveProperty("serial", "msg-json"); + expect(msg).toHaveProperty("action", "message.created"); }); }); diff --git a/test/unit/commands/rooms/messages/update.test.ts b/test/unit/commands/rooms/messages/update.test.ts new file mode 100644 index 00000000..85fa333e --- /dev/null +++ b/test/unit/commands/rooms/messages/update.test.ts @@ -0,0 +1,348 @@ +import { describe, it, expect, beforeEach } from "vitest"; +import { runCommand } from "@oclif/test"; +import { getMockAblyChat } from "../../../../helpers/mock-ably-chat.js"; +import { captureJsonLogs } from "../../../../helpers/ndjson.js"; +import { + standardHelpTests, + standardArgValidationTests, + standardFlagTests, +} from "../../../../helpers/standard-tests.js"; + +describe("rooms:messages:update command", () => { + beforeEach(() => { + getMockAblyChat(); + }); + + standardHelpTests("rooms:messages:update", import.meta.url); + standardArgValidationTests("rooms:messages:update", import.meta.url, { + requiredArgs: ["test-room", "serial-001", "updated-text"], + }); + standardFlagTests("rooms:messages:update", import.meta.url, [ + "--json", + "--metadata", + "--headers", + "--description", + ]); + + describe("functionality", () => { + it("should update a message successfully", async () => { + const chatMock = getMockAblyChat(); + const room = chatMock.rooms._getRoom("test-room"); + + room.messages.update.mockResolvedValue({ + serial: "serial-001", + clientId: "mock-client-id", + text: "updated-text", + timestamp: new Date(), + version: { serial: "version-serial-001", timestamp: new Date() }, + }); + + const { stdout } = await runCommand( + ["rooms:messages:update", "test-room", "serial-001", "updated-text"], + import.meta.url, + ); + + expect(room.messages.update).toHaveBeenCalledWith( + "serial-001", + { text: "updated-text" }, + undefined, + ); + expect(stdout).toContain("updated"); + expect(stdout).toContain("test-room"); + }); + + it("should pass metadata when --metadata provided", async () => { + const chatMock = getMockAblyChat(); + const room = chatMock.rooms._getRoom("test-room"); + + room.messages.update.mockResolvedValue({ + serial: "serial-001", + clientId: "mock-client-id", + text: "updated-text", + timestamp: new Date(), + version: { serial: "version-serial-001", timestamp: new Date() }, + }); + + await runCommand( + [ + "rooms:messages:update", + "test-room", + "serial-001", + "updated-text", + "--metadata", + '{"edited":true}', + ], + import.meta.url, + ); + + expect(room.messages.update).toHaveBeenCalledWith( + "serial-001", + { text: "updated-text", metadata: { edited: true } }, + undefined, + ); + }); + + it("should pass headers when --headers provided", async () => { + const chatMock = getMockAblyChat(); + const room = chatMock.rooms._getRoom("test-room"); + + room.messages.update.mockResolvedValue({ + serial: "serial-001", + clientId: "mock-client-id", + text: "updated-text", + timestamp: new Date(), + version: { serial: "version-serial-001", timestamp: new Date() }, + }); + + await runCommand( + [ + "rooms:messages:update", + "test-room", + "serial-001", + "updated-text", + "--headers", + '{"source":"cli"}', + ], + import.meta.url, + ); + + expect(room.messages.update).toHaveBeenCalledWith( + "serial-001", + { text: "updated-text", headers: { source: "cli" } }, + undefined, + ); + }); + + it("should pass description as OperationDetails", async () => { + const chatMock = getMockAblyChat(); + const room = chatMock.rooms._getRoom("test-room"); + + room.messages.update.mockResolvedValue({ + serial: "serial-001", + clientId: "mock-client-id", + text: "updated-text", + timestamp: new Date(), + version: { serial: "version-serial-001", timestamp: new Date() }, + }); + + await runCommand( + [ + "rooms:messages:update", + "test-room", + "serial-001", + "updated-text", + "--description", + "typo-fix", + ], + import.meta.url, + ); + + expect(room.messages.update).toHaveBeenCalledWith( + "serial-001", + { text: "updated-text" }, + { description: "typo-fix" }, + ); + }); + + it("should not pass details when no description", async () => { + const chatMock = getMockAblyChat(); + const room = chatMock.rooms._getRoom("test-room"); + + room.messages.update.mockResolvedValue({ + serial: "serial-001", + clientId: "mock-client-id", + text: "updated-text", + timestamp: new Date(), + version: { serial: "version-serial-001", timestamp: new Date() }, + }); + + await runCommand( + ["rooms:messages:update", "test-room", "serial-001", "updated-text"], + import.meta.url, + ); + + expect(room.messages.update).toHaveBeenCalledWith( + "serial-001", + { text: "updated-text" }, + undefined, + ); + }); + + it("should emit JSON envelope with --json", async () => { + const chatMock = getMockAblyChat(); + const room = chatMock.rooms._getRoom("test-room"); + + room.messages.update.mockResolvedValue({ + serial: "serial-001", + clientId: "mock-client-id", + text: "updated-text", + timestamp: new Date(), + version: { serial: "version-serial-001", timestamp: new Date() }, + }); + + const records = await captureJsonLogs(async () => { + await runCommand( + [ + "rooms:messages:update", + "test-room", + "serial-001", + "updated-text", + "--json", + ], + import.meta.url, + ); + }); + + const results = records.filter( + (r) => r.type === "result" && r.room === "test-room", + ); + expect(results.length).toBeGreaterThan(0); + const record = results[0]; + expect(record).toHaveProperty("type", "result"); + expect(record).toHaveProperty("command", "rooms:messages:update"); + expect(record).toHaveProperty("success", true); + expect(record).toHaveProperty("room", "test-room"); + expect(record).toHaveProperty("serial", "serial-001"); + expect(record).toHaveProperty("versionSerial", "version-serial-001"); + }); + + it("should display version serial in human output", async () => { + const chatMock = getMockAblyChat(); + const room = chatMock.rooms._getRoom("test-room"); + + room.messages.update.mockResolvedValue({ + serial: "serial-001", + clientId: "mock-client-id", + text: "updated-text", + timestamp: new Date(), + version: { serial: "version-serial-001", timestamp: new Date() }, + }); + + const { stdout } = await runCommand( + ["rooms:messages:update", "test-room", "serial-001", "updated-text"], + import.meta.url, + ); + + expect(stdout).toContain("version-serial-001"); + }); + }); + + describe("error handling", () => { + it("should handle invalid metadata JSON", async () => { + const { error } = await runCommand( + [ + "rooms:messages:update", + "test-room", + "serial-001", + "updated-text", + "--metadata", + "invalid-json", + ], + import.meta.url, + ); + + expect(error).toBeDefined(); + expect(error?.message).toMatch(/Invalid metadata JSON/i); + }); + + it("should reject non-object metadata", async () => { + const { error } = await runCommand( + [ + "rooms:messages:update", + "test-room", + "serial-001", + "updated-text", + "--metadata", + "42", + ], + import.meta.url, + ); + + expect(error).toBeDefined(); + expect(error?.message).toMatch(/Metadata must be a JSON object/i); + }); + + it("should reject array metadata", async () => { + const { error } = await runCommand( + [ + "rooms:messages:update", + "test-room", + "serial-001", + "updated-text", + "--metadata", + "[1,2,3]", + ], + import.meta.url, + ); + + expect(error).toBeDefined(); + expect(error?.message).toMatch(/Metadata must be a JSON object/i); + }); + + it("should handle invalid headers JSON", async () => { + const { error } = await runCommand( + [ + "rooms:messages:update", + "test-room", + "serial-001", + "updated-text", + "--headers", + "invalid-json", + ], + import.meta.url, + ); + + expect(error).toBeDefined(); + expect(error?.message).toMatch(/Invalid headers JSON/i); + }); + + it("should reject non-object headers", async () => { + const { error } = await runCommand( + [ + "rooms:messages:update", + "test-room", + "serial-001", + "updated-text", + "--headers", + "42", + ], + import.meta.url, + ); + + expect(error).toBeDefined(); + expect(error?.message).toMatch(/Headers must be a JSON object/i); + }); + + it("should reject array headers", async () => { + const { error } = await runCommand( + [ + "rooms:messages:update", + "test-room", + "serial-001", + "updated-text", + "--headers", + "[1,2,3]", + ], + import.meta.url, + ); + + expect(error).toBeDefined(); + expect(error?.message).toMatch(/Headers must be a JSON object/i); + }); + + it("should handle API error", async () => { + const chatMock = getMockAblyChat(); + const room = chatMock.rooms._getRoom("test-room"); + + room.messages.update.mockRejectedValue(new Error("Update failed")); + + const { error } = await runCommand( + ["rooms:messages:update", "test-room", "serial-001", "updated-text"], + import.meta.url, + ); + + expect(error).toBeDefined(); + expect(error?.message).toContain("Update failed"); + }); + }); +}); diff --git a/test/unit/commands/rooms/occupancy/get.test.ts b/test/unit/commands/rooms/occupancy/get.test.ts index f3d456e0..f26027dc 100644 --- a/test/unit/commands/rooms/occupancy/get.test.ts +++ b/test/unit/commands/rooms/occupancy/get.test.ts @@ -32,7 +32,6 @@ describe("rooms:occupancy:get command", () => { import.meta.url, ); - expect(room.attach).toHaveBeenCalled(); expect(room.occupancy.get).toHaveBeenCalled(); expect(stdout).toContain("Connections: 5"); expect(stdout).toContain("Presence Members: 3"); @@ -79,9 +78,7 @@ describe("rooms:occupancy:get command", () => { it("should output JSON error on failure", async () => { const chatMock = getMockAblyChat(); const room = chatMock.rooms._getRoom("test-room"); - room.attach.mockImplementation(async () => { - throw new Error("Room attach timeout"); - }); + room.occupancy.get.mockRejectedValue(new Error("Service unavailable")); const { stdout } = await runCommand( ["rooms:occupancy:get", "test-room", "--json"],