feat(cache): add provider prompt cache planner#1426
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds prompt-cache detection and strategy, integrates cache keys/breakpoints across multiple LLM providers, propagates cache-read/write token accounting through streaming events, updates usage costing and DB schema, and adjusts presenter/session modelConfig to include conversationId. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Presenter as DeepChat Presenter
participant CacheStrategy as PromptCacheStrategy
participant Provider as LLM Provider
participant LLMAPI as LLM API
participant Usage as Usage Tracker
Client->>Presenter: runStreamForMessage(message, sessionId)
Presenter->>CacheStrategy: resolvePromptCachePlan(providerId, apiType, modelId, messages, conversationId)
CacheStrategy-->>Presenter: PromptCachePlan (mode, ttl, cacheKey?, breakpointPlan?)
Presenter->>Provider: sendRequest(messages, modelConfig)
Provider->>CacheStrategy: applyPromptCache(plan, messages)
CacheStrategy-->>Provider: cachedRequest (prompt_cache_key / modified messages / cache_control)
Provider->>LLMAPI: chat.completions.create(cachedRequest)
LLMAPI-->>Provider: stream (deltas + usage with cached_tokens, cache_write_tokens)
Provider->>Presenter: emitStreamEvent(usage: {prompt_tokens, completion_tokens, cached_tokens?, cache_write_tokens?})
Presenter->>Usage: accumulate(usage)
Usage->>Usage: normalizeUsageCounts(...) & estimateUsageCostUsd(...)
Usage-->>Presenter: finalized usage & cost
Presenter->>Client: deliver stream events with cache metrics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 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 |
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/presenter/deepchatAgentPresenter/accumulator.ts (1)
166-172:⚠️ Potential issue | 🟡 MinorGuard optional
cache_write_tokensbefore overwriting metadata.At Line 171, assigning the optional field directly can erase a previously captured value if a later usage event omits it.
💡 Suggested fix
case 'usage': { state.metadata.inputTokens = event.usage.prompt_tokens state.metadata.outputTokens = event.usage.completion_tokens state.metadata.totalTokens = event.usage.total_tokens state.metadata.cachedInputTokens = event.usage.cached_tokens - state.metadata.cacheWriteInputTokens = event.usage.cache_write_tokens + if (typeof event.usage.cache_write_tokens === 'number') { + state.metadata.cacheWriteInputTokens = event.usage.cache_write_tokens + } break }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/deepchatAgentPresenter/accumulator.ts` around lines 166 - 172, The assignment in the case 'usage' block unconditionally overwrites state.metadata.cacheWriteInputTokens with event.usage.cache_write_tokens which may be omitted in later usage events; update the case 'usage' handler in accumulator.ts so you only set state.metadata.cacheWriteInputTokens when event.usage.cache_write_tokens is defined (e.g., check for !== undefined or hasOwnProperty) and leave the existing metadata value unchanged otherwise, keeping other assignments (inputTokens/outputTokens/totalTokens/cachedInputTokens) as-is.
🧹 Nitpick comments (1)
test/main/presenter/llmProviderPresenter/awsBedrockProvider.test.ts (1)
112-132: Test uses camelCase field names for cache tokens.The mock uses
cacheReadInputTokensandcacheWriteInputTokens(camelCase), while the actual Bedrock SDK usescache_read_input_tokensandcache_creation_input_tokens(snake_case). This works because the implementation'sgetBedrockUsageNumberhelper accepts both naming conventions.Consider adding a complementary test with snake_case field names to ensure the production path is also covered.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/llmProviderPresenter/awsBedrockProvider.test.ts` around lines 112 - 132, Add a complementary test case that uses the Bedrock SDK's snake_case cache token fields so the real production shape is covered: duplicate the existing test that builds chunks via createBedrockChunk (and that exercises getBedrockUsageNumber indirectly) but replace the usage object fields cacheReadInputTokens/cacheWriteInputTokens with cache_read_input_tokens and cache_creation_input_tokens, then assert the same aggregated token totals/output as the camelCase test to ensure both naming conventions are handled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/newAgentPresenter/index.ts`:
- Around line 2215-2218: The backfill is overwriting parsed metadata by setting
cacheWriteInputTokens to a hardcoded 0; update the object construction in
newAgentPresenter (the spread of metadata and the subsequent fields) to preserve
any existing value by using the parsed metadata property with a safe fallback
(e.g., cacheWriteInputTokens: metadata.cacheWriteInputTokens ?? 0) instead of 0
so historical cache-write usage is not lost; do the same pattern if
cachedInputTokens should also preserve metadata.cachedInputTokens.
---
Outside diff comments:
In `@src/main/presenter/deepchatAgentPresenter/accumulator.ts`:
- Around line 166-172: The assignment in the case 'usage' block unconditionally
overwrites state.metadata.cacheWriteInputTokens with
event.usage.cache_write_tokens which may be omitted in later usage events;
update the case 'usage' handler in accumulator.ts so you only set
state.metadata.cacheWriteInputTokens when event.usage.cache_write_tokens is
defined (e.g., check for !== undefined or hasOwnProperty) and leave the existing
metadata value unchanged otherwise, keeping other assignments
(inputTokens/outputTokens/totalTokens/cachedInputTokens) as-is.
---
Nitpick comments:
In `@test/main/presenter/llmProviderPresenter/awsBedrockProvider.test.ts`:
- Around line 112-132: Add a complementary test case that uses the Bedrock SDK's
snake_case cache token fields so the real production shape is covered: duplicate
the existing test that builds chunks via createBedrockChunk (and that exercises
getBedrockUsageNumber indirectly) but replace the usage object fields
cacheReadInputTokens/cacheWriteInputTokens with cache_read_input_tokens and
cache_creation_input_tokens, then assert the same aggregated token totals/output
as the camelCase test to ensure both naming conventions are handled.
🪄 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
Run ID: 728b1b14-806a-46f1-b84a-cb800037ccb7
📒 Files selected for processing (24)
src/main/presenter/deepchatAgentPresenter/accumulator.tssrc/main/presenter/deepchatAgentPresenter/index.tssrc/main/presenter/deepchatAgentPresenter/process.tssrc/main/presenter/llmProviderPresenter/promptCacheCapabilities.tssrc/main/presenter/llmProviderPresenter/promptCacheStrategy.tssrc/main/presenter/llmProviderPresenter/providers/anthropicProvider.tssrc/main/presenter/llmProviderPresenter/providers/awsBedrockProvider.tssrc/main/presenter/llmProviderPresenter/providers/openAICompatibleProvider.tssrc/main/presenter/llmProviderPresenter/providers/openAIResponsesProvider.tssrc/main/presenter/newAgentPresenter/index.tssrc/main/presenter/sqlitePresenter/tables/deepchatUsageStats.tssrc/main/presenter/usageStats.tssrc/shared/types/agent-interface.d.tssrc/shared/types/core/llm-events.tstest/main/presenter/deepchatAgentPresenter/accumulator.test.tstest/main/presenter/deepchatAgentPresenter/messageStore.test.tstest/main/presenter/llmProviderPresenter/anthropicProvider.test.tstest/main/presenter/llmProviderPresenter/awsBedrockProvider.test.tstest/main/presenter/llmProviderPresenter/openAICompatibleProvider.test.tstest/main/presenter/llmProviderPresenter/openAIResponsesProvider.test.tstest/main/presenter/llmProviderPresenter/promptCacheStrategy.test.tstest/main/presenter/newAgentPresenter/usageDashboard.test.tstest/main/presenter/sqlitePresenter.test.tstest/main/presenter/usageStats.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
test/main/presenter/newAgentPresenter/usageDashboard.test.ts (1)
518-524: Consider assertingcache_write_input_tokensin live update test.The test explicitly sets
cacheWriteInputTokens: 0in the metadata (line 513) but the assertion doesn't verify this field was persisted. Adding it would strengthen test coverage for the live update path.Proposed fix
expect(row).toMatchObject({ source: 'live', cached_input_tokens: 20, + cache_write_input_tokens: 0, input_tokens: 140, output_tokens: 60 })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/newAgentPresenter/usageDashboard.test.ts` around lines 518 - 524, The test for live updates should also assert that cache_write_input_tokens was persisted: after fetching the row with sqlitePresenter.deepchatUsageStatsTable.getByMessageId('message-1'), add an expectation that the returned object includes cache_write_input_tokens: 0 (since the test metadata set cacheWriteInputTokens: 0). This ensures the live update path correctly maps the metadata field cacheWriteInputTokens to the stored column cache_write_input_tokens.src/main/presenter/llmProviderPresenter/providers/anthropicProvider.ts (1)
562-571: Non-streaming completions does not propagatecached_tokensandcache_write_tokensto the response.The
buildAnthropicUsageSnapshotreturnscached_tokensandcache_write_tokenswhen present, butresultResp.totalUsageonly extracts the base fields. If downstream consumers ofLLMResponse.totalUsageexpect cache-related metrics for non-streaming calls, they won't receive them.If this is intentional (e.g., only streaming paths need detailed cache stats), consider adding a brief comment clarifying the design decision. Otherwise, consider propagating:
♻️ Optional: propagate cache tokens in non-streaming response
resultResp.totalUsage = { prompt_tokens: usageSnapshot?.prompt_tokens ?? 0, completion_tokens: usageSnapshot?.completion_tokens ?? 0, - total_tokens: usageSnapshot?.total_tokens ?? 0 + total_tokens: usageSnapshot?.total_tokens ?? 0, + ...(usageSnapshot?.cached_tokens != null && { cached_tokens: usageSnapshot.cached_tokens }), + ...(usageSnapshot?.cache_write_tokens != null && { cache_write_tokens: usageSnapshot.cache_write_tokens }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/llmProviderPresenter/providers/anthropicProvider.ts` around lines 562 - 571, The non-streaming response populates resultResp.totalUsage from buildAnthropicUsageSnapshot but only copies prompt_tokens/completion_tokens/total_tokens, dropping cached_tokens and cache_write_tokens; update the code that sets resultResp.totalUsage (the block that assigns from usageSnapshot) to also include cached_tokens and cache_write_tokens when they exist (e.g., set cached_tokens: usageSnapshot?.cached_tokens ?? 0 and cache_write_tokens: usageSnapshot?.cache_write_tokens ?? 0), or if omission was intentional add a short clarifying comment above this assignment explaining why cache metrics are excluded; refer to buildAnthropicUsageSnapshot and resultResp.totalUsage to locate the change.src/main/presenter/llmProviderPresenter/providers/zenmuxProvider.ts (1)
54-60: Unsafe type cast to access privateanthropicfield.The pattern
as unknown as { anthropic?: Anthropic }bypasses TypeScript's protection of private fields. IfAnthropicProviderrefactors the field name or visibility, this will silently break at runtime.Consider one of these alternatives:
♻️ Option 1: Expose a protected setter in AnthropicProvider
In
anthropicProvider.ts:protected setAnthropicClient(client: Anthropic): void { this.anthropic = client }Then in
ZenmuxAnthropicDelegate:- const self = this as unknown as { anthropic?: Anthropic } - self.anthropic = new Anthropic({ + this.setAnthropicClient(new Anthropic({ apiKey, baseURL: this.provider.baseUrl || ZENMUX_ANTHROPIC_BASE_URL, defaultHeaders: this.defaultHeaders, fetchOptions - }) + }))♻️ Option 2: Make the field protected in base class
Change
private anthropic!: Anthropictoprotected anthropic!: AnthropicinAnthropicProvider, allowing direct assignment in subclasses.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/llmProviderPresenter/providers/zenmuxProvider.ts` around lines 54 - 60, The code uses an unsafe cast to set the private anthropic field from ZenmuxAnthropicDelegate; instead, modify the AnthropicProvider base class to either (a) add a protected setter method like setAnthropicClient(client: Anthropic) and call that from ZenmuxAnthropicDelegate, or (b) change the field declaration from private anthropic!: Anthropic to protected anthropic!: Anthropic so the subclass can assign it directly; update ZenmuxAnthropicDelegate to use the new setter or direct assignment of this.anthropic rather than the "as unknown as" cast.test/main/presenter/llmProviderPresenter/zenmuxProvider.test.ts (1)
167-333: Consider adding edge case tests for routing robustness.The current tests cover the happy paths well. Consider adding tests for:
- Case variations in model ID:
isAnthropicModeluses.toLowerCase(), but a test confirmingANTHROPIC/CLAUDE-SONNETroutes correctly would document this behavior.- Anthropic initialization failure: When
apiKeyis missing,ensureAnthropicDelegateReadyshould throw. A test verifying this error path would improve coverage.💡 Example additional test cases
it('routes uppercase ANTHROPIC/* models through the Anthropic endpoint', async () => { const provider = new ZenmuxProvider(createProvider(), createConfigPresenter()) const result = await provider.generateText('hello', 'ANTHROPIC/CLAUDE-SONNET-4-5') expect(mockAnthropicMessagesCreate).toHaveBeenCalled() }) it('throws when Anthropic API key is missing', async () => { const provider = new ZenmuxProvider( createProvider({ apiKey: '' }), createConfigPresenter() ) await expect(provider.generateText('hello', 'anthropic/claude-sonnet-4-5')) .rejects.toThrow('Anthropic SDK not initialized') })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/llmProviderPresenter/zenmuxProvider.test.ts` around lines 167 - 333, Add two edge-case tests to zenmuxProvider.test.ts: one that calls ZenmuxProvider.generateText with an uppercase model id like 'ANTHROPIC/CLAUDE-SONNET-4-5' to assert the request is routed through anthropic (mockAnthropicMessagesCreate called) to validate the toLowerCase behavior of isAnthropicModel, and another that constructs ZenmuxProvider with an empty apiKey (createProvider({ apiKey: '' })) and asserts that generateText for an anthropic model rejects/throws (e.g., expecting ensureAnthropicDelegateReady or the anthropic delegate initialization to throw a clear "Anthropic SDK not initialized" error) so the missing-API-key path is covered.src/renderer/settings/components/ProviderApiConfig.vue (1)
211-220: Hardcoded provider allowlist may drift from backendDEFAULT_PROVIDERS.This
EDITABLE_BASE_URL_PROVIDER_IDSset is maintained separately from the canonicalDEFAULT_PROVIDERSlist insrc/main/presenter/configPresenter/providers.ts. If new providers that should have editable base URLs are added to the backend, this frontend list will need manual synchronization.Consider either:
- Adding an
editableBaseUrlflag to the provider schema in the backend and passing it through to the frontend- Documenting this coupling so future contributors know to update both locations
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/settings/components/ProviderApiConfig.vue` around lines 211 - 220, The frontend uses a hardcoded EDITABLE_BASE_URL_PROVIDER_IDS set that can drift from the backend's canonical DEFAULT_PROVIDERS; add an editableBaseUrl boolean to the provider schema on the backend (attached to DEFAULT_PROVIDERS), ensure the presenter passes that flag through to the renderer, and change the frontend to compute editable providers from the incoming providers list (e.g., provider.editableBaseUrl) instead of the hardcoded EDITABLE_BASE_URL_PROVIDER_IDS; alternatively, add a clear comment/documentation near EDITABLE_BASE_URL_PROVIDER_IDS and DEFAULT_PROVIDERS explaining the coupling and required sync steps for future contributors.src/renderer/src/i18n/en-US/settings.json (1)
613-618: Unused i18n keys:baseUrlUnlockdialog strings are never referenced in the component.The
baseUrlUnlock.title,baseUrlUnlock.description, andbaseUrlUnlock.confirmkeys in the i18n file are defined but not used byProviderApiConfig.vue. TherequestBaseUrlUnlockfunction simply setsbaseUrlUnlocked.value = truewithout displaying any confirmation dialog. The component directly shows an editable input field instead.Either remove these unused i18n keys or implement the confirmation dialog to match the intended UI structure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/src/i18n/en-US/settings.json` around lines 613 - 618, The i18n keys under baseUrlUnlock (baseUrlUnlock.title, baseUrlUnlock.description, baseUrlUnlock.confirm) are unused by ProviderApiConfig.vue; either delete these keys from src/renderer/src/i18n/en-US/settings.json or implement the confirmation dialog flow in ProviderApiConfig.vue: update requestBaseUrlUnlock to open a modal/confirmation component that uses $t('settings.baseUrlUnlock.title'), $t('settings.baseUrlUnlock.description') and $t('settings.baseUrlUnlock.confirm') and only set baseUrlUnlocked.value = true when the user confirms; ensure the modal component and its confirm handler are wired into the existing component state and translation namespace.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/presenter/llmProviderPresenter/providers/zenmuxProvider.ts`:
- Around line 209-225: The current routing in getEmbeddings and getDimensions
sends anthropic/* models to the Anthropic delegate, but Anthropic doesn't
support embeddings and the inherited BaseLLMProvider methods will throw at
runtime; update both getEmbeddings(modelId, texts) and getDimensions(modelId) to
detect isAnthropicModel(modelId) and immediately throw a clear, specific error
like "Embeddings not supported for Anthropic models: <modelId>" (do not call
ensureAnthropicDelegateReady or delegate methods for Anthropic models), leaving
openaiDelegate usage unchanged for non-Anthropic models so callers fail fast
with a helpful message.
---
Nitpick comments:
In `@src/main/presenter/llmProviderPresenter/providers/anthropicProvider.ts`:
- Around line 562-571: The non-streaming response populates
resultResp.totalUsage from buildAnthropicUsageSnapshot but only copies
prompt_tokens/completion_tokens/total_tokens, dropping cached_tokens and
cache_write_tokens; update the code that sets resultResp.totalUsage (the block
that assigns from usageSnapshot) to also include cached_tokens and
cache_write_tokens when they exist (e.g., set cached_tokens:
usageSnapshot?.cached_tokens ?? 0 and cache_write_tokens:
usageSnapshot?.cache_write_tokens ?? 0), or if omission was intentional add a
short clarifying comment above this assignment explaining why cache metrics are
excluded; refer to buildAnthropicUsageSnapshot and resultResp.totalUsage to
locate the change.
In `@src/main/presenter/llmProviderPresenter/providers/zenmuxProvider.ts`:
- Around line 54-60: The code uses an unsafe cast to set the private anthropic
field from ZenmuxAnthropicDelegate; instead, modify the AnthropicProvider base
class to either (a) add a protected setter method like
setAnthropicClient(client: Anthropic) and call that from
ZenmuxAnthropicDelegate, or (b) change the field declaration from private
anthropic!: Anthropic to protected anthropic!: Anthropic so the subclass can
assign it directly; update ZenmuxAnthropicDelegate to use the new setter or
direct assignment of this.anthropic rather than the "as unknown as" cast.
In `@src/renderer/settings/components/ProviderApiConfig.vue`:
- Around line 211-220: The frontend uses a hardcoded
EDITABLE_BASE_URL_PROVIDER_IDS set that can drift from the backend's canonical
DEFAULT_PROVIDERS; add an editableBaseUrl boolean to the provider schema on the
backend (attached to DEFAULT_PROVIDERS), ensure the presenter passes that flag
through to the renderer, and change the frontend to compute editable providers
from the incoming providers list (e.g., provider.editableBaseUrl) instead of the
hardcoded EDITABLE_BASE_URL_PROVIDER_IDS; alternatively, add a clear
comment/documentation near EDITABLE_BASE_URL_PROVIDER_IDS and DEFAULT_PROVIDERS
explaining the coupling and required sync steps for future contributors.
In `@src/renderer/src/i18n/en-US/settings.json`:
- Around line 613-618: The i18n keys under baseUrlUnlock (baseUrlUnlock.title,
baseUrlUnlock.description, baseUrlUnlock.confirm) are unused by
ProviderApiConfig.vue; either delete these keys from
src/renderer/src/i18n/en-US/settings.json or implement the confirmation dialog
flow in ProviderApiConfig.vue: update requestBaseUrlUnlock to open a
modal/confirmation component that uses $t('settings.baseUrlUnlock.title'),
$t('settings.baseUrlUnlock.description') and
$t('settings.baseUrlUnlock.confirm') and only set baseUrlUnlocked.value = true
when the user confirms; ensure the modal component and its confirm handler are
wired into the existing component state and translation namespace.
In `@test/main/presenter/llmProviderPresenter/zenmuxProvider.test.ts`:
- Around line 167-333: Add two edge-case tests to zenmuxProvider.test.ts: one
that calls ZenmuxProvider.generateText with an uppercase model id like
'ANTHROPIC/CLAUDE-SONNET-4-5' to assert the request is routed through anthropic
(mockAnthropicMessagesCreate called) to validate the toLowerCase behavior of
isAnthropicModel, and another that constructs ZenmuxProvider with an empty
apiKey (createProvider({ apiKey: '' })) and asserts that generateText for an
anthropic model rejects/throws (e.g., expecting ensureAnthropicDelegateReady or
the anthropic delegate initialization to throw a clear "Anthropic SDK not
initialized" error) so the missing-API-key path is covered.
In `@test/main/presenter/newAgentPresenter/usageDashboard.test.ts`:
- Around line 518-524: The test for live updates should also assert that
cache_write_input_tokens was persisted: after fetching the row with
sqlitePresenter.deepchatUsageStatsTable.getByMessageId('message-1'), add an
expectation that the returned object includes cache_write_input_tokens: 0 (since
the test metadata set cacheWriteInputTokens: 0). This ensures the live update
path correctly maps the metadata field cacheWriteInputTokens to the stored
column cache_write_input_tokens.
🪄 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
Run ID: dd029a65-0cad-4b89-a03c-ce6ee5516282
📒 Files selected for processing (22)
src/main/presenter/llmProviderPresenter/promptCacheCapabilities.tssrc/main/presenter/llmProviderPresenter/providers/anthropicProvider.tssrc/main/presenter/llmProviderPresenter/providers/zenmuxProvider.tssrc/main/presenter/newAgentPresenter/index.tssrc/renderer/settings/components/ProviderApiConfig.vuesrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/types/i18n.d.tstest/main/presenter/llmProviderPresenter/promptCacheStrategy.test.tstest/main/presenter/llmProviderPresenter/zenmuxProvider.test.tstest/main/presenter/newAgentPresenter/usageDashboard.test.tstest/renderer/components/ProviderApiConfig.test.ts
✅ Files skipped from review due to trivial changes (8)
- src/renderer/src/i18n/fr-FR/settings.json
- src/renderer/src/i18n/zh-HK/settings.json
- src/renderer/src/i18n/zh-TW/settings.json
- src/renderer/src/i18n/ja-JP/settings.json
- src/renderer/src/i18n/ko-KR/settings.json
- src/renderer/src/i18n/pt-BR/settings.json
- src/renderer/src/i18n/fa-IR/settings.json
- test/main/presenter/llmProviderPresenter/promptCacheStrategy.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/presenter/llmProviderPresenter/promptCacheCapabilities.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/main/presenter/llmProviderPresenter/providers/zenmuxProvider.ts (2)
54-60: Type cast bypasses encapsulation of the parent class'santhropicfield.The
as unknown as { anthropic?: Anthropic }pattern is a maintenance risk. IfAnthropicProviderrenames or restructures the field, this will silently break at runtime.Consider exposing a protected setter in
AnthropicProvideror using a different initialization pattern that doesn't require reaching into private state.♻️ Alternative: add a protected method in AnthropicProvider
In
AnthropicProvider, add:protected setAnthropicClient(client: Anthropic): void { this.anthropic = client }Then in
ZenmuxAnthropicDelegate:- const self = this as unknown as { anthropic?: Anthropic } - self.anthropic = new Anthropic({ + this.setAnthropicClient(new Anthropic({ apiKey, baseURL: this.provider.baseUrl || ZENMUX_ANTHROPIC_BASE_URL, defaultHeaders: this.defaultHeaders, fetchOptions - }) + }))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/llmProviderPresenter/providers/zenmuxProvider.ts` around lines 54 - 60, Replace the unsafe cast that writes into the parent class's private field by adding a protected setter on AnthropicProvider (e.g., protected setAnthropicClient(client: Anthropic): void { this.anthropic = client }) and then call that setter from ZenmuxAnthropicDelegate instead of using "this as unknown as { anthropic?: Anthropic }"; update the code that constructs the Anthropic instance (the new Anthropic({...}) call) to pass the instance into setAnthropicClient so the child no longer mutates private state via type casting.
104-112: Consider a more descriptive error message when API key is missing.When
ensureClientInitialized()fails due to a missing API key, the error "Anthropic SDK not initialized" doesn't indicate the root cause. Users may not realize they need to configure an API key.♻️ Proposed improvement
private async ensureAnthropicDelegateReady(): Promise<ZenmuxAnthropicDelegate> { await this.anthropicDelegate.ensureClientInitialized() if (!this.anthropicDelegate.isClientInitialized()) { - throw new Error('Anthropic SDK not initialized') + throw new Error('Anthropic SDK not initialized: API key may be missing') } return this.anthropicDelegate }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/presenter/llmProviderPresenter/providers/zenmuxProvider.ts` around lines 104 - 112, The current ensureAnthropicDelegateReady method throws a generic "Anthropic SDK not initialized" error; update it to detect and surface missing API key details by checking anthropicDelegate.ensureClientInitialized()/isClientInitialized() results and, when initialization failed due to a missing key, throw a more descriptive error (e.g., mention missing Anthropic API key and which env var/config to set). Modify ensureAnthropicDelegateReady to inspect anthropicDelegate for an explicit missing-key condition (or catch the error from ensureClientInitialized()), and include that detail in the thrown Error so callers of ensureAnthropicDelegateReady get actionable guidance.test/main/presenter/llmProviderPresenter/zenmuxProvider.test.ts (1)
210-221: Test relies heavily on internal implementation details.The test directly manipulates private delegate state (
clientInitialized,isInitialized,anthropic) usingas anycasts. This creates tight coupling between the test and internal implementation, making refactoring harder.Consider adding a test-only initialization method or testing through the public API by ensuring proper mock setup that allows natural initialization flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/main/presenter/llmProviderPresenter/zenmuxProvider.test.ts` around lines 210 - 221, The test mutates private delegate state (anthropicDelegate.clientInitialized/isInitialized/anthropic) which couples tests to internals; instead add a test-only initialization helper on the provider (e.g., initializeTestAnthropicClient) that sets up the delegate’s anthropic client with mockAnthropicMessagesCreate and mockAnthropicModelsList and marks it initialized, or drive setup through the public initialization path by spying/mock-resolving ensureClientInitialized to return a client configured with those mocks; update the test to call that helper or the public init so it no longer uses (provider as any).anthropicDelegate mutation and still verifies behavior via the public API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/presenter/llmProviderPresenter/providers/zenmuxProvider.ts`:
- Around line 54-60: Replace the unsafe cast that writes into the parent class's
private field by adding a protected setter on AnthropicProvider (e.g., protected
setAnthropicClient(client: Anthropic): void { this.anthropic = client }) and
then call that setter from ZenmuxAnthropicDelegate instead of using "this as
unknown as { anthropic?: Anthropic }"; update the code that constructs the
Anthropic instance (the new Anthropic({...}) call) to pass the instance into
setAnthropicClient so the child no longer mutates private state via type
casting.
- Around line 104-112: The current ensureAnthropicDelegateReady method throws a
generic "Anthropic SDK not initialized" error; update it to detect and surface
missing API key details by checking
anthropicDelegate.ensureClientInitialized()/isClientInitialized() results and,
when initialization failed due to a missing key, throw a more descriptive error
(e.g., mention missing Anthropic API key and which env var/config to set).
Modify ensureAnthropicDelegateReady to inspect anthropicDelegate for an explicit
missing-key condition (or catch the error from ensureClientInitialized()), and
include that detail in the thrown Error so callers of
ensureAnthropicDelegateReady get actionable guidance.
In `@test/main/presenter/llmProviderPresenter/zenmuxProvider.test.ts`:
- Around line 210-221: The test mutates private delegate state
(anthropicDelegate.clientInitialized/isInitialized/anthropic) which couples
tests to internals; instead add a test-only initialization helper on the
provider (e.g., initializeTestAnthropicClient) that sets up the delegate’s
anthropic client with mockAnthropicMessagesCreate and mockAnthropicModelsList
and marks it initialized, or drive setup through the public initialization path
by spying/mock-resolving ensureClientInitialized to return a client configured
with those mocks; update the test to call that helper or the public init so it
no longer uses (provider as any).anthropicDelegate mutation and still verifies
behavior via the public API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 75c72e8e-4826-4081-80c0-515cbb1a2294
📒 Files selected for processing (2)
src/main/presenter/llmProviderPresenter/providers/zenmuxProvider.tstest/main/presenter/llmProviderPresenter/zenmuxProvider.test.ts
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 by CodeRabbit
New Features
UI
Localization