fix: custom video upload help and error messages #8619
Conversation
WalkthroughReplaces per-error boolean flags with a unified Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
View your CI Pipeline Execution ↗ for commit 77d36de
☁️ Nx Cloud last updated this comment at |
github-actions
bot
requested a deployment
to
Preview - journeys-admin
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
|
The latest updates on your projects.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@apps/journeys-admin/src/components/Editor/Slider/Settings/Drawer/VideoLibrary/VideoFromMux/AddByFile/AddByFile.tsx`:
- Around line 61-70: The duration check in AddByFile.tsx is too permissive:
replace the current conditional that uses "duration && duration < 0.5" with a
strict numeric validation that treats null/NaN/zero as invalid and enforces a
minimum of 1 second; use the value returned from getVideoDuration (refer to the
duration variable and files[0]) and call setErrorType('file-duration-too-short')
when duration is missing, NaN, or less than 1.0 seconds so sub‑1s clips are
rejected reliably.
- Around line 95-99: The onDropRejected handler is assigning
fileRejections[0].errors[0].code (ErrorCode | string) directly to setErrorType
which expects a restricted customErrorCode; add a type guard in onDropRejected
to check whether the dropped error code is one of the allowed customErrorCode
values (e.g., via a whitelist/enum or an isCustomErrorCode function) and call
setErrorType with that value, otherwise call
setErrorType('general-upload-error') as the fallback; update references in the
same function (onDropRejected, FileRejection) to use the guard before setting
state.
🧹 Nitpick comments (1)
apps/journeys-admin/src/components/Editor/Slider/Settings/Drawer/VideoLibrary/VideoFromMux/AddByFile/utils/getVideoDuration/getVideoDuration.spec.tsx (1)
4-13: Avoidanyfor the mocked video element.
Use a small typed mock to keep the spec type-safe.♻️ Suggested typing tweak
describe('getVideoDuration', () => { - let mockVideo: any + type MockVideo = Partial<HTMLVideoElement> & { + onloadedmetadata: (() => void) | null + onerror: (() => void) | null + } + let mockVideo: MockVideoAs per coding guidelines, define a type instead of
anywhere possible.
...c/components/Editor/Slider/Settings/Drawer/VideoLibrary/VideoFromMux/AddByFile/AddByFile.tsx
Show resolved
Hide resolved
...c/components/Editor/Slider/Settings/Drawer/VideoLibrary/VideoFromMux/AddByFile/AddByFile.tsx
Show resolved
Hide resolved
github-actions
bot
requested a deployment
to
Preview - journeys-admin
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/journeys-admin/src/components/Editor/Slider/Settings/Drawer/VideoLibrary/VideoFromMux/AddByFile/AddByFile.spec.tsx (1)
89-98: Add mock forgetVideoDurationto prevent test hangs.When
dropTestVideo()is called in tests (lines 89-98, 188-227), it triggersonDropAcceptedwhich awaitsgetVideoDuration. In JSDOM, the video element'sonloadedmetadataevent never fires for File objects, causing the Promise to hang indefinitely and tests to timeout.Add this mock at the top of the test file with the existing mocks:
🛠️ Add getVideoDuration mock
jest.mock('./utils/getVideoDuration/getVideoDuration', () => ({ getVideoDuration: jest.fn().mockResolvedValue(5) }))
♻️ Duplicate comments (2)
apps/journeys-admin/src/components/Editor/Slider/Settings/Drawer/VideoLibrary/VideoFromMux/AddByFile/AddByFile.tsx (2)
61-70: Duration check is inconsistent with the error message.The code checks
duration < 0.5but the error message states "Minimum duration is 1 second" (line 128). Additionally, the checkduration && duration < 0.5allows0orNaNthrough since0is falsy. This was flagged in a previous review.🛠️ Proposed fix
let duration: number | null = null try { duration = await getVideoDuration(files[0]) } catch (error) { return setErrorType('general-upload-error') } - if (duration && duration < 0.5) { + if (duration == null || !Number.isFinite(duration) || duration < 1) { return setErrorType('file-duration-too-short') }
95-99: Type error:fileRejections[0].errors[0].codeisErrorCode | string, not assignable tocustomErrorCode.The
as customErrorCodecast is unsafe because react-dropzone's error code can be an arbitrary string. This will cause a TypeScript error (TS2345) as indicated by pipeline failures. Add validation before casting.🛠️ Proposed fix
const onDropRejected = async ( fileRejections: FileRejection[] ): Promise<void> => { - setErrorType(fileRejections[0].errors[0].code as customErrorCode) + const code = fileRejections[0]?.errors[0]?.code + const validCodes: string[] = [ + ErrorCode.FileTooLarge, + ErrorCode.FileInvalidType, + ErrorCode.TooManyFiles, + ErrorCode.FileTooSmall + ] + if (code != null && validCodes.includes(code)) { + setErrorType(code as customErrorCode) + } else { + setErrorType('general-upload-error') + } }
🧹 Nitpick comments (2)
apps/journeys-admin/src/components/Editor/Slider/Settings/Drawer/VideoLibrary/VideoFromMux/AddByFile/AddByFile.tsx (1)
43-48: Type definition should be exported or moved to a shared types file.The
customErrorCodetype is defined inside the component function, which limits reusability and makes testing harder. Consider moving it outside the component or to a shared types file.♻️ Suggested refactor
+type CustomErrorCode = + | ErrorCode + | 'file-duration-too-short' + | 'general-upload-error' + export function AddByFile({ onChange }: AddByFileProps): ReactElement { // ... - type customErrorCode = - | ErrorCode - | 'file-duration-too-short' - | 'general-upload-error' - - const [errorType, setErrorType] = useState<customErrorCode | null>(null) + const [errorType, setErrorType] = useState<CustomErrorCode | null>(null)apps/journeys-admin/src/components/Editor/Slider/Settings/Drawer/VideoLibrary/VideoFromMux/AddByFile/AddByFile.spec.tsx (1)
155-362: Consider adding tests for duration validation errors.The implementation adds
file-duration-too-shorterror handling, but there's no test verifying this behavior. Consider adding a test that mocksgetVideoDurationto return a value below the threshold.🧪 Suggested test
it('should show error for video duration too short', async () => { const { getVideoDuration } = require('./utils/getVideoDuration/getVideoDuration') getVideoDuration.mockResolvedValueOnce(0.4) // Below 0.5s threshold (or 1s if fixed) render( <TestWrapper> <AddByFile onChange={jest.fn()} /> </TestWrapper> ) await dropTestVideo() await waitFor(() => { expect(screen.getByText('Upload Failed!')).toBeInTheDocument() expect( screen.getByText('Video is too short. Minimum duration is 1 second.') ).toBeInTheDocument() }) })
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.