upgrade ably-chat-js#18
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis update revises how room handles are retrieved from the chat client by removing explicit options for reactions and typing features across several commands. The typing heartbeat logic is updated for increased frequency and reliability. The dependency "@ably/chat" is upgraded, and a minor test import refactor is made. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI_Command
participant ChatClient
participant Room
User->>CLI_Command: Start typing in room
CLI_Command->>ChatClient: rooms.get(roomId)
ChatClient->>Room: Return Room instance (no options)
CLI_Command->>Room: typing.keystroke() (every 450ms)
Note over CLI_Command,Room: Typing heartbeat maintained
User->>CLI_Command: Interrupt (SIGINT)
CLI_Command->>ChatClient: Cleanup and disconnect
Possibly related PRs
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
src/commands/rooms/typing/start.ts (1)
150-178: 🛠️ Refactor suggestionUnhandled keystroke failures may spam the log
room.typing.keystroke()runs every interval but failures are only logged; the timer keeps firing, so persistent errors will fill logs quickly.Consider:
- Backing off (exponential or fixed) after N consecutive failures.
- Clearing the interval on unrecoverable errors to reduce noise.
-let failureStreak = 0; -setInterval(() => { - room.typing.keystroke().catch((error: Error) => { - failureStreak++; - this.logCliEvent(flags, "typing", "keystrokeError", - `keystroke() failed (${failureStreak}x): ${error.message}`); - if (failureStreak >= 3) { - clearInterval(this.typingIntervalId!); - this.typingIntervalId = null; - } - }); -}, KEYSTROKE_INTERVAL); +let failureStreak = 0; +this.typingIntervalId = setInterval(async () => { + try { + await room.typing.keystroke(); + failureStreak = 0; // reset on success + } catch (err) { + failureStreak++; + this.logCliEvent( + flags, + "typing", + "keystrokeError", + `keystroke() failed (${failureStreak}×): ${ + err instanceof Error ? err.message : String(err) + }`, + ); + if (failureStreak >= 3) { + clearInterval(this.typingIntervalId!); + this.typingIntervalId = null; + } + } +}, KEYSTROKE_INTERVAL);
🧹 Nitpick comments (2)
src/commands/rooms/typing/start.ts (2)
8-16: Keystroke interval of 450 ms is extremely aggressiveAt ~2 Hz every client will publish ~133 events/minute while typing, which may:
- Increase mobile battery/network consumption
- Trigger rate-limits on free accounts (20 msgs/s hard-limit)
- Generate unnecessary load on Ably
Ably’s docs recommend ≤1 heartbeat every ~8–10 s. Consider a safer value (e.g. 2 000 ms) or make it user-configurable.
-const KEYSTROKE_INTERVAL = 450; // ms +// Default to 2 s; override with ABLY_KEYSTROKE_MS or a --interval flag if needed +const KEYSTROKE_INTERVAL = + Number(process.env.ABLY_KEYSTROKE_MS) > 0 + ? Number(process.env.ABLY_KEYSTROKE_MS) + : 2_000;
227-357:process.exit()will bypass oclif’sfinally()– double-check shutdown sequenceInside the SIGINT handler you:
- Call
resolve()- Immediately call
process.exit(0)Because
process.exitaborts the event-loop, thefinally()of this command (and parentChatBaseCommand) will never execute, potentially:
- Losing final telemetry or performance traces
- Leaking resources if future cleanup is added only in
finally()Consider removing
process.exit(0)and let the promise resolve naturally, or move all mandatory cleanup tofinally().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
package.json(1 hunks)src/commands/rooms/reactions/send.ts(1 hunks)src/commands/rooms/reactions/subscribe.ts(1 hunks)src/commands/rooms/typing/start.ts(7 hunks)src/commands/rooms/typing/subscribe.ts(2 hunks)test/unit/services/config-manager.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e-tests
- GitHub Check: test
- GitHub Check: e2e-tests
- GitHub Check: test
🔇 Additional comments (7)
package.json (1)
70-70:Details
❓ Verification inconclusive
Verified the @ably/chat dependency upgrade
This upgrade from "^0.5.1" to "^0.7.0" aligns with the changes in room handling across the codebase, removing the need for explicit feature option objects.
🏁 Script executed:
#!/bin/bash # Check for breaking changes or notable updates in the Ably Chat changelog between 0.5.1 and 0.7.0 # Look for the repository or changelog curl -s https://api.github.com/repos/ably/ably-chat-js/releases | grep -E "tag_name|body" | grep -A1 "v0.[5-7]" || echo "Could not fetch releases" # Alternative - check NPM for version information echo -e "\nNPM version information:" npm view @ably/chat versions --json | grep -E "0\.[5-7]\.[0-9]"Length of output: 740
#!/bin/bash
Retrieve the repository URL for @ably/chat to locate its changelog
npm view @ably/chat repository --json
#!/bin/bash
Fetch the changelog section for v0.7.0 of ably-chat-js to verify room handling & feature options notes
curl -s https://raw.githubusercontent.com/ably/ably-chat-js/main/CHANGELOG.md
| sed -n '/## [0.7.0]/,/## [0./[!]/p'
#!/bin/bash
Fetch the official release notes for v0.7.0 of ably-chat-js to confirm room handling and feature options changes
curl -s https://api.github.com/repos/ably/ably-chat-js/releases/tags/v0.7.0 | jq -r '.body'
Verify @ably/chat v0.7.0 Room Handling Changes
I wasn’t able to locate the official changelog or release notes for v0.7.0. Please double-check Ably Chat’s CHANGELOG.md or GitHub releases to confirm:
- v0.7.0 explicitly removes the need for feature-option objects in room handling
- There are no additional breaking changes affecting how rooms are created or joined
test/unit/services/config-manager.test.ts (1)
11-12: Improved Chai import patternThe refactored import improves clarity by separating the named import
expectfrom the namespace import of Chai.src/commands/rooms/reactions/subscribe.ts (1)
112-112:Details
✅ Verification successful
Simplified room retrieval by removing explicit empty reactions option
The empty reactions options object is no longer needed with the updated
@ably/chatlibrary. This simplifies the API usage and makes the code more maintainable.
🏁 Script executed:
#!/bin/bash # Check if similar changes were applied consistently across other reaction-related files echo "Checking other reaction files for similar options removal:" grep -r "rooms.get(.*reactions" src/commands/rooms/reactions/ || echo "No remaining rooms.get calls with reactions options found"Length of output: 315
Simplified room retrieval: removed explicit empty reactions option
The explicit empty reactions option is no longer required with the latest @ably/chat update, making the code cleaner and more maintainable.
• Verified in src/commands/rooms/reactions/subscribe.ts (line 112)
• Confirmed no otherrooms.getcalls with reactions options remain in src/commands/rooms/reactions/src/commands/rooms/typing/subscribe.ts (2)
101-101: Simplified room retrieval by removing typing configuration optionsThe typing timeout configuration was removed, now relying on the defaults provided by the updated library. This simplifies the API usage and aligns with the removal of explicit options in other commands.
167-167:Details
✅ Verification successful
Removed explicit typing for event parameter
Removed the
TypingEventtype annotation, now relying on TypeScript's type inference. This change works because the types are properly defined in the@ably/chatlibrary.
🏁 Script executed:
#!/bin/bash # Check if the TypingEvent import was also removed from the imports at the top of the file echo "Checking if TypingEvent import was removed from the top:" head -10 src/commands/rooms/typing/subscribe.ts | grep -o "TypingEvent" || echo "TypingEvent import was correctly removed" echo -e "\nChecking if similar changes were applied consistently in other typing-related files:" grep -r "rooms.get(.*typing" src/commands/rooms/typing/ || echo "No remaining rooms.get calls with typing options found"Length of output: 647
✅ TypingEvent Annotation Removal Verified
Removed the explicit
TypingEventannotation and its import insrc/commands/rooms/typing/subscribe.ts, and confirmed there are no remainingrooms.get(…typing…)calls in other typing-related files. Relying on TypeScript’s inference is safe given the types defined in@ably/chat.src/commands/rooms/reactions/send.ts (1)
133-134:Details
❓ Verification inconclusive
Verify reactions feature is still enabled after dropping explicit options
rooms.get(roomId)is now called without{ reactions: {} }.
In@ably/chat ≥ 0.7.0reactions are enabled by default, but if anyone is still on an earlier minor version (or forces the feature-flags off), this could silently break reactions.
🏁 Script executed:
#!/bin/bash # Find remaining calls that still pass the old reactions option rg --line-number --context 2 $'rooms\\.get\\([^)]*reactions' || echo "✅ no legacy calls found" # Quickly smoke-test that sending a reaction still works (requires network): node - <<'JS' import { createClient } from '@ably/chat'; (async () => { const client = await createClient({ apiKey: process.env.ABLY_API_KEY }); const room = await client.rooms.get('test-room'); await room.attach(); await room.reactions.send({ type: '👍' }); console.log('reaction sent'); await client.close(); })(); JSLength of output: 1132
Verify reactions feature remains enabled without explicit options
We’ve confirmed there are no remaining
rooms.get(..., { reactions: … })calls. To ensure reactions still work:
- Confirm your
@ably/chatdependency is pinned to version ≥0.7.0 (reactions on by default).- Manually smoke-test sending a reaction in an environment where
@ably/chatis installed:# Check @ably/chat version version=$(node -p 'require("./package.json").dependencies["@ably/chat"]||require("./package.json").devDependencies["@ably/chat"]') echo "@ably/chat@$version" # Smoke-test reaction sending node - <<'JS' const { createClient } = require('@ably/chat'); (async () => { const client = await createClient({ apiKey: process.env.ABLY_API_KEY }); const room = await client.rooms.get('test-room'); await room.attach(); await room.reactions.send({ type: '👍' }); console.log('✅ Reaction sent'); await client.close(); })(); JSsrc/commands/rooms/typing/start.ts (1)
341-343: Good call: unsubscribing connection listeners before closing the clientPreventing lingering event handlers avoids memory leaks when this module is embedded in longer-lived processes.
splindsay-92
force-pushed
the
upgrade-chat-js
branch
2 times, most recently
from
1b2d02d to
f4814a9
Compare
- Also includes a couple of fixes to properly terminate the process.
splindsay-92
force-pushed
the
upgrade-chat-js
branch
from
f4814a9 to
8f810ff
Compare
| ); | ||
| this.log( | ||
| " ably rooms typing start - Start typing in a chat room", | ||
| " ably rooms typing keystroke - Start typing in a chat room", |
There was a problem hiding this comment.
Looking at the impl of keystroke, we call it on a timer - is having the name keystroke a bit misleading, as it's not a single keystroke?
There was a problem hiding this comment.
What about having an additional boolean arg, autotype, to control that behaviour? I think for the CLI users are likely to want some way of maintaining the indicator, so this will give us that while making things clear. :)
There was a problem hiding this comment.
start might be a better word but I like keystroke because it matches the method name.
I agree with @splindsay-92 with some flag to control whether you want it on a loop or not. I'd default it to loop as it is now.
Surprised this is needed given the recent PR #18
Surprised this is needed given the recent PR #18
Surprised this is needed given the recent PR #18
Surprised this is needed given the recent PR #18
Surprised this is needed given the recent PR #18
4 participants
Summary by CodeRabbit