REFACTOR AWS transcription to local transcription #31
Conversation
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
UPDATE swagger docs
Will be handled by whisper-asr
…cription container
ADD transcription local functionality
|
| Severity | Issue | Location |
|---|---|---|
| WARNING | Incorrect status rollback on transcription failure | server/controllers/transcriptionController.js:39 |
| WARNING | Missing error object logging | server/controllers/analysisEntryController.js:33 |
| WARNING | Potential null pointer access | server/integrations/whisper-asr-webservice/transcribe.js:8 |
Recommendation: Address critical issues before merge
Review Details (18 files)
Files:
- server/controllers/transcriptionController.js (1 issue)
- server/controllers/analysisEntryController.js (1 issue)
- server/integrations/whisper-asr-webservice/transcribe.js (1 issue)
- compose.yaml, package.json, .env.example, server/integrations/s3-client/s3.js, and others
Checked: Security, bugs, performance, error handling
Key Issues Found:
1. Incorrect Status Rollback (WARNING)
File: server/controllers/transcriptionController.js:39
When transcription fails, the status is incorrectly set back to IN_PROGRESS instead of PENDING, preventing retry attempts.
Fix: Change status to PENDING for failed jobs.
2. Missing Error Details (WARNING)
File: server/controllers/analysisEntryController.js:33
Error logging does not include the actual error object, making debugging difficult.
Fix: Include error parameter in logError call.
3. Potential Null Pointer (WARNING)
File: server/integrations/whisper-asr-webservice/transcribe.js:8
Accessing transcriptionJob.AnalysisEntry without null check could cause runtime errors.
Fix: Add null check before accessing nested properties.
Positive Changes:
- Good separation of concerns with new transcription controller
- Improved S3 client architecture (external/internal split)
- Removed AWS dependencies, reducing external service costs
- Simplified transcription normalizer logic
- Docker compose properly configured for local transcription
This is done to accomodate for upcoming internal S3 endpoint
It was set to 'IN_PROGRESS' when it should have been 'PENDING' so it would be picked up again by the CRON job
This was not included due to an oversight -- Once a transcription Job is completed, it will now update the status of the transcription job in DB to 'COMPLETED'
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Types of Changes
Related issue
Explanation of changes/Implementation
Screenshots (if applicable)
Pull Request Checklist
Additional Notes