feat: add error handling better logging and tests for mux uploads#8700
Open
feat: add error handling better logging and tests for mux uploads#8700
Conversation
Contributor
WalkthroughAdds tests for processVideoUploads and expands ProcessVideoUploadJobData export. Changes introduce explicit mux 'errored' and 'timeout' handling with early returns, structured logging enhancements, and updated control flow for creating variants, DB updates, and enqueueing download jobs. Changes
Sequence Diagram(s)sequenceDiagram
participant Worker
participant Service
participant MuxAPI
participant Database
participant DownloadQueue
participant Logger
Worker->>Service: invoke processVideoUpload(jobData)
Service->>Database: getVideo(videoId)
Service->>MuxAPI: poll waitForMuxVideoCompletion(muxVideoId)
alt mux ready
MuxAPI-->>Service: playbackId, status=ready
Service->>Database: createVideoVariant(...) / updateVideo(...)
Service->>DownloadQueue: enqueue download job (backoff, cleanup)
Service->>Logger: info structured (videoId, muxVideoId, playbackId)
else mux errored
MuxAPI-->>Service: status=errored
Service->>Logger: error structured (videoId, muxVideoId, assetId)
Service-->>Worker: return / exit
else timeout
MuxAPI-->>Service: status=timeout
Service->>Logger: warn structured (videoId, muxVideoId)
Service-->>Worker: return / exit
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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
🤖 Fix all issues with AI agents
In `@apis/api-media/src/workers/processVideoUploads/service/service.ts`:
- Around line 205-206: The code may throw a TypeError when
muxVideoAsset.playback_ids is an empty array because playback_ids?.[0].id
assumes the first element exists; update the playbackId extraction to use safe
optional chaining (e.g., use muxVideoAsset?.playback_ids?.[0]?.id) so it yields
undefined when no playback id exists, and keep the existing ready-status check
(muxVideoAsset.status === 'ready') to gate downstream logic; modify the
playbackId assignment where the variable playbackId is defined in the service.ts
file to use the additional ?. before .id.
🧹 Nitpick comments (3)
apis/api-media/src/workers/processVideoUploads/service/service.ts (1)
133-158: Consider using early return after successful variant creation.Per coding guidelines, early returns improve readability. After successfully creating the variant, an early return would clarify that the success path is complete and avoid unnecessary condition checks.
♻️ Suggested refactor
if (finalStatus === 'ready' && playbackId) { await createVideoVariant({ videoId, edition, languageId, version, muxVideoId, playbackId, metadata, logger }) + return } + if (finalStatus === 'errored') { logger?.error( { videoId, muxVideoId, finalStatus }, 'Video upload processing failed due to Mux error' ) return } if (finalStatus === 'timeout') { logger?.warn( { videoId, muxVideoId, finalStatus }, 'Video upload processing failed due to timeout' ) }apis/api-media/src/workers/processVideoUploads/service/service.spec.ts (2)
44-132: Consider adding test coverage for timeout and edge cases.The service has additional code paths that could benefit from test coverage:
- Timeout scenario (
finalStatus === 'timeout')muxVideonot found (returns early)muxVideo.assetIdis null (returns early)These would increase confidence in the error handling logic.
📋 Example test for timeout scenario
it('logs warning when mux video processing times out', async () => { prismaMock.muxVideo.findUnique.mockResolvedValue({ id: 'mux-video-id', assetId: 'asset-id' } as any) // Mock getVideo to always return 'preparing' status ;(getVideo as jest.Mock).mockResolvedValue({ status: 'preparing', playback_ids: [] }) // Note: This test would be slow with real intervals. // Consider extracting interval/maxAttempts as injectable config for testing. // ... assertions for timeout logging })
21-25: Mock logger typing could be more precise.The
as unknown as Loggercast works but loses type safety. Consider usingjest.mocked<Logger>()or a more complete mock that matches the Logger interface.
Comment on lines
205
to
206
| const playbackId = muxVideoAsset?.playback_ids?.[0].id | ||
| if (playbackId != null && muxVideoAsset.status === 'ready') { |
Contributor
There was a problem hiding this comment.
Potential TypeError when playback_ids is an empty array.
If playback_ids is [], then playback_ids?.[0] returns undefined, and accessing .id on undefined will throw a TypeError. Add optional chaining before .id.
🐛 Proposed fix
- const playbackId = muxVideoAsset?.playback_ids?.[0].id
+ const playbackId = muxVideoAsset?.playback_ids?.[0]?.id🤖 Prompt for AI Agents
In `@apis/api-media/src/workers/processVideoUploads/service/service.ts` around
lines 205 - 206, The code may throw a TypeError when muxVideoAsset.playback_ids
is an empty array because playback_ids?.[0].id assumes the first element exists;
update the playbackId extraction to use safe optional chaining (e.g., use
muxVideoAsset?.playback_ids?.[0]?.id) so it yields undefined when no playback id
exists, and keep the existing ready-status check (muxVideoAsset.status ===
'ready') to gate downstream logic; modify the playbackId assignment where the
variable playbackId is defined in the service.ts file to use the additional ?.
before .id.
|
View your CI Pipeline Execution ↗ for commit e6f47d2
☁️ Nx Cloud last updated this comment at |
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
Bug Fixes
Tests