[DX-960] Add automatic pagination support to list and history commands#170
Draft
umair-ably wants to merge 3 commits intoDX-919-pushfrom
Draft
[DX-960] Add automatic pagination support to list and history commands#170umair-ably wants to merge 3 commits intoDX-919-pushfrom
umair-ably wants to merge 3 commits intoDX-919-pushfrom
Conversation
…ptions, publishing, and configuration
- Guard deprecation warning in set-apns-p12 for JSON mode - Use formatWarning() and formatClientId() in push channels list - Validate --data flag parses to a JSON object in push devices save - Validate all JSON flags (--recipient, --payload, --data, --apns, --fcm, --web) parse to objects in push publish - Let E2E test setup failures propagate instead of silently skipping
|
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:
WalkthroughIntroduces comprehensive pagination support across the Ably CLI by adding a new pagination utility module ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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
changed the title
umair-ably
changed the title
There was a problem hiding this comment.
Pull request overview
Adds shared pagination helpers and wires them into multiple CLI list/history commands so outputs respect --limit while correctly indicating when additional results exist.
Changes:
- Introduces
collectPaginatedResults,collectHttpPaginatedResults, andcollectFilteredPaginatedResultsutilities plus unit tests. - Updates multiple commands to fetch additional pages when needed and to emit
hasMore(and optional warnings about extra page fetches). - Updates test mocks and command unit tests to return PaginatedResult-like objects.
Reviewed changes
Copilot reviewed 34 out of 34 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/utils/pagination.ts |
Adds generic pagination collection helpers used by commands. |
src/commands/channels/list.ts |
Uses HTTP pagination collector for /channels enumeration; returns hasMore. |
src/commands/channels/history.ts |
Uses pagination collector for channel history; returns hasMore and limit warnings. |
src/commands/logs/history.ts |
Uses pagination collector for app log history; returns hasMore. |
src/commands/logs/push/history.ts |
Uses pagination collector for push log history; returns hasMore. |
src/commands/logs/connection-lifecycle/history.ts |
Uses pagination collector for connection lifecycle logs; returns hasMore. |
src/commands/push/devices/list.ts |
Uses pagination collector for device registrations list; returns hasMore. |
src/commands/push/channels/list.ts |
Uses pagination collector for channel subscriptions list; returns hasMore. |
src/commands/push/channels/list-channels.ts |
Uses pagination collector for listChannels; returns hasMore. |
src/commands/rooms/messages/history.ts |
Uses pagination collector for chat message history; returns hasMore. |
src/commands/rooms/list.ts |
Uses filtered pagination + dedupe for chat rooms enumeration; returns hasMore. |
src/commands/spaces/list.ts |
Uses filtered pagination + dedupe for spaces enumeration; returns hasMore. |
src/commands/apps/list.ts |
Adds --limit and hasMore for apps listing (client-side slice). |
src/commands/apps/rules/list.ts |
Adds --limit and hasMore for namespaces/rules listing (client-side slice). |
src/commands/auth/keys/list.ts |
Adds --limit and hasMore for keys listing (client-side slice). |
src/commands/integrations/list.ts |
Adds --limit and hasMore for integrations listing (client-side slice). |
src/commands/queues/list.ts |
Adds --limit and hasMore for queues listing (client-side slice). |
test/helpers/mock-ably-rest.ts |
Adds createMockPaginatedResult and updates default mocks to return paginated-like results. |
test/helpers/mock-ably-chat.ts |
Adds a history mock on room messages. |
test/unit/utils/pagination.test.ts |
New unit tests for pagination utilities. |
test/unit/commands/channels/list.test.ts |
Updates mocks to use paginated-like results. |
test/unit/commands/channels/history.test.ts |
Updates mocks to use paginated-like results. |
test/unit/commands/logs/history.test.ts |
Updates mocks to use paginated-like results and simulates hasMore. |
test/unit/commands/logs/push/history.test.ts |
Updates mocks to use paginated-like results. |
test/unit/commands/logs/connection-lifecycle/history.test.ts |
Updates mocks to use paginated-like results. |
test/unit/commands/push/devices/list.test.ts |
Updates mocks to use paginated-like results. |
test/unit/commands/push/channels/list.test.ts |
Updates mocks to use paginated-like results. |
test/unit/commands/push/channels/list-channels.test.ts |
Updates mocks to use paginated-like results. |
test/unit/commands/rooms/messages.test.ts |
Updates history mocks to use paginated-like results. |
test/unit/commands/rooms/messages/history.test.ts |
Updates history mocks to use paginated-like results. |
test/unit/commands/rooms/list.test.ts |
Updates mocks to use paginated-like results. |
test/unit/commands/spaces/list.test.ts |
Updates mocks to use paginated-like results. |
README.md |
Documents new --limit flags for control-plane list commands. |
.claude/skills/ably-new-command/references/patterns.md |
Documents new pagination patterns for new commands. |
💡 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.
umair-ably
force-pushed
the
DX-960-pagination
branch
from
5b1d40e to
74c8680
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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/spaces/list.ts (1)
129-143:⚠️ Potential issue | 🟠 MajorDon't print a false negative when
hasMoreis already true.With the new pagination path,
collectFilteredPaginatedResults()can stop early and still returnhasMore: true. If that happens before any matching spaces are found, Line 141 still printsNo active spaces found.and exits even though later pages may contain matches. Gate this branch on!hasMore, or raise the scan cap for this command.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/spaces/list.ts` around lines 129 - 143, The command prints "No active spaces found." even when pagination indicates more pages exist; update the early-exit condition in the spaces list flow to only log and return when there are genuinely no further pages to scan. Specifically, in the branch that checks limitedSpaces.length === 0, also require !hasMore (from the collectFilteredPaginatedResults result) before logging and returning; alternatively, if intended, raise the scan cap used by collectFilteredPaginatedResults so it scans further, but the minimal fix is to gate the no-results branch on !hasMore to avoid false negatives.
🧹 Nitpick comments (12)
test/helpers/mock-ably-chat.ts (1)
240-244: Consider usingcreateMockPaginatedResultfor consistency.The inline paginated result mock is missing
isLast(),first(), andcurrent()methods thatcreateMockPaginatedResultintest/helpers/mock-ably-rest.tsprovides. While the current shape works for the pagination utilities (which only useitems,hasNext(),next()), using the shared helper ensures interface consistency and prevents potential test failures if other code expects the full shape.♻️ Proposed refactor using createMockPaginatedResult
Add the import at the top of the file:
import { createMockPaginatedResult } from "./mock-ably-rest.js";Then update the mock:
- history: vi.fn().mockResolvedValue({ - items: [], - hasNext: () => false, - next: async () => null, - }), + history: vi.fn().mockResolvedValue(createMockPaginatedResult([])),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/helpers/mock-ably-chat.ts` around lines 240 - 244, Replace the ad-hoc history mock with the shared helper to ensure full paginated result shape: import and use createMockPaginatedResult from mock-ably-rest and return createMockPaginatedResult([]) from the history mock (the current vi.fn().mockResolvedValue({...}) block). This ensures methods like isLast(), first(), and current() are present alongside items, hasNext(), and next(), keeping the test utilities and expected interfaces consistent.test/helpers/mock-ably-rest.ts (1)
254-280: Make paginated test helper support arbitrary page chains.
createMockPaginatedResultcurrently accepts only one explicitnextPage, which makes it hard to model 3+ non-empty pages in tests. Expanding this helper to accept a page list would improve coverage for deeper pagination flows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/helpers/mock-ably-rest.ts` around lines 254 - 280, The helper createMockPaginatedResult currently only models a single nextPage; change its API to accept an array of pages (e.g., items: T[], pages?: Array<{items: T[]; hasNext?: boolean}>) or a single pages: Array<T[]> so you can build an arbitrary chain, then construct the linked results for each page so next() returns the following page's result or null, hasNext()/isLast() reflect whether a subsequent page exists, and first()/current() still resolve to the appropriate result; implement this by mapping the input pages into result objects up front and wiring each result.next to return the next mapped result (or null) to support 3+ non-empty pages while leaving the public method names (createMockPaginatedResult, next, hasNext, isLast, first, current) unchanged.test/unit/commands/push/devices/list.test.ts (1)
70-85: Add assertions for pagination metadata and a multi-page case.The test now uses paginated mocks, but it doesn’t assert the new pagination contract (
hasMore) or exercise the multi-page branch (pagesConsumed > 1warning path). Adding those assertions will prevent regressions in the new pagination behavior.💡 Minimal assertion update
const result = JSON.parse(stdout); expect(result).toHaveProperty("type", "result"); expect(result).toHaveProperty("success", true); expect(result).toHaveProperty("devices"); + expect(result).toHaveProperty("hasMore", false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/push/devices/list.test.ts` around lines 70 - 85, Update the "should output JSON when requested" test to also assert the pagination contract and add a new multi-page scenario: when using getMockAblyRest() / mock.push.admin.deviceRegistrations.list with createMockPaginatedResult, assert the parsed JSON contains the pagination metadata (e.g., result.pagination.hasMore is present and correct) and keep devices assertions; add a separate test that returns a multi-page paginated mock (simulate pagesConsumed > 1) and assert the multi-page warning path by capturing runCommand's stderr (or combined output) contains the expected warning message about pagesConsumed > 1. Ensure you reference the same symbols: runCommand, getMockAblyRest, mock.push.admin.deviceRegistrations.list, and createMockPaginatedResult.src/commands/apps/rules/list.ts (1)
59-62: Apply pagination at the service layer if the REST API supports it.The
listNamespaces()method currently accepts onlyappIdand fetches all namespaces, then the command truncates locally at line 61. If the/apps/{appId}/namespacesendpoint supports query parameters for pagination, updatelistNamespaces()to accept and pass alimitparameter, reducing unnecessary over-fetch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/apps/rules/list.ts` around lines 59 - 62, The command is fetching all namespaces then slicing locally; instead, update the service method listNamespaces to accept a limit (and optionally offset/page) and pass that through to the REST call so the server paginates (e.g., /apps/{appId}/namespaces?limit=...). Change the call site in this file to pass flags.limit into controlApi.listNamespaces(appId, flags.limit) and update the control API implementation (the listNamespaces function) to accept the new parameter and include it as a query parameter in the HTTP request; also update any other callers of listNamespaces to provide a limit or default it to undefined so behavior remains backward-compatible..claude/skills/ably-new-command/references/patterns.md (1)
162-175: Prefer stderr in the command templates for warning output.The examples currently log warnings with
this.log(...). For command patterns, usethis.logToStderr(...)for warnings so generated commands follow current output-channel conventions.Based on learnings: In the ably/ably-cli TypeScript (oclif) codebase, warning messages should use `this.logToStderr(formatWarning("..."))` and be guarded from `--json` output paths.♻️ Suggested template adjustment
- this.log(formatWarning(`Fetched ${pagesConsumed} pages to retrieve ${messages.length} results. Each page incurs additional API requests.`)); + this.logToStderr(formatWarning(`Fetched ${pagesConsumed} pages to retrieve ${messages.length} results. Each page incurs additional API requests.`)); @@ - if (warning) this.log(warning); + if (warning) this.logToStderr(warning); @@ - this.log(formatWarning(`Fetched ${pagesConsumed} pages to retrieve ${items.length} results. Each page incurs additional API requests.`)); + this.logToStderr(formatWarning(`Fetched ${pagesConsumed} pages to retrieve ${items.length} results. Each page incurs additional API requests.`)); @@ - if (warning) this.log(warning); + if (warning) this.logToStderr(warning);Also applies to: 367-385
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/ably-new-command/references/patterns.md around lines 162 - 175, The warning outputs currently use this.log(...) even when they should go to stderr; update the code so any warning text (the formatWarning(...) call when pagesConsumed > 1 and the formatLimitWarning(...) call when hasMore) is emitted with this.logToStderr(...) and only when !this.shouldOutputJson(flags); keep the json path using this.logJsonResult({ messages, hasMore }, flags) unchanged and ensure you reference the existing symbols pagesConsumed, shouldOutputJson(flags), formatWarning, formatLimitWarning, hasMore, logJsonResult, and replace this.log(...) for warnings with this.logToStderr(...).src/commands/channels/list.ts (1)
107-113: Usethis.logToStderr()for warning output.Consistent with other commands, warning messages should go to stderr.
♻️ Proposed fix
if (pagesConsumed > 1 && !this.shouldOutputJson(flags)) { - this.log( + this.logToStderr( formatWarning( `Fetched ${pagesConsumed} pages to retrieve ${channels.length} results. Each page incurs additional API requests.`, ), ); }Based on learnings: "Always use this.logToStderr(formatWarning('...')) for warning messages."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/channels/list.ts` around lines 107 - 113, The warning currently uses this.log(...) in the conditional that checks pagesConsumed and shouldOutputJson; replace the this.log call with this.logToStderr(formatWarning(...)) so warnings are written to stderr consistently (keep the same message using pagesConsumed and channels.length and leave the surrounding conditional intact). Update the occurrence where formatWarning is passed to this.log in the channels listing logic to call this.logToStderr instead, ensuring behavior matches other commands that route warnings to stderr.src/commands/logs/push/history.ts (1)
70-76: Usethis.logToStderr()for warning output.Same issue as other history commands - warning messages should go to stderr.
♻️ Proposed fix
if (pagesConsumed > 1 && !this.shouldOutputJson(flags)) { - this.log( + this.logToStderr( formatWarning( `Fetched ${pagesConsumed} pages to retrieve ${messages.length} results. Each page incurs additional API requests.`, ), ); }Based on learnings: "Always use this.logToStderr(formatWarning('...')) for warning messages."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/logs/push/history.ts` around lines 70 - 76, The warning currently uses this.log(...) which sends output to stdout; change it to this.logToStderr(...) so warnings go to stderr. In the block that checks pagesConsumed > 1 && !this.shouldOutputJson(flags), replace the call this.log(formatWarning(...)) with this.logToStderr(formatWarning(...)) while preserving the same message content (use pagesConsumed and messages.length as before) so behavior is identical except the output stream.src/commands/rooms/list.ts (2)
124-130: Usethis.logToStderr()for warning output.Consistent with other commands, warning messages should go to stderr.
♻️ Proposed fix
if (pagesConsumed > 1 && !this.shouldOutputJson(flags)) { - this.log( + this.logToStderr( formatWarning( `Fetched ${pagesConsumed} pages to retrieve ${limitedRooms.length} results. Each page incurs additional API requests.`, ), ); }Based on learnings: "Always use this.logToStderr(formatWarning('...')) for warning messages."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/rooms/list.ts` around lines 124 - 130, The warning currently uses this.log(...) in the rooms listing code (check pagesConsumed, shouldOutputJson and the formatWarning call); change it to use this.logToStderr(formatWarning(...)) so the warning is emitted to stderr consistently with other commands and only when pagesConsumed > 1 && !this.shouldOutputJson(flags). Replace the this.log(...) call with this.logToStderr(...) while keeping the same message text and conditional logic.
110-121: Filter predicate has side effects - consider documenting or refactoring.The filter function mutates
channel.channelIdandchannel.room(lines 118-119). While this works because JavaScript passes objects by reference, side effects in filter predicates are unconventional and can be surprising. The pattern is functional but worth a brief inline comment explaining the intentional mutation for room name normalization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/rooms/list.ts` around lines 110 - 121, The predicate passed to filter mutates the RoomItem (it overwrites channel.channelId and channel.room) which is surprising; either move the normalization out of the filter or document the intent. Fix by turning the predicate into a pure boolean test (use channelId/includes and regex match, check seenRooms) and then perform the mutation in a separate mapping step (e.g., after filtering, set channel.channelId/channel.room to the extracted roomName), or if you keep the in-filter mutation, add a concise inline comment above the predicate explaining that mutation of channel.channelId and channel.room is intentional for room name normalization (references: RoomItem, seenRooms, channel.channelId, channel.room).src/commands/logs/connection-lifecycle/history.ts (1)
69-75: Usethis.logToStderr()for warning output.Same issue as other history commands - warning messages should go to stderr.
♻️ Proposed fix
if (pagesConsumed > 1 && !this.shouldOutputJson(flags)) { - this.log( + this.logToStderr( formatWarning( `Fetched ${pagesConsumed} pages to retrieve ${messages.length} results. Each page incurs additional API requests.`, ), ); }Based on learnings: "Always use this.logToStderr(formatWarning('...')) for warning messages."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/logs/connection-lifecycle/history.ts` around lines 69 - 75, The warning is currently emitted with this.log(...) which writes to stdout; change it to this.logToStderr(...) so warnings go to stderr. Specifically, in the block that checks pagesConsumed > 1 and !this.shouldOutputJson(flags), replace the this.log(formatWarning(...)) call with this.logToStderr(formatWarning(...))—keep the same formatWarning(...) payload and leave the pagesConsumed, messages.length and shouldOutputJson(flags) logic untouched.src/commands/logs/history.ts (1)
69-75: Usethis.logToStderr()for warning output.Per project conventions, warning messages should be written to stderr, not stdout. This prevents warnings from polluting piped output.
♻️ Proposed fix
if (pagesConsumed > 1 && !this.shouldOutputJson(flags)) { - this.log( + this.logToStderr( formatWarning( `Fetched ${pagesConsumed} pages to retrieve ${messages.length} results. Each page incurs additional API requests.`, ), ); }Based on learnings: "In the ably/ably-cli TypeScript (oclif) codebase, do not use this.warn() directly in command implementations. Always use this.logToStderr(formatWarning('...')) for warning messages."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/logs/history.ts` around lines 69 - 75, The current warning uses this.log(...) which writes to stdout; change it to write to stderr by calling this.logToStderr(...) instead when emitting the warning (the conditional that checks pagesConsumed > 1 and !this.shouldOutputJson(flags)); replace the this.log(formatWarning(...)) call with this.logToStderr(formatWarning(...)) so warnings use stderr per project convention, keeping the same formatWarning text and preserving the pagesConsumed / messages.length values.test/unit/commands/spaces/list.test.ts (1)
50-53: Add a real multi-page fixture forspaces:list.Every mocked response here is still a single page, so the new cross-page behavior in
src/commands/spaces/list.tsnever runs. A two-page fixture with duplicates split across pages would exerciseseenSpaces,hasMore, and the multi-page warning path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/commands/spaces/list.test.ts` around lines 50 - 53, The test currently only returns a single-page response via mock.request with createMockPaginatedResult(createMockSpaceChannelItems()), so the new multi-page logic in src/commands/spaces/list.ts (seenSpaces, hasMore and the multi-page warning path) never executes; update the test to mock multiple paginated responses by using mock.request.mockResolvedValueOnce for the first page (hasMore: true) and mockResolvedValueOnce for the second page (hasMore: false), crafting the two page fixtures with createMockPaginatedResult and createMockSpaceChannelItems variants such that at least one space ID appears on both pages to exercise deduplication and trigger the multi-page warning path.
🤖 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/apps/rules/list.ts`:
- Around line 117-124: The warning is being written to stdout with this.log(...)
inside the hasMore block; change it to emit to stderr and avoid JSON mode by
using this.logToStderr(...) and wrapping the call so it only runs when not in
JSON output mode (respecting the command's JSON flag handling). Update the
branch that builds warning via formatLimitWarning(namespaces.length,
flags.limit, "channel rules") to call this.logToStderr(...) instead of
this.log(...) and ensure the warning is skipped when output is JSON.
In `@src/commands/auth/keys/list.ts`:
- Around line 111-114: The limit warning is being printed to stdout via
this.log(...) in the hasMore branch; change it to emit on stderr using
this.logToStderr and wrap the warning in the standard warning formatter. Replace
the this.log(warning) call with
this.logToStderr(formatWarning(formatLimitWarning(keys.length, flags.limit,
"keys"))) so the warning produced by formatLimitWarning(...) is sent to stderr
using formatWarning(...).
In `@src/commands/channels/history.ts`:
- Around line 94-100: Replace the two warning prints that currently call
this.log(formatWarning(...)) with this.logToStderr(formatWarning(...));
specifically change the warning inside the pagesConsumed > 1 &&
!this.shouldOutputJson(flags) branch (the one that mentions fetched pages and
messages) and the other similar warning later in the file so warnings go to
stderr while preserving the existing shouldOutputJson gating and the same
formatWarning text.
In `@src/commands/push/channels/list-channels.ts`:
- Around line 53-59: The warning message is currently sent to stdout via
this.log(...) in the pagesConsumed check; update the call to
this.logToStderr(formatWarning(...)) so warnings are emitted to stderr instead.
Locate the block that checks "if (pagesConsumed > 1 &&
!this.shouldOutputJson(flags))" and replace the this.log(...) invocation with
this.logToStderr(...), and make the same replacement in the other warning branch
referenced (the similar block around the 81-88 range) that also uses
formatWarning and shouldOutputJson(flags).
In `@src/commands/push/channels/list.ts`:
- Around line 75-81: The pagination warning currently uses
this.log(formatWarning(...)) which should write to stderr; replace the calls
where pagesConsumed is checked (the block that calls this.log with
formatWarning) with this.logToStderr(formatWarning(...)) and do the same for the
other similar warning block (the second occurrence that also checks
pagesConsumed / shouldOutputJson). Ensure you keep the use of
shouldOutputJson(flags), formatWarning(...), and the existing message text
unchanged while swapping this.log to this.logToStderr.
In `@src/commands/push/devices/list.ts`:
- Around line 71-77: The warning currently written with
this.log(formatWarning(...)) (e.g., the pagesConsumed/devices message) is going
to stdout; change it to this.logToStderr(formatWarning(...)) while keeping the
existing guard that checks !this.shouldOutputJson(flags) so JSON output isn't
polluted; update the other similar warning block (the second occurrence guarded
by shouldOutputJson) the same way and ensure you reference the same
formatWarning(...) call and variables pagesConsumed and devices to locate the
spots.
In `@src/commands/rooms/messages/history.ts`:
- Around line 143-149: The warning currently uses this.log(...) which writes to
stdout and can contaminate --json output; change calls that emit
formatWarning(...) (e.g., the block checking pagesConsumed > 1 with
!this.shouldOutputJson(flags) and the similar block later around the other
warning) to use this.logToStderr(formatWarning(...)) instead of this.log(...).
Locate usages by searching for formatWarning(...) and the pagesConsumed /
shouldOutputJson(flags) checks and replace the this.log invocation with
this.logToStderr to route warnings to stderr.
In `@src/commands/spaces/list.ts`:
- Around line 118-123: The warning is being written to stdout with this.log(...)
which mixes diagnostics into command output; change the call inside the
pagesConsumed > 1 && !this.shouldOutputJson(flags) block to use
this.logToStderr(formatWarning(...)) instead so the formatted warning is sent to
stderr. Update the invocation that currently logs the message built with
formatWarning(`Fetched ${pagesConsumed} pages to retrieve
${limitedSpaces.length} results...`) leaving the condition and message intact.
Ensure you only replace this.log with this.logToStderr in that block so JSON
output paths (shouldOutputJson) remain unchanged.
- Around line 104-114: The filter currently treats IDs like "orders::$spacey" as
spaces because it uses includes("::$space") and /^(.+?)::\$space.*$/; update the
check to ensure "$space" is a full namespace segment by using a stricter regex
(e.g. test for ::\$space followed by end or another ::) before extracting the
prefix; in the block around SpaceItem/channel handling (references: SpaceItem,
seenSpaces, channel.channelId, spaceNameMatch) replace the includes and the
existing match with a single regex test/extract (e.g. /^(.+?)::\$space(?:$|::)/)
so only true "::\$space" segments are accepted, then proceed to add to
seenSpaces and set channel.channelId/channel.spaceName as before.
In `@src/utils/pagination.ts`:
- Around line 24-27: The function collectPaginatedResults currently accepts any
integer for the limit parameter which allows non-positive values and leads to
invalid slice semantics; add a guard at the start of collectPaginatedResults
that handles limit <= 0 (either by throwing a clear ArgumentError or by treating
it as 0 and returning an empty PaginationResult) and ensure you clamp or
normalize limit with something like maxLimit = Math.max(0, limit) before any
slice/truncation logic; apply the same guard/normalization to the other
pagination helper in this file that also accepts a limit (the helper referenced
at lines 69-74) so no negative or zero limits produce incorrect slice(0, -n)
behavior.
- Around line 91-95: The hasMore boolean currently includes pagesConsumed >=
maxPages which can return true even when no more pages exist; change the hasMore
calculation to only consider actual evidence of more results (items.length >
limit or currentPage?.hasNext()) — remove the pagesConsumed >= maxPages clause
from hasMore (symbols: hasMore, items, limit, currentPage.hasNext(),
pagesConsumed, maxPages, truncated) so hasMore is only true when there are extra
items or the currentPage reports a next page.
---
Outside diff comments:
In `@src/commands/spaces/list.ts`:
- Around line 129-143: The command prints "No active spaces found." even when
pagination indicates more pages exist; update the early-exit condition in the
spaces list flow to only log and return when there are genuinely no further
pages to scan. Specifically, in the branch that checks limitedSpaces.length ===
0, also require !hasMore (from the collectFilteredPaginatedResults result)
before logging and returning; alternatively, if intended, raise the scan cap
used by collectFilteredPaginatedResults so it scans further, but the minimal fix
is to gate the no-results branch on !hasMore to avoid false negatives.
---
Nitpick comments:
In @.claude/skills/ably-new-command/references/patterns.md:
- Around line 162-175: The warning outputs currently use this.log(...) even when
they should go to stderr; update the code so any warning text (the
formatWarning(...) call when pagesConsumed > 1 and the formatLimitWarning(...)
call when hasMore) is emitted with this.logToStderr(...) and only when
!this.shouldOutputJson(flags); keep the json path using this.logJsonResult({
messages, hasMore }, flags) unchanged and ensure you reference the existing
symbols pagesConsumed, shouldOutputJson(flags), formatWarning,
formatLimitWarning, hasMore, logJsonResult, and replace this.log(...) for
warnings with this.logToStderr(...).
In `@src/commands/apps/rules/list.ts`:
- Around line 59-62: The command is fetching all namespaces then slicing
locally; instead, update the service method listNamespaces to accept a limit
(and optionally offset/page) and pass that through to the REST call so the
server paginates (e.g., /apps/{appId}/namespaces?limit=...). Change the call
site in this file to pass flags.limit into controlApi.listNamespaces(appId,
flags.limit) and update the control API implementation (the listNamespaces
function) to accept the new parameter and include it as a query parameter in the
HTTP request; also update any other callers of listNamespaces to provide a limit
or default it to undefined so behavior remains backward-compatible.
In `@src/commands/channels/list.ts`:
- Around line 107-113: The warning currently uses this.log(...) in the
conditional that checks pagesConsumed and shouldOutputJson; replace the this.log
call with this.logToStderr(formatWarning(...)) so warnings are written to stderr
consistently (keep the same message using pagesConsumed and channels.length and
leave the surrounding conditional intact). Update the occurrence where
formatWarning is passed to this.log in the channels listing logic to call
this.logToStderr instead, ensuring behavior matches other commands that route
warnings to stderr.
In `@src/commands/logs/connection-lifecycle/history.ts`:
- Around line 69-75: The warning is currently emitted with this.log(...) which
writes to stdout; change it to this.logToStderr(...) so warnings go to stderr.
Specifically, in the block that checks pagesConsumed > 1 and
!this.shouldOutputJson(flags), replace the this.log(formatWarning(...)) call
with this.logToStderr(formatWarning(...))—keep the same formatWarning(...)
payload and leave the pagesConsumed, messages.length and shouldOutputJson(flags)
logic untouched.
In `@src/commands/logs/history.ts`:
- Around line 69-75: The current warning uses this.log(...) which writes to
stdout; change it to write to stderr by calling this.logToStderr(...) instead
when emitting the warning (the conditional that checks pagesConsumed > 1 and
!this.shouldOutputJson(flags)); replace the this.log(formatWarning(...)) call
with this.logToStderr(formatWarning(...)) so warnings use stderr per project
convention, keeping the same formatWarning text and preserving the pagesConsumed
/ messages.length values.
In `@src/commands/logs/push/history.ts`:
- Around line 70-76: The warning currently uses this.log(...) which sends output
to stdout; change it to this.logToStderr(...) so warnings go to stderr. In the
block that checks pagesConsumed > 1 && !this.shouldOutputJson(flags), replace
the call this.log(formatWarning(...)) with this.logToStderr(formatWarning(...))
while preserving the same message content (use pagesConsumed and messages.length
as before) so behavior is identical except the output stream.
In `@src/commands/rooms/list.ts`:
- Around line 124-130: The warning currently uses this.log(...) in the rooms
listing code (check pagesConsumed, shouldOutputJson and the formatWarning call);
change it to use this.logToStderr(formatWarning(...)) so the warning is emitted
to stderr consistently with other commands and only when pagesConsumed > 1 &&
!this.shouldOutputJson(flags). Replace the this.log(...) call with
this.logToStderr(...) while keeping the same message text and conditional logic.
- Around line 110-121: The predicate passed to filter mutates the RoomItem (it
overwrites channel.channelId and channel.room) which is surprising; either move
the normalization out of the filter or document the intent. Fix by turning the
predicate into a pure boolean test (use channelId/includes and regex match,
check seenRooms) and then perform the mutation in a separate mapping step (e.g.,
after filtering, set channel.channelId/channel.room to the extracted roomName),
or if you keep the in-filter mutation, add a concise inline comment above the
predicate explaining that mutation of channel.channelId and channel.room is
intentional for room name normalization (references: RoomItem, seenRooms,
channel.channelId, channel.room).
In `@test/helpers/mock-ably-chat.ts`:
- Around line 240-244: Replace the ad-hoc history mock with the shared helper to
ensure full paginated result shape: import and use createMockPaginatedResult
from mock-ably-rest and return createMockPaginatedResult([]) from the history
mock (the current vi.fn().mockResolvedValue({...}) block). This ensures methods
like isLast(), first(), and current() are present alongside items, hasNext(),
and next(), keeping the test utilities and expected interfaces consistent.
In `@test/helpers/mock-ably-rest.ts`:
- Around line 254-280: The helper createMockPaginatedResult currently only
models a single nextPage; change its API to accept an array of pages (e.g.,
items: T[], pages?: Array<{items: T[]; hasNext?: boolean}>) or a single pages:
Array<T[]> so you can build an arbitrary chain, then construct the linked
results for each page so next() returns the following page's result or null,
hasNext()/isLast() reflect whether a subsequent page exists, and
first()/current() still resolve to the appropriate result; implement this by
mapping the input pages into result objects up front and wiring each result.next
to return the next mapped result (or null) to support 3+ non-empty pages while
leaving the public method names (createMockPaginatedResult, next, hasNext,
isLast, first, current) unchanged.
In `@test/unit/commands/push/devices/list.test.ts`:
- Around line 70-85: Update the "should output JSON when requested" test to also
assert the pagination contract and add a new multi-page scenario: when using
getMockAblyRest() / mock.push.admin.deviceRegistrations.list with
createMockPaginatedResult, assert the parsed JSON contains the pagination
metadata (e.g., result.pagination.hasMore is present and correct) and keep
devices assertions; add a separate test that returns a multi-page paginated mock
(simulate pagesConsumed > 1) and assert the multi-page warning path by capturing
runCommand's stderr (or combined output) contains the expected warning message
about pagesConsumed > 1. Ensure you reference the same symbols: runCommand,
getMockAblyRest, mock.push.admin.deviceRegistrations.list, and
createMockPaginatedResult.
In `@test/unit/commands/spaces/list.test.ts`:
- Around line 50-53: The test currently only returns a single-page response via
mock.request with createMockPaginatedResult(createMockSpaceChannelItems()), so
the new multi-page logic in src/commands/spaces/list.ts (seenSpaces, hasMore and
the multi-page warning path) never executes; update the test to mock multiple
paginated responses by using mock.request.mockResolvedValueOnce for the first
page (hasMore: true) and mockResolvedValueOnce for the second page (hasMore:
false), crafting the two page fixtures with createMockPaginatedResult and
createMockSpaceChannelItems variants such that at least one space ID appears on
both pages to exercise deduplication and trigger the multi-page warning path.
🪄 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: 2d80ce26-d6d5-4260-b00c-404ee2a3205d
📒 Files selected for processing (34)
.claude/skills/ably-new-command/references/patterns.mdREADME.mdsrc/commands/apps/list.tssrc/commands/apps/rules/list.tssrc/commands/auth/keys/list.tssrc/commands/channels/history.tssrc/commands/channels/list.tssrc/commands/integrations/list.tssrc/commands/logs/connection-lifecycle/history.tssrc/commands/logs/history.tssrc/commands/logs/push/history.tssrc/commands/push/channels/list-channels.tssrc/commands/push/channels/list.tssrc/commands/push/devices/list.tssrc/commands/queues/list.tssrc/commands/rooms/list.tssrc/commands/rooms/messages/history.tssrc/commands/spaces/list.tssrc/utils/pagination.tstest/helpers/mock-ably-chat.tstest/helpers/mock-ably-rest.tstest/unit/commands/channels/history.test.tstest/unit/commands/channels/list.test.tstest/unit/commands/logs/connection-lifecycle/history.test.tstest/unit/commands/logs/history.test.tstest/unit/commands/logs/push/history.test.tstest/unit/commands/push/channels/list-channels.test.tstest/unit/commands/push/channels/list.test.tstest/unit/commands/push/devices/list.test.tstest/unit/commands/rooms/list.test.tstest/unit/commands/rooms/messages.test.tstest/unit/commands/rooms/messages/history.test.tstest/unit/commands/spaces/list.test.tstest/unit/utils/pagination.test.ts
umair-ably
force-pushed
the
DX-960-pagination
branch
from
74c8680 to
953eef1
Compare
umair-ably
force-pushed
the
DX-919-push
branch
3 times, most recently
from
3dd2973 to
d415eb2
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
src/utils/pagination.ts) withcollectPaginatedResults,collectHttpPaginatedResults, andcollectFilteredPaginatedResultsthat automatically follow cursor-based pages until the user's--limitis satisfiedhasMoreto JSON output envelopes so consumers can detect truncation programmaticallycount == limit)Motivation
Previously, commands like
ably channels list --limit 200silently returned only the first page (typically 100 items) even when more existed. Users had no way to get all results without scripting the SDK directly. Now the CLI transparently fetches as many pages as needed to honour--limit.Review strategy
src/utils/pagination.ts,test/unit/utils/pagination.test.tshasMoreedge cases, filtered variant withmaxPagessafety capsrc/commands/channels/history.ts,src/commands/logs/history.ts,src/commands/logs/connection-lifecycle/history.ts,src/commands/logs/push/history.ts,src/commands/rooms/messages/history.tscollectPaginatedResults(firstPage, flags.limit), multi-page warning,hasMorein JSONsrc/commands/channels/list.ts,src/commands/push/channels/list-channels.ts,src/commands/push/channels/list.ts,src/commands/push/devices/list.tscollectHttpPaginatedResultssrc/commands/rooms/list.ts,src/commands/spaces/list.tscollectFilteredPaginatedResultsbecause client-side filtering (chat channels, spaces) means raw items != final itemssrc/commands/apps/list.ts,src/commands/apps/rules/list.ts,src/commands/auth/keys/list.ts,src/commands/integrations/list.ts,src/commands/queues/list.ts--limitflag added; no pagination utility needed since Control API returns all resultstest/unit/commands/*/...,test/helpers/mock-ably-rest.ts,test/helpers/mock-ably-chat.tsPaginatedResultshape withhasNext()/next(); tests verifyhasMoreand limit behavioursrc/utils/output.ts,README.md,.claude/skills/...formatLimitWarningonly shown whenhasMoreis true; README auto-regeneratedTest plan
pnpm test:unitpasses — all pagination and command tests greenably channels list --limit 5on an app with >5 active channels returns exactly 5 and shows the limit warningably channels history <channel> --limit 200fetches multiple pages transparently--jsonoutput includes"hasMore": true/false🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
--limitoption to list commands (apps, rules, auth keys, integrations, queues, channels, rooms, and spaces) with a default of 100 results per request.hasMorefield indicating whether additional results are available.Improvements