feat(api): introduce RequestConfigBuilder for SDK-agnostic abort signal support#1008
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
…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
…onfigBuilder (Zoo-Code-Org#615) - Add default empty object parameter to addHeaders() so calling with undefined no longer throws TypeError from Object.keys(undefined) - Reorder mergeAbortSignals to check primarySignal.aborted before allocating AbortController, preventing unnecessary controller creation
|
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 as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesThe provider layer adds a generic fluent request configuration builder, timeout-aware abort-signal utilities, public exports, comprehensive Vitest coverage, and documentation covering usage, extension, and API behavior. Request configuration and abort handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HandlerMetadata
participant RequestConfigBuilder
participant AbortSignalUtilities
participant RequestConfig
HandlerMetadata->>RequestConfigBuilder: provide abort signal and request metadata
RequestConfigBuilder->>AbortSignalUtilities: merge controller, metadata signal, and timeout
AbortSignalUtilities-->>RequestConfigBuilder: return merged signal and cleanup
RequestConfigBuilder-->>RequestConfig: build shallow-copied options
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![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 (3)
src/api/providers/config-builder/README.md (2)
196-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocumented
mergeAbortSignalsimplementation doesn't match the actual code.This section shows a hand-written implementation (manual
AbortController+addEventListener, with a special case returningprimarySignalunchanged whensecondarySignalis already aborted). The real implementation inabort-signal.tsdelegates to nativeAbortSignal.any(), which behaves differently — notably, it will immediately reflect an already-aborted secondary signal, the opposite of what this doc claims. This will mislead anyone extending the builder based on this doc.🤖 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/README.md` around lines 196 - 221, Update the “How mergeAbortSignals Works” section to accurately document the actual implementation in mergeAbortSignals and its use of native AbortSignal.any(). Remove the hand-written AbortController example and state that any already-aborted input, including secondarySignal, causes the returned signal to be aborted immediately.
479-495: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAPI reference is missing
addMergedSignal/mergeAbortSignalAndTimeout.The Instance/Static Methods tables don't document
addMergedSignal, even though it's the method intended to address issue#404(merging a provider's internal controller with an external abort signal and optional timeout). Worth adding given it's a key part of this PR's purpose.🤖 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/README.md` around lines 479 - 495, Update the README API reference tables for the configuration builder to document the missing addMergedSignal instance method and mergeAbortSignalAndTimeout static method, including their parameters, return types, and behavior for combining provider/external abort signals with an optional timeout. Keep the descriptions aligned with the actual method signatures and implementation.src/api/providers/config-builder/request-config-builder.ts (1)
42-50: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winHeader merge is case-sensitive; HTTP header names are not.
addHeadersmerges via plain object spread on raw key strings. Differently-cased keys (e.g."x-custom"vs"X-Custom") won't be recognized as the same header, so a lateraddHeaderscall intended to override an earlier one could instead add a duplicate header with different casing sent to the wire.🤖 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 42 - 50, Update addHeaders to merge HTTP header names case-insensitively, so differently-cased keys are treated as the same header and later values override earlier ones. Preserve the existing headers and options behavior while ensuring the resulting headers do not contain duplicate names differing only by casing.
🤖 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/request-config-builder.ts`:
- Around line 63-73: Update RequestConfigBuilder.addMergedSignal and build so
the internal cleanup handle is not stored in or returned as part of the SDK
options object. Expose cleanup separately through the builder’s public result or
accessor, while preserving the merged signal in the built options and existing
behavior when no merged signal was added.
---
Nitpick comments:
In `@src/api/providers/config-builder/README.md`:
- Around line 196-221: Update the “How mergeAbortSignals Works” section to
accurately document the actual implementation in mergeAbortSignals and its use
of native AbortSignal.any(). Remove the hand-written AbortController example and
state that any already-aborted input, including secondarySignal, causes the
returned signal to be aborted immediately.
- Around line 479-495: Update the README API reference tables for the
configuration builder to document the missing addMergedSignal instance method
and mergeAbortSignalAndTimeout static method, including their parameters, return
types, and behavior for combining provider/external abort signals with an
optional timeout. Keep the descriptions aligned with the actual method
signatures and implementation.
In `@src/api/providers/config-builder/request-config-builder.ts`:
- Around line 42-50: Update addHeaders to merge HTTP header names
case-insensitively, so differently-cased keys are treated as the same header and
later values override earlier ones. Preserve the existing headers and options
behavior while ensuring the resulting headers do not contain duplicate names
differing only by casing.
🪄 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: b693b2e0-7220-4c8d-8f69-b87c93c8adb4
📒 Files selected for processing (6)
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.tssrc/api/providers/utils/__tests__/abort-signal.spec.tssrc/api/providers/utils/abort-signal.ts
7e9d8d4 to
c0a7e12
Compare
github-actions
Bot
added
the
awaiting-review
2 participants
feat(api): introduce RequestConfigBuilder for SDK-agnostic abort signal support
Problem
Currently each provider manually constructs its request config by spreading
{ signal: metadata?.abortSignal, headers: ... }— a repetitive pattern that:queryParams, Bedrock usesbody, Anthropic usesapiVersionProposed Solution
Introduce
RequestConfigBuilder— a generic, SDK-agnostic request configuration builder that provides a fluent API for building type-safe request configurations.Usage Example
Key Features
TOptions extends Record<string, any>ensures compile-time correctness per SDKmergeAbortSignals()returns primary directly when no secondary providedbuild()returns shallow copyAPI Methods
Instance Methods (all chainable)
addAbortSignal(metadata?)— Extracts abort signal from metadata and adds it to configaddHeaders(headers?)— Merges custom headers (empty objects skipped, no default param overhead)setOption(key, value)— Type-safe single option settergetOption(key)— Get an option by keybuild()— Returns shallow copy for immutability;undefinedif no options setaddMergedSignal(internalController, metadata?, timeoutMs?)— Merge internal controller + external signal with timeout support (for providers that maintain their own AbortController)Static Methods
fromMetadata(metadata?, extraOptions?)— Quick factory for simple signal + options scenariosmergeAbortSignals(primary, secondary?)— Combines two signals (returns primary directly when no secondary — zero overhead)Files to Add/Modify
src/api/providers/config-builder/request-config-builder.tssrc/api/providers/utils/abort-signal.tsmergeAbortSignalAndTimeout()+mergeAbortSignals()(~49 lines)src/api/providers/__tests__/request-config-builder.spec.tssrc/api/providers/utils/__tests__/abort-signal.spec.tssrc/api/providers/config-builder/README.mdsrc/api/providers/index.tsRequestConfigBuilderfrom the providers barrelDesign Principles
build()returns a shallow copy; constructor copies defaultOptionsTOptions extends Record<string, any>ensures compile-time correctness per SDKmergeAbortSignals()returns primary directly when no secondary providedKey Improvements vs Original PR #701 Draft
addHeaders(headers: Record<string, string> = {})default param overheadheaders?: Record<string, string>mergeAbortSignals([single])creates unnecessary AbortControlleraddMergedSignal()for providers with internal AbortControllerRelationship to Other PRs/Issues
abortSignalto metadata interface and wired it through Task.ts. This builder builds on top of that foundation.Next Steps (after merge)
addMergedSignal()instead of manual signal mergingOpenAiRequestConfigBuilder,BedrockRequestConfigBuilder) with provider-specific methods likeaddPath(),addQueryParams(),setApiVersion()Related
Related to #404 — provides the infrastructure foundation for consistent abort signal handling across all providers. Actual fix will be implemented when refactoring individual providers in a follow-up PR.
Closes #1007
Summary by CodeRabbit
Summary by CodeRabbit
New Features
RequestConfigBuilderfor fluent setup of headers, options, and cancellation signals.Documentation
Tests