feat: support transcription for openai-compatible providers - #995
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe PR adds OpenAI-compatible provider settings, separate transcription-provider defaults, capability-based transcription selection, configurable transcription endpoints and models, shared API-key resolution, and desktop wiring for provider state synchronization. ChangesAI provider and transcription support
Permission schema formatting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Settings
participant ProviderConfig
participant KeyResolver
participant AudioMemo
participant Transcription
Settings->>ProviderConfig: provide provider list and transcription default
ProviderConfig->>KeyResolver: resolve candidate API keys
KeyResolver-->>ProviderConfig: return usable key
ProviderConfig->>AudioMemo: resolve transcription target
AudioMemo->>Transcription: submit audio with endpoint and model
Transcription-->>AudioMemo: return transcript or error
Possibly related PRs
Suggested labels: 🚥 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 |
9c4953c to
ae18310
Compare
- Add optional transcriptionModel field to openai-compatible provider config - Add defaultTranscriptionProviderId to settings schema (independent of chat default) - Add supportsTranscription flag to AiProviderInfo catalog - Add aiProviderSupportsTranscription() helper that merges catalog constants with per-entry user configuration for openai-compatible providers Transcription eligibility is now derived: hosted providers are known at compile time (OpenAI/Google: true, Anthropic/OpenRouter: false), while openai-compatible entries become eligible when the user supplies a non-empty transcriptionModel.
Replace the compile-time TRANSCRIPTION_PROVIDERS constant with a dynamic transcriptionProviders() function that derives eligible providers from the configured state. Changes: - transcriptionProviders(): returns all providers where aiProviderSupportsTranscription() is true, sorted OpenAI > Google > openai-compatible, with the transcription default first within each group - pickTranscriptionConfig(): returns the first candidate (no longer hardcoded to only OpenAI/Google) - resolveTranscriptionTarget(): iterates candidates, skips keyless entries - defaultTranscriptionProvider(): resolves the transcription default, falling back to the first transcription-capable entry - AiProvidersState gains defaultTranscriptionProviderId - withAiProviderAdded/Removed maintain the transcription default alongside the chat default The TranscriptionProvider type is widened to include 'openai-compatible' alongside 'openai' and 'google'.
Extend transcribeAudio and transcribeWithOpenAi to accept optional
baseUrl and model parameters. When provided (openai-compatible
providers), the client posts to {baseUrl}/audio/transcriptions instead
of the hardcoded api.openai.com endpoint, and uses the user-supplied
model instead of the default with fallback retry.
The provider routing is simplified: anything that isn't 'google' now
flows through transcribeWithOpenAi, since openai-compatible providers
use the same /v1/audio/transcriptions API shape.
…ipeline memoNoteBody now accepts AiProviderConfig (instead of the removed TranscriptionConfig) and forwards baseUrl + transcriptionModel to transcribeAudio when the provider is openai-compatible. The error message for missing transcription config is updated from 'No OpenAI or Gemini model is configured' to the generic 'No transcription provider is configured'. Exports are updated to include the new public API surface: aiProviderSupportsTranscription, defaultTranscriptionProvider, resolveTranscriptionTarget, transcriptionProviders.
…scriptionProviderId Every AiProvidersState construction site now includes the new defaultTranscriptionProviderId field. useAiProviders gains a makeTranscriptionDefault callback that writes the transcription default independently of the chat default. New openai-compatible entries are initialized with an empty transcriptionModel (transcription off by default).
ae18310 to
aabace9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/components/settings/add-ai-provider-dialog.tsx (1)
126-135: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStale validation errors can survive a provider switch.
These
setValuecalls resetmodel/baseUrlto valid defaults but don't clear or re-trigger validation, so a previously shown "Enter a model id."/"Enter an http(s) endpoint URL." error can linger on screen even though the field now holds a valid value (until the next submit attempt re-validates).🐛 Suggested fix
const next = aiProvider(aiProviderIdSchema.parse(value)) setValue('provider', next.id) - setValue('model', next.models[0].id) + setValue('model', next.models[0].id, { shouldValidate: true }) setValue( 'baseUrl', next.id === 'openai-compatible' ? DEFAULT_OPENAI_COMPATIBLE_BASE_URL : '', + { shouldValidate: true }, )🤖 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 `@apps/desktop/src/components/settings/add-ai-provider-dialog.tsx` around lines 126 - 135, Update the provider-switch handler in onValueChange to clear or re-trigger validation for the model and baseUrl fields after setting their new defaults, so stale errors are removed immediately while preserving validation for future edits and submissions.packages/core/src/actions/audio-memo.ts (1)
543-552: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTranscription key lookup bypasses
aiApiKeyForConfig, breaking keyless openai-compatible transcription.
getKeyhere resolves keys via rawgetSecret(aiKeySecretName(id)), unlike the enrichment-key path a few lines below (line 580) which correctly usesaiApiKeyForConfig(enrichmentConfig). For anopenai-compatibleprovider configured without an API key (keyHint === '', a valid "no-key compatible endpoint" peraudio-memo-title.ts's doc comment), no secret was ever stored, sogetSecretreturnsnullhere —resolveTranscriptionTargetthen treats this provider as keyless and skips it, ultimately reporting'no-key'even though the provider is fully usable. The downstream message ("The API key for the configured openai-compatible model is missing from the keychain") is also misleading for this case.This file already imports
aiApiKeyForConfigand applies it correctly elsewhere (line 580) —getKeyfor the transcription target just needs the same treatment.🐛 Proposed fix
const keys = new Map<string, Promise<string | null>>() + const providersById = new Map(input.providers.providers.map((provider) => [provider.id, provider])) const getKey = (id: string): Promise<string | null> => { let key = keys.get(id) if (key === undefined) { - key = getSecret(aiKeySecretName(id)).catch(() => null) + const providerConfig = providersById.get(id) + key = ( + providerConfig === undefined + ? getSecret(aiKeySecretName(id)) + : aiApiKeyForConfig(providerConfig) + ).catch(() => null) keys.set(id, key) } return key }🤖 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 `@packages/core/src/actions/audio-memo.ts` around lines 543 - 552, Update the transcription target key resolver getKey to obtain each provider key through the existing aiApiKeyForConfig flow, matching the enrichment-key lookup below, while preserving the existing per-ID promise caching and null-on-error behavior. Do not use raw getSecret(aiKeySecretName(id)) for this lookup so keyless openai-compatible providers remain eligible.
🧹 Nitpick comments (5)
packages/core/src/ai/language-model.ts (1)
41-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the expanded public API contract.
languageModelis exported and now exposes OpenAI-compatible endpoint, model, and empty-key behavior without API documentation. Add a concise JSDoc contract above the function.As per coding guidelines, “Always document public APIs.”
🤖 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 `@packages/core/src/ai/language-model.ts` around lines 41 - 48, Document the exported languageModel function with concise JSDoc describing its OpenAI-compatible endpoint support, configured model selection, and behavior when the API key is empty. Place the documentation directly above the function declaration and preserve the existing implementation.Source: Coding guidelines
packages/core/src/ai/validate-key.ts (1)
28-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing doc on new public interface.
ApiKeyValidationInputis a new exported type with no doc comment describing its fields (particularly the optionalbaseUrl, which is only meaningful foropenai-compatible).As per coding guidelines,
**/*.{ts,tsx}: "Always document public APIs."🤖 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 `@packages/core/src/ai/validate-key.ts` around lines 28 - 32, Document the exported ApiKeyValidationInput interface and its fields, including that baseUrl is optional and applies only to the openai-compatible provider. Follow the repository’s existing public API documentation style.Source: Coding guidelines
packages/core/src/ai/openai-compatible.ts (1)
7-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing docs on exported helpers.
normalizeOpenAICompatibleBaseUrlandisHttpBaseUrlare exported without doc comments (unlikeisPlainHttpRemoteBaseUrlbelow, which explains its rationale well).As per coding guidelines,
**/*.{ts,tsx}: "Always document public APIs."📝 Suggested docs
+/** Strips whitespace and a trailing slash so base URLs compare/concatenate cleanly. */ export function normalizeOpenAICompatibleBaseUrl(value: string): string { return value.trim().replace(/\/+$/u, '') } +/** Whether `value` is a well-formed http(s) URL with no query string or fragment. */ export function isHttpBaseUrl(value: string): boolean {🤖 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 `@packages/core/src/ai/openai-compatible.ts` around lines 7 - 22, Add doc comments to the exported helpers normalizeOpenAICompatibleBaseUrl and isHttpBaseUrl, documenting their purpose and behavior consistently with the existing isPlainHttpRemoteBaseUrl documentation. Keep the implementations unchanged.Source: Coding guidelines
packages/core/src/settings/schema.test.ts (1)
294-344: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a dedicated
defaultTranscriptionProviderIddescribe block.New
aiProviderstests cover the schema's baseUrl validation well, but there's no test mirroring the existingdescribe('defaultAiProviderId', ...)block (string passthrough + invalid-value degrade-to-null) for the newly addeddefaultTranscriptionProviderIdfield — only its empty-document default is checked.✅ Suggested addition
+ describe('defaultTranscriptionProviderId', () => { + it('passes a string id through and defaults invalid values to null', () => { + expect( + settingsSchema.parse({ defaultTranscriptionProviderId: 'abc' }) + .defaultTranscriptionProviderId, + ).toBe('abc') + expect( + settingsSchema.parse({ defaultTranscriptionProviderId: null }) + .defaultTranscriptionProviderId, + ).toBeNull() + expect( + settingsSchema.parse({ defaultTranscriptionProviderId: 42 }) + .defaultTranscriptionProviderId, + ).toBeNull() + }) + })🤖 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 `@packages/core/src/settings/schema.test.ts` around lines 294 - 344, Add a dedicated describe block for defaultTranscriptionProviderId, mirroring the existing defaultAiProviderId tests: verify valid string values are preserved and invalid values degrade to null, while retaining the existing empty-document default coverage.packages/core/src/actions/audio-memo.ts (1)
388-400: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a type-guard predicate instead of
as TranscriptionProvider.
input.config.provider as TranscriptionProvidercasts past the type system rather than narrowing it; the safety here relies on callers only ever passing transcription-capable configs (enforced byaiProviderSupportsTranscriptionelsewhere), which the type system can't see. A small exported predicate (e.g.isTranscriptionProvider(provider): provider is TranscriptionProviderinprovider-config.ts) would make this a real narrowing check instead of an assertion.As per coding guidelines, "Avoid unnecessary type assertions" and "Use discriminated unions and type guards for variant data; export helper predicates when they clarify a public contract."
🤖 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 `@packages/core/src/actions/audio-memo.ts` around lines 388 - 400, Replace the `as TranscriptionProvider` assertion in the `transcribeAudio` call with a type-guard-based narrowing check. Add and export an `isTranscriptionProvider` predicate in `provider-config.ts`, use it to validate or narrow `input.config.provider` before constructing the transcription request, and preserve the existing provider-specific configuration behavior.Source: Coding guidelines
🤖 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 `@apps/desktop/src-tauri/capabilities/default.json`:
- Line 21: Document the renderer network trust boundary for the http:default
capability near its broad http and https URL allowlist, explicitly noting that
it applies to main and note windows and may expose custom AI endpoints to
webview-rendered note content. Preserve the intended localhost/LAN/clear-text
support, and identify either validated Tauri-command routing or a tighter
host/port scope as the required mitigation for untrusted renderer content.
In `@apps/desktop/src/capability-http-scope.test.ts`:
- Around line 24-26: Replace the type assertion applied to the JSON loaded in
the capability test with Zod runtime validation. Define or reuse a schema for
the capability object whose permissions accept both string identifiers and
objects containing identifier and allow fields, then parse the result before
use; do not restrict validation to a url field.
In `@packages/core/src/ai/provider-config.ts`:
- Around line 37-51: Update withAiProviderAdded so
defaultTranscriptionProviderId is set to entry.id only when makeDefault or
isFirst is true and aiProviderSupportsTranscription(entry) is true; otherwise
preserve state.defaultTranscriptionProviderId. Keep the defaultProviderId logic
unchanged.
In `@packages/core/src/ai/transcribe.test.ts`:
- Around line 301-321: Update transcribeWithOpenAi and the openai-compatible
transcribeAudio flow so the Authorization header is omitted when request.apiKey
is empty, while preserving the Bearer header for non-empty keys. Extend the
existing transcribeAudio (openai-compatible) tests with an apiKey: '' case and
assert Authorization is absent.
---
Outside diff comments:
In `@apps/desktop/src/components/settings/add-ai-provider-dialog.tsx`:
- Around line 126-135: Update the provider-switch handler in onValueChange to
clear or re-trigger validation for the model and baseUrl fields after setting
their new defaults, so stale errors are removed immediately while preserving
validation for future edits and submissions.
In `@packages/core/src/actions/audio-memo.ts`:
- Around line 543-552: Update the transcription target key resolver getKey to
obtain each provider key through the existing aiApiKeyForConfig flow, matching
the enrichment-key lookup below, while preserving the existing per-ID promise
caching and null-on-error behavior. Do not use raw
getSecret(aiKeySecretName(id)) for this lookup so keyless openai-compatible
providers remain eligible.
---
Nitpick comments:
In `@packages/core/src/actions/audio-memo.ts`:
- Around line 388-400: Replace the `as TranscriptionProvider` assertion in the
`transcribeAudio` call with a type-guard-based narrowing check. Add and export
an `isTranscriptionProvider` predicate in `provider-config.ts`, use it to
validate or narrow `input.config.provider` before constructing the transcription
request, and preserve the existing provider-specific configuration behavior.
In `@packages/core/src/ai/language-model.ts`:
- Around line 41-48: Document the exported languageModel function with concise
JSDoc describing its OpenAI-compatible endpoint support, configured model
selection, and behavior when the API key is empty. Place the documentation
directly above the function declaration and preserve the existing
implementation.
In `@packages/core/src/ai/openai-compatible.ts`:
- Around line 7-22: Add doc comments to the exported helpers
normalizeOpenAICompatibleBaseUrl and isHttpBaseUrl, documenting their purpose
and behavior consistently with the existing isPlainHttpRemoteBaseUrl
documentation. Keep the implementations unchanged.
In `@packages/core/src/ai/validate-key.ts`:
- Around line 28-32: Document the exported ApiKeyValidationInput interface and
its fields, including that baseUrl is optional and applies only to the
openai-compatible provider. Follow the repository’s existing public API
documentation style.
In `@packages/core/src/settings/schema.test.ts`:
- Around line 294-344: Add a dedicated describe block for
defaultTranscriptionProviderId, mirroring the existing defaultAiProviderId
tests: verify valid string values are preserved and invalid values degrade to
null, while retaining the existing empty-document default coverage.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2b27082c-ac35-4b68-8327-b45b44c5f255
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (58)
apps/desktop/package.jsonapps/desktop/src-tauri/capabilities/default.jsonapps/desktop/src/capability-http-scope.test.tsapps/desktop/src/components/chat/chat-screen.test.tsxapps/desktop/src/components/settings/add-ai-provider-dialog.tsxapps/desktop/src/components/settings/ai-provider-row.tsxapps/desktop/src/components/settings/ai-providers-section.test.tsxapps/desktop/src/components/settings/describe-assets-field.tsxapps/desktop/src/editor/ai-menu/use-editor-ai-menu.tsxapps/desktop/src/hooks/use-add-ai-provider-submit.tsapps/desktop/src/hooks/use-ai-providers.tsapps/desktop/src/hooks/use-audio-memo-pipeline.tsapps/desktop/src/lib/asset-describe-controller.test.tsxapps/desktop/src/lib/capture-controller.test.tsxapps/desktop/src/lib/chat-model-groups.tsapps/desktop/src/lib/transcription-reconciler.test.tsxapps/desktop/src/mobile/add-ai-provider-drawer.test.tsxapps/desktop/src/mobile/add-ai-provider-drawer.tsxapps/desktop/src/mobile/ai-provider-actions-drawer.tsxapps/desktop/src/mobile/screens/chat.test.tsxapps/desktop/src/mobile/screens/settings.tsxapps/desktop/src/providers/asset-describe-provider.tsxapps/desktop/src/providers/capture-provider.tsxapps/desktop/src/providers/chat-provider.test.tsxapps/desktop/src/providers/chat-provider.tsxpackages/core/package.jsonpackages/core/src/actions/asset-description.test.tspackages/core/src/actions/asset-description.tspackages/core/src/actions/audio-memo.test.tspackages/core/src/actions/audio-memo.tspackages/core/src/actions/capture-enrichment.tspackages/core/src/actions/capture-harness.tspackages/core/src/ai/audio-memo-title.test.tspackages/core/src/ai/audio-memo-title.tspackages/core/src/ai/chat/model-options.test.tspackages/core/src/ai/chat/stream-chat.tspackages/core/src/ai/describe-asset.tspackages/core/src/ai/describe-page.tspackages/core/src/ai/language-model.test.tspackages/core/src/ai/language-model.tspackages/core/src/ai/openai-compatible.tspackages/core/src/ai/provider-catalog.test.tspackages/core/src/ai/provider-catalog.tspackages/core/src/ai/provider-config.test.tspackages/core/src/ai/provider-config.tspackages/core/src/ai/secrets.test.tspackages/core/src/ai/secrets.tspackages/core/src/ai/transcribe.test.tspackages/core/src/ai/transcribe.tspackages/core/src/ai/transform-selection.tspackages/core/src/ai/validate-key.test.tspackages/core/src/ai/validate-key.tspackages/core/src/exports/ai-actions.tspackages/core/src/exports/platform.tspackages/core/src/settings/schema.test.tspackages/core/src/settings/schema.tsplugins/tauri-plugin-keyboard/permissions/schemas/schema.jsonplugins/tauri-plugin-recording/permissions/schemas/schema.json
| export function withAiProviderAdded( | ||
| state: AiProvidersState, | ||
| entry: AiProviderConfig, | ||
| makeDefault: boolean, | ||
| ): AiProvidersState { | ||
| const isFirst = state.providers.length === 0 | ||
| return { | ||
| providers: [...state.providers, entry], | ||
| defaultProviderId: | ||
| makeDefault || state.providers.length === 0 ? entry.id : state.defaultProviderId, | ||
| defaultProviderId: makeDefault || isFirst ? entry.id : state.defaultProviderId, | ||
| defaultTranscriptionProviderId: | ||
| makeDefault || (isFirst && aiProviderSupportsTranscription(entry)) | ||
| ? entry.id | ||
| : state.defaultTranscriptionProviderId, | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
makeDefault branch doesn't gate on transcription capability, contradicting its own doc comment.
The doc above this function says the transcription default "follows the same rule for transcription-capable entries," but the makeDefault branch sets defaultTranscriptionProviderId = entry.id unconditionally — unlike the isFirst branch, which correctly checks aiProviderSupportsTranscription(entry). Adding a non-transcription-capable provider (e.g. Anthropic) as the default provider would also incorrectly mark it as the default transcription provider.
Every current reader (defaultTranscriptionProvider, transcriptionProviders) re-validates capability before trusting this id, so there's no visible symptom today, but the stored state is inconsistent with its own contract.
🐛 Proposed fix
defaultTranscriptionProviderId:
- makeDefault || (isFirst && aiProviderSupportsTranscription(entry))
+ (makeDefault || isFirst) && aiProviderSupportsTranscription(entry)
? entry.id
: state.defaultTranscriptionProviderId,📝 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.
| export function withAiProviderAdded( | |
| state: AiProvidersState, | |
| entry: AiProviderConfig, | |
| makeDefault: boolean, | |
| ): AiProvidersState { | |
| const isFirst = state.providers.length === 0 | |
| return { | |
| providers: [...state.providers, entry], | |
| defaultProviderId: | |
| makeDefault || state.providers.length === 0 ? entry.id : state.defaultProviderId, | |
| defaultProviderId: makeDefault || isFirst ? entry.id : state.defaultProviderId, | |
| defaultTranscriptionProviderId: | |
| makeDefault || (isFirst && aiProviderSupportsTranscription(entry)) | |
| ? entry.id | |
| : state.defaultTranscriptionProviderId, | |
| } | |
| } | |
| export function withAiProviderAdded( | |
| state: AiProvidersState, | |
| entry: AiProviderConfig, | |
| makeDefault: boolean, | |
| ): AiProvidersState { | |
| const isFirst = state.providers.length === 0 | |
| return { | |
| providers: [...state.providers, entry], | |
| defaultProviderId: makeDefault || isFirst ? entry.id : state.defaultProviderId, | |
| defaultTranscriptionProviderId: | |
| (makeDefault || isFirst) && aiProviderSupportsTranscription(entry) | |
| ? entry.id | |
| : state.defaultTranscriptionProviderId, | |
| } | |
| } |
🤖 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 `@packages/core/src/ai/provider-config.ts` around lines 37 - 51, Update
withAiProviderAdded so defaultTranscriptionProviderId is set to entry.id only
when makeDefault or isFirst is true and aiProviderSupportsTranscription(entry)
is true; otherwise preserve state.defaultTranscriptionProviderId. Keep the
defaultProviderId logic unchanged.
| describe('transcribeAudio (openai-compatible)', () => { | ||
| it('posts multipart with a custom model to the configured base URL', async () => { | ||
| const calls: RecordedCall[] = [] | ||
| const fetchFn = recordingFetch(calls, () => jsonResponse(200, { text: ' hello ' })) | ||
|
|
||
| const text = await transcribeAudio( | ||
| request({ | ||
| provider: 'openai-compatible', | ||
| baseUrl: 'https://transcribe.example.com/v1', | ||
| model: 'whisper-large-v3', | ||
| fetchFn, | ||
| }), | ||
| ) | ||
|
|
||
| expect(text).toBe('hello') | ||
| expect(calls).toHaveLength(1) | ||
| expect(calls[0]!.url).toBe('https://transcribe.example.com/v1/audio/transcriptions') | ||
| expect(calls[0]!.headers['Authorization']).toBe('Bearer sk-test') | ||
| const form = calls[0]!.body as FormData | ||
| expect(form.get('model')).toBe('whisper-large-v3') | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Omit authorization for no-key transcription endpoints.
This new flow only tests sk-test, but transcribeWithOpenAi always sends Authorization: Bearer ${request.apiKey}. An empty optional key therefore produces Bearer instead of no header, unlike languageModel; no-auth compatible transcription servers can reject that request.
Proposed fix
- headers: { Authorization: `Bearer ${request.apiKey}` },
+ headers:
+ request.apiKey.trim() === ''
+ ? undefined
+ : { Authorization: `Bearer ${request.apiKey}` },Add a compatible-provider test with apiKey: '' asserting Authorization is absent.
🤖 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 `@packages/core/src/ai/transcribe.test.ts` around lines 301 - 321, Update
transcribeWithOpenAi and the openai-compatible transcribeAudio flow so the
Authorization header is omitted when request.apiKey is empty, while preserving
the Bearer header for non-empty keys. Extend the existing transcribeAudio
(openai-compatible) tests with an apiKey: '' case and assert Authorization is
absent.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/components/settings/add-ai-provider-dialog.tsx (1)
126-135: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStale validation errors can survive a provider switch.
These
setValuecalls resetmodel/baseUrlto valid defaults but don't clear or re-trigger validation, so a previously shown "Enter a model id."/"Enter an http(s) endpoint URL." error can linger on screen even though the field now holds a valid value (until the next submit attempt re-validates).🐛 Suggested fix
const next = aiProvider(aiProviderIdSchema.parse(value)) setValue('provider', next.id) - setValue('model', next.models[0].id) + setValue('model', next.models[0].id, { shouldValidate: true }) setValue( 'baseUrl', next.id === 'openai-compatible' ? DEFAULT_OPENAI_COMPATIBLE_BASE_URL : '', + { shouldValidate: true }, )🤖 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 `@apps/desktop/src/components/settings/add-ai-provider-dialog.tsx` around lines 126 - 135, Update the provider-switch handler in onValueChange to clear or re-trigger validation for the model and baseUrl fields after setting their new defaults, so stale errors are removed immediately while preserving validation for future edits and submissions.packages/core/src/actions/audio-memo.ts (1)
543-552: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTranscription key lookup bypasses
aiApiKeyForConfig, breaking keyless openai-compatible transcription.
getKeyhere resolves keys via rawgetSecret(aiKeySecretName(id)), unlike the enrichment-key path a few lines below (line 580) which correctly usesaiApiKeyForConfig(enrichmentConfig). For anopenai-compatibleprovider configured without an API key (keyHint === '', a valid "no-key compatible endpoint" peraudio-memo-title.ts's doc comment), no secret was ever stored, sogetSecretreturnsnullhere —resolveTranscriptionTargetthen treats this provider as keyless and skips it, ultimately reporting'no-key'even though the provider is fully usable. The downstream message ("The API key for the configured openai-compatible model is missing from the keychain") is also misleading for this case.This file already imports
aiApiKeyForConfigand applies it correctly elsewhere (line 580) —getKeyfor the transcription target just needs the same treatment.🐛 Proposed fix
const keys = new Map<string, Promise<string | null>>() + const providersById = new Map(input.providers.providers.map((provider) => [provider.id, provider])) const getKey = (id: string): Promise<string | null> => { let key = keys.get(id) if (key === undefined) { - key = getSecret(aiKeySecretName(id)).catch(() => null) + const providerConfig = providersById.get(id) + key = ( + providerConfig === undefined + ? getSecret(aiKeySecretName(id)) + : aiApiKeyForConfig(providerConfig) + ).catch(() => null) keys.set(id, key) } return key }🤖 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 `@packages/core/src/actions/audio-memo.ts` around lines 543 - 552, Update the transcription target key resolver getKey to obtain each provider key through the existing aiApiKeyForConfig flow, matching the enrichment-key lookup below, while preserving the existing per-ID promise caching and null-on-error behavior. Do not use raw getSecret(aiKeySecretName(id)) for this lookup so keyless openai-compatible providers remain eligible.
🧹 Nitpick comments (5)
packages/core/src/ai/language-model.ts (1)
41-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the expanded public API contract.
languageModelis exported and now exposes OpenAI-compatible endpoint, model, and empty-key behavior without API documentation. Add a concise JSDoc contract above the function.As per coding guidelines, “Always document public APIs.”
🤖 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 `@packages/core/src/ai/language-model.ts` around lines 41 - 48, Document the exported languageModel function with concise JSDoc describing its OpenAI-compatible endpoint support, configured model selection, and behavior when the API key is empty. Place the documentation directly above the function declaration and preserve the existing implementation.Source: Coding guidelines
packages/core/src/ai/validate-key.ts (1)
28-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing doc on new public interface.
ApiKeyValidationInputis a new exported type with no doc comment describing its fields (particularly the optionalbaseUrl, which is only meaningful foropenai-compatible).As per coding guidelines,
**/*.{ts,tsx}: "Always document public APIs."🤖 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 `@packages/core/src/ai/validate-key.ts` around lines 28 - 32, Document the exported ApiKeyValidationInput interface and its fields, including that baseUrl is optional and applies only to the openai-compatible provider. Follow the repository’s existing public API documentation style.Source: Coding guidelines
packages/core/src/ai/openai-compatible.ts (1)
7-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing docs on exported helpers.
normalizeOpenAICompatibleBaseUrlandisHttpBaseUrlare exported without doc comments (unlikeisPlainHttpRemoteBaseUrlbelow, which explains its rationale well).As per coding guidelines,
**/*.{ts,tsx}: "Always document public APIs."📝 Suggested docs
+/** Strips whitespace and a trailing slash so base URLs compare/concatenate cleanly. */ export function normalizeOpenAICompatibleBaseUrl(value: string): string { return value.trim().replace(/\/+$/u, '') } +/** Whether `value` is a well-formed http(s) URL with no query string or fragment. */ export function isHttpBaseUrl(value: string): boolean {🤖 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 `@packages/core/src/ai/openai-compatible.ts` around lines 7 - 22, Add doc comments to the exported helpers normalizeOpenAICompatibleBaseUrl and isHttpBaseUrl, documenting their purpose and behavior consistently with the existing isPlainHttpRemoteBaseUrl documentation. Keep the implementations unchanged.Source: Coding guidelines
packages/core/src/settings/schema.test.ts (1)
294-344: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a dedicated
defaultTranscriptionProviderIddescribe block.New
aiProviderstests cover the schema's baseUrl validation well, but there's no test mirroring the existingdescribe('defaultAiProviderId', ...)block (string passthrough + invalid-value degrade-to-null) for the newly addeddefaultTranscriptionProviderIdfield — only its empty-document default is checked.✅ Suggested addition
+ describe('defaultTranscriptionProviderId', () => { + it('passes a string id through and defaults invalid values to null', () => { + expect( + settingsSchema.parse({ defaultTranscriptionProviderId: 'abc' }) + .defaultTranscriptionProviderId, + ).toBe('abc') + expect( + settingsSchema.parse({ defaultTranscriptionProviderId: null }) + .defaultTranscriptionProviderId, + ).toBeNull() + expect( + settingsSchema.parse({ defaultTranscriptionProviderId: 42 }) + .defaultTranscriptionProviderId, + ).toBeNull() + }) + })🤖 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 `@packages/core/src/settings/schema.test.ts` around lines 294 - 344, Add a dedicated describe block for defaultTranscriptionProviderId, mirroring the existing defaultAiProviderId tests: verify valid string values are preserved and invalid values degrade to null, while retaining the existing empty-document default coverage.packages/core/src/actions/audio-memo.ts (1)
388-400: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a type-guard predicate instead of
as TranscriptionProvider.
input.config.provider as TranscriptionProvidercasts past the type system rather than narrowing it; the safety here relies on callers only ever passing transcription-capable configs (enforced byaiProviderSupportsTranscriptionelsewhere), which the type system can't see. A small exported predicate (e.g.isTranscriptionProvider(provider): provider is TranscriptionProviderinprovider-config.ts) would make this a real narrowing check instead of an assertion.As per coding guidelines, "Avoid unnecessary type assertions" and "Use discriminated unions and type guards for variant data; export helper predicates when they clarify a public contract."
🤖 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 `@packages/core/src/actions/audio-memo.ts` around lines 388 - 400, Replace the `as TranscriptionProvider` assertion in the `transcribeAudio` call with a type-guard-based narrowing check. Add and export an `isTranscriptionProvider` predicate in `provider-config.ts`, use it to validate or narrow `input.config.provider` before constructing the transcription request, and preserve the existing provider-specific configuration behavior.Source: Coding guidelines
🤖 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 `@apps/desktop/src-tauri/capabilities/default.json`:
- Line 21: Document the renderer network trust boundary for the http:default
capability near its broad http and https URL allowlist, explicitly noting that
it applies to main and note windows and may expose custom AI endpoints to
webview-rendered note content. Preserve the intended localhost/LAN/clear-text
support, and identify either validated Tauri-command routing or a tighter
host/port scope as the required mitigation for untrusted renderer content.
In `@apps/desktop/src/capability-http-scope.test.ts`:
- Around line 24-26: Replace the type assertion applied to the JSON loaded in
the capability test with Zod runtime validation. Define or reuse a schema for
the capability object whose permissions accept both string identifiers and
objects containing identifier and allow fields, then parse the result before
use; do not restrict validation to a url field.
In `@packages/core/src/ai/provider-config.ts`:
- Around line 37-51: Update withAiProviderAdded so
defaultTranscriptionProviderId is set to entry.id only when makeDefault or
isFirst is true and aiProviderSupportsTranscription(entry) is true; otherwise
preserve state.defaultTranscriptionProviderId. Keep the defaultProviderId logic
unchanged.
In `@packages/core/src/ai/transcribe.test.ts`:
- Around line 301-321: Update transcribeWithOpenAi and the openai-compatible
transcribeAudio flow so the Authorization header is omitted when request.apiKey
is empty, while preserving the Bearer header for non-empty keys. Extend the
existing transcribeAudio (openai-compatible) tests with an apiKey: '' case and
assert Authorization is absent.
---
Outside diff comments:
In `@apps/desktop/src/components/settings/add-ai-provider-dialog.tsx`:
- Around line 126-135: Update the provider-switch handler in onValueChange to
clear or re-trigger validation for the model and baseUrl fields after setting
their new defaults, so stale errors are removed immediately while preserving
validation for future edits and submissions.
In `@packages/core/src/actions/audio-memo.ts`:
- Around line 543-552: Update the transcription target key resolver getKey to
obtain each provider key through the existing aiApiKeyForConfig flow, matching
the enrichment-key lookup below, while preserving the existing per-ID promise
caching and null-on-error behavior. Do not use raw
getSecret(aiKeySecretName(id)) for this lookup so keyless openai-compatible
providers remain eligible.
---
Nitpick comments:
In `@packages/core/src/actions/audio-memo.ts`:
- Around line 388-400: Replace the `as TranscriptionProvider` assertion in the
`transcribeAudio` call with a type-guard-based narrowing check. Add and export
an `isTranscriptionProvider` predicate in `provider-config.ts`, use it to
validate or narrow `input.config.provider` before constructing the transcription
request, and preserve the existing provider-specific configuration behavior.
In `@packages/core/src/ai/language-model.ts`:
- Around line 41-48: Document the exported languageModel function with concise
JSDoc describing its OpenAI-compatible endpoint support, configured model
selection, and behavior when the API key is empty. Place the documentation
directly above the function declaration and preserve the existing
implementation.
In `@packages/core/src/ai/openai-compatible.ts`:
- Around line 7-22: Add doc comments to the exported helpers
normalizeOpenAICompatibleBaseUrl and isHttpBaseUrl, documenting their purpose
and behavior consistently with the existing isPlainHttpRemoteBaseUrl
documentation. Keep the implementations unchanged.
In `@packages/core/src/ai/validate-key.ts`:
- Around line 28-32: Document the exported ApiKeyValidationInput interface and
its fields, including that baseUrl is optional and applies only to the
openai-compatible provider. Follow the repository’s existing public API
documentation style.
In `@packages/core/src/settings/schema.test.ts`:
- Around line 294-344: Add a dedicated describe block for
defaultTranscriptionProviderId, mirroring the existing defaultAiProviderId
tests: verify valid string values are preserved and invalid values degrade to
null, while retaining the existing empty-document default coverage.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2b27082c-ac35-4b68-8327-b45b44c5f255
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (58)
apps/desktop/package.jsonapps/desktop/src-tauri/capabilities/default.jsonapps/desktop/src/capability-http-scope.test.tsapps/desktop/src/components/chat/chat-screen.test.tsxapps/desktop/src/components/settings/add-ai-provider-dialog.tsxapps/desktop/src/components/settings/ai-provider-row.tsxapps/desktop/src/components/settings/ai-providers-section.test.tsxapps/desktop/src/components/settings/describe-assets-field.tsxapps/desktop/src/editor/ai-menu/use-editor-ai-menu.tsxapps/desktop/src/hooks/use-add-ai-provider-submit.tsapps/desktop/src/hooks/use-ai-providers.tsapps/desktop/src/hooks/use-audio-memo-pipeline.tsapps/desktop/src/lib/asset-describe-controller.test.tsxapps/desktop/src/lib/capture-controller.test.tsxapps/desktop/src/lib/chat-model-groups.tsapps/desktop/src/lib/transcription-reconciler.test.tsxapps/desktop/src/mobile/add-ai-provider-drawer.test.tsxapps/desktop/src/mobile/add-ai-provider-drawer.tsxapps/desktop/src/mobile/ai-provider-actions-drawer.tsxapps/desktop/src/mobile/screens/chat.test.tsxapps/desktop/src/mobile/screens/settings.tsxapps/desktop/src/providers/asset-describe-provider.tsxapps/desktop/src/providers/capture-provider.tsxapps/desktop/src/providers/chat-provider.test.tsxapps/desktop/src/providers/chat-provider.tsxpackages/core/package.jsonpackages/core/src/actions/asset-description.test.tspackages/core/src/actions/asset-description.tspackages/core/src/actions/audio-memo.test.tspackages/core/src/actions/audio-memo.tspackages/core/src/actions/capture-enrichment.tspackages/core/src/actions/capture-harness.tspackages/core/src/ai/audio-memo-title.test.tspackages/core/src/ai/audio-memo-title.tspackages/core/src/ai/chat/model-options.test.tspackages/core/src/ai/chat/stream-chat.tspackages/core/src/ai/describe-asset.tspackages/core/src/ai/describe-page.tspackages/core/src/ai/language-model.test.tspackages/core/src/ai/language-model.tspackages/core/src/ai/openai-compatible.tspackages/core/src/ai/provider-catalog.test.tspackages/core/src/ai/provider-catalog.tspackages/core/src/ai/provider-config.test.tspackages/core/src/ai/provider-config.tspackages/core/src/ai/secrets.test.tspackages/core/src/ai/secrets.tspackages/core/src/ai/transcribe.test.tspackages/core/src/ai/transcribe.tspackages/core/src/ai/transform-selection.tspackages/core/src/ai/validate-key.test.tspackages/core/src/ai/validate-key.tspackages/core/src/exports/ai-actions.tspackages/core/src/exports/platform.tspackages/core/src/settings/schema.test.tspackages/core/src/settings/schema.tsplugins/tauri-plugin-keyboard/permissions/schemas/schema.jsonplugins/tauri-plugin-recording/permissions/schemas/schema.json
🛑 Comments failed to post (2)
apps/desktop/src-tauri/capabilities/default.json (1)
21-21: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== repository files matching default.json ==" git ls-files | rg 'apps/desktop/src-tauri/capabilities/.*\.json$|apps/desktop/src-tauri/src/.*\.rs$|capabilities|permissions' echo echo "== default.json ==" cat -n apps/desktop/src-tauri/capabilities/default.json echo echo "== Tauri capability/network permission references ==" rg -n '"http":|"http:default"|default://localhost|http://localhost|https://localhost|https://.*\*|http://.*\*|permissions|allow\[' apps/desktop/src-tauri apps/desktop -g '!node_modules' || true echo echo "== command definitions invocations ==" fd -e rs . apps/desktop/src-tauri/src -x sh -c 'echo "--- $1"; ast-grep outline "$1" || true' sh {}Repository: team-reflect/reflect-open
Length of output: 50381
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== capability-http-scope.test.ts ==" cat -n apps/desktop/src/capability-http-scope.test.ts echo echo "== AI provider related files ==" git ls-files apps/desktop/src | rg -i 'provider|ai|openai|anthropic|settings' || true echo echo "== http:default / custom provider endpoint references ==" rg -n 'http:default|baseUrl|base.*url|baseUrl:|openai|anthropic|custom provider|Custom|provider.*endpoint|endpoint.*provider|http\.fetch|fetch\(' apps/desktop/src apps/desktop/src-tauri/src -g '!node_modules' || true echo echo "== tauri plugin-http permission schema/deny/allow usage ==" rg -n '"http"|"http:default"|deny\[\]|allow\[\]|permission' plugins tauri\.conf\.json apps/desktop/src-tauri/src-tauri/Cargo.toml 2>/dev/null || true echo echo "== invoke_handler and tauri command registrations ==" cat -n apps/desktop/src-tauri/src/lib.rs | sed -n '1,180p'Repository: team-reflect/reflect-open
Length of output: 49354
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== provider-fetch.ts ==" cat -n apps/desktop/src/lib/provider-fetch.ts echo echo "== AI provider submit hook ==" cat -n apps/desktop/src/hooks/use-add-ai-provider-submit.ts | sed -n '1,140p' echo echo "== AI provider section relevant validation ==" cat -n apps/desktop/src/components/settings/ai-providers-section.tsx | sed -n '130,220p' echo echo "== provider url validation helpers ==" rg -n 'function isPlainHttpRemoteBaseUrl|isPlainHttpRemoteBaseUrl|isHttpBaseUrl|normalizeOpenAICompatibleBaseUrl|normalizeBaseUrl|providerFetch' apps/desktop/src/lib apps/desktop/src/hooks apps/desktop/src/components/settings -g '*.ts' -g '*.tsx' || true echo echo "== settings provider provider config interfaces ==" cat -n apps/desktop/src/hooks/use-ai-providers.ts | sed -n '1,140p'Repository: team-reflect/reflect-open
Length of output: 19035
Document the renderer network trust boundary for
http:default.The scope intentionally covers custom AI endpoints on localhost/LAN/clear-text hosts, but
default.jsonapplies to the main and note windows. If webview-rendered note content can execute fetches directly, this grants that content the same network reach; consider routing provider endpoints through a Tauri command with validated endpoints or tightening the host/port scope if the renderer is untrusted.🤖 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 `@apps/desktop/src-tauri/capabilities/default.json` at line 21, Document the renderer network trust boundary for the http:default capability near its broad http and https URL allowlist, explicitly noting that it applies to main and note windows and may expose custom AI endpoints to webview-rendered note content. Preserve the intended localhost/LAN/clear-text support, and identify either validated Tauri-command routing or a tighter host/port scope as the required mitigation for untrusted renderer content.apps/desktop/src/capability-http-scope.test.ts (1)
24-26: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Locate target file and related capability JSON" fd -a 'capability-http-scope\.test\.ts$|default\.json$(|/tauri/capabilities/)' . | sed 's#^\./##' | head -50 echo echo "Inspect target file around imports and target lines" file="$(fd 'capability-http-scope\.test\.ts$' . | head -1)" if [ -n "${file:=}" ]; then wc -l "$file" sed -n '1,80p' "$file" | cat -n fi echo echo "Search for existing capability/tauri JSON Zod parsing patterns" rg -n "CapabilitySchema|permissions.*zod|default\.json|JSON\.parse.*as|readFileSync.*json" -S --glob '!node_modules' --glob '!dist' . | head -120Repository: team-reflect/reflect-open
Length of output: 6576
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Inspect default capability JSON shape" cat -n apps/desktop/src-tauri/capabilities/default.json | sed -n '1,220p' echo echo "Check Tauri Rust capability parsing code" sed -n '70,110p' apps/desktop/src-tauri/src/lib.rs | cat -nRepository: team-reflect/reflect-open
Length of output: 2792
Validate the capability JSON with Zod instead of asserting its parsed shape.
JSON.parse(...) as ...provides no runtime validation for the file-derived capability file. Thedefault.jsonpermissions also include string identifiers plus objects withidentifier/allow, so the Zod schema should narrow that union rather than just checkingurl.Proposed fix
+import { z } from 'zod' +const ScopeEntrySchema = z.object({ + url: z.string(), +}) +const ObjectPermissionSchema = z.object({ + identifier: z.string(), + allow: z.array(ScopeEntrySchema), +}) +const CapabilitySchema = z.object({ + permissions: z.array(z.union([z.string(), ObjectPermissionSchema])), +}) -const capability = JSON.parse( +const capability = CapabilitySchema.parse(JSON.parse( readFileSync(new URL('../src-tauri/capabilities/default.json', import.meta.url), 'utf8'), -) as { permissions: (string | Permission)[] } +))📝 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.import { z } from 'zod' const ScopeEntrySchema = z.object({ url: z.string(), }) const ObjectPermissionSchema = z.object({ identifier: z.string(), allow: z.array(ScopeEntrySchema), }) const CapabilitySchema = z.object({ permissions: z.array(z.union([z.string(), ObjectPermissionSchema])), }) const capability = CapabilitySchema.parse(JSON.parse( readFileSync(new URL('../src-tauri/capabilities/default.json', import.meta.url), 'utf8'), ))🤖 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 `@apps/desktop/src/capability-http-scope.test.ts` around lines 24 - 26, Replace the type assertion applied to the JSON loaded in the capability test with Zod runtime validation. Define or reuse a schema for the capability object whose permissions accept both string identifiers and objects containing identifier and allow fields, then parse the result before use; do not restrict validation to a url field.Source: Coding guidelines
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/hooks/use-ai-providers.ts (1)
71-117: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winValidate
baseUrlbefore storing the keychain secret for OpenAI-compatible providers.
draft.baseUrlis normalized but not checked beforesetSecret, so an invalid OpenAI-compatible URL can be written to the keychain even though the settings entry later parses out with.safeParse()/no.catch()before the schema transform. Add theisHttpBaseUrl(draft.baseUrl ?? '')guard before persisting the secret, as the add-provider forms already require before submission.🤖 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 `@apps/desktop/src/hooks/use-ai-providers.ts` around lines 71 - 117, Validate the OpenAI-compatible provider’s baseUrl before calling setSecret in addProvider. Use isHttpBaseUrl(draft.baseUrl ?? '') as a guard, rejecting invalid URLs before any keychain write while preserving the existing behavior for other provider types.Source: Coding guidelines
♻️ Duplicate comments (1)
packages/core/src/ai/transcribe.test.ts (1)
301-321: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStill no coverage for the no-key OpenAI-compatible case — underlying header bug appears unresolved.
Per the prior review,
transcribeWithOpenAiunconditionally sendsAuthorization: Bearer ${request.apiKey}; the transcribe.ts snippet in this diff's context still shows that unconditional header. None of the newtranscribeAudio (openai-compatible)tests useapiKey: '', so the no-auth compatible-server scenario (whichsecrets.test.tsandlanguage-model.test.tsnow explicitly support) remains untested and, per the source snippet, still broken — a compatible server without auth would receive a malformedBearerheader.🐛 Suggested test addition (mirrors the fix requested previously)
+ it('omits Authorization when the endpoint requires no key', async () => { + const calls: RecordedCall[] = [] + const fetchFn = recordingFetch(calls, () => jsonResponse(200, { text: 'hello' })) + + await transcribeAudio( + request({ + provider: 'openai-compatible', + apiKey: '', + baseUrl: 'https://transcribe.example.com/v1', + model: 'whisper-large-v3', + fetchFn, + }), + ) + + expect(calls[0]!.headers['Authorization']).toBeUndefined() + })🤖 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 `@packages/core/src/ai/transcribe.test.ts` around lines 301 - 321, Add coverage in the transcribeAudio (openai-compatible) tests for an empty apiKey, asserting the request omits the Authorization header while still using the configured base URL and model. Update transcribeWithOpenAi to add the Bearer Authorization header only when request.apiKey is non-empty, preserving authenticated requests unchanged.
🧹 Nitpick comments (1)
packages/core/src/actions/audio-memo.ts (1)
379-400: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer a type guard over the
as TranscriptionProviderassertion.
input.config.provider as TranscriptionProvidersilently widens/narrows past the fact thatAiProviderConfig.provideralso includesanthropic/openrouter, whichTranscriptionProvider's own doc comment says "should never reachtranscribeAudio". It's safe today only because callers (resolveTranscriptionTarget/transcriptionProviders) pre-filter viaaiProviderSupportsTranscription, but the assertion itself doesn't encode that invariant.♻️ Suggested approach: export and use a type-guard predicate
// provider-config.ts +export function isTranscriptionProvider( + provider: AiProviderConfig['provider'], +): provider is TranscriptionProvider { + return provider === 'openai' || provider === 'google' || provider === 'openai-compatible' +}// audio-memo.ts - const text = await transcribeAudio({ - provider: input.config.provider as TranscriptionProvider, + if (!isTranscriptionProvider(input.config.provider)) { + throw new Error(`unsupported transcription provider: ${input.config.provider}`) + } + const text = await transcribeAudio({ + provider: input.config.provider,As per coding guidelines,
**/*.{ts,tsx}: "Never useanyoras any.", "Avoid unnecessary type assertions and do not use assertions to parse JSON.", and "Use discriminated unions and type guards for variant data; export helper predicates when they clarify a public contract."🤖 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 `@packages/core/src/actions/audio-memo.ts` around lines 379 - 400, Replace the `as TranscriptionProvider` assertion in `memoNoteBody` with an exported type-guard predicate that validates whether `input.config.provider` supports transcription, then call `transcribeAudio` only through the narrowed provider type. Reuse the existing provider-support definitions such as `aiProviderSupportsTranscription` or `transcriptionProviders` so unsupported `anthropic` and `openrouter` values cannot reach `transcribeAudio`, preserving the current behavior for valid transcription providers.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@apps/desktop/src/hooks/use-ai-providers.ts`:
- Around line 71-117: Validate the OpenAI-compatible provider’s baseUrl before
calling setSecret in addProvider. Use isHttpBaseUrl(draft.baseUrl ?? '') as a
guard, rejecting invalid URLs before any keychain write while preserving the
existing behavior for other provider types.
---
Duplicate comments:
In `@packages/core/src/ai/transcribe.test.ts`:
- Around line 301-321: Add coverage in the transcribeAudio (openai-compatible)
tests for an empty apiKey, asserting the request omits the Authorization header
while still using the configured base URL and model. Update transcribeWithOpenAi
to add the Bearer Authorization header only when request.apiKey is non-empty,
preserving authenticated requests unchanged.
---
Nitpick comments:
In `@packages/core/src/actions/audio-memo.ts`:
- Around line 379-400: Replace the `as TranscriptionProvider` assertion in
`memoNoteBody` with an exported type-guard predicate that validates whether
`input.config.provider` supports transcription, then call `transcribeAudio` only
through the narrowed provider type. Reuse the existing provider-support
definitions such as `aiProviderSupportsTranscription` or
`transcriptionProviders` so unsupported `anthropic` and `openrouter` values
cannot reach `transcribeAudio`, preserving the current behavior for valid
transcription providers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 69127053-5c81-4493-a783-581448599669
📒 Files selected for processing (27)
apps/desktop/src/components/settings/describe-assets-field.tsxapps/desktop/src/hooks/use-ai-providers.tsapps/desktop/src/hooks/use-audio-memo-pipeline.tsapps/desktop/src/lib/asset-describe-controller.test.tsxapps/desktop/src/lib/capture-controller.test.tsxapps/desktop/src/lib/transcription-reconciler.test.tsxapps/desktop/src/providers/asset-describe-provider.tsxapps/desktop/src/providers/capture-provider.tsxapps/desktop/src/providers/chat-provider.tsxpackages/core/src/actions/asset-description.test.tspackages/core/src/actions/audio-memo.test.tspackages/core/src/actions/audio-memo.tspackages/core/src/actions/capture-harness.tspackages/core/src/ai/audio-memo-title.test.tspackages/core/src/ai/chat/model-options.test.tspackages/core/src/ai/language-model.test.tspackages/core/src/ai/provider-catalog.tspackages/core/src/ai/provider-config.test.tspackages/core/src/ai/provider-config.tspackages/core/src/ai/secrets.test.tspackages/core/src/ai/transcribe.test.tspackages/core/src/ai/transcribe.tspackages/core/src/exports/ai-actions.tspackages/core/src/settings/schema.test.tspackages/core/src/settings/schema.tsplugins/tauri-plugin-keyboard/permissions/schemas/schema.jsonplugins/tauri-plugin-recording/permissions/schemas/schema.json
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/desktop/src/lib/asset-describe-controller.test.tsx
- packages/core/src/ai/chat/model-options.test.ts
- plugins/tauri-plugin-keyboard/permissions/schemas/schema.json
- packages/core/src/ai/audio-memo-title.test.ts
- plugins/tauri-plugin-recording/permissions/schemas/schema.json
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Remove duplicate TranscriptionTarget, TranscriptionMiss, and resolveTranscriptionTarget definitions left over from a merge between the master-base version and the current-branch version. The retained version uses the dynamic transcriptionProviders() function with the correct defaultTranscriptionProviderId. Also: - Add TRANSCRIPTION_PROVIDERS constant for desktop consent UI - Remove unused memoNoteBody function (dead code) - Fix type narrowing with as TranscriptionProvider assertion - Fix test snapshots missing defaultTranscriptionProviderId Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…lt provider - Split default checkbox into chat and transcription options in add forms - Add transcriptionModel field to add-provider forms (openai-compatible) - Extend AiProviderRow with second row for transcription (model + default) - Add setTranscriptionModel hook and transcriptionProvider to useAiProviders - Update AiProviderActionsDrawer with transcription model editing - Update NO_PROVIDER_REASON to include OpenAI-compatible providers - Export transcription model constants from @reflect/core Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The openai-compatible transcription provider was effectively non-functional: two wiring gaps stopped it from ever reaching the user's local endpoint. 1. transcribeSessionParts dropped baseUrl and transcriptionModel when calling transcribeAudio, so audio was sent to OpenAI's hosted endpoint with gpt-4o-mini-transcribe regardless of the configured local server. 2. resolveTranscriptionTarget resolved keys via a raw getSecret, which bypassed aiApiKeyForConfig's no-key handling - a keyless openai-compatible entry (a local server with no API key) always read null from the keychain and was skipped as 'no-key', silently stopping every pass before any transcription call. The reconciler now forwards config.baseUrl and config.transcriptionModel for openai-compatible providers, and resolveTranscriptionTarget receives the full AiProviderConfig so callers can use aiApiKeyForConfig for key resolution.
|
Closing this PR in favor of #1007. The scope grew beyond what the original PR body described. #995 started as transcription-only, but the actual diff now includes independent chat and transcription defaults, a #1007 starts from the same commits with a clean review surface and an accurate PR body. It stays draft until I manually open it. |
1 participant
Problem
#996 intentionally scoped
openai-compatibleto chat, editor AI transforms, capture enrichment, asset descriptions, and audio memo title generation — "where the configured chat model can be used directly." Transcription was excluded because the/v1/audio/transcriptionsendpoint is a different API surface from/v1/chat/completions, and the model types differ (speech-to-text vs chat).However, for users with OpenAI-compatible transcription endpoints (local Whisper servers, Groq, or API proxies that expose
/v1/audio/transcriptions), being able to configure transcription through the sameopenai-compatibleprovider entry is natural — the base URL is the same, only the model differs.Changes
Schema:
openai-compatibleentries gain an optionaltranscriptionModelfield (empty = transcription not supported). A newdefaultTranscriptionProviderIdsetting is introduced, independent of the chat default.Catalog:
AiProviderInfogainssupportsTranscription— a compile-time constant for hosted providers (OpenAI/Google: true, Anthropic/OpenRouter: false), andfalseforopenai-compatible(the user opts in per-entry viatranscriptionModel).aiProviderSupportsTranscription()merges both sources.Provider-config: The hardcoded
TRANSCRIPTION_PROVIDERSconstant is replaced bytranscriptionProviders()— a dynamic function that derives eligible providers from the configured state.resolveTranscriptionTargetandpickTranscriptionConfignow work with any provider that has transcription capability.Transcribe client:
transcribeAudioaccepts optionalbaseUrlandmodelparams. When the provider isopenai-compatible, the client posts to{baseUrl}/audio/transcriptionswith the user-supplied model.Audio memo pipeline:
memoNoteBodyforwardsbaseUrl+transcriptionModelto the transcription client when the provider isopenai-compatible.Still deferred
transcriptionModelfield anddefaultTranscriptionProviderId🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes