Skip to content

fix(pagination): keep the cursor and filters when fetching the next page - #134

Open
pjcdawkins wants to merge 2 commits into
mainfrom
cli-165-organizationsubscriptionlist-repeats-first-50-projects
Open

fix(pagination): keep the cursor and filters when fetching the next page#134
pjcdawkins wants to merge 2 commits into
mainfrom
cli-165-organizationsubscriptionlist-repeats-first-50-projects

Conversation

@pjcdawkins

Copy link
Copy Markdown
Contributor

Problem

organization:subscription:list repeats the first 50 projects regardless of --page or --count. For an organization with 89 subscriptions, --count 0 --format tsv --no-header returned 100 rows containing 50 distinct values.

Three defects, all verified against the API:

  1. The pagination cursor was discarded. These endpoints paginate with an opaque cursor in the _links.next URL (page[after]=…). The commands requested that URL but passed their query parameters again as Guzzle's query request option, which replaces a URL's query string rather than adding to it. The cursor was stripped and page 1 was fetched again.
  2. The next URL cannot simply be followed as-is. The subscriptions endpoint omits a filter written as filter[status][value][], which is the form the CLI sends, so the two query strings have to be merged.
  3. range is not a real parameter. The page size parameter is page[size] (max 100 on this endpoint). range was silently ignored, which is why --count had no effect. --page N was likewise sent as a page query parameter that the endpoint does not accept.

Defect 1 affects 10 loops across 9 files. Six are unbounded while ($url) loops, so rather than duplicating items they never terminate — organization:user:list issued 183 identical requests in 30s with pagination disabled in the configuration, and never returned. Also affected: team:user:list and team:project:list with --count 0, the shared team and organization-member loaders, team:create's label uniqueness check, and team:project:add's project lookup.

Change

PaginationUtil::nextPage() merges the next page URL's query with the query used for the current page, and returns null when the next request would repeat the one just made, so a loop cannot spin forever. Every loop that follows a next link with request options now goes through it.

organization:subscription:list and team:project:list send page[size] instead of range. --count on the subscription list accepts up to the API maximum of 100, and --count 0 uses that page size. --page N walks the cursor to reach page N, which costs N sequential requests.

Loops that already passed no request options are unchanged (Api::listMembers(), TeamUserDeleteCommand, TeamListCommand::loadTeamsOnProject()), as is AccessApi, which uses Collection::fetchNextPage().

Verification

make lint and make test in legacy/ pass. PaginationUtilTest covers filter and sort preservation, page size precedence, no next page, and the repeat-request guard. (WelcomeCommandTest::testWelcomeOnLocalContainer fails on main too, for an unrelated missing detection.git_remote_name default.)

Against an organization with 89 subscriptions, 52 members and a team of 31 projects:

Command Before After
org:subs --count 0 100 rows, 50 distinct 89 rows, 89 distinct
org:subs --page 1 -c 50 / --page 2 -c 50 identical 50 and 39 rows, no overlap
org:subs -c 5 50 rows 5 rows
org:users with pagination disabled never returned 52 rows
team:projects -c 5 31 rows 5 rows
team:projects --count 0 31 rows 31 rows

Verbose output confirms that follow-up requests carry both page[after] and the filter[...] parameters, and that --page 3 -c 5 makes exactly three requests with an advancing cursor.

The multi-page paths in team:create and team:project:add were not exercised live — reaching them needs an organization with more than 50 teams, or a write to a real team — but they use the same helper and loop shape as the paths above.

🤖 Generated with Claude Code

The API list endpoints paginate with an opaque cursor in the _links.next
URL, and that URL does not always repeat the parameters that were sent:
the subscriptions endpoint drops a filter written as
filter[status][value][], which is the form the CLI uses. The commands
passed their query parameters again as Guzzle's "query" request option,
which replaces a URL's query string rather than adding to it, so the
cursor was discarded and the first page was fetched repeatedly.

Loops bounded by a "next URL differs" check returned each item twice and
stopped early: organization:subscription:list returned 100 rows with 50
distinct values for an organization with 89 subscriptions. Unbounded
loops never terminated at all, for example organization:user:list when
pagination is disabled in the configuration.

PaginationUtil::nextPage() merges the next page URL's query with the
current one, and returns null when the next request would repeat the one
just made, so a loop cannot spin forever.

organization:subscription:list and team:project:list also sent the page
size as "range", which the API ignores; the parameter is "page[size]".
That is why --count had no effect. organization:subscription:list now
accepts up to the API maximum of 100, and --page walks the cursor to
reach the requested page.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 29, 2026 05:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes cursor-based pagination in the legacy PHP CLI by ensuring “next” links preserve the cursor and any prior query filters/sorts when fetching subsequent pages, and by preventing infinite loops caused by repeating requests. It centralizes the behavior in a new PaginationUtil::nextPage() helper and updates affected commands to use API-correct paging parameters.

Changes:

  • Add PaginationUtil::nextPage() to merge the current request query with the API-provided _links.next query (cursor), with a guard to avoid repeat requests.
  • Refactor multiple command pagination loops to route “next page” handling through PaginationUtil.
  • Update affected commands to use page[size] (and cursor-walking for --page) instead of unsupported parameters like range/page, and add PHPUnit coverage for the helper.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
legacy/src/Util/PaginationUtil.php New helper to compute the next request (URL + merged query) for cursor pagination and prevent repeat-request loops.
legacy/tests/Util/PaginationUtilTest.php Unit tests covering query merge behavior (filters/sorts), precedence, no-next-page, and repeat-request guard.
legacy/src/Command/Team/User/TeamUserListCommand.php Use PaginationUtil for safe next-page traversal while listing team members.
legacy/src/Command/Team/TeamCreateCommand.php Fix team label uniqueness check pagination by preserving cursor + query across pages.
legacy/src/Command/Team/TeamCommandBase.php Update shared team/team-project loaders to follow next links without losing query/cursor and avoid infinite loops.
legacy/src/Command/Team/Project/TeamProjectListCommand.php Use page[size] and PaginationUtil for stable paging and correct --count behavior.
legacy/src/Command/Team/Project/TeamProjectAddCommand.php Ensure org-project lookup pagination preserves query/cursor across pages.
legacy/src/Command/Organization/User/OrganizationUserProjectsCommand.php Use PaginationUtil for paging user extended-access results safely.
legacy/src/Command/Organization/User/OrganizationUserListCommand.php Use PaginationUtil to preserve paging cursor and avoid repeated requests when loading org members.
legacy/src/Command/Organization/OrganizationSubscriptionListCommand.php Correct subscription paging (page[size], cursor-walking for --page), and preserve filters across pages.
legacy/src/Command/Organization/OrganizationCommandBase.php Use PaginationUtil in interactive member selection paging.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +46 to +47
->addOption('page', null, InputOption::VALUE_REQUIRED, 'Page number. This enables pagination, despite configuration or --count.')
->addOption('count', 'c', InputOption::VALUE_REQUIRED, 'The number of items to display per page. Use 0 to disable pagination. Ignored if --page is specified.');
->addOption('count', 'c', InputOption::VALUE_REQUIRED, 'The number of items to display per page (max: ' . self::MAX_COUNT . '). Use 0 to disable pagination.');

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Reworded to "despite the configuration or --count 0", which is what --page actually overrides: --count still sets the page size when --page is used.

--count still sets the page size when --page is used. Only "--count 0",
which disables pagination, is overridden.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants