Skip to content

fix(secrets): paginate ListSecrets to return all entries#2394

Merged
malinskibeniamin merged 4 commits intomasterfrom
bm/fix-secrets-pagination-list-all
Apr 20, 2026
Merged

fix(secrets): paginate ListSecrets to return all entries#2394
malinskibeniamin merged 4 commits intomasterfrom
bm/fix-secrets-pagination-list-all

Conversation

@malinskibeniamin
Copy link
Copy Markdown
Contributor

Summary

  • useListSecretsQuery issued a single ListSecrets call with pageSize=25 and ignored nextPageToken, silently hiding every secret past the first page. Users with >25 secrets could not see entries like PGDB_DSN on the Secrets Store page (and anywhere else the hook is consumed: MCP servers, knowledge bases, AI agents, shadow links, pipeline command menu, Redpanda Connect onboarding).
  • Rewrote the hook to loop nextPageToken via callUnaryMethod until the server returns an empty token, aggregating all pages into a single { secrets } result. Page size now 50, matching the server-side maximum declared in proto/redpanda/api/dataplane/v1/secret.proto.
  • Existing mutation invalidations keep working (same createConnectQueryKey base key).

Repro

  1. Seed a cluster with >25 secrets.
  2. Curl console-*.byoc.../redpanda.api.console.v1alpha1.SecretService/ListSecrets with {"request":{"pageSize":25}} — response includes a nextPageToken.
  3. Open /secrets in the UI — any secret alphabetically past the first 25 is missing.

After this fix the UI paginates server-side transparently and the table renders the full set (verified against a tenant with 119 secrets).

Test plan

  • bun vitest run src/components/pages/secrets-store — 17 / 17 pass, including a new follows nextPageToken until all secrets are returned test that asserts the hook walks page1 → page2 → stop.
  • tsgo --noEmit clean on touched files.
  • Manual smoke on a preprod cluster with >50 secrets — confirm Secrets Store lists all entries and the MCP / knowledgebase / agent secret pickers see the full set.

Follow-ups (not in this PR)

MAX_PAGE_SIZE = 25 is still used as a "fetch all" value in other list hooks (topics, users, security, ai-agent, pipeline, service-accounts, shadowlinks, quotas, remote-mcp, transforms). Same silent-truncation bug applies; each needs the same pagination-loop treatment. Filing / fixing separately to keep this PR scoped.

🤖 Generated with Claude Code

malinskibeniamin and others added 2 commits April 20, 2026 19:09
useListSecretsQuery sent a single request with pageSize=25 and dropped
everything past the first page, silently hiding secrets (e.g. PGDB_DSN
in accounts with >25 secrets). Loop nextPageToken via callUnaryMethod
until exhausted and aggregate into a single result, at the server-side
max of 50 per page.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add a hard cap of 200 iterations and a same-token guard so a misbehaving
server cannot trap the client in an infinite pagination loop. Also honor
react-query's AbortSignal so in-flight pages cancel on unmount or refetch.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@malinskibeniamin
Copy link
Copy Markdown
Contributor Author

@claude review

@claude
Copy link
Copy Markdown

claude Bot commented Apr 20, 2026

Claude finished @malinskibeniamin's task in 2m 12s —— View job


PR Review

  • Gather context and read changed files
  • Review secret.tsx implementation
  • Review test file changes
  • Check CLAUDE.md guidelines
  • Post final review

Summary

This is a correct, well-scoped fix for a real user-facing bug. The pagination loop is sound, the safety guards are thoughtful, and the tests cover the meaningful edge cases. A few minor observations below.


frontend/src/react-query/api/secret.tsx

The approach is idiomatic. Switching from Connect Query's useQuery to vanilla TanStack useQuery with callUnaryMethod + useTransport is the right pattern for multi-page aggregation — Connect Query's useQuery wrapper doesn't expose iteration control.

SECRETS_LIST_PAGE_SIZE = 50 — matches the server-side proto upper bound. Good.

Abort signal forwarding (line 67 + line 76): The early-check signal?.aborted at the top of each iteration combined with passing { signal } to callUnaryMethod covers both the between-requests and in-flight cancellation cases. Clean.

Non-advancing token guard (lines 87–89): Correct. Detects stuck → stuck in one extra round-trip and throws.

Max-pages cap (line 65): SECRETS_LIST_MAX_PAGES = 200 is a reasonable backstop. At 50 secrets/page that's 10k secrets — safe headroom. One consideration: without retry: false on the query, React Query would retry a thrown error up to 3 times by default. For the max-pages scenario that's 200 calls × 4 attempts = 800 server round-trips before settling in error state. This won't happen in practice but is worth noting. The non-advancing-token guard is only 2 × 4 = 8 calls, which is fine. Fix this →

Options type narrowed (line 48): The old signature accepted QueryOptions<GenMessage<ListSecretsRequest>, ListSecretsResponse> (which includes staleTime, gcTime, select, etc.). The new signature only allows { enabled?: boolean }. All current callers only use enabled, so there's no breakage today — but this is a public API narrowing. If any future caller wants to pass staleTime they'll need to widen the type again. Minor, but worth being intentional about.

Mutation invalidation (lines 126–131, 151–156, 173–178): Uses exact: false against the base Connect Query key, which is a prefix of the new custom key ([...connectKey, { nameContains }]). Invalidations will correctly match all variants (no-filter and filtered). ✅

SECRETS_LIST_MAX_PAGES is exported but has no external consumers. It's used only inside this module. Fine to export for testability, but worth knowing it's internal-only right now.


frontend/src/components/pages/secrets-store/secrets-store-list-page.test.tsx

New test: follows nextPageToken until all secrets are returned (line 90) — covers the core fix. Asserts both secrets appear and that listSecrets was called twice with the correct pageToken on the second call. ✅

New test: stops and surfaces an error if the server returns a non-advancing pageToken (line 128) — correctly asserts the guard trips after 2 calls. This works reliably because the test harness has retry: false configured (confirmed in test-utils.tsx). ✅

No test for SECRETS_LIST_MAX_PAGES exhaustion — a minor gap. Testing 200 iterations isn't practical, but a test that sets a small cap (e.g., via mocking the constant or passing a custom limit) would give direct coverage. Low priority given the guard is proven by the non-advancing test.

Fixture comment (line 49): // Hoisted once — 25 rows = 3 pages at the page's hard-coded pageSize of 10. The "hoisted once" phrasing is a bit unusual for a module-level const (reads like it was formerly inside a beforeAll). Purely cosmetic.


Verdict

LGTM with minor suggestions. The core fix is correct and safe to ship. The retry: false addition would be the one change I'd consider before merge — everything else is nits.
| Branch: bm/fix-secrets-pagination-list-all

malinskibeniamin and others added 2 commits April 20, 2026 19:23
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
queryFn throws intentionally on guard violations (non-advancing token,
max-pages exceeded). React Query's default 3-retry policy would
otherwise multiply the already-bounded runaway-pagination traffic by 4×
before surfacing the failure.

Addresses PR review feedback on #2394.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@malinskibeniamin
Copy link
Copy Markdown
Contributor Author

Addressed review:

  • Added retry: false to useListSecretsQuery so the thrown guard errors (non-advancing token, max-pages cap) don't get multiplied 4× by React Query's default retry policy. Commit: 7dbcc64.

Skipped intentionally:

  • Options type narrowing — kept { enabled?: boolean } since that's all every current caller uses; wider signature can be reintroduced if a future caller actually needs staleTime/select, which would also force us to revisit how those interact with a multi-page aggregating queryFn.
  • SECRETS_LIST_MAX_PAGES exhaustion test — agree with the reviewer's "low priority" call. The non-advancing-token test exercises the same throw path; running 200 mocked iterations just to reach the other throw adds test time without catching a distinct bug class.
  • Fixture-comment phrasing nit — cosmetic, leaving alone.

@malinskibeniamin malinskibeniamin enabled auto-merge (squash) April 20, 2026 17:39
@malinskibeniamin malinskibeniamin self-assigned this Apr 20, 2026
@malinskibeniamin malinskibeniamin requested review from a team, SpicyPete, datamali, eblairmckee and sago2k8 and removed request for a team April 20, 2026 18:15
@malinskibeniamin malinskibeniamin added bug Something isn't working frontend ai-jam AI Jam April 2026 related work ui/ux team/ux labels Apr 20, 2026
@malinskibeniamin malinskibeniamin merged commit a902c15 into master Apr 20, 2026
17 checks passed
@malinskibeniamin malinskibeniamin deleted the bm/fix-secrets-pagination-list-all branch April 20, 2026 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-jam AI Jam April 2026 related work bug Something isn't working frontend team/ux ui/ux

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants