fix(stt): append api-version for Azure OpenAI Whisper endpoint#217
fix(stt): append api-version for Azure OpenAI Whisper endpoint#217wwenrr wants to merge 3 commits intodaggerhashimoto:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/services/openai-whisper.test.ts (1)
14-82: Good test coverage for the three URL construction scenarios.The tests correctly verify:
- Base URL unchanged when no API version configured
api-versionquery param appended when configured- Existing
api-versionin URL is preserved (not overwritten)Consider adding a test for whitespace-only
openaiApiVersion(e.g.,' ') to verify the.trim()behavior works as expected, ensuring it falls back to the base URL.💡 Optional: Add edge case test for whitespace-only API version
it('uses base whisper URL when API version is whitespace-only', async () => { vi.doMock('../lib/config.js', () => ({ config: { openaiApiKey: 'sk-test', openaiApiVersion: ' ', language: 'en' }, })); vi.doMock('../lib/constants.js', () => ({ OPENAI_WHISPER_URL: 'https://api.openai.com/v1/audio/transcriptions', })); vi.mocked(fetch).mockResolvedValue( new Response(JSON.stringify({ text: 'ok' }), { status: 200 }), ); const { transcribe } = await import('./openai-whisper.js'); await transcribe(Buffer.from('abc'), 'sample.webm'); expect(fetch).toHaveBeenCalledWith( 'https://api.openai.com/v1/audio/transcriptions', expect.objectContaining({ method: 'POST' }), ); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/services/openai-whisper.test.ts` around lines 14 - 82, Add a test in openai-whisper.test.ts that mocks config.config.openaiApiVersion to a whitespace-only string (e.g., ' '), mocks OPENAI_WHISPER_URL to the base URL, stubs fetch to return a successful Response, imports transcribe from ./openai-whisper.js, calls transcribe(Buffer.from('abc'), 'sample.webm'), and asserts fetch was called with the base OPENAI_WHISPER_URL (no ?api-version appended); reference the transcribe function, config.openaiApiVersion, and OPENAI_WHISPER_URL when locating where to add the new test and reuse the existing fetch mock setup/assertion pattern from the other tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/services/openai-whisper.test.ts`:
- Around line 14-82: Add a test in openai-whisper.test.ts that mocks
config.config.openaiApiVersion to a whitespace-only string (e.g., ' '), mocks
OPENAI_WHISPER_URL to the base URL, stubs fetch to return a successful Response,
imports transcribe from ./openai-whisper.js, calls
transcribe(Buffer.from('abc'), 'sample.webm'), and asserts fetch was called with
the base OPENAI_WHISPER_URL (no ?api-version appended); reference the transcribe
function, config.openaiApiVersion, and OPENAI_WHISPER_URL when locating where to
add the new test and reuse the existing fetch mock setup/assertion pattern from
the other tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: df4de8c7-b586-410f-984f-6712a9383a68
📒 Files selected for processing (3)
server/lib/config.tsserver/services/openai-whisper.test.tsserver/services/openai-whisper.ts
|
Addressed the CodeRabbit suggestion ✅\n\nAdded a new test case for whitespace-only API version config:\n- now verifies we keep the base \n- Ensures no is appended when the configured value trims to empty\n\nPushed in commit: \n\nNote: I couldn't run the local test command in this environment because isn't installed here, but the test follows the existing test harness/mocking pattern in this file. |
Summary
Fixes Azure OpenAI Whisper transcription failures by appending
api-versionwhen configured.What changed
openaiApiVersionconfig value from env:OPENAI_API_VERSIONAZURE_OPENAI_API_VERSION(fallback alias)server/services/openai-whisper.tsto build request URL dynamically:OPENAI_WHISPER_URLby default?api-version=<value>when version env is setapi-versionif already present in URLserver/services/openai-whisper.test.tsfor all three URL behaviors.Verification
npx vitest run server/services/openai-whisper.test.ts server/routes/transcribe.test.tsnpm run build:serverFixes #171
Summary by CodeRabbit
New Features
Tests