Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughA new CLI command Changes
Sequence DiagramsequenceDiagram
participant User as User/CLI
participant Cmd as ChannelsInspect
participant Config as Config Manager
participant Browser as Browser/Dashboard
User->>Cmd: channels:inspect [channel-name] --app [app-id]
Cmd->>Config: Get current account
alt Account missing
Cmd-->>User: Error: No account configured
else Account exists
Cmd->>Config: Get app (from --app flag or current)
alt App missing
Cmd-->>User: Error: No app selected
else App exists
Cmd->>Cmd: Build dashboard URL<br/>(account, app, channel)
alt Web CLI mode
Cmd-->>User: Display: Visit [URL]
else Normal mode
Cmd->>Browser: Open URL
Browser-->>User: Dashboard opened
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
Opens the Ably dashboard channel page in the browser for a given channel name. Constructs the URL from the configured account ID and app ID, with URL-encoding for special characters in channel names. Supports --app flag to override the current app, web CLI mode (prints URL instead of opening browser), and validates that both account and app are configured before proceeding. This is change obviously falls short of a full channel inspector experience in the CLI, but at least the functionality is exposed to cli users, and provides a quick jump to the dashboard inspectors.
zknill
force-pushed
the
zak/channel-inspect-command
branch
from
cb427ae to
0b69485
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/unit/commands/channels/inspect.test.ts (1)
23-53: MockopenUrlto avoid triggering real browser-open syscalls and decouple assertions fromopenUrl's output format.The tests in this file execute the command in-process via
runCommand(), which calls the realopenUrl()utility. This causes actual browser-launch syscalls to fire in CI and couples the test assertions toopenUrl's internal print format — if that format ever changes, these tests fail even though the command logic is correct.Instead, mock
openUrland assert it was called with the expected URL:♻️ Suggested refactor
+import { vi } from "vitest"; + +vi.mock("../../../src/utils/open-url.js", () => ({ + default: vi.fn().mockResolvedValue(undefined), +})); + +import openUrl from "../../../src/utils/open-url.js"; it("should open browser with correct dashboard URL", async () => { const mockConfig = getMockConfigManager(); const accountId = mockConfig.getCurrentAccount()!.accountId!; const appId = mockConfig.getCurrentAppId()!; - const { stdout } = await runCommand( + await runCommand( ["channels:inspect", "my-channel"], import.meta.url, ); - expect(stdout).toContain("Opening"); - expect(stdout).toContain("in your browser"); - expect(stdout).toContain( - `https://ably.com/accounts/${accountId}/apps/${appId}/channels/my-channel`, - ); + expect(openUrl).toHaveBeenCalledWith( + `https://ably.com/accounts/${accountId}/apps/${appId}/channels/my-channel`, + expect.anything(), + ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/channels/inspect.test.ts` around lines 23 - 53, The tests currently call runCommand(...) which triggers the real openUrl() and causes browser syscalls and brittle output assertions; update the tests in inspect.test.ts to mock/stub the openUrl function (the openUrl export used by the channels:inspect command) before calling runCommand, assert openUrl was invoked once with the exact dashboard URL (including percent-encoding for "my-channel/foo#bar"), and remove or replace assertions that check for "Opening"/"in your browser" or rely on openUrl's stdout format; ensure you reference the openUrl symbol when creating the mock and keep other uses of runCommand the same.
🤖 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/inspect.ts`:
- Line 46: Replace the hardcoded dashboard base in the URL construction with a
configurable dashboardHost sourced from a new global flag or environment
variable; specifically, add a `dashboard-host` global flag to
AblyBaseCommand.globalFlags (or read process.env.ABLY_DASHBOARD_URL as a
fallback), then build the URL using that dashboardHost when forming the const
url that currently uses `https://ably.com` (keeping the rest: accountId, appId,
and encodeURIComponent(args.channel)); ensure the code uses
flags["dashboard-host"] || process.env.ABLY_DASHBOARD_URL || 'https://ably.com'
as the final host so staging/enterprise overrides work.
---
Nitpick comments:
In `@test/unit/commands/channels/inspect.test.ts`:
- Around line 23-53: The tests currently call runCommand(...) which triggers the
real openUrl() and causes browser syscalls and brittle output assertions; update
the tests in inspect.test.ts to mock/stub the openUrl function (the openUrl
export used by the channels:inspect command) before calling runCommand, assert
openUrl was invoked once with the exact dashboard URL (including
percent-encoding for "my-channel/foo#bar"), and remove or replace assertions
that check for "Opening"/"in your browser" or rely on openUrl's stdout format;
ensure you reference the openUrl symbol when creating the mock and keep other
uses of runCommand the same.
| ); | ||
| } | ||
|
|
||
| const url = `https://ably.com/accounts/${accountId}/apps/${appId}/channels/${encodeURIComponent(args.channel)}`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
# Search for controlHost usage and other dashboard URL patterns
rg -n "ably\.com|dashboardHost|controlHost" src/commands/ --type ts -A 2 -B 2 | head -100Repository: ably/ably-cli
Length of output: 3771
🏁 Script executed:
# Check the base command and config for any dashboard host configuration
fd -e ts -e js src/ | xargs grep -l "dashboardHost\|dashboard.*url" 2>/dev/null | head -20Repository: ably/ably-cli
Length of output: 378
🏁 Script executed:
# Look at the base-command.ts to see what host configs are exposed
cat src/base-command.ts | head -150Repository: ably/ably-cli
Length of output: 5269
🏁 Script executed:
# Check if there are other commands that construct URLs to ably.com
rg "https://ably\.com" src/commands/ --type ts -B 5 -A 2Repository: ably/ably-cli
Length of output: 2654
Hardcoded dashboard base URL violates the "no hardcoded endpoint URLs" guideline.
Line 46 hardcodes https://ably.com as the dashboard base. This breaks for staging/enterprise environments and is not configurable. A dashboardHost flag or env-override (e.g., ABLY_DASHBOARD_URL) should supply the base, consistent with how control-host is used elsewhere in the codebase.
♻️ Suggested refactor
- const url = `https://ably.com/accounts/${accountId}/apps/${appId}/channels/${encodeURIComponent(args.channel)}`;
+ const dashboardHost = flags["dashboard-host"] ?? process.env.ABLY_DASHBOARD_URL ?? "https://ably.com";
+ const url = `${dashboardHost}/accounts/${accountId}/apps/${appId}/channels/${encodeURIComponent(args.channel)}`;Alternatively, add a dashboard-host global flag to AblyBaseCommand.globalFlags (similar to control-host) and use flags["dashboard-host"] here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/commands/channels/inspect.ts` at line 46, Replace the hardcoded dashboard
base in the URL construction with a configurable dashboardHost sourced from a
new global flag or environment variable; specifically, add a `dashboard-host`
global flag to AblyBaseCommand.globalFlags (or read
process.env.ABLY_DASHBOARD_URL as a fallback), then build the URL using that
dashboardHost when forming the const url that currently uses `https://ably.com`
(keeping the rest: accountId, appId, and encodeURIComponent(args.channel));
ensure the code uses flags["dashboard-host"] || process.env.ABLY_DASHBOARD_URL
|| 'https://ably.com' as the final host so staging/enterprise overrides work.
|
|
||
| const url = `https://ably.com/accounts/${accountId}/apps/${appId}/channels/${encodeURIComponent(args.channel)}`; | ||
|
|
||
| if (this.isWebCliMode) { |
There was a problem hiding this comment.
Rather than if here, should we add the condition to openUrl?
| ); | ||
| } | ||
|
|
||
| const url = `https://ably.com/accounts/${accountId}/apps/${appId}/channels/${encodeURIComponent(args.channel)}`; |
There was a problem hiding this comment.
Ditto coderabbit, this needs to work on the staging sites so should be configurable. It'll also need to be added to the terminal server, because that's where the web CLI will be running
2 participants
Opens the Ably dashboard channel page in the browser for a given channel name. Constructs the URL from the configured account ID and app ID, with URL-encoding for special characters in channel names. Supports --app flag to override the current app, web CLI mode (prints URL instead of opening browser), and validates that both account and app are configured before proceeding.
This is change obviously falls short of a full channel inspector experience in the CLI, but at least the functionality is exposed to cli users, and provides a quick jump to the dashboard inspectors.
Summary by CodeRabbit
New Features
channels:inspectCLI command for quickly accessing and reviewing channel details directly within the Ably dashboard. Supports optional app selection via the--appflag, which defaults to your current app selection. Automatically handles special characters in channel names.Note
Low Risk
Adds a new command that only constructs/opens a URL and performs simple config validation, with good unit test coverage and no data-path or auth logic changes.
Overview
Adds a new
channels:inspectCLI command that builds an Ably dashboard URL from the configured account/app (or--app) and opens it in the browser (or prints it in web CLI mode).Includes validation for missing account/app configuration and URL-encodes channel names; adds unit tests covering normal vs web CLI mode, flag precedence, encoding, and error cases.
Written by Cursor Bugbot for commit 0b69485. This will update automatically on new commits. Configure here.