Add pagination of batch requests - #64061
Open
Wesley Wigham (weswigham) wants to merge 2 commits into
Open
Conversation
… async client it exposed
Wesley Wigham (weswigham)
requested review from
Andrew Branch (andrewbranch)
and
a balanced review from Copilot
August 27, 2026 22:39
| async connect(): Promise<void> { | ||
| if (this.connected) return; | ||
| connect(): Promise<void> { | ||
| if (this.connected) return Promise.resolve(); |
Member
Author
There was a problem hiding this comment.
Somehow the new large-file stress test actually triggered the hang (when used in conjunction with the full suite of API tests) that copilot found a few PRs ago so I went ahead and fixed it here.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds transparent pagination for large batch API responses.
Changes:
- Adds server-side paging and continuation tokens.
- Reassembles pages in synchronous and asynchronous clients.
- Adds protocol types, options, and pagination tests.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
tsc/internal/api/session.go |
Implements pagination and continuation storage. |
tsc/internal/api/session_batch_test.go |
Tests server pagination behavior. |
tsc/internal/api/proto.go |
Extends and custom-encodes the batch protocol. |
packages/typescript/src/api/sync/client.ts |
Reassembles paginated synchronous responses. |
packages/typescript/src/api/async/client.ts |
Reassembles asynchronous responses and deduplicates connection attempts. |
packages/typescript/src/api/proto.ts |
Excludes nested batch requests from request types. |
packages/typescript/src/api/proto.generated.ts |
Adds pagination protocol fields. |
packages/typescript/src/api/options.ts |
Exposes the response-page size option. |
packages/typescript/test/sync/api-generators.test.ts |
Tests synchronous pagination. |
packages/typescript/test/async/api.test.ts |
Tests asynchronous pagination. |
Suppressed comments (1)
packages/typescript/src/api/options.ts:25
- This is not a strict maximum: the server deliberately returns an intact response when one item alone exceeds the limit (see the added oversized-single-response test). Document that exception so callers do not rely on this option to guarantee that every decoded page stays below a hard transport/string-size ceiling.
/** Maximum encoded byte size of each batch response page. Defaults to 300 million bytes. */
maxResponseBytesPerPage?: number;
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+975
to
+977
| s.batchResponsePages.Store(continuationToken, batchResponsePage{ | ||
| encodedResponses: slices.Clone(page.encodedResponses[pageLength:]), | ||
| }) |
| pageParams.maxResponseBytesPerPage = this.maxResponseBytesPerPage; | ||
| } | ||
| const page = this.apiRequest("batchRequests", pageParams); | ||
| responses = responses.concat(page.responses); |
| pageParams.maxResponseBytesPerPage = this.options.maxResponseBytesPerPage; | ||
| } | ||
| const page = await this.sendRequestWithTiming(requestType, pageParams); | ||
| responses = responses.concat(page.responses); |
Comment on lines
+975
to
+977
| s.batchResponsePages.Store(continuationToken, batchResponsePage{ | ||
| encodedResponses: slices.Clone(page.encodedResponses[pageLength:]), | ||
| }) |
Comment on lines
+11
to
+12
| /** Maximum encoded byte size of each batch response page. Defaults to 300 million bytes. */ | ||
| maxResponseBytesPerPage?: number; |
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.
Fixes an issue Titian Cernicova-Dragomir (@dragomirtitian) brought up around the JS max string size (!) for large batches of request responses. On the API client side, there's a
maxResponseBytesPerPageoption that's passed thru to the server with batch requests to limit response sizes , but the server has a default value of 300,000,000 bytes (which is a nice round number just under 66% of the v8 default string max size - to account for base64 encoding overhead) if that's not provided. Paged server responses are automatically reassembled into the full response objects back on the JS end, so nobody should really have to think about the protocol-level pagination.This gets a little weird on the server-side to avoid double-encoding the response messages. Specifically, we have to encode the individual messages early so we can get their size and decide where to paginate and then custom-encode the batch response to be able to directly inline the already-encoded nested response objects. All fine enough things to do, I think, just a little weird compared to other API responses.
A small aside, but this PR also forbids nesting
batchRequestprotocol methods withinbatchRequestcalls - the client already flattens incoming generators, and forbidding it makes pagination much simpler, so seems preferable to complicating pagination further.