chat: expose underlying types more clearly in output #92
Conversation
When doing JSON representation, use the underlying Chat types rather than CLI wrappers.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughUpdates adjust message history to output raw items, change message send to return the sent Message object (including serial), align subscription types, broaden occupancy typing to OccupancyData with switch case breaks, and refactor presence enter/subscribe to emit a new eventData shape encapsulating the full event plus roomId, timestamp, and success. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant CLI as CLI (rooms/messages/send)
participant RC as RoomClient
participant S as Chat Service
U->>CLI: run send with payload
CLI->>RC: room.messages.send(messageToSend)
RC->>S: send(payload)
S-->>RC: Message (includes serial)
RC-->>CLI: Message
CLI-->>U: Output (JSON or text) using returned Message (serial logged in text)
sequenceDiagram
autonumber
participant U as User
participant CLI as CLI (rooms/messages/history)
participant RC as RoomClient
U->>CLI: request history (limit)
CLI->>RC: room.messages.history({ limit })
RC-->>CLI: { items: [Message...] }
CLI-->>U: JSON with messages = raw items (no per-item mapping)
sequenceDiagram
autonumber
participant S as Presence Stream
participant CLI as CLI (presence subscribe/enter)
participant U as User
S-->>CLI: Presence event
CLI->>CLI: Build eventData { event, roomId, timestamp, success }
CLI-->>U: Log/JSON using eventData (text logs still show member details)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
AndyTWF
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
src/commands/rooms/messages/subscribe.ts (5)
3-10: Remove unused import and prefer enum for status.
- Room is unused; drop it to satisfy ESLint.
- While here, import RoomStatus to avoid stringly-typed status checks below.
import { ChatClient, Subscription, StatusSubscription, ChatMessageEvent, - Room, + RoomStatus, RoomStatusChange, } from "@ably/chat";
241-272: Use RoomStatus enum instead of string literals.Avoids typos and aligns with other files using RoomStatus.
- if (statusChange.current === "attached") { + if (statusChange.current === RoomStatus.Attached) { this.logCliEvent( flags, "room", "statusAttached", "Room status is ATTACHED.", ); ... - } else if (statusChange.current === "failed") { + } else if (statusChange.current === RoomStatus.Failed) { const errorMsg = room.error?.message || "Unknown error";
62-66: Prefer CLI logging over console.warn in cleanup timeout.Keeps logs consistent and structured.
- console.warn("Ably client cleanup timed out after 2 seconds"); + this.logCliEvent({}, "connection", "cleanupTimeout", "Ably client cleanup timed out after 2 seconds");
306-312: Guard against non-positive/NaN duration from env.Match waitUntilInterruptedOrTimeout’s semantics and avoid passing NaN.
- const effectiveDuration = - typeof flags.duration === "number" && flags.duration > 0 - ? flags.duration - : process.env.ABLY_CLI_DEFAULT_DURATION - ? Number(process.env.ABLY_CLI_DEFAULT_DURATION) - : undefined; + const envDuration = process.env.ABLY_CLI_DEFAULT_DURATION + ? Number(process.env.ABLY_CLI_DEFAULT_DURATION) + : undefined; + const effectiveDuration = + typeof flags.duration === "number" && flags.duration > 0 + ? flags.duration + : envDuration && envDuration > 0 + ? envDuration + : undefined;
114-118: Align forced-exit gating with ABLY_CLI_TEST_MODE (and consider removing).You already trigger an exit on timeout inside waitUntilInterruptedOrTimeout; this extra exit can duplicate behavior. If you keep it, gate consistently on ABLY_CLI_TEST_MODE.
- if (process.env.NODE_ENV !== "test") { + if (process.env.ABLY_CLI_TEST_MODE !== "true") { setTimeout(() => { process.exit(0); }, 100); }src/commands/rooms/presence/subscribe.ts (1)
251-257: Guard env duration like other commands/util.Prevents NaN/zero durations slipping through.
- const effectiveDuration = - typeof flags.duration === "number" && flags.duration > 0 - ? flags.duration - : process.env.ABLY_CLI_DEFAULT_DURATION - ? Number(process.env.ABLY_CLI_DEFAULT_DURATION) - : undefined; + const envDuration = process.env.ABLY_CLI_DEFAULT_DURATION + ? Number(process.env.ABLY_CLI_DEFAULT_DURATION) + : undefined; + const effectiveDuration = + typeof flags.duration === "number" && flags.duration > 0 + ? flags.duration + : envDuration && envDuration > 0 + ? envDuration + : undefined;src/commands/rooms/presence/enter.ts (1)
193-198: Guard env duration consistently.Same rationale as other commands.
- const effectiveDuration = - typeof flags.duration === "number" && flags.duration > 0 - ? flags.duration - : process.env.ABLY_CLI_DEFAULT_DURATION - ? Number(process.env.ABLY_CLI_DEFAULT_DURATION) - : undefined; + const envDuration = process.env.ABLY_CLI_DEFAULT_DURATION + ? Number(process.env.ABLY_CLI_DEFAULT_DURATION) + : undefined; + const effectiveDuration = + typeof flags.duration === "number" && flags.duration > 0 + ? flags.duration + : envDuration && envDuration > 0 + ? envDuration + : undefined;src/commands/rooms/occupancy/subscribe.ts (3)
43-64: Toughen cleanup: handle failed state and avoid console.warn.
- Also skip close when state is failed (consistent with other files).
- Prefer this.warn or logCliEvent over console.warn.
- private async properlyCloseAblyClient(): Promise<void> { - if (!this.ablyClient || this.ablyClient.connection.state === "closed") { + private async properlyCloseAblyClient(): Promise<void> { + if ( + !this.ablyClient || + this.ablyClient.connection.state === "closed" || + this.ablyClient.connection.state === "failed" + ) { return; } @@ - const timeout = setTimeout(() => { - console.warn("Ably client cleanup timed out after 3 seconds"); + const timeout = setTimeout(() => { + this.warn("Ably client cleanup timed out after 3 seconds"); resolve(); }, 3000); @@ - this.ablyClient!.connection.once("closed", onClosed); - this.ablyClient!.connection.once("failed", onClosed); + this.ablyClient!.connection.once("closed", onClosed); + this.ablyClient!.connection.once("failed", onClosed);
453-465: Prefer single cleanup path; reuse properlyCloseAblyClient.You close here and again in the class-level finally(). Consider centralizing to avoid duplicate handling.
- if (this.ablyClient && this.ablyClient.connection.state !== "closed") { - this.logCliEvent( - flags || {}, - "connection", - "finalCloseAttempt", - "Ensuring connection is closed in finally block.", - ); - this.ablyClient.connection.off(); - this.ablyClient.close(); - } + await this.properlyCloseAblyClient();
469-473: Narrow displayOccupancyMetrics to OccupancyData.You only pass OccupancyData; simplifying the type and body reduces branching.
- private displayOccupancyMetrics( - occupancyMetrics: OccupancyData | OccupancyEvent, + private displayOccupancyMetrics( + occupancyMetrics: OccupancyData, roomId: string | null, flags: Record<string, unknown>, isInitial = false, ): void { @@ - // Type guard to handle both OccupancyData and OccupancyEvent - const connections = - "connections" in occupancyMetrics ? occupancyMetrics.connections : 0; - const presenceMembers = - "presenceMembers" in occupancyMetrics - ? occupancyMetrics.presenceMembers - : undefined; + const { connections = 0, presenceMembers } = occupancyMetrics as OccupancyData;Also applies to: 497-504
src/commands/rooms/messages/send.ts (2)
461-474: Avoid double room release (run() and finally()).You release in run() and again in finally(). Centralize in finally() to keep behavior consistent and avoid redundant calls.
Outside the shown range, consider removing the release block in run() and rely solely on the finally() override for releasing/closing.
8-12: Optional: reuse SDK type for message-to-send (if exported).If @ably/chat exports a MessageToSend type, prefer importing it instead of redefining.
Would you like me to check the SDK typings and update accordingly?
Also applies to: 14-21
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
src/commands/rooms/messages/history.ts(1 hunks)src/commands/rooms/messages/send.ts(5 hunks)src/commands/rooms/messages/subscribe.ts(15 hunks)src/commands/rooms/occupancy/subscribe.ts(9 hunks)src/commands/rooms/presence/enter.ts(1 hunks)src/commands/rooms/presence/subscribe.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/commands/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/AI-Assistance.mdc)
src/commands/**/*.ts: Commands should follow the oclif structure: export default class MyCommand extends Command { ... }
Command should support auth via the standard auth helper (import { getAuth } from '../../helpers/auth') and use it in the run method.
Use try/catch for error handling in commands, and provide user-friendly error messages (e.g., handle error.code === 40100 for authentication failures).
Do not use direct HTTP requests (e.g., fetch) for data plane APIs; use the Ably SDK instead.
Do not use hardcoded endpoint URLs; use configuration values (e.g., flags.controlHost, config.controlHost) when constructing URLs.
Do not use non-standard command structures (e.g., export default class { async execute(args) { ... } }); always use the oclif Command class.
Files:
src/commands/rooms/messages/subscribe.tssrc/commands/rooms/presence/enter.tssrc/commands/rooms/messages/history.tssrc/commands/rooms/messages/send.tssrc/commands/rooms/occupancy/subscribe.tssrc/commands/rooms/presence/subscribe.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/Development.mdc)
**/*.{ts,tsx}: Use TypeScript and follow best practice naming conventions
ESLint is used to ensure all code adheres best practices; ensure you check that all code passes the linter before concluding your work.
Files:
src/commands/rooms/messages/subscribe.tssrc/commands/rooms/presence/enter.tssrc/commands/rooms/messages/history.tssrc/commands/rooms/messages/send.tssrc/commands/rooms/occupancy/subscribe.tssrc/commands/rooms/presence/subscribe.ts
src/commands/**/*.{ts,js}
📄 CodeRabbit inference engine (.cursor/rules/Development.mdc)
Follow oclif framework best practices as described in the oclif documentation.
Files:
src/commands/rooms/messages/subscribe.tssrc/commands/rooms/presence/enter.tssrc/commands/rooms/messages/history.tssrc/commands/rooms/messages/send.tssrc/commands/rooms/occupancy/subscribe.tssrc/commands/rooms/presence/subscribe.ts
🧬 Code graph analysis (1)
src/commands/rooms/messages/subscribe.ts (1)
src/utils/long-running.ts (1)
waitUntilInterruptedOrTimeout(11-77)
🪛 GitHub Check: setup
src/commands/rooms/messages/subscribe.ts
[warning] 8-8:
'Room' is defined but never used. Allowed unused vars must match /^_/u
🪛 GitHub Check: test
src/commands/rooms/messages/subscribe.ts
[warning] 8-8:
'Room' is defined but never used. Allowed unused vars must match /^_/u
🪛 GitHub Check: e2e-cli
src/commands/rooms/messages/subscribe.ts
[warning] 8-8:
'Room' is defined but never used. Allowed unused vars must match /^_/u
🔇 Additional comments (7)
src/commands/rooms/messages/history.ts (2)
83-86: LGTM: Param object style for history call.
92-96: Expose raw Message items in JSON output (breaking shape).Good move toward underlying types. Flagging that this changes JSON shape; ensure downstream consumers/docs are updated.
src/commands/rooms/presence/subscribe.ts (1)
178-242: Event payload now surfaces the full PresenceEvent — thumbs up.Consistent with PR goal to expose underlying types; JSON/non-JSON paths look correct.
src/commands/rooms/presence/enter.ts (1)
145-160: EventData shape: good alignment with subscribe.ts.Emitting the full event meets the “expose underlying types” objective.
src/commands/rooms/occupancy/subscribe.ts (2)
173-203: Nice: added breaks to prevent switch fall‑through.
250-256: Initial occupancy display path looks correct.Passes OccupancyData, matches new typing.
src/commands/rooms/messages/send.ts (1)
3-3: Great: return the actual sent Message and surface serial.
- Importing Message and threading the sent object through results meets CHA-1163.
- Non-JSON path now prints the serial — perfect.
Please confirm the @ably/chat version in this repo guarantees that room.messages.send returns a Message (not void). If older versions exist in supported environments, we may need a defensive fallback.
Also applies to: 16-16, 292-297, 416-420, 435-435
This change makes it so that underlying Chat types are exposed (especially when in JSON mode). This means that we don't hide important information / have to update things every time a Chat update is made.
Also updated a small number of non-JSON areas (e.g. having the serial returned on message send).
CHA-1163
Summary by CodeRabbit
New Features
Refactor