-
-
Notifications
You must be signed in to change notification settings - Fork 128
feat: opus 4.6 model & additional config for provider clients #278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughAdds Claude Opus 4.6 and reorganizes Anthropic model metadata; extends client config interfaces to inherit underlying SDK options and forwards full config to SDK constructors across Anthropic, Gemini, and Grok; refactors Gemini summarize adapter to accept unified config; consolidates Grok type exports. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
View your CI Pipeline Execution ↗ for commit 0f58b36
☁️ Nx Cloud last updated this comment at |
@tanstack/ai
@tanstack/ai-anthropic
@tanstack/ai-client
@tanstack/ai-devtools-core
@tanstack/ai-gemini
@tanstack/ai-grok
@tanstack/ai-ollama
@tanstack/ai-openai
@tanstack/ai-openrouter
@tanstack/ai-preact
@tanstack/ai-react
@tanstack/ai-react-ui
@tanstack/ai-solid
@tanstack/ai-solid-ui
@tanstack/ai-svelte
@tanstack/ai-vue
@tanstack/ai-vue-ui
@tanstack/preact-ai-devtools
@tanstack/react-ai-devtools
@tanstack/solid-ai-devtools
commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/typescript/ai-gemini/src/adapters/summarize.ts`:
- Around line 229-231: The factory is passing the SDK/client config (options:
Omit<GeminiSummarizeConfig,'apiKey'>) as the adapter-options third parameter to
GeminiSummarizeAdapter, which currently masks a type mismatch and will break
when GeminiSummarizeAdapterOptions gains fields; update the call sites (the
factory that returns new GeminiSummarizeAdapter and the geminiSummarize helper)
to pass adapter options separately (e.g., pass {} or a dedicated adapterOptions
argument) so the third parameter is a GeminiSummarizeAdapterOptions while the
client config remains the second-parameter config (GeminiSummarizeConfig);
ensure signatures reflect separate clientConfig vs adapterOptions to avoid
silent shape mispasses.
🧹 Nitpick comments (6)
packages/typescript/ai-gemini/src/adapters/summarize.ts (1)
16-19: Empty interface extension — consider using a type alias or adding a doc note.
GeminiSummarizeConfigis currently identical toGeminiClientConfig. This is fine as a future extension point, but a brief comment clarifying the intent (e.g., "extend with summarize-specific options as needed") would help.packages/typescript/ai-grok/src/utils/client.ts (1)
11-17: Minor:||vs??forbaseURLdefault.Line 15 uses
||, which means an explicitly provided empty string''would also fall through to the default. This is likely fine in practice, but??would be more precise if the intent is to only default onundefined/null.Proposed fix
- baseURL: config.baseURL || 'https://api.x.ai/v1', + baseURL: config.baseURL ?? 'https://api.x.ai/v1',packages/typescript/ai-anthropic/src/index.ts (1)
22-22: Nit:ANTHROPIC_MODELSvalue export is placed between adapter exports and the "Type Exports" section.Consider moving this runtime value export below the type exports section header or into its own labeled block, to keep the adapter/type organizational structure consistent.
Suggested reorganization
-export { ANTHROPIC_MODELS } from './model-meta' // ============================================================================ // Type Exports // ============================================================================ +export { ANTHROPIC_MODELS } from './model-meta' export type { AnthropicChatModelProviderOptionsByName, AnthropicModelInputModalitiesByName, AnthropicChatModel, } from './model-meta'packages/typescript/ai-anthropic/src/model-meta.ts (1)
349-392: Stale commented-out code — missingCLAUDE_OPUS_4_6entry.The commented-out
ANTHROPIC_MODEL_METAblock (lines 349–359) and the type helpers below it don't includeCLAUDE_OPUS_4_6. If this code is planned for future use, it should be updated. If it's abandoned, consider removing it to reduce noise.packages/typescript/ai-anthropic/src/utils/client.ts (2)
14-17: Nit:apiKey: config.apiKeyis redundant after spreadingconfig.Since
configalready containsapiKey, the explicit override on line 16 has no effect. It's not harmful, but if the intent is clarity, a comment would be more informative than a no-op override.♻️ Suggested simplification
return new Anthropic_SDK({ ...config, - apiKey: config.apiKey, })
45-47:substring(7)yields only ~4 random characters; considersubstring(2)for more entropy.
Math.random().toString(36)produces something like"0.k5b8c0m3xo". Usingsubstring(7)skips most of the random portion, leaving ~4 characters. The more common idiom issubstring(2)(or.slice(2)) to capture the full random segment after"0.".♻️ Suggested fix
- return `${prefix}-${Date.now()}-${Math.random().toString(36).substring(7)}` + return `${prefix}-${Date.now()}-${Math.random().toString(36).substring(2)}`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/typescript/ai-gemini/src/adapters/summarize.ts (1)
131-132:⚠️ Potential issue | 🟡 MinorInconsistent unit: "words" here vs "tokens" in
summarize(Line 84).
summarizeinstructs"Keep the summary under ${options.maxLength} tokens"whilesummarizeStreamsays"words". These should use the same unit for consistent behavior.Proposed fix
const lengthInstructions = options.maxLength - ? ` Keep the summary under ${options.maxLength} words.` + ? ` Keep the summary under ${options.maxLength} tokens.` : ''
🧹 Nitpick comments (1)
packages/typescript/ai-gemini/src/adapters/summarize.ts (1)
51-53:GeminiSummarizeAdapterOptionsis now unused.The constructor was updated to
(config: GeminiSummarizeConfig, model: TModel)and no longer accepts adapter options. This interface is dead code. Consider removing it, or if it's intended for future use, add aTODOto clarify intent.
🎯 Changes
✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
New Features
Updates