Draft
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe pull request converts synchronous subscription calls to awaited asynchronous operations across multiple CLI commands, introduces a helper function for friendly Ably error hints, redirects SDK error logs to stderr, and appends contextual error messages to improve user experience with error information. Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
umair-ably
force-pushed
the
DX-255-error-codes
branch
from
4aee569 to
e9fc4da
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces friendlier, actionable messaging for known Ably error codes (notably capability/auth failures) and adds unit coverage to verify the updated error output across subscribe-style commands.
Changes:
- Added
getFriendlyAblyErrorHint()and appended its hint output tofail()formatting. - Updated multiple subscribe commands to
awaitsubscription setup and added new unit tests for capability errors. - Adjusted SDK error logging behavior in non-verbose mode to avoid polluting stdout.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
src/utils/errors.ts |
Adds Ably error-code → friendly-hint lookup helper. |
src/base-command.ts |
Appends friendly hint to human-readable errors; tweaks SDK error logging destination. |
src/commands/channels/subscribe.ts |
Refactors channel subscribe flow to collect/await subscription “promises”. |
src/commands/channels/presence/subscribe.ts |
Awaits presence subscription setup. |
src/commands/channels/presence/enter.ts |
Awaits presence subscription setup when --show-others is enabled. |
src/commands/channels/occupancy/subscribe.ts |
Awaits occupancy subscription setup. |
src/commands/logs/subscribe.ts |
Collects and awaits log-type subscriptions. |
src/commands/logs/push/subscribe.ts |
Awaits push log channel subscribe. |
src/commands/logs/connection-lifecycle/subscribe.ts |
Awaits connection lifecycle subscribe. |
src/commands/logs/channel-lifecycle/subscribe.ts |
Awaits channel lifecycle subscribe. |
test/unit/utils/errors.test.ts |
Adds unit tests for error message extraction and hint lookup. |
test/unit/commands/channels/subscribe.test.ts |
Adds capability-error test coverage for channel subscribe. |
test/unit/commands/channels/presence/subscribe.test.ts |
Adds capability-error test coverage for presence subscribe. |
test/unit/commands/channels/occupancy/subscribe.test.ts |
Adds capability-error test coverage for occupancy subscribe. |
Comments suppressed due to low confidence (4)
src/commands/logs/push/subscribe.ts:92
await channel.subscribe(...)here is unlikely to be a reliable way to detect attach/capability/auth failures (the repo's Ably realtime mocksubscribereturns void). If the intent is to fail the command with a friendly hint on capability errors, explicitlyawait channel.attach()(and let that throw/reject) before registering the subscription callback.
// Subscribe to the channel
await channel.subscribe((message) => {
const timestamp = formatMessageTimestamp(message.timestamp);
const event = message.name || "unknown";
const logEvent = {
channel: channelName,
data: message.data,
event,
timestamp,
};
this.logCliEvent(
flags,
src/commands/logs/connection-lifecycle/subscribe.ts:95
await channel.subscribe(...)is being used as if it will reject on attach/capability/auth failures, but in the project's Ably realtime mocksubscribeis synchronous and does not return a Promise. If you need deterministic failure handling (to show friendly Ably error hints), explicitlyawait channel.attach()(and let that reject with ErrorInfo) before registering the subscription callback.
// Subscribe to connection lifecycle logs
await channel.subscribe((message: Ably.Message) => {
const timestamp = formatMessageTimestamp(message.timestamp);
const event = {
timestamp,
event: message.name || "connection.lifecycle",
data: message.data,
id: message.id,
};
src/commands/logs/channel-lifecycle/subscribe.ts:98
await channel.subscribe(...)likely won't reliably wait for attachment or reject on capability/auth failures (the repo's realtime mocksubscribereturns void). Consider explicitlyawait channel.attach()first so attach errors surface to thecatchand can be formatted with friendly hints.
// Subscribe to the channel
await channel.subscribe((message) => {
const timestamp = formatMessageTimestamp(message.timestamp);
const event = message.name || "unknown";
const logEvent = {
channel: channelName,
data: message.data,
event,
timestamp,
src/commands/channels/occupancy/subscribe.ts:120
await channel.subscribe(...)likely does not reliably wait for attachment or reject on capability/auth failures (the repo's realtime mock implementation ofsubscribereturns void). To make the new "capability error" handling deterministic, explicitlyawait channel.attach()in the try-block (so it can throw Ably ErrorInfo with code/status/href) and then register the subscription without relying onsubscribe's return value.
await channel.subscribe(occupancyEventName, (message: Ably.Message) => {
const timestamp = formatMessageTimestamp(message.timestamp);
const event = {
channel: channelName,
event: occupancyEventName,
data: message.data,
timestamp,
};
this.logCliEvent(
flags,
"occupancy",
"occupancyUpdate",
`Occupancy update received for channel ${channelName}`,
event,
);
if (this.shouldOutputJson(flags)) {
this.logJsonEvent(event, flags);
} else {
this.log(
`${formatTimestamp(timestamp)} ${formatResource(`Channel: ${channelName}`)} | ${formatEventType("Occupancy Update")}`,
);
if (message.data !== null && message.data !== undefined) {
this.log(
`${formatLabel("Occupancy Data")} ${JSON.stringify(message.data, null, 2)}`,
);
}
this.log(""); // Empty line for better readability
}
});
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/base-command.ts (2)
1507-1518: Use the resolved error code for both the hint and the banner.The new hint lookup only checks
cmdError.code, but the banner below already falls back tocmdError.context.errorCode. Reusing the same resolvedcodekeeps those two outputs from diverging.Suggested refactor
- let humanMessage = cmdError.message; - const friendlyHint = getFriendlyAblyErrorHint(cmdError.code); + const code = cmdError.code ?? cmdError.context.errorCode; + let humanMessage = cmdError.message; + const friendlyHint = getFriendlyAblyErrorHint(code); if (friendlyHint) { humanMessage += `\n${friendlyHint}`; } - const code = cmdError.code ?? cmdError.context.errorCode; if (code !== undefined) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/base-command.ts` around lines 1507 - 1518, The friendly hint currently uses cmdError.code directly which can diverge from the banner's resolved code; resolve the code first (const code = cmdError.code ?? cmdError.context.errorCode) and pass that resolved code into getFriendlyAblyErrorHint (e.g., getFriendlyAblyErrorHint(code)) so both the hint and the banner use the same value, then build humanMessage and helpUrl logic using that same code and cmdError.context.helpUrl.
940-943: Please add coverage for the stderr stream change.This moves observable SDK error output from stdout to stderr in non-verbose, non-JSON mode. A CLI runner assertion on
stderr()would lock the new contract in and prevent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/base-command.ts` around lines 940 - 943, Add a test that locks the new contract by asserting the SDK error goes to stderr in non-verbose, non-JSON mode: call the CLI entry point or the command class (the code path that invokes logToStderr) with verbose=false and json=false, simulate or trigger an Ably SDK error message, and assert the test runner's stderr() contains the string "[AblySDK Error]" and the expected message while stdout remains unchanged; use the existing CLI test helper/runner used elsewhere in tests to invoke the command and check stderr() so the behavior of logToStderr is covered and will prevent regressions.test/unit/utils/errors.test.ts (1)
56-59: Simplify the undefined test case.The
[].pop() as number | undefinedpattern is unnecessarily convoluted. TypeScript allows passingundefineddirectly since the function parameter likely acceptsnumber | undefined.✨ Simplified version
it("should return undefined when code is not provided", () => { - const code = [].pop() as number | undefined; - expect(getFriendlyAblyErrorHint(code)).toBeUndefined(); + expect(getFriendlyAblyErrorHint(undefined)).toBeUndefined(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/utils/errors.test.ts` around lines 56 - 59, The test "should return undefined when code is not provided" uses an unnecessary construct ([] .pop() as number | undefined); simplify it by passing undefined directly to getFriendlyAblyErrorHint (e.g., set the test variable to undefined or call getFriendlyAblyErrorHint(undefined)) so the assertion remains expect(getFriendlyAblyErrorHint(code)).toBeUndefined(); this keeps the test intent clear and removes the convoluted type trick.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/channels/subscribe.ts`:
- Line 157: The mock Ably client's subscribe method currently returns undefined
(causing subscribe to behave differently than the real SDK), so update the mock
method (in test/helpers/mock-ably-realtime.ts — the mock's subscribe
implementation) to return a resolved Promise (e.g., return Promise.resolve() or
Promise.resolve<void>()) so it matches the real Ably SDK's Promise<void>
behavior and ensures subscribePromises (used in
src/commands/channels/subscribe.ts) await real resolution timing in tests.
In `@src/utils/errors.ts`:
- Around line 23-28: Update the capability hint strings for Ably error codes
40160, 40161, and 40171 in src/utils/errors.ts to use credential-neutral wording
(e.g., replace "API key" with "credential" or "token/key") so
getFriendlyAblyErrorHint() works for both token and API-key auth; ensure each
message still points users to check channel capability configuration in the Ably
dashboard and clearly references the capability restriction without assuming a
specific credential type.
In `@test/unit/commands/channels/occupancy/subscribe.test.ts`:
- Around line 127-137: The test's Error construction inside the Object.assign
uses a multi-line string which breaks Prettier; replace the multi-line new
Error(...) message with a single-line string literal so the Error is created as
new Error("Channel denied access based on given capability") within the existing
Object.assign expression (look for the Object.assign(...) containing new
Error(...) in subscribe.test.ts) and run Prettier to ensure formatting passes.
In `@test/unit/commands/channels/presence/subscribe.test.ts`:
- Around line 191-202: The multi-line error message passed into the Error
constructor inside the channel.presence.subscribe.mockRejectedValue call should
be a single-line string; update the Error(...) argument in the mocked rejection
(the Object.assign block used with channel.presence.subscribe.mockRejectedValue)
so the message "Channel denied access based on given capability" is on one line
to satisfy Prettier formatting.
In `@test/unit/commands/channels/subscribe.test.ts`:
- Around line 248-258: Prettier is failing because the Error instantiation uses
a multi-line string; update the Error construction inside the Object.assign call
so the message is a single-line string literal (i.e., change new Error("Channel
denied access based on given capability",) to a single-line new Error("Channel
denied access based on given capability")) within the test that contains the
Object.assign/new Error pattern to satisfy formatting.
---
Nitpick comments:
In `@src/base-command.ts`:
- Around line 1507-1518: The friendly hint currently uses cmdError.code directly
which can diverge from the banner's resolved code; resolve the code first (const
code = cmdError.code ?? cmdError.context.errorCode) and pass that resolved code
into getFriendlyAblyErrorHint (e.g., getFriendlyAblyErrorHint(code)) so both the
hint and the banner use the same value, then build humanMessage and helpUrl
logic using that same code and cmdError.context.helpUrl.
- Around line 940-943: Add a test that locks the new contract by asserting the
SDK error goes to stderr in non-verbose, non-JSON mode: call the CLI entry point
or the command class (the code path that invokes logToStderr) with verbose=false
and json=false, simulate or trigger an Ably SDK error message, and assert the
test runner's stderr() contains the string "[AblySDK Error]" and the expected
message while stdout remains unchanged; use the existing CLI test helper/runner
used elsewhere in tests to invoke the command and check stderr() so the behavior
of logToStderr is covered and will prevent regressions.
In `@test/unit/utils/errors.test.ts`:
- Around line 56-59: The test "should return undefined when code is not
provided" uses an unnecessary construct ([] .pop() as number | undefined);
simplify it by passing undefined directly to getFriendlyAblyErrorHint (e.g., set
the test variable to undefined or call getFriendlyAblyErrorHint(undefined)) so
the assertion remains expect(getFriendlyAblyErrorHint(code)).toBeUndefined();
this keeps the test intent clear and removes the convoluted type trick.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 676006c6-bb17-41cd-845b-57b8e786968d
📒 Files selected for processing (14)
src/base-command.tssrc/commands/channels/occupancy/subscribe.tssrc/commands/channels/presence/enter.tssrc/commands/channels/presence/subscribe.tssrc/commands/channels/subscribe.tssrc/commands/logs/channel-lifecycle/subscribe.tssrc/commands/logs/connection-lifecycle/subscribe.tssrc/commands/logs/push/subscribe.tssrc/commands/logs/subscribe.tssrc/utils/errors.tstest/unit/commands/channels/occupancy/subscribe.test.tstest/unit/commands/channels/presence/subscribe.test.tstest/unit/commands/channels/subscribe.test.tstest/unit/utils/errors.test.ts
umair-ably
force-pushed
the
DX-255-error-codes
branch
from
e9fc4da to
cd5760a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes