🦋 Changeset detected

Latest commit: a4b1ab5

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Codeowners approval required for this PR:

• ✅ @cloudflare/wrangler

✅ All changesets look good

The list.ts just passes page and per_page directly -- no auto-pagination. The test mock handler (without { once: true }) can handle multiple requests, which is needed since the mock now simulates pagination, but the list command itself only makes one request per invocation. The test assertions changed from count === 1 to count > 0 and dataCount === 1, which accounts for the mock potentially serving the pagination-capable handler. This is fine.

After thorough review, I've found one real logic issue:

Bug in get.ts: When getPipeline throws an error that is NOT an APIError at all (e.g., a TypeError, network error, etc.), or is an APIError with a code other than 1000/2 -- the code correctly re-throws. However, when it IS code 1000/2 and findPipelineByName itself throws an unexpected error (e.g., network failure during the list call), that error propagates from line 46 and the original error from getPipeline is lost. This is actually fine -- the secondary failure is more relevant.

Actually, I realize there's no real bug here. The whole flow is correct.

Let me look one more time at a subtlety -- in get.ts, error is used on line 63 (throw error), but if findPipelineByName successfully returns a pipeline (line 47-48), we fall through to line 62 where !pipeline is false, so we skip the throw. But error is a const in the catch scope, so TypeScript is happy. And it's never used if pipeline was found. Good.

Let me check one more edge case in the test expectations -- the "resolves pipeline by name across multiple pages" test:

The test sets perPageLimit: 1 and creates 3 pipelines, but only target_pipeline should match the name filter. The mock filters by name, so filteredPipelines has length 1. With perPage: 1, first page returns the single match, second page returns empty, so listRequest.count === 2. The test asserts expect(listRequest.count).toBe(2). This seems correct given the mock behavior.

Wait actually -- the mock does client-side filtering by name. If expectedName is "target_pipeline", then filteredPipelines will be [{id: "pipeline_target", name: "target_pipeline", ...}] (length 1). With perPageLimit: 1, perPage = min(requestedPerPage, 1) = 1. Page 1: startIndex = 0, returns 1 item. Page 2: startIndex = 1, returns 0 items. So indeed 2 requests, 1 with data. The assertion listRequest.count === 2 is correct.

After this thorough review, I believe the code is well-structured and correct. The patterns are consistent, error handling is proper, and tests cover the key scenarios. There are no logic bugs, security issues, or backward compatibility problems.

LGTM

github run

npm i https://pkg.pr.new/create-cloudflare@13960

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@13960

npm i https://pkg.pr.new/miniflare@13960

npm i https://pkg.pr.new/@cloudflare/pages-shared@13960

npm i https://pkg.pr.new/@cloudflare/unenv-preset@13960

npm i https://pkg.pr.new/@cloudflare/vite-plugin@13960

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@13960

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@13960

npm i https://pkg.pr.new/@cloudflare/workers-utils@13960

npm i https://pkg.pr.new/wrangler@13960

commit: a4b1ab5

I think this will need a docs update with the new options...

I think this will need a docs update with the new options...

you mean bonk won't do this for me 😢. Docs update incoming...

@petebacondarwin corresponding docs PR cloudflare/cloudflare-docs#31025