refactor: enhance scalability of openrouter client#11
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded@Simplereally has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 18 minutes and 56 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR introduces a singleton-pattern OpenRouter client with caching, client-side retry logic with exponential backoff, per-request timeouts, and abort signal handling. It extends GenerateOptions with Changes
Sequence DiagramssequenceDiagram
participant Client as Application
participant Cache as ClientCache
participant Gen as generate()
participant Fetch as highThroughputFetch
participant API as OpenRouter API
Client->>Cache: createOpenRouterClient(apiKey)
Cache->>Cache: Check if cached & apiKey matches
Cache-->>Client: Return cached client
Client->>Gen: generate(prompt, options)
loop Retry Loop (maxRetries)
Gen->>Fetch: streamText(request, {timeout})
Fetch->>API: HTTP POST
alt Success
API-->>Fetch: Stream response
Fetch-->>Gen: Process stream
Gen->>Gen: Extract & return text
Gen-->>Client: Return result
else Retryable Error
API-->>Fetch: Error (5xx, etc.)
Fetch-->>Gen: Throw error
Gen->>Gen: calculateRetryDelay(exponential backoff)
Gen->>Gen: sleep(delay)
Note over Gen: Retry next iteration
else Non-Retryable or Max Retries Exceeded
Gen-->>Client: Throw OpenRouterError
end
end
sequenceDiagram
participant Client as Application
participant Clear as clearClientCache()
participant Cache as ClientCache
Client->>Clear: clearClientCache()
Clear->>Cache: Reset cachedClient & cachedApiKey
Cache->>Cache: Clear singleton state
Clear-->>Client: void
Note over Client: Next createOpenRouterClient() call<br/>will create fresh instance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🤖 Fix all issues with AI agents
In @lib/openrouter/openrouter-client.ts:
- Around line 204-206: The GenerateOptions.timeoutMs field is declared but never
used; update the generate function to read timeoutMs from its options and pass
it into highThroughputFetch (instead of always using DEFAULT_TIMEOUT_MS), or
remove timeoutMs from GenerateOptions if per-request timeouts are not supported;
specifically, modify the generate function to extract timeoutMs (fallback to
DEFAULT_TIMEOUT_MS when undefined) and forward that value to highThroughputFetch
so each request honors the caller-provided timeout.
🧹 Nitpick comments (3)
lib/openrouter/openrouter-client.ts (2)
165-170: Fragile status code detection in error messages.Checking for status codes by string matching in error messages (e.g.,
"429") could lead to false positives. An error message like"User 429 not found"or"Connection to port 5000 failed"would incorrectly match.Consider checking if the error has a
statusproperty instead, or use a more specific pattern like matching"status 429"or"HTTP 429".Proposed improvement
function isRetryableError(error: unknown): boolean { if (error instanceof Error) { const message = error.message.toLowerCase() // Network errors if ( message.includes("fetch failed") || message.includes("network") || message.includes("econnreset") || message.includes("etimedout") || message.includes("socket hang up") ) { return true } - // Rate limiting or server errors (check status in error message) - for (const status of RETRY_CONFIG.retryableStatuses) { - if (message.includes(String(status))) { - return true - } - } + // Check for status property on error object + const errorWithStatus = error as { status?: number } + if ( + typeof errorWithStatus.status === "number" && + RETRY_CONFIG.retryableStatuses.includes(errorWithStatus.status as 408 | 429 | 500 | 502 | 503 | 504) + ) { + return true + } } return false }
297-302: Hardcoded status code loses error context.The
OpenRouterErroralways uses status500regardless of the actual error. If the original error was a 429 (rate limit) or 408 (timeout), this information is lost.Consider extracting and preserving the original status if available.
Proposed fix
+function extractStatusFromError(error: Error): number | undefined { + const errorWithStatus = error as { status?: number } + if (typeof errorWithStatus.status === "number") { + return errorWithStatus.status + } + return undefined +} // Wrap in OpenRouterError for consistent error handling throw new OpenRouterError( lastError?.message ?? "Generation failed after retries", "GENERATION_FAILED", - 500 + lastError ? extractStatusFromError(lastError) ?? 500 : 500 )lib/openrouter/openrouter-client.test.ts (1)
55-58: Unusual semicolon placement andascasts.The leading semicolons (
;(getOpenRouterApiKey as Mock)) are a defensive pattern against ASI but look unusual. Consider usingvoidoperator or restructuring.Per coding guidelines, avoid
ascasts. Consider using a type-safe mock helper instead.Suggested improvement
+// Type-safe mock access +const mockGetOpenRouterApiKey = vi.mocked(getOpenRouterApiKey) +const mockCreateOpenRouter = vi.mocked(createOpenRouter) +const mockStreamText = vi.mocked(streamText) beforeEach(() => { // Reset all mocks and clear cache before each test vi.clearAllMocks() clearClientCache() - // Setup default mocks - ; (getOpenRouterApiKey as Mock).mockReturnValue(mockApiKey) - ; (createOpenRouter as Mock).mockReturnValue(mockModelFn) + // Setup default mocks + mockGetOpenRouterApiKey.mockReturnValue(mockApiKey) + mockCreateOpenRouter.mockReturnValue(mockModelFn) mockOpenRouterInstance.mockReturnValue("model-instance") })
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/openrouter/index.tslib/openrouter/openrouter-client.test.tslib/openrouter/openrouter-client.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Always leverage TypeScript (strict mode); avoid usage ofanywhich defeats the point of using TypeScript.
Let TypeScript inference work; export explicit prop types for shared/public components.
Noanytype in TypeScript. Useunknownand narrow instead.
Avoidascasts in TypeScript; prefersatisfies, generics, and runtime validation.
@ts-expect-errorallowed only with a comment explaining why + link/TODO to remove.
Files:
lib/openrouter/index.tslib/openrouter/openrouter-client.test.tslib/openrouter/openrouter-client.ts
🔇 Additional comments (10)
lib/openrouter/openrouter-client.ts (4)
98-122: LGTM!The singleton pattern is correctly implemented with API key validation and cache invalidation when the key changes.
127-130: LGTM!Simple and effective cache clearing implementation.
313-338: LGTM!The
generateStreamfunction correctly delegates tostreamTextand the documented lack of retry logic for streaming is appropriate.
347-356: LGTM!
OpenRouterErroris a well-structured custom error class.lib/openrouter/index.ts (1)
7-10: LGTM!Clean barrel export that correctly exposes the new
clearClientCachefunction alongside existing exports.lib/openrouter/openrouter-client.test.ts (5)
65-103: LGTM!Good test coverage for the singleton client creation, including cache reuse and API key change scenarios.
105-115: LGTM!Correctly validates that clearing the cache forces a new client to be created.
117-258: LGTM!Comprehensive test coverage for the
generatefunction including retry behavior, abort handling, and custom options. The retry count expectations correctly account for the initial attempt plus retries.
260-294: LGTM!Good coverage for
generateStreamincluding thedisableReasoningoption propagation.
296-311: LGTM!Good coverage for the
OpenRouterErrorclass construction with and without optional status.
1 participant
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.