feat(api): introduce RequestConfigBuilder for SDK-agnostic abort signal support#701
Conversation
…nfiguration Body: Implement generic request configuration builder with chainable methods (addAbortSignal, addHeaders, setOption), static factory methods (fromMetadata, mergeAbortSignals), and 40 unit tests.
… and multi-SDK examples
easonLiangWorldedtech
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughA new generic ChangesRequestConfigBuilder: Implementation, Tests, and Docs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: private package registry requires authentication. Disable ESLint in CodeRabbit settings or use public packages. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/api/providers/config-builder/request-config-builder.ts (1)
119-129: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider simplifying the early-abort case.
When
primarySignalis already aborted, the code creates a newAbortController, aborts it, and returns its signal. Since the caller can useprimarySignaldirectly in this case (it's already aborted), you could return it instead:const controller = new AbortController() if (primarySignal.aborted) { - controller.abort() - return controller.signal + return primarySignal }This avoids allocating an unnecessary controller. The current implementation is correct and defensive, so this is purely an optimization.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/api/providers/config-builder/request-config-builder.ts` around lines 119 - 129, In the mergeAbortSignals method, the code currently creates a new AbortController and aborts it when primarySignal is already aborted, then returns the new controller's signal. Since an already-aborted signal serves the same purpose, simplify this early-abort case by returning primarySignal directly instead of allocating and aborting a new controller. This optimization eliminates unnecessary object creation while maintaining the same behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/api/providers/config-builder/README.md`:
- Line 18: The link fragment in the table of contents entry
`#how-mergesignals-works` does not match the actual heading "How
`mergeAbortSignals` Works" on line 196. Update the link fragment from
`#how-mergesignals-works` to `#how-mergeabortsignals-works` so that it correctly
points to the corresponding heading and the anchor link functions properly.
---
Nitpick comments:
In `@src/api/providers/config-builder/request-config-builder.ts`:
- Around line 119-129: In the mergeAbortSignals method, the code currently
creates a new AbortController and aborts it when primarySignal is already
aborted, then returns the new controller's signal. Since an already-aborted
signal serves the same purpose, simplify this early-abort case by returning
primarySignal directly instead of allocating and aborting a new controller. This
optimization eliminates unnecessary object creation while maintaining the same
behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 513b97a4-e024-4847-beb2-9648492cd853
📒 Files selected for processing (4)
src/api/providers/__tests__/request-config-builder.spec.tssrc/api/providers/config-builder/README.mdsrc/api/providers/config-builder/request-config-builder.tssrc/api/providers/index.ts
…ls early-abort - Fix README TOC: change #how-mergesignals-works to #how-mergeabortsignals-works to match the actual heading anchor - Simplify mergeAbortSignals: return primarySignal directly when it's already aborted instead of creating a new AbortController
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Thanks for working on this — the intent to reduce duplication across providers is appreciated. However, after reviewing this against the tracking issue (#404) and its sub-issues (#615–#618), I think this PR should either be closed or substantially reworked.
The sub-issues explicitly chose direct wiring over an abstraction
The issue decomposition already prescribes specific patterns for each provider group:
- #616 (simple providers, ~18 of them): "Use the simple direct object pattern (
{ signal: metadata?.abortSignal }) rather than the variadic spread-of-array trick." — a one-liner per provider, no builder needed. - #617 (bridging providers): inline
addEventListener("abort", ..., { once: true })+ pre-aborted guard — provider-specific wiring. - #618 (SDK quirks): each provider has unique config placement (CancellationToken bridge,
configobject, etc.).
None of these reference or require a RequestConfigBuilder. The builder solves a problem the decomposition deliberately decided not to have.
The only reusable piece is mergeAbortSignals — and the platform already provides it
AbortSignal.any() has been available since Node 20.3.0 and this project pins 20.20.2. The hand-rolled mergeAbortSignals can be replaced with:
AbortSignal.any([primarySignal, secondarySignal])This also fixes a semantic issue in the current implementation: when secondarySignal is already aborted, the method returns the (non-aborted) primarySignal — contradicting the JSDoc ("If any signal is aborted, the returned signal will be aborted"). AbortSignal.any() handles this correctly.
No current consumer
No existing provider imports RequestConfigBuilder, and per the sub-issues above, none of the planned provider work will need it either. The abstraction is unvalidated.
Recommendation: Close this PR. The provider migrations in #616/#617/#618 should use the direct patterns prescribed in those issues, with AbortSignal.any() where signal merging is needed (#617). If a builder abstraction proves necessary after those migrations land, it can be introduced then with real consumers to validate the API.
|
I think this work might be premature issue #615 is still open, I'd focus on that first. |
github-actions
Bot
added
the
awaiting-author
|
Thanks for the review. I will work on issue #615 first. |
3 participants
Summary
This PR introduces
RequestConfigBuilder— a generic, SDK-agnostic request configuration builder that provides a fluent API for building type-safe request configurations. It serves as the second foundation for abort signal support in Zoo-Code, complementing #674 which wired theAbortSignalthrough the core metadata layer.Why this builder?
#674 successfully added
abortSignal?: AbortSignaltoApiHandlerCreateMessageMetadataand wired it fromTask.ts. However, each provider still manually constructs its request config by spreading{ signal: metadata?.abortSignal, headers: ... }— a repetitive pattern that:queryParams, Bedrock usesbody, Anthropic usesapiVersionThis builder solves all of that with a unified, chainable API:
Changes
src/api/providers/config-builder/request-config-builder.tssrc/api/providers/__tests__/request-config-builder.spec.tssrc/api/providers/config-builder/README.mdsrc/api/providers/index.tsRequestConfigBuilderfrom the providers barrelAPI Overview
Instance Methods (all chainable)
addAbortSignal(metadata?)— Extracts abort signal from metadata and adds it to configaddHeaders(headers)— Merges custom headers (empty objects skipped)setOption(key, value)— Type-safe single option settergetOption(key)— Get an option by keybuild()— Returns shallow copy for immutability;undefinedif no options setStatic Methods
fromMetadata(metadata?, extraOptions?)— Quick factory for simple signal + options scenariosmergeAbortSignals(primary, secondary?)— Combines two signals (abort when either fires)Design Principles
build()returns a shallow copy; constructor copies defaultOptionsTOptions extends Record<string, any>ensures compile-time correctness per SDKPros & Cons
✅ Pros
TOptions)build()returns shallow copyRelationship to other PRs
abortSignalto metadata interface and wired it through Task.ts. This PR builds on top of that foundation.Next Steps (after merge)
OpenAiRequestConfigBuilder,BedrockRequestConfigBuilder, etc.) with provider-specific methods likeaddPath(),addQueryParams(),setApiVersion()Related
Closes #434 (supersedes the manual approach with a reusable pattern)
Summary by CodeRabbit