[DX-928] - Updates Repo Docs and Tests#152
Merged
umair-ably merged 4 commits intomainfrom Mar 6, 2026
Merged
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:
WalkthroughThis PR consolidates and migrates Ably CLI guidance and documentation by removing fragmented cursor rule files (.cursor/rules/*.mdc), consolidating their content into a comprehensive AGENTS.md file, updating all documentation references accordingly, and adding extensive unit test coverage for CLI commands across accounts, logs, rooms, and spaces modules. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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 |
There was a problem hiding this comment.
Pull request overview
Updates repository documentation and unit tests to match newer LLM/agent conventions, remove stale guidance, and increase unit test coverage across utilities and CLI commands.
Changes:
- Added extensive Vitest unit tests for utilities and multiple oclif commands (spaces, rooms, logs, accounts).
- Refreshed docs to point contributors/agents to
AGENTS.mdand updated project structure/testing guidance. - Removed stale planning/docs artifacts (workplans README/resource, old Cursor/Claude rule docs, historical alignment plan).
Reviewed changes
Copilot reviewed 39 out of 40 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/utils/string-distance.test.ts | Adds unit coverage for fuzzy “closest match” helper. |
| test/unit/utils/prompt-confirmation.test.ts | Adds unit coverage for readline confirmation prompt behavior. |
| test/unit/utils/json-formatter.test.ts | Adds unit coverage for JSON detection/formatting and circular refs. |
| test/unit/services/stats-display.test.ts | Adds unit coverage for stats display output modes and private formatters. |
| test/unit/commands/spaces/members/subscribe.test.ts | Adds command tests for member subscription output and JSON mode. |
| test/unit/commands/spaces/members/enter.test.ts | Adds command tests for entering spaces, profile parsing, JSON errors. |
| test/unit/commands/spaces/locks/acquire.test.ts | Adds command tests for lock acquisition, JSON parsing, JSON output. |
| test/unit/commands/spaces/locations/set.test.ts | Adds command tests for location validation, setting, JSON output. |
| test/unit/commands/spaces/list.test.ts | Adds list filtering/deduplication tests and JSON structure assertions. |
| test/unit/commands/spaces/cursors/set.test.ts | Adds cursor validation/merging behavior tests and JSON output checks. |
| test/unit/commands/rooms/typing/keystroke.test.ts | Adds tests for typing keystroke and --auto-type behavior. |
| test/unit/commands/rooms/reactions/send.test.ts | Adds tests for reaction send, metadata parsing, JSON success/error. |
| test/unit/commands/rooms/presence/enter.test.ts | Adds tests for presence enter, data parsing, filtering self events, JSON output. |
| test/unit/commands/rooms/occupancy/subscribe.test.ts | Adds tests for occupancy subscribe behavior and JSON event typing. |
| test/unit/commands/rooms/occupancy/get.test.ts | Adds tests for occupancy snapshot retrieval + JSON error handling. |
| test/unit/commands/rooms/list.test.ts | Adds room listing filter/dedup/metrics tests + JSON output. |
| test/unit/commands/logs/subscribe.test.ts | Extends tests for rewind and log type filtering behavior. |
| test/unit/commands/logs/push/subscribe.test.ts | Adds unit tests for push log subscription and rewind flag. |
| test/unit/commands/logs/history.test.ts | Adds tests for history param parsing and limit/empty messaging. |
| test/unit/commands/accounts/switch.test.ts | Adds coverage for switching aliases, missing accounts, expired token behavior. |
| test/unit/commands/accounts/list.test.ts | Adds coverage for empty + populated account listings and JSON output. |
| test/unit/commands/accounts/current.test.ts | Adds coverage for current account display, fallback behavior, errors. |
| docs/workplans/resources/2025-05-cli-drawer.tsx | Removes stale workplan UI resource. |
| docs/workplans/README.md | Removes stale workplans documentation. |
| docs/Troubleshooting.md | Updates guidance on .js import extensions and config commands; points to AGENTS.md. |
| docs/Testing.md | Updates links and refreshes test folder structure documentation. |
| docs/Project-Structure.md | Refreshes repo structure overview and notes terminal server move. |
| docs/Debugging.md | Updates doc references and config command guidance. |
| align-cli.md | Removes outdated planning document. |
| README.md | Updates contributing pointers to CONTRIBUTING.md / AGENTS.md; removes old AI section. |
| CONTRIBUTING.md | Updates workflow references and release docs commands. |
| AGENTS.md | Expands and formalizes agent/developer workflow and conventions. |
| .cursor/rules/Workflow.mdc | Removes stale Cursor rule doc in favor of AGENTS.md. |
| .cursor/rules/Project.mdc | Removes stale Cursor rule doc in favor of AGENTS.md. |
| .cursor/rules/Development.mdc | Removes stale Cursor rule doc in favor of AGENTS.md. |
| .cursor/rules/Ably.mdc | Removes stale Cursor rule doc in favor of AGENTS.md. |
| .cursor/rules/AI-Assistance.mdc | Removes stale Cursor rule doc in favor of AGENTS.md. |
| .cursor/rules/.cursorindexingignore | Removes Cursor indexing ignore file. |
| .claude/CLAUDE.md | Replaces detailed instructions with a pointer to AGENTS.md. |
Comments suppressed due to low confidence (2)
test/unit/utils/json-formatter.test.ts:1
- This mutates Chalk’s global color level for the entire test process and never restores it, which can create cross-test coupling (especially for snapshot/string expectations that include ANSI). Capture the previous
chalk.leveland restore it in anafterAll(or use a per-test override) to keep this suite isolated.
.claude/CLAUDE.md:1 - This file now contains a bare path string, which isn’t very discoverable for readers and may not work for tooling expecting Markdown content (it’s not a symlink). Consider replacing it with a short Markdown stub that links to
../AGENTS.md(and briefly explains thatAGENTS.mdis the canonical source of workflow/conventions), or make it an actual git symlink if that’s the intent.
# AGENTS.md - Ably CLI
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (19)
test/unit/commands/spaces/locks/acquire.test.ts (1)
6-10: AddafterEachhook to clean up mocks between tests.The coding guidelines require cleaning up resources in
afterEachhooks. While using vitest mocks rather than sinon stubs, adding cleanup ensures test isolation and prevents mock state from leaking between tests.♻️ Suggested fix
import { describe, it, expect, beforeEach } from "vitest"; +import { afterEach, vi } from "vitest"; import { runCommand } from "@oclif/test"; import { getMockAblySpaces } from "../../../../helpers/mock-ably-spaces.js"; import { getMockAblyRealtime } from "../../../../helpers/mock-ably-realtime.js"; describe("spaces:locks:acquire command", () => { beforeEach(() => { getMockAblyRealtime(); getMockAblySpaces(); }); + + afterEach(() => { + vi.restoreAllMocks(); + });As per coding guidelines: "Always clean up resources and restore sinon stubs in test afterEach() hooks".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/spaces/locks/acquire.test.ts` around lines 6 - 10, Add an afterEach hook to reset/restore Vitest mocks after each test to prevent state leakage; next to the existing beforeEach calling getMockAblyRealtime() and getMockAblySpaces(), add afterEach(() => { vi.resetAllMocks(); vi.restoreAllMocks(); }) (or call any provided mock cleanup helpers if present) so getMockAblyRealtime and getMockAblySpaces mocks are cleared between tests.test/unit/commands/spaces/cursors/set.test.ts (2)
147-151: Fragile JSON assertions rely on exact formatting.String containment checks like
'"x": 100'are sensitive to JSON formatting (spacing, indentation). Consider parsing the JSON for more robust assertions.♻️ Proposed fix
- expect(stdout).toContain('"success"'); - expect(stdout).toContain("true"); - expect(stdout).toContain("test-space"); - expect(stdout).toContain('"x": 100'); - expect(stdout).toContain('"y": 200'); + const output = JSON.parse(stdout); + expect(output.success).toBe(true); + expect(output.space).toBe("test-space"); + expect(output.position.x).toBe(100); + expect(output.position.y).toBe(200);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/spaces/cursors/set.test.ts` around lines 147 - 151, The current assertions in the test use fragile string contains checks (e.g., expect(stdout).toContain('"x": 100')) — instead, parse the CLI output JSON from stdout (use JSON.parse on stdout) and assert the parsed object's properties (e.g., parsed.success, parsed.space or parsed.data.space, and parsed.cursor.x / parsed.cursor.y) with strict equality checks; update the assertions that reference stdout, expect, and the x/y checks to validate the parsed object fields rather than string fragments.
81-100: Consider verifyingspace.enterwas called for consistency.The test at line 71 verifies
space.enterwas called, but this test and the merge test below omit that verification. If entering the space is a prerequisite for setting cursors, consider adding the same assertion for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/spaces/cursors/set.test.ts` around lines 81 - 100, Add a check that the test ensures the client entered the space before setting cursors: after obtaining `space = spacesMock._getSpace("test-space")` and running `runCommand(...)`, add an assertion that `space.enter` was called (e.g., expect(space.enter).toHaveBeenCalled()) alongside the existing `expect(space.cursors.set)...` so this test mirrors the earlier one and the merge test that verify entry as a prerequisite for setting cursors.test/unit/commands/spaces/locations/set.test.ts (1)
74-75: Unused_getSpacecall.Line 75 calls
spacesMock._getSpace("test-space")but the result is not assigned or used. Either remove this line or assign it to a variable if it's intended for setup side effects.Proposed fix
it("should output JSON on success with --duration 0", async () => { - const spacesMock = getMockAblySpaces(); - spacesMock._getSpace("test-space"); + getMockAblySpaces(); const location = { x: 10, y: 20 };Or if the call is needed for side effects (like pre-creating the space mock), add a comment:
const spacesMock = getMockAblySpaces(); + // Pre-create space mock for test spacesMock._getSpace("test-space");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/spaces/locations/set.test.ts` around lines 74 - 75, The call to spacesMock._getSpace("test-space") in the test is unused; either remove this line or make its intent explicit: if you need the returned mock, assign it (e.g., const space = spacesMock._getSpace("test-space")), or if it’s being invoked for side effects (pre-creating the space), keep the call but add a clarifying inline comment above it stating that side effects are intended; update the test in set.test.ts using getMockAblySpaces and _getSpace accordingly.test/unit/services/stats-display.test.ts (1)
68-81: Consider testing private methods through public interface.Accessing private methods via
Record<string, ...>casting works but is brittle—tests will silently break if the method is renamed or removed. If these formatting methods contain important logic worth testing directly, consider either:
- Testing indirectly through
display()output assertions- Extracting to a utility module with exported functions
That said, the parameterized test cases are well-chosen and cover key boundary transitions (0, bytes, KB, MB).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/services/stats-display.test.ts` around lines 68 - 81, Tests are calling the private method formatBytes on StatsDisplay via casting which is brittle; instead either exercise the logic through the public StatsDisplay.display(...) method (create inputs that cause the code path to format 0, 500, 1536, 1048576 bytes and assert output contains the expected formatted strings) or move the formatting logic out into a new exported utility (e.g., bytesFormatter.formatBytes) and update tests to import and test that exported function directly; update test file to remove the Record<string,...> access and use the chosen public/display-based assertions or the new exported function.test/unit/commands/rooms/occupancy/subscribe.test.ts (2)
5-8: AddafterEachhook for consistent mock cleanup.The tests manually restore spies inline, but centralizing cleanup in
afterEachensures reliable restoration even if tests fail early.♻️ Proposed fix
describe("rooms:occupancy:subscribe command", () => { beforeEach(() => { getMockAblyChat(); }); + + afterEach(() => { + vi.restoreAllMocks(); + });As per coding guidelines: "Always clean up resources and restore sinon stubs in test afterEach() hooks".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/rooms/occupancy/subscribe.test.ts` around lines 5 - 8, Add a centralized cleanup: inside the "rooms:occupancy:subscribe command" describe block (where getMockAblyChat() is called in beforeEach), add an afterEach hook that restores all sinon stubs/spies (e.g., call sinon.restore()) so inline manual restores are removed and mocks are reliably cleaned even on test failure.
77-81: Remove redundant assertion.The assertion on line 80 duplicates the one at line 66 within the same test.
♻️ Proposed fix
await commandPromise; logSpy.mockRestore(); - expect(room.occupancy.subscribe).toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/rooms/occupancy/subscribe.test.ts` around lines 77 - 81, The test contains a duplicate assertion: remove the redundant expect(room.occupancy.subscribe).toHaveBeenCalled() at the end of the test (the one that duplicates the earlier assertion), leaving only the original assertion (the earlier expect) so the test still verifies room.occupancy.subscribe was called without repeating it.test/unit/commands/rooms/reactions/send.test.ts (1)
5-8: AddafterEachhook to clean up mocks.The coding guidelines require cleaning up resources and restoring stubs in
afterEach()hooks. ThebeforeEachinitializes mocks, but there's no corresponding cleanup.♻️ Proposed fix
+import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { runCommand } from "@oclif/test"; import { getMockAblyChat } from "../../../../helpers/mock-ably-chat.js"; describe("rooms:reactions:send command", () => { beforeEach(() => { getMockAblyChat(); }); + + afterEach(() => { + vi.restoreAllMocks(); + });As per coding guidelines: "Always clean up resources and restore sinon stubs in test afterEach() hooks".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/rooms/reactions/send.test.ts` around lines 5 - 8, Add an afterEach hook to clean up the mocks initialized by getMockAblyChat() — e.g., add afterEach(() => { sinon.restore(); /* and call any specific cleanup like restoreMockAblyChat() if such helper exists */ }); — this ensures stubs and mock state created in beforeEach via getMockAblyChat() are restored between tests.test/unit/commands/rooms/list.test.ts (2)
59-65: Consider a more specific assertion for room count.The assertion
expect(stdout).toContain("2")could match unrelated text containing "2". Consider a more targeted pattern.💡 Suggested improvement
// room1 has 3 sub-channels but should appear only once in the count - expect(stdout).toContain("2"); - expect(stdout).toContain("active chat rooms"); + expect(stdout).toMatch(/2\s+active chat rooms/);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/rooms/list.test.ts` around lines 59 - 65, The test "should deduplicate rooms from sub-channels" uses a brittle assertion expect(stdout).toContain("2"); replace it with a targeted assertion against stdout (from runCommand(["rooms:list"], import.meta.url)) that matches the exact count and label, e.g. use a regex or exact-line match to assert the output contains the line "2 active chat rooms" (or anchors like /^\s*2\s+active chat rooms/m) so the test only passes when the room count is exactly 2.
5-49: AddafterEachhook for mock cleanup.Consistent with the other test files, this suite should include an
afterEachhook to restore mocks.♻️ Proposed fix
+import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { runCommand } from "@oclif/test"; import { getMockAblyRest } from "../../../helpers/mock-ably-rest.js"; describe("rooms:list command", () => { // ... mock data ... beforeEach(() => { const mock = getMockAblyRest(); mock.request.mockResolvedValue(mockChatChannelsResponse); }); + + afterEach(() => { + vi.restoreAllMocks(); + });As per coding guidelines: "Always clean up resources and restore sinon stubs in test afterEach() hooks".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/rooms/list.test.ts` around lines 5 - 49, Add an afterEach cleanup hook to restore/reset mocks created in beforeEach: after calling getMockAblyRest() and mocking mock.request in beforeEach, add an afterEach that resets and restores those mocks (e.g., call jest.resetAllMocks() and jest.restoreAllMocks() or sinon.restore() as appropriate) so the mocked Ably REST client (from getMockAblyRest) and mock.request are cleaned up between tests.test/unit/commands/rooms/occupancy/get.test.ts (1)
5-8: AddafterEachhook for mock cleanup.♻️ Proposed fix
+import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { runCommand } from "@oclif/test"; import { getMockAblyChat } from "../../../../helpers/mock-ably-chat.js"; describe("rooms:occupancy:get command", () => { beforeEach(() => { getMockAblyChat(); }); + + afterEach(() => { + vi.restoreAllMocks(); + });As per coding guidelines: "Always clean up resources and restore sinon stubs in test afterEach() hooks".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/rooms/occupancy/get.test.ts` around lines 5 - 8, Add an afterEach hook to the describe("rooms:occupancy:get command") suite that cleans up mocks created in beforeEach: call the cleanup function(s) returned by getMockAblyChat() or invoke sinon.restore()/sandbox.restore() to restore any sinon stubs and teardown Ably mocks so tests don’t leak state; place the afterEach adjacent to the existing beforeEach in the same suite.test/unit/commands/rooms/presence/enter.test.ts (1)
5-8: AddafterEachhook for consistent mock cleanup.The tests manually restore spies (lines 120, 166), but if a test fails before reaching
mockRestore(), the spy remains active and can affect subsequent tests. Centralizing cleanup inafterEachensures reliable restoration.♻️ Proposed fix
describe("rooms:presence:enter command", () => { beforeEach(() => { getMockAblyChat(); }); + + afterEach(() => { + vi.restoreAllMocks(); + });As per coding guidelines: "Always clean up resources and restore sinon stubs in test afterEach() hooks".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/rooms/presence/enter.test.ts` around lines 5 - 8, Add an afterEach hook inside the describe("rooms:presence:enter command") block to centrally restore sinon spies/stubs instead of relying on per-test mockRestore() calls; specifically, after calling getMockAblyChat() in beforeEach, add an afterEach that calls sinon.restore() (or sandbox.restore() if a sandbox pattern is used) so any spies/stubs created in tests (those currently undone via mockRestore()) are always cleaned up even if a test fails.test/unit/commands/rooms/typing/keystroke.test.ts (1)
25-43: The--auto-typecase is too weak to prove auto-typing.With
--duration 0, any duration-gated loop can exit immediately, so this test mostly shows that the flag is accepted and a message is printed. Please drive a non-zero auto-type window here and assert the repeatedroom.typing.keystrokebehavior you expect.As per coding guidelines, "Add or update Unit, Integration, and E2E tests as appropriate for your changes, following the testing strategy defined in
docs/Testing.md".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/rooms/typing/keystroke.test.ts` around lines 25 - 43, The test's --auto-type assertion is weak because using "--duration 0" exits immediately; update the test for rooms:typing:keystroke to use a non-zero duration (e.g., "--duration 100" or another short ms value) and assert repeated calls to room.typing.keystroke (for example expect(room.typing.keystroke).toHaveBeenCalledTimesGreaterThan(1) or at least toHaveBeenCalledTimes(>1)); if necessary use fake timers or await a small delay after runCommand to allow multiple keystroke iterations to fire; target symbols: runCommand invocation for "rooms:typing:keystroke", the "--auto-type" flag, and room.typing.keystroke.test/unit/commands/logs/history.test.ts (1)
26-45: Also assert that invalid ranges short-circuit before callinghistory().Right now this only checks the error message. If the command regresses and still hits Ably before throwing, the test will keep passing.
Proposed assertion
expect(error!.message).toContain( "--start must be earlier than or equal to --end", ); + expect(channel.history).not.toHaveBeenCalled();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/logs/history.test.ts` around lines 26 - 45, Add an assertion to ensure invalid date ranges short-circuit before contacting Ably: after calling runCommand with the inverted --start/--end, verify the mock channel's history method (channel.history) was never called (e.g., expect(channel.history).not.toHaveBeenCalled()) so the test fails if the command still invokes getMockAblyRest()/channel.history; keep the existing checks for error and message and place this new assertion alongside them in the same test case.test/unit/commands/logs/subscribe.test.ts (1)
96-105: Replace this duplicate rewind case with a real boundary check.The suite already covers the positive
--rewindpath at Lines 84-93, so this adds maintenance without much new signal. Since the test name says "--rewind > 0", I’d repurpose this to cover--rewind 0and assert that no rewind params are passed, or fold both positives intoit.each(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/logs/subscribe.test.ts` around lines 96 - 105, The test duplicate for the positive rewind case should be replaced with a boundary check for zero: update the spec inside the "rewind and type filtering" describe block to call runCommand(["logs:subscribe", "--rewind", "0"], import.meta.url) (instead of the existing positive case) and assert that mock.channels.get was called with "[meta]log" and no params or an empty params object (i.e., ensure no rewind param is passed). Locate the test that uses getMockAblyRealtime(), runCommand and mock.channels.get and change the expectation accordingly so it verifies the --rewind 0 behavior rather than duplicating the >0 case.test/unit/commands/accounts/list.test.ts (1)
6-8: Remove emptybeforeEachhook.The empty hook with only a comment adds noise. If config reset is handled by setup.ts, this hook serves no purpose. Either remove it or document the reset behavior in a file-level comment instead.
🧹 Suggested cleanup
describe("accounts:list command", () => { - beforeEach(() => { - // Config is auto-reset by setup.ts - }); + // Config is auto-reset by setup.ts before each test describe("no accounts", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/accounts/list.test.ts` around lines 6 - 8, Remove the empty beforeEach hook in test/unit/commands/accounts/list.test.ts (the block that only contains the comment “// Config is auto-reset by setup.ts”); if you want to keep the intent visible, move that note into a brief file-level comment at the top of the test file explaining that setup.ts resets config, otherwise simply delete the beforeEach block to eliminate noise.AGENTS.md (1)
20-34: Add language specifier to fenced code block.The directory structure code block is missing a language specifier, which triggers a markdownlint warning (MD040). Adding
textorplaintextimproves accessibility and consistency.📝 Suggested fix
-``` +```text . ├── src/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 20 - 34, Update the fenced directory-structure code block in AGENTS.md to include a language specifier (e.g., ```text or ```plaintext) to satisfy markdownlint MD040; locate the directory tree block shown starting with "." and change the opening backticks to include the specifier so the code fence becomes ```text (or ```plaintext)..claude/CLAUDE.md (1)
1-1: Consider adding context about this reference file.The file contains only a bare path
../AGENTS.mdwithout explanation. Adding a brief comment would help future maintainers understand the purpose of this file.📝 Suggested improvement
+<!-- Claude Code instructions are in AGENTS.md at the repository root --> ../AGENTS.md🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/CLAUDE.md at line 1, The file .claude/CLAUDE.md currently contains only the bare path "../AGENTS.md"; update it to include a short explanatory comment about why it references that file and what maintainers should expect (e.g., "See ../AGENTS.md for contributor guidelines and agent configs"), so anyone reading the CLAUDE.md understands the purpose of the reference and where to find related agent documentation.CONTRIBUTING.md (1)
56-56: Consider usingpnpm exec oclif readmefor portability.The change from
npx oclif readmetooclif readmeassumes oclif is globally installed. Contributors may not have it globally available. Usingpnpm exec oclif readmewould be more consistent with other commands in this file and ensure it works for all contributors.📝 Suggested fix
- - Run `oclif readme` to regenerate the README with updated command documentation. + - Run `pnpm exec oclif readme` to regenerate the README with updated command documentation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CONTRIBUTING.md` at line 56, Update the README regeneration instruction to use the project-local runner: replace the current "oclif readme" invocation in the CONTRIBUTING.md step that says "Run `oclif readme` to regenerate the README" with "pnpm exec oclif readme" so contributors without a global oclif install can run the command consistently with other pnpm-executed commands in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/Project-Structure.md`:
- Line 7: The fenced tree block in the docs currently uses a plain
triple-backtick fence; update that opening fence to specify the language `text`
(i.e., change ``` to ```text) so the fenced tree block is lint-clean for
markdownlint-cli2/MD040 and remains rendered as plain text.
- Around line 37-43: The docs tree entry "commands/channel-rule/" is pointing to
the wrong location; update the Project-Structure.md entry to match the actual
CLI layout used in the PR by replacing "commands/channel-rule/" with
"commands/apps/channel-rules/" (note the plural "channel-rules" and nested
"apps" folder) so the docs reflect the real path referenced by the CLI commands.
In `@test/unit/commands/accounts/current.test.ts`:
- Around line 110-132: The test title says "JSON output" but never triggers the
JSON branch; update the call to runCommand to invoke the JSON path (e.g., pass
the "--json" flag to runCommand for the "accounts:current" command) and replace
the freeform stdout assertions with a JSON parse/assert: call
runCommand(["accounts:current", "--json"], import.meta.url) and then parse
stdout as JSON and assert parsed.account.name === mockAccountName and
parsed.user.email === mockUserEmail (use existing helpers like
getMockConfigManager and accessToken unchanged).
In `@test/unit/commands/accounts/switch.test.ts`:
- Around line 106-128: After running the command, the test must also assert that
the account selection actually changed; update the test (which uses
getMockConfigManager, mock.storeAccount and runCommand) to query the mock config
manager for the current selection and assert it equals the expired account (by
name "expired-acct" or accountId "expired-id") so the test verifies the command
both warns on 401 and successfully switches the selected account.
- Around line 11-17: The "expired-token" test must also assert that the account
was actually switched to "expired-acct" despite the 401; update the test in
switch.test.ts (the test titled "should warn on expired token but still switch")
to, after invoking the accounts switch command, verify the current account alias
or command exit state indicating success (e.g., check the config/currentAccount
alias or the CLI command's exit code) to confirm the switch to "expired-acct"
occurred in addition to the existing warning assertions.
In `@test/unit/commands/logs/history.test.ts`:
- Around line 6-8: The tests in history.test.ts use Vitest mocks (vi.fn /
mockResolvedValue) and rely on global reset instead of the required sinon-stub
pattern; replace vi.fn/mockResolvedValue usages with sinon stubs created via
sinon.stub(...) (targeting the same functions set up by getMockAblyRest()) and
add an afterEach() hook that restores those stubs (either via sinon.restore() or
restoring individual stubs) instead of relying solely on resetMockAblyRest();
ensure getMockAblyRest() still sets up the mock behavior but returns/exports the
sinon stub references so the afterEach can call restore, or call
resetMockAblyRest() from afterEach if you choose to keep centralized cleanup but
then update the project's guideline accordingly.
In `@test/unit/commands/logs/push/subscribe.test.ts`:
- Around line 43-67: The test currently captures subscribeCallback from
channel.subscribe but never invokes it, so call subscribeCallback with a
representative Ably message object (e.g., { data: { message: "test msg",
severity: "warning" }, name: "log" }) after runCommand starts, then assert
stdout contains the formatted severity and message; update references to
channel.subscribe/subscribeCallback and the runCommand invocation in the test to
emit the message and verify the rendered output for severity parsing/formatting.
In `@test/unit/commands/rooms/typing/keystroke.test.ts`:
- Around line 1-8: The tests override room.attach in the "rooms:typing:keystroke
command" suite (via getMockAblyChat and the stubs created around the room
object) but never restore them; add an afterEach() teardown that restores those
sinon stubs (either call sinon.restore() if you used sinon.stub globally or call
stub.restore() for the specific room.attach stubs you created), ensuring the
afterEach runs in the same describe block that calls getMockAblyChat so no
room.attach overrides leak between tests.
In `@test/unit/commands/spaces/cursors/set.test.ts`:
- Around line 6-10: Add an afterEach hook that cleans up mocks and restores
sinon stubs: call sinon.restore() and invoke any mock cleanup/stop functions for
the Ably mocks created by getMockAblyRealtime() and getMockAblySpaces() (or call
cleanup/stop/restore on the returned mock objects if those helpers return
mocks). Ensure the afterEach runs in the same describe block as the beforeEach
so stubs and mock resources are reset between tests.
In `@test/unit/commands/spaces/list.test.ts`:
- Around line 40-43: The beforeEach hook uses getMockAblyRest() (a singleton)
and sets mock.request.mockResolvedValue(mockSpaceChannelsResponse) without
clearing prior call history, causing flaky toHaveBeenCalledOnce() assertions;
update the beforeEach to call mock.request.mockClear() (or mockReset()) on the
returned mock before setting mockResolvedValue so previous calls are wiped for
each test, referring to getMockAblyRest(), the mock.request property, and the
beforeEach hook to locate the change.
In `@test/unit/commands/spaces/locations/set.test.ts`:
- Around line 7-10: Add an afterEach hook to restore/clean up the mocks created
by getMockAblyRealtime and getMockAblySpaces to avoid test pollution; implement
afterEach that calls the appropriate cleanup (e.g., if your mock helpers return
restore functions call them, or if they use sinon internally call
sinon.restore()), and place this afterEach alongside the existing beforeEach in
the same test file to ensure getMockAblyRealtime and getMockAblySpaces are
cleaned up after each test.
In `@test/unit/commands/spaces/members/enter.test.ts`:
- Around line 6-10: Add a matching afterEach cleanup to the
"spaces:members:enter command" test suite that restores stubs and frees mocks
set up by getMockAblyRealtime() and getMockAblySpaces(); specifically, add an
afterEach that calls sinon.restore() (or equivalent stub restore) and any mock
teardown functions provided by getMockAblyRealtime/getMockAblySpaces so the test
suite does not leak mocked state between tests.
In `@test/unit/commands/spaces/members/subscribe.test.ts`:
- Around line 6-10: Add an afterEach cleanup hook to the
"spaces:members:subscribe command" test to restore mocks and avoid test
pollution: implement afterEach(() => { sinon.restore(); /* and call any teardown
helpers for getMockAblyRealtime()/getMockAblySpaces() if available, e.g.
restoreMockAblyRealtime(); restoreMockAblySpaces(); */ }); so that all sinon
stubs and the Ably mock state created by getMockAblyRealtime and
getMockAblySpaces are reset after each test.
---
Nitpick comments:
In @.claude/CLAUDE.md:
- Line 1: The file .claude/CLAUDE.md currently contains only the bare path
"../AGENTS.md"; update it to include a short explanatory comment about why it
references that file and what maintainers should expect (e.g., "See ../AGENTS.md
for contributor guidelines and agent configs"), so anyone reading the CLAUDE.md
understands the purpose of the reference and where to find related agent
documentation.
In `@AGENTS.md`:
- Around line 20-34: Update the fenced directory-structure code block in
AGENTS.md to include a language specifier (e.g., ```text or ```plaintext) to
satisfy markdownlint MD040; locate the directory tree block shown starting with
"." and change the opening backticks to include the specifier so the code fence
becomes ```text (or ```plaintext).
In `@CONTRIBUTING.md`:
- Line 56: Update the README regeneration instruction to use the project-local
runner: replace the current "oclif readme" invocation in the CONTRIBUTING.md
step that says "Run `oclif readme` to regenerate the README" with "pnpm exec
oclif readme" so contributors without a global oclif install can run the command
consistently with other pnpm-executed commands in the file.
In `@test/unit/commands/accounts/list.test.ts`:
- Around line 6-8: Remove the empty beforeEach hook in
test/unit/commands/accounts/list.test.ts (the block that only contains the
comment “// Config is auto-reset by setup.ts”); if you want to keep the intent
visible, move that note into a brief file-level comment at the top of the test
file explaining that setup.ts resets config, otherwise simply delete the
beforeEach block to eliminate noise.
In `@test/unit/commands/logs/history.test.ts`:
- Around line 26-45: Add an assertion to ensure invalid date ranges
short-circuit before contacting Ably: after calling runCommand with the inverted
--start/--end, verify the mock channel's history method (channel.history) was
never called (e.g., expect(channel.history).not.toHaveBeenCalled()) so the test
fails if the command still invokes getMockAblyRest()/channel.history; keep the
existing checks for error and message and place this new assertion alongside
them in the same test case.
In `@test/unit/commands/logs/subscribe.test.ts`:
- Around line 96-105: The test duplicate for the positive rewind case should be
replaced with a boundary check for zero: update the spec inside the "rewind and
type filtering" describe block to call runCommand(["logs:subscribe", "--rewind",
"0"], import.meta.url) (instead of the existing positive case) and assert that
mock.channels.get was called with "[meta]log" and no params or an empty params
object (i.e., ensure no rewind param is passed). Locate the test that uses
getMockAblyRealtime(), runCommand and mock.channels.get and change the
expectation accordingly so it verifies the --rewind 0 behavior rather than
duplicating the >0 case.
In `@test/unit/commands/rooms/list.test.ts`:
- Around line 59-65: The test "should deduplicate rooms from sub-channels" uses
a brittle assertion expect(stdout).toContain("2"); replace it with a targeted
assertion against stdout (from runCommand(["rooms:list"], import.meta.url)) that
matches the exact count and label, e.g. use a regex or exact-line match to
assert the output contains the line "2 active chat rooms" (or anchors like
/^\s*2\s+active chat rooms/m) so the test only passes when the room count is
exactly 2.
- Around line 5-49: Add an afterEach cleanup hook to restore/reset mocks created
in beforeEach: after calling getMockAblyRest() and mocking mock.request in
beforeEach, add an afterEach that resets and restores those mocks (e.g., call
jest.resetAllMocks() and jest.restoreAllMocks() or sinon.restore() as
appropriate) so the mocked Ably REST client (from getMockAblyRest) and
mock.request are cleaned up between tests.
In `@test/unit/commands/rooms/occupancy/get.test.ts`:
- Around line 5-8: Add an afterEach hook to the describe("rooms:occupancy:get
command") suite that cleans up mocks created in beforeEach: call the cleanup
function(s) returned by getMockAblyChat() or invoke
sinon.restore()/sandbox.restore() to restore any sinon stubs and teardown Ably
mocks so tests don’t leak state; place the afterEach adjacent to the existing
beforeEach in the same suite.
In `@test/unit/commands/rooms/occupancy/subscribe.test.ts`:
- Around line 5-8: Add a centralized cleanup: inside the
"rooms:occupancy:subscribe command" describe block (where getMockAblyChat() is
called in beforeEach), add an afterEach hook that restores all sinon stubs/spies
(e.g., call sinon.restore()) so inline manual restores are removed and mocks are
reliably cleaned even on test failure.
- Around line 77-81: The test contains a duplicate assertion: remove the
redundant expect(room.occupancy.subscribe).toHaveBeenCalled() at the end of the
test (the one that duplicates the earlier assertion), leaving only the original
assertion (the earlier expect) so the test still verifies
room.occupancy.subscribe was called without repeating it.
In `@test/unit/commands/rooms/presence/enter.test.ts`:
- Around line 5-8: Add an afterEach hook inside the
describe("rooms:presence:enter command") block to centrally restore sinon
spies/stubs instead of relying on per-test mockRestore() calls; specifically,
after calling getMockAblyChat() in beforeEach, add an afterEach that calls
sinon.restore() (or sandbox.restore() if a sandbox pattern is used) so any
spies/stubs created in tests (those currently undone via mockRestore()) are
always cleaned up even if a test fails.
In `@test/unit/commands/rooms/reactions/send.test.ts`:
- Around line 5-8: Add an afterEach hook to clean up the mocks initialized by
getMockAblyChat() — e.g., add afterEach(() => { sinon.restore(); /* and call any
specific cleanup like restoreMockAblyChat() if such helper exists */ }); — this
ensures stubs and mock state created in beforeEach via getMockAblyChat() are
restored between tests.
In `@test/unit/commands/rooms/typing/keystroke.test.ts`:
- Around line 25-43: The test's --auto-type assertion is weak because using
"--duration 0" exits immediately; update the test for rooms:typing:keystroke to
use a non-zero duration (e.g., "--duration 100" or another short ms value) and
assert repeated calls to room.typing.keystroke (for example
expect(room.typing.keystroke).toHaveBeenCalledTimesGreaterThan(1) or at least
toHaveBeenCalledTimes(>1)); if necessary use fake timers or await a small delay
after runCommand to allow multiple keystroke iterations to fire; target symbols:
runCommand invocation for "rooms:typing:keystroke", the "--auto-type" flag, and
room.typing.keystroke.
In `@test/unit/commands/spaces/cursors/set.test.ts`:
- Around line 147-151: The current assertions in the test use fragile string
contains checks (e.g., expect(stdout).toContain('"x": 100')) — instead, parse
the CLI output JSON from stdout (use JSON.parse on stdout) and assert the parsed
object's properties (e.g., parsed.success, parsed.space or parsed.data.space,
and parsed.cursor.x / parsed.cursor.y) with strict equality checks; update the
assertions that reference stdout, expect, and the x/y checks to validate the
parsed object fields rather than string fragments.
- Around line 81-100: Add a check that the test ensures the client entered the
space before setting cursors: after obtaining `space =
spacesMock._getSpace("test-space")` and running `runCommand(...)`, add an
assertion that `space.enter` was called (e.g.,
expect(space.enter).toHaveBeenCalled()) alongside the existing
`expect(space.cursors.set)...` so this test mirrors the earlier one and the
merge test that verify entry as a prerequisite for setting cursors.
In `@test/unit/commands/spaces/locations/set.test.ts`:
- Around line 74-75: The call to spacesMock._getSpace("test-space") in the test
is unused; either remove this line or make its intent explicit: if you need the
returned mock, assign it (e.g., const space =
spacesMock._getSpace("test-space")), or if it’s being invoked for side effects
(pre-creating the space), keep the call but add a clarifying inline comment
above it stating that side effects are intended; update the test in set.test.ts
using getMockAblySpaces and _getSpace accordingly.
In `@test/unit/commands/spaces/locks/acquire.test.ts`:
- Around line 6-10: Add an afterEach hook to reset/restore Vitest mocks after
each test to prevent state leakage; next to the existing beforeEach calling
getMockAblyRealtime() and getMockAblySpaces(), add afterEach(() => {
vi.resetAllMocks(); vi.restoreAllMocks(); }) (or call any provided mock cleanup
helpers if present) so getMockAblyRealtime and getMockAblySpaces mocks are
cleared between tests.
In `@test/unit/services/stats-display.test.ts`:
- Around line 68-81: Tests are calling the private method formatBytes on
StatsDisplay via casting which is brittle; instead either exercise the logic
through the public StatsDisplay.display(...) method (create inputs that cause
the code path to format 0, 500, 1536, 1048576 bytes and assert output contains
the expected formatted strings) or move the formatting logic out into a new
exported utility (e.g., bytesFormatter.formatBytes) and update tests to import
and test that exported function directly; update test file to remove the
Record<string,...> access and use the chosen public/display-based assertions or
the new exported function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fea65f16-934c-4162-81a2-a8941405e247
📒 Files selected for processing (40)
.claude/CLAUDE.md.claude/CLAUDE.md.cursor/rules/.cursorindexingignore.cursor/rules/AI-Assistance.mdc.cursor/rules/Ably.mdc.cursor/rules/Development.mdc.cursor/rules/Project.mdc.cursor/rules/Workflow.mdcAGENTS.mdCONTRIBUTING.mdREADME.mdalign-cli.mddocs/Debugging.mddocs/Project-Structure.mddocs/Testing.mddocs/Troubleshooting.mddocs/workplans/README.mddocs/workplans/resources/2025-05-cli-drawer.tsxtest/unit/commands/accounts/current.test.tstest/unit/commands/accounts/list.test.tstest/unit/commands/accounts/switch.test.tstest/unit/commands/logs/history.test.tstest/unit/commands/logs/push/subscribe.test.tstest/unit/commands/logs/subscribe.test.tstest/unit/commands/rooms/list.test.tstest/unit/commands/rooms/occupancy/get.test.tstest/unit/commands/rooms/occupancy/subscribe.test.tstest/unit/commands/rooms/presence/enter.test.tstest/unit/commands/rooms/reactions/send.test.tstest/unit/commands/rooms/typing/keystroke.test.tstest/unit/commands/spaces/cursors/set.test.tstest/unit/commands/spaces/list.test.tstest/unit/commands/spaces/locations/set.test.tstest/unit/commands/spaces/locks/acquire.test.tstest/unit/commands/spaces/members/enter.test.tstest/unit/commands/spaces/members/subscribe.test.tstest/unit/services/stats-display.test.tstest/unit/utils/json-formatter.test.tstest/unit/utils/prompt-confirmation.test.tstest/unit/utils/string-distance.test.ts
💤 Files with no reviewable changes (9)
- .cursor/rules/Project.mdc
- docs/workplans/resources/2025-05-cli-drawer.tsx
- docs/workplans/README.md
- .cursor/rules/Workflow.mdc
- .cursor/rules/Development.mdc
- .cursor/rules/.cursorindexingignore
- .cursor/rules/AI-Assistance.mdc
- align-cli.md
- .cursor/rules/Ably.mdc
umair-ably
force-pushed
the
DX-928-repo-update
branch
from
76c7235 to
5e0eb32
Compare
umair-ably
force-pushed
the
DX-928-repo-update
branch
from
5e0eb32 to
362c05d
Compare
sacOO7
reviewed
Mar 6, 2026
sacOO7
approved these changes
Mar 6, 2026
Contributor
sacOO7
left a comment
sacOO7
left a comment
There was a problem hiding this comment.
We planning to use Claude going forward, so .cursor related config. is deleted right?
Btw, I checked if any workflows from .cursor/rules/AI-Assistance.mdc needs to be added to new AGENTS.md. Seems everything is covered : )
So, LGTM !
- Deletes stale files - Audits and updates docs for accuracy and relevance - Improves test coverage
Co-authored-by: sachin shinde <sachinshinde7676@gmail.com>
umair-ably
force-pushed
the
DX-928-repo-update
branch
from
1c42c11 to
2d57732
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
Release Notes
Documentation
Tests
Chores