feat(moonshot): implement provider with model picker, API fetching, streaming, and model metadata#857
Open
grizmin wants to merge 2 commits into
Open
feat(moonshot): implement provider with model picker, API fetching, streaming, and model metadata#857grizmin wants to merge 2 commits into
grizmin wants to merge 2 commits into
Conversation
…reaming - Fix duplicate ModelPicker by adding moonshot to PROVIDERS_WITH_CUSTOM_MODEL_UI - Fix HTTP 404 on Refresh Models by preserving /v1 in fetcher URL - Fix empty model list (consequence of 404 fix) - Fix 'No output generated' error by switching MoonshotHandler from OpenAICompatibleHandler (Vercel AI SDK) to OpenAiHandler (OpenAI Node SDK) which is what Moonshot's own API docs recommend - Add dedicated Moonshot fetcher with proper model ID prefixing - Update tests to match new handler implementation All 6,660 backend and 1,432 webview-ui tests pass.
grizmin
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
Contributor
📝 WalkthroughWalkthroughThis PR adds Moonshot as a dynamic provider, including model typing, model fetching and caching, handler behavior updates, router-model aggregation, and settings UI support for refreshing and selecting fetched models. ChangesMoonshot dynamic provider integration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
webview-ui/src/components/settings/providers/Moonshot.tsx (1)
1-172: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMissing local Vitest coverage for new stateful component logic.
This component adds a non-trivial state machine (refresh status, message listener, error/success reconciliation,
ModelPickerwiring) with no accompanying test file in this PR.Do you want me to draft a
Moonshot.spec.tsxcovering the refresh flow, the message-listener success/error paths, and the missing-API-key guard?As per path instructions, "Prefer local
webview-uitests for React/webview behavior such as component rendering, local state, hooks, form dirty-state, validation, or prop wiring. Add or update Vitest coverage underwebview-ui/src/**/__tests__."🤖 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 `@webview-ui/src/components/settings/providers/Moonshot.tsx` around lines 1 - 172, Add local Vitest coverage for Moonshot’s new stateful behavior by creating a test near Moonshot that exercises the refresh flow, the window message listener paths, and the missing API key guard. Cover the `handleRefreshModels` logic, the `useEffect` message handling for both `singleRouterModelFetchResponse` error and `routerModels` success reconciliation, and verify the `ModelPicker`/button wiring in `Moonshot`. Ensure the tests validate that `refreshStatus`, `refreshError`, and the disabled/loading states change as expected.Source: Path instructions
src/core/webview/webviewMessageHandler.ts (1)
1139-1153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the repeated credential-resolution/flush/candidate pattern.
This block duplicates the exact same shape used for
poeanddeepseekjust above it (resolve frommessage.valuesvsapiConfiguration, conditionally flush on explicit override, push a candidate). A small helper would reduce copy/paste risk as more single-apiKey providers are added.♻️ Example helper extraction
+function resolveApiKeyGatedCandidate( + key: RouterName, + apiKeyField: string, + baseUrlField: string, + message: WebviewMessage, + apiConfiguration: ProviderSettings, +) { + const apiKey = (message?.values as any)?.[apiKeyField] ?? (apiConfiguration as any)[apiKeyField] + const baseUrl = (message?.values as any)?.[baseUrlField] ?? (apiConfiguration as any)[baseUrlField] + return { apiKey, baseUrl, hasExplicitOverride: !!(message?.values as any)?.[apiKeyField] || !!(message?.values as any)?.[baseUrlField] } +}🤖 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/core/webview/webviewMessageHandler.ts` around lines 1139 - 1153, The Moonshot handling in webviewMessageHandler repeats the same credential resolution, optional flush, and candidate push pattern already used for poe and deepseek. Extract that shared logic into a small helper so the provider-specific code only supplies the provider name and credential keys, while the helper handles resolving from message.values vs apiConfiguration, calling flushModels on explicit overrides, and adding the candidate. Keep the Moonshot call site aligned with the existing provider blocks to reduce copy/paste drift as more single-apiKey providers are added.
🤖 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/fetchers/moonshot.ts`:
- Around line 66-87: The fallback in moonshot model fetching is assigning
guessed pricing for unknown IDs, which conflicts with
`MoonshotHandler.resolveModelInfo` treating those same models as unpriced.
Update the unknown-model branch in `src/api/providers/fetchers/moonshot.ts` so
it matches `resolveModelInfo`’s behavior for IDs not in `moonshotModels`,
leaving pricing fields unset/undefined instead of hardcoding values. Keep the
known-model path unchanged and ensure the fallback description still identifies
the model.
In `@webview-ui/src/components/settings/providers/Moonshot.tsx`:
- Around line 35-59: The Moonshot message handler in useEffect is setting error
state too broadly, which can surface errors from unrelated fetches. Update the
singleRouterModelFetchResponse branch to mirror the routerModels success guard
by only calling setRefreshStatus("error") and setRefreshError when refreshStatus
is "loading" and the message is for the moonshot provider. Keep the existing
moonshot providerName check in handleMessage so only refresh-triggered failures
affect the UI.
---
Nitpick comments:
In `@src/core/webview/webviewMessageHandler.ts`:
- Around line 1139-1153: The Moonshot handling in webviewMessageHandler repeats
the same credential resolution, optional flush, and candidate push pattern
already used for poe and deepseek. Extract that shared logic into a small helper
so the provider-specific code only supplies the provider name and credential
keys, while the helper handles resolving from message.values vs
apiConfiguration, calling flushModels on explicit overrides, and adding the
candidate. Keep the Moonshot call site aligned with the existing provider blocks
to reduce copy/paste drift as more single-apiKey providers are added.
In `@webview-ui/src/components/settings/providers/Moonshot.tsx`:
- Around line 1-172: Add local Vitest coverage for Moonshot’s new stateful
behavior by creating a test near Moonshot that exercises the refresh flow, the
window message listener paths, and the missing API key guard. Cover the
`handleRefreshModels` logic, the `useEffect` message handling for both
`singleRouterModelFetchResponse` error and `routerModels` success
reconciliation, and verify the `ModelPicker`/button wiring in `Moonshot`. Ensure
the tests validate that `refreshStatus`, `refreshError`, and the
disabled/loading states change as expected.
🪄 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: 040c25c1-8e98-4429-ac98-112116194e26
📒 Files selected for processing (15)
packages/types/src/provider-settings.tssrc/api/providers/__tests__/moonshot.spec.tssrc/api/providers/fetchers/__tests__/moonshot.spec.tssrc/api/providers/fetchers/modelCache.tssrc/api/providers/fetchers/moonshot.tssrc/api/providers/moonshot.tssrc/core/webview/__tests__/ClineProvider.spec.tssrc/core/webview/__tests__/webviewMessageHandler.routerModels.spec.tssrc/core/webview/__tests__/webviewMessageHandler.spec.tssrc/core/webview/webviewMessageHandler.tssrc/shared/api.tswebview-ui/src/components/settings/providers/Moonshot.tsxwebview-ui/src/components/settings/utils/providerModelConfig.tswebview-ui/src/components/ui/hooks/useSelectedModel.tswebview-ui/src/utils/__tests__/validate.spec.ts
Comment on lines
+66
to
+87
| for (const model of data.data) { | ||
| const modelId = typeof model.id === "string" && model.id ? model.id : null | ||
| if (!modelId) continue | ||
|
|
||
| const knownSpecs = moonshotModels[modelId as keyof typeof moonshotModels] | ||
|
|
||
| if (knownSpecs) { | ||
| models[modelId] = { ...knownSpecs } | ||
| } else { | ||
| models[modelId] = { | ||
| maxTokens: 16_000, | ||
| contextWindow: 262_144, | ||
| supportsImages: false, | ||
| supportsPromptCache: true, | ||
| inputPrice: 0.6, | ||
| outputPrice: 2.5, | ||
| cacheWritesPrice: 0, | ||
| cacheReadsPrice: 0.15, | ||
| description: `Moonshot model: ${modelId}`, | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Unknown-model pricing defaults conflict with MoonshotHandler.resolveModelInfo.
This fallback assigns concrete pricing (inputPrice: 0.6, outputPrice: 2.5, cacheReadsPrice: 0.15) for any model ID not present in moonshotModels. But src/api/providers/moonshot.ts's resolveModelInfo deliberately strips pricing to undefined for the same unknown IDs, stating pricing should show as "unknown" instead of a guessed rate. Since this fetcher's output populates routerModels?.moonshot shown in the ModelPicker, users will see a specific (and likely wrong) price estimate for a model that the handler will actually track as unpriced at runtime.
🔧 Suggested alignment
} else {
models[modelId] = {
- maxTokens: 16_000,
contextWindow: 262_144,
supportsImages: false,
supportsPromptCache: true,
- inputPrice: 0.6,
- outputPrice: 2.5,
- cacheWritesPrice: 0,
- cacheReadsPrice: 0.15,
description: `Moonshot model: ${modelId}`,
}
}🤖 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/fetchers/moonshot.ts` around lines 66 - 87, The fallback in
moonshot model fetching is assigning guessed pricing for unknown IDs, which
conflicts with `MoonshotHandler.resolveModelInfo` treating those same models as
unpriced. Update the unknown-model branch in
`src/api/providers/fetchers/moonshot.ts` so it matches `resolveModelInfo`’s
behavior for IDs not in `moonshotModels`, leaving pricing fields unset/undefined
instead of hardcoding values. Keep the known-model path unchanged and ensure the
fallback description still identifies the model.
Comment on lines
+35
to
+59
| useEffect(() => { | ||
| const handleMessage = (event: MessageEvent<ExtensionMessage>) => { | ||
| const message = event.data | ||
| if (message.type === "singleRouterModelFetchResponse" && !message.success) { | ||
| const providerName = message.values?.provider as RouterName | ||
| if (providerName === "moonshot") { | ||
| moonshotErrorJustReceived.current = true | ||
| setRefreshStatus("error") | ||
| setRefreshError(message.error) | ||
| } | ||
| } else if (message.type === "routerModels") { | ||
| if (refreshStatus === "loading") { | ||
| if (!moonshotErrorJustReceived.current) { | ||
| setRefreshStatus("success") | ||
| queryClient.invalidateQueries({ queryKey: ["routerModels"] }) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| window.addEventListener("message", handleMessage) | ||
| return () => { | ||
| window.removeEventListener("message", handleMessage) | ||
| } | ||
| }, [refreshStatus, queryClient]) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Error branch doesn't check refreshStatus, unlike the success branch — can show spurious errors.
The routerModels success branch (Line 46) only reacts when refreshStatus === "loading", but the singleRouterModelFetchResponse error branch (Lines 38-44) sets refreshStatus to "error" unconditionally whenever a moonshot fetch fails — including background/unrelated batch fetches not triggered by this component's "Refresh Models" button. This can surface a misleading error banner to the user who never clicked refresh.
🔧 Proposed fix
if (message.type === "singleRouterModelFetchResponse" && !message.success) {
const providerName = message.values?.provider as RouterName
- if (providerName === "moonshot") {
+ if (providerName === "moonshot" && refreshStatus === "loading") {
moonshotErrorJustReceived.current = true
setRefreshStatus("error")
setRefreshError(message.error)
}
} else if (message.type === "routerModels") {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useEffect(() => { | |
| const handleMessage = (event: MessageEvent<ExtensionMessage>) => { | |
| const message = event.data | |
| if (message.type === "singleRouterModelFetchResponse" && !message.success) { | |
| const providerName = message.values?.provider as RouterName | |
| if (providerName === "moonshot") { | |
| moonshotErrorJustReceived.current = true | |
| setRefreshStatus("error") | |
| setRefreshError(message.error) | |
| } | |
| } else if (message.type === "routerModels") { | |
| if (refreshStatus === "loading") { | |
| if (!moonshotErrorJustReceived.current) { | |
| setRefreshStatus("success") | |
| queryClient.invalidateQueries({ queryKey: ["routerModels"] }) | |
| } | |
| } | |
| } | |
| } | |
| window.addEventListener("message", handleMessage) | |
| return () => { | |
| window.removeEventListener("message", handleMessage) | |
| } | |
| }, [refreshStatus, queryClient]) | |
| useEffect(() => { | |
| const handleMessage = (event: MessageEvent<ExtensionMessage>) => { | |
| const message = event.data | |
| if (message.type === "singleRouterModelFetchResponse" && !message.success) { | |
| const providerName = message.values?.provider as RouterName | |
| if (providerName === "moonshot" && refreshStatus === "loading") { | |
| moonshotErrorJustReceived.current = true | |
| setRefreshStatus("error") | |
| setRefreshError(message.error) | |
| } | |
| } else if (message.type === "routerModels") { | |
| if (refreshStatus === "loading") { | |
| if (!moonshotErrorJustReceived.current) { | |
| setRefreshStatus("success") | |
| queryClient.invalidateQueries({ queryKey: ["routerModels"] }) | |
| } | |
| } | |
| } | |
| } | |
| window.addEventListener("message", handleMessage) | |
| return () => { | |
| window.removeEventListener("message", handleMessage) | |
| } | |
| }, [refreshStatus, queryClient]) |
🤖 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 `@webview-ui/src/components/settings/providers/Moonshot.tsx` around lines 35 -
59, The Moonshot message handler in useEffect is setting error state too
broadly, which can surface errors from unrelated fetches. Update the
singleRouterModelFetchResponse branch to mirror the routerModels success guard
by only calling setRefreshStatus("error") and setRefreshError when refreshStatus
is "loading" and the message is for the moonshot provider. Keep the existing
moonshot providerName check in handleMessage so only refresh-triggered failures
affect the UI.
… docs Add kimi-k2.7-code, kimi-k2.7-code-highspeed, and kimi-k2.6 to static models map with accurate pricing from Moonshot documentation. Also fix kimi-k2.5 to correctly mark supportsImages: true.
grizmin
changed the title
github-actions
Bot
added
the
awaiting-review
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
1 participant
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.
Summary
Implement the Moonshot provider from scratch, enabling full model selection, API-based model fetching, and working streaming responses.
What was missing
The previous Moonshot implementation was non-functional:
MoonshotHandlerextendedOpenAICompatibleHandler(Vercel AI SDK) which has compatibility issues with the Moonshot APIChanges
Handler refactor (
src/api/providers/moonshot.ts)MoonshotHandlerfromOpenAICompatibleHandler(Vercel AI SDK) toOpenAiHandler(OpenAI Node SDK)getModel(),processUsageMetrics(),addMaxTokensIfNeeded()Model fetching (
src/api/providers/fetchers/moonshot.ts)getMoonshotModels()fetcher that callsGET /v1/modelsModel picker UI (
webview-ui/src/components/settings/providers/Moonshot.tsx)Model metadata (
packages/types/src/providers/moonshot.ts)kimi-k2.7-code,kimi-k2.7-code-highspeed,kimi-k2.6with correct pricing from official docskimi-k2.5to correctly marksupportsImages: trueInfrastructure
moonshottodynamicProviderslist (enables model fetching pipeline)modelCache.ts)requestRouterModelsTesting