Conversation
Will be handled by whisper-asr
…cription container
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| languageCode: 'es-ES', | ||
| }; | ||
|
|
||
| if (process.env.TRANSCRIPTION_ENABLED === 'true') { |
There was a problem hiding this comment.
🔴 CRITICAL: Environment variable never set
Code checks process.env.TRANSCRIPTION_ENABLED === 'true' but this environment variable is never defined in compose.yaml. Transcription will never be enabled.
Fix: Add TRANSCRIPTION_ENABLED: 'true' to the server environment variables in compose.yaml.
There was a problem hiding this comment.
Removed, will not longer be used
| const transcriptionJobResultString = await fetchSingleTranscriptionJob(transcriptionJobDetails.AnalysisEntry.analysis_id, transcriptionJob.TranscriptionJobName); | ||
|
|
||
| // 3. Parse the transcription job result (JSON string to object) | ||
| const transcriptionJobResult = JSON.parse(transcriptionJobResultString); |
There was a problem hiding this comment.
🔴 CRITICAL: References undefined variables
Lines 18 and 21 reference transcriptionJobResultString and transcriptionJobResult that were removed when AWS Transcribe logic was deleted. This function will crash.
Fix: Remove or fix these undefined variable references.
There was a problem hiding this comment.
this file has been removed
| }); | ||
|
|
||
| // Make request with query parameters | ||
| const response = await axios.post(`http://localhost:9007/asr?${params.toString()}`, form, { |
There was a problem hiding this comment.
Uses http://localhost:9007/asr which won't work in production. Should use environment variable.
| const response = await axios.post(`http://localhost:9007/asr?${params.toString()}`, form, { | |
| const transcriptionServiceUrl = process.env.TRANSCRIPTION_SERVICE_URL || 'http://localhost:9007/asr'; | |
| const response = await axios.post(`${transcriptionServiceUrl}?${params.toString()}`, form, { |
There was a problem hiding this comment.
fixed - endpoint is now set as an env variable
| import { processPendingTranscriptionJobs } from '../controllers/transcriptionController.js'; | ||
| import { logError } from '../config/loggerFunctions.js'; | ||
|
|
||
| export const getPendingTranscriptionJobScheduler = new CronJob('* * * * *', async () => { |
There was a problem hiding this comment.
Runs every minute (* * * * *) which might be excessive for transcription jobs. Consider running every 5-15 minutes.
| export const getPendingTranscriptionJobScheduler = new CronJob('* * * * *', async () => { | |
| export const getPendingTranscriptionJobScheduler = new CronJob('*/5 * * * *', async () => { |
|
|
||
| logInfo(`Transcription job ${transcriptionJob.analysis_entry_id} completed successfully`); | ||
| } catch (error) { | ||
| logError(`Error processing transcription job, ${transcriptionJob.analysis_entry_id}`, error); |
There was a problem hiding this comment.
Only logs errors but doesn't update the database job status to FAILED. Failed jobs remain in PENDING state indefinitely.
Fix: Update job status to FAILED in the catch block before logging the error.
There was a problem hiding this comment.
This is intentional, so it gets picked up again in the next iteration
| task: 'transcribe', | ||
| output: 'json', | ||
| word_timestamps: 'false', // Works with video audio | ||
| language: 'es', |
There was a problem hiding this comment.
Hardcodes language: 'es' but the database stores languageCode: 'es-ES'. Should use the actual language code from the database.
suggestion\n const params = new URLSearchParams({\n task: 'transcribe',\n output: 'json',\n word_timestamps: 'false',\n language: transcriptionJob.language_code.split('-')[0], // Extract 'es' from 'es-ES'\n vad_filter: 'true',\n });\n
✅ Previous Issues Addressed - PR MergedThe following issues from my previous review have been resolved in the merged PR:
SummaryPR Status: ✅ MERGED (2026-01-02T12:56:43Z) This was a major architectural refactor replacing AWS Transcribe with local Whisper ASR service (
Recommendation: All critical and warning issues from the original review have been addressed in the merged implementation. |
1 participant
This is done to not rely on AWS