feat: handle video in image #3012
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds video support and binary-content handling across the web client: detects video MIME/types, renders Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant S as SearchUploadBar
participant M as UploadModal
participant FR as FileReader
participant C as CodeTab (handleCreateFile)
participant FS as Storage/FS
U->>S: Click "Upload"
S->>M: Open modal with selected files
loop For each file
M->>M: Determine type (binary vs text) via isBinaryFile/getMimeType
alt Binary (image/video/other)
M->>FR: readAsArrayBuffer(file)
FR-->>M: ArrayBuffer
M->>M: Convert to Uint8Array
M->>C: onCreateFile(path, Uint8Array)
else Text
M->>FR: readAsText(file)
FR-->>M: string
M->>C: onCreateFile(path, string)
end
C->>FS: Write file (binary or text)
FS-->>C: Ack
end
C-->>M: Done
M-->>S: Close modal
S-->>U: Upload complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/constants/src/files.ts (1)
100-115: Consider renamingIMAGE_EXTENSIONSto reflect video support.The constant name
IMAGE_EXTENSIONSsuggests it contains only image MIME types, but it now includes video formats. While this appears intentional for treating videos as "image-like" in the UI, the naming is misleading.Consider renaming to
MEDIA_EXTENSIONSorIMAGE_AND_VIDEO_EXTENSIONSfor clarity.Apply this diff to improve clarity:
-export const IMAGE_EXTENSIONS = [ +export const MEDIA_EXTENSIONS = [ 'image/jpeg', 'image/png', 'image/gif', 'image/webp', 'image/svg+xml', 'image/bmp', 'image/ico', 'image/x-icon', 'image/avif', 'video/mp4', 'video/webm', 'video/ogg', 'video/quicktime', 'video/x-msvideo', ];Note: This would require updating all references to
IMAGE_EXTENSIONSthroughout the codebase.packages/utility/src/file.ts (1)
110-113: Consider renamingisImageFileto reflect video support.The function name
isImageFilesuggests it checks for image files only, but it now returnstruefor video files as well. This is intentional based on the PR design, but the naming is misleading.Consider renaming to
isMediaFileorisImageOrVideoFilefor clarity, or add a JSDoc comment explaining that videos are included.Apply this diff:
+/** + * Check if a file is an image or video based on its MIME type + * @param fileName - The filename to check + * @returns True if the file is an image or video, false otherwise + */ export const isImageFile = (fileName: string): boolean => { const mimeType = getMimeType(fileName); return IMAGE_EXTENSIONS.includes(mimeType); };Note: A more comprehensive solution would be to rename both
IMAGE_EXTENSIONSandisImageFileas suggested in the previous file.apps/web/client/src/app/project/[id]/_components/left-panel/design-panel/image-tab/image-item.tsx (1)
79-88: RequiremimeTypeor improve fallback for video files.The component currently falls back to
image/*whenimage.mimeTypeis missing, mislabeling video blobs. Update theImagetype to makemimeTyperequired or implement a fallback based on file extension (e.g. usevideo/*for.mp4,.webm).apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/modals/upload-modal.tsx (1)
80-103: Binary file handling implementation is correct.The logic appropriately:
- Detects binary files using extension-based checks
- Reads binary content as
ArrayBufferand converts toUint8Array- Reads text content as strings
- Maintains type safety throughout the Promise chain
Consider adding file size validation.
To prevent potential memory issues when uploading very large files, consider adding a size check before reading:
for (let i = 0; i < selectedFiles.length; i++) { const file = selectedFiles[i]; if (!file) continue; + + // Validate file size (e.g., 50MB limit) + const MAX_FILE_SIZE = 50 * 1024 * 1024; + if (file.size > MAX_FILE_SIZE) { + toast.error(`File "${file.name}" exceeds maximum size of 50MB`); + continue; + } const fileName = file.name;Consider using next-intl for user-facing text.
As per coding guidelines for
apps/web/client/src/**/*.tsx, avoid hardcoded user-facing text. Consider extracting strings like "Upload Files", "Directory Path", etc., to next-intl messages for better internationalization support.apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/index.tsx (1)
418-433: Implementation correctly handles both text and binary content.The function appropriately:
- Accepts both
stringandUint8Arraycontent types- Uses a sensible default value (
'') for text files- Delegates to the underlying
writeFileAPI which supports both types- Provides appropriate user feedback via toast notifications
Consider using next-intl for toast messages.
As per coding guidelines, consider extracting hardcoded strings (lines 428-430) to next-intl messages for better internationalization support. This applies to other toast messages in this file as well (lines 388-390, 412-413, 445-447).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/file-content/code-editor.tsx(2 hunks)apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/header-controls.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/index.tsx(2 hunks)apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/modals/upload-modal.tsx(3 hunks)apps/web/client/src/app/project/[id]/_components/left-panel/design-panel/image-tab/image-grid.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/left-panel/design-panel/image-tab/image-item.tsx(4 hunks)apps/web/client/src/app/project/[id]/_components/left-panel/design-panel/image-tab/search-upload-bar.tsx(2 hunks)packages/constants/src/files.ts(1 hunks)packages/utility/src/file.ts(2 hunks)packages/utility/test/file.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
packages/constants/src/files.tsapps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/index.tsxapps/web/client/src/app/project/[id]/_components/left-panel/design-panel/image-tab/search-upload-bar.tsxapps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/file-content/code-editor.tsxapps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/modals/upload-modal.tsxapps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/header-controls.tsxapps/web/client/src/app/project/[id]/_components/left-panel/design-panel/image-tab/image-item.tsxpackages/utility/test/file.test.tsapps/web/client/src/app/project/[id]/_components/left-panel/design-panel/image-tab/image-grid.tsxpackages/utility/src/file.ts
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
packages/constants/src/files.tsapps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/index.tsxapps/web/client/src/app/project/[id]/_components/left-panel/design-panel/image-tab/search-upload-bar.tsxapps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/file-content/code-editor.tsxapps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/modals/upload-modal.tsxapps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/header-controls.tsxapps/web/client/src/app/project/[id]/_components/left-panel/design-panel/image-tab/image-item.tsxpackages/utility/test/file.test.tsapps/web/client/src/app/project/[id]/_components/left-panel/design-panel/image-tab/image-grid.tsxpackages/utility/src/file.ts
apps/web/client/src/app/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/app/**/*.tsx: Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Do not use process.env in client code; import env from @/env insteadAvoid hardcoded user-facing text; use next-intl messages/hooks
Files:
apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/index.tsxapps/web/client/src/app/project/[id]/_components/left-panel/design-panel/image-tab/search-upload-bar.tsxapps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/file-content/code-editor.tsxapps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/modals/upload-modal.tsxapps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/header-controls.tsxapps/web/client/src/app/project/[id]/_components/left-panel/design-panel/image-tab/image-item.tsxapps/web/client/src/app/project/[id]/_components/left-panel/design-panel/image-tab/image-grid.tsx
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks insteadUse path aliases @/* and ~/* for imports mapping to src/*
Files:
apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/index.tsxapps/web/client/src/app/project/[id]/_components/left-panel/design-panel/image-tab/search-upload-bar.tsxapps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/file-content/code-editor.tsxapps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/modals/upload-modal.tsxapps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/header-controls.tsxapps/web/client/src/app/project/[id]/_components/left-panel/design-panel/image-tab/image-item.tsxapps/web/client/src/app/project/[id]/_components/left-panel/design-panel/image-tab/image-grid.tsx
apps/web/client/src/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable references across renders
Keep the active MobX store in a useRef and perform async cleanup with setTimeout(() => storeRef.current?.clear(), 0) to avoid route-change races
Avoid useMemo for creating MobX store instances
Avoid putting the MobX store instance in effect dependency arrays if it causes loops; split concerns by domain
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable identities across renders
Keep the active MobX store in a useRef and clean up asynchronously with setTimeout(() => storeRef.current?.clear(), 0)
Do not use useMemo to create MobX stores
Avoid placing MobX store instances in effect dependency arrays if it causes loops; split concerns instead
observer components must be client components; place a single client boundary at the feature entry; child observers need not repeat 'use client'
Files:
apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/index.tsxapps/web/client/src/app/project/[id]/_components/left-panel/design-panel/image-tab/search-upload-bar.tsxapps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/file-content/code-editor.tsxapps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/modals/upload-modal.tsxapps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/header-controls.tsxapps/web/client/src/app/project/[id]/_components/left-panel/design-panel/image-tab/image-item.tsxapps/web/client/src/app/project/[id]/_components/left-panel/design-panel/image-tab/image-grid.tsx
apps/web/client/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Default to Server Components; add 'use client' only when using events, state/effects, browser APIs, or client-only libs
Files:
apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/index.tsxapps/web/client/src/app/project/[id]/_components/left-panel/design-panel/image-tab/search-upload-bar.tsxapps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/file-content/code-editor.tsxapps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/modals/upload-modal.tsxapps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/header-controls.tsxapps/web/client/src/app/project/[id]/_components/left-panel/design-panel/image-tab/image-item.tsxapps/web/client/src/app/project/[id]/_components/left-panel/design-panel/image-tab/image-grid.tsx
🧬 Code graph analysis (5)
apps/web/client/src/app/project/[id]/_components/left-panel/design-panel/image-tab/search-upload-bar.tsx (1)
apps/web/client/src/components/store/editor/font/index.ts (1)
isUploading(298-300)
apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/file-content/code-editor.tsx (2)
apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/shared/types.ts (1)
BinaryEditorFile(15-19)packages/utility/src/file.ts (1)
getMimeType(85-108)
apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/modals/upload-modal.tsx (1)
packages/utility/src/file.ts (1)
isBinaryFile(9-12)
packages/utility/test/file.test.ts (1)
packages/utility/src/file.ts (1)
isImageFile(110-113)
apps/web/client/src/app/project/[id]/_components/left-panel/design-panel/image-tab/image-grid.tsx (1)
apps/web/client/src/components/store/editor/image/index.ts (1)
search(119-121)
🔇 Additional comments (15)
packages/utility/src/file.ts (1)
88-104: LGTM!The video format detection is implemented correctly and follows the same pattern as image format handling. The explicit checks for common video formats before delegating to
mime.getTypeensure predictable behavior.apps/web/client/src/app/project/[id]/_components/left-panel/design-panel/image-tab/image-grid.tsx (1)
57-57: LGTM!The updated empty state message correctly reflects the expanded video support and provides clear feedback to users.
apps/web/client/src/app/project/[id]/_components/left-panel/design-panel/image-tab/search-upload-bar.tsx (1)
21-21: LGTM!All video-related updates are consistent and correctly implemented:
- The
acceptattribute properly allows both image and video file selection- User-facing messages accurately reflect the expanded functionality
Also applies to: 28-28, 36-36, 68-68
packages/utility/test/file.test.ts (2)
127-132: LGTM!The test group rename appropriately reflects the expanded video support. The addition of
document.txtto the test cases improves coverage by verifying that plain text files correctly returnfalse.
134-140: LGTM!The new test group correctly verifies that video files are recognized by
isImageFile, aligning with the implementation changes. The test covers common video formats comprehensively.apps/web/client/src/app/project/[id]/_components/left-panel/design-panel/image-tab/image-item.tsx (3)
54-56: LGTM!The video detection logic correctly checks both the MIME type and file extension, providing a robust fallback mechanism. The case-insensitive regex pattern appropriately covers common video formats.
180-201: LGTM!The video rendering implementation is well-designed:
- Appropriate attributes for grid preview context (
muted,loop,playsInline)- Good UX with hover-triggered playback
- Efficient loading with
preload="metadata"- Proper cleanup by resetting
currentTimeon mouse leave
283-283: LGTM!The conditional dialog title appropriately distinguishes between deleting videos and images, providing clear context for the user action.
apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/file-content/code-editor.tsx (2)
43-46: LGTM!The
isVideoFilehelper function is correctly implemented, reusing thegetMimeTypeutility for consistent MIME type detection.
178-195: LGTM!The conditional rendering correctly distinguishes between videos and images:
- Videos include
controlsfor user interaction in the code editor context- Consistent styling between both media types
- Fallback message for unsupported browsers improves accessibility
apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/header-controls.tsx (1)
21-21: LGTM!The type signature update correctly broadens the
onCreateFilecallback to accept binary content (Uint8Array), enabling video and other binary file uploads. This change aligns with the broader video support implementation across the codebase.apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/modals/upload-modal.tsx (3)
14-14: LGTM!The
isBinaryFileimport is correctly added to support binary file detection based on file extensions.
23-23: LGTM!The type signature correctly broadens
contentto accept both string andUint8Array, aligning with the binary file support introduced across the codebase.
78-78: Ensure backend path sanitizes user‐controlled paths. Client‐sidepath.joinnormalization alone can’t prevent traversal—verify the upload API handler resolves, normalizes, and restrictscurrentPath+fileNameto a safe directory before writing.apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/index.tsx (1)
28-28: LGTM!The
CodeTabRefinterface correctly updateshandleCreateFileto acceptstring | Uint8Array, maintaining consistency with the upload modal and binary file support.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/context-pills/image-pill.tsx (1)
40-53: Consider accessibility and error handling for video elements.The video element lacks accessibility attributes and error handling. Consider these improvements:
- Add a
titleoraria-labelattribute for screen readers- Add an
onErrorhandler to gracefully handle loading failures- Consider adding a fallback UI or message when video fails to load
Apply this diff to improve accessibility and error handling:
- {isVideo ? ( - <video - src={context.content} - className="w-full h-full object-cover rounded-l-md" - muted - playsInline - /> - ) : ( + {isVideo ? ( + <video + src={context.content} + className="w-full h-full object-cover rounded-l-md" + muted + playsInline + title={context.displayName} + aria-label={`Video: ${context.displayName}`} + onError={(e) => { + console.error('Failed to load video:', context.displayName); + e.currentTarget.style.display = 'none'; + }} + /> + ) : (apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/modals/upload-modal.tsx (1)
83-90: Consider adding null checks for type safety.While
reader.resultis never null whenonloadfires in practice, TypeScript types it as nullable. Adding a defensive check improves type safety.reader.onload = () => { + if (!reader.result) { + reject(new Error('Failed to read file: result is null')); + return; + } if (isBinary) { // For binary files, convert ArrayBuffer to Uint8Array resolve(new Uint8Array(reader.result as ArrayBuffer)); } else { // For text files, use string result resolve(reader.result as string); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/modals/upload-modal.tsx(3 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/context-pills/image-pill.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
apps/web/client/src/app/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/app/**/*.tsx: Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Do not use process.env in client code; import env from @/env insteadAvoid hardcoded user-facing text; use next-intl messages/hooks
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/context-pills/image-pill.tsxapps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/modals/upload-modal.tsx
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks insteadUse path aliases @/* and ~/* for imports mapping to src/*
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/context-pills/image-pill.tsxapps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/modals/upload-modal.tsx
apps/web/client/src/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable references across renders
Keep the active MobX store in a useRef and perform async cleanup with setTimeout(() => storeRef.current?.clear(), 0) to avoid route-change races
Avoid useMemo for creating MobX store instances
Avoid putting the MobX store instance in effect dependency arrays if it causes loops; split concerns by domain
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable identities across renders
Keep the active MobX store in a useRef and clean up asynchronously with setTimeout(() => storeRef.current?.clear(), 0)
Do not use useMemo to create MobX stores
Avoid placing MobX store instances in effect dependency arrays if it causes loops; split concerns instead
observer components must be client components; place a single client boundary at the feature entry; child observers need not repeat 'use client'
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/context-pills/image-pill.tsxapps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/modals/upload-modal.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/context-pills/image-pill.tsxapps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/modals/upload-modal.tsx
apps/web/client/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Default to Server Components; add 'use client' only when using events, state/effects, browser APIs, or client-only libs
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/context-pills/image-pill.tsxapps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/modals/upload-modal.tsx
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/context-pills/image-pill.tsxapps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/modals/upload-modal.tsx
🧬 Code graph analysis (1)
apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/modals/upload-modal.tsx (1)
packages/utility/src/file.ts (1)
isBinaryFile(9-12)
🔇 Additional comments (5)
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/context-pills/image-pill.tsx (1)
19-20: No action needed: mimeType is always set on ImageMessageContext
ThemimeTypeproperty is required by the type and populated in all production code paths; the optional chaining only covers test‐only edge cases.apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/modals/upload-modal.tsx (4)
14-14: LGTM!The
isBinaryFileimport enables proper detection of binary files during upload, supporting the new video/binary content handling.
23-23: LGTM!Expanding
onCreateFileto acceptUint8Arrayalongsidestringcorrectly supports binary uploads. Ensure all callers have been updated to handle both content types.
79-79: LGTM!Using file extension to determine binary vs. text is appropriate for client-side UX. Backend validation should remain the source of truth for file type verification.
81-100: LGTM!The conditional file reading logic correctly handles binary and text files using the appropriate FileReader methods. The implementation properly converts
ArrayBuffertoUint8Arrayfor binary content and maintains string content for text files.
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
packages/utility/src/file.ts
Outdated
| export const isVideoFile = (fileNameOrMimeType: string): boolean => { | ||
| // If it looks like a MIME type (contains '/'), check it directly | ||
| if (fileNameOrMimeType.includes('/')) { | ||
| return fileNameOrMimeType.startsWith('video/'); |
There was a problem hiding this comment.
Consider normalizing the input to lowercase in isVideoFile when the argument contains '/' to correctly handle MIME types with uppercase letters.
| return fileNameOrMimeType.startsWith('video/'); | |
| return fileNameOrMimeType.toLowerCase().startsWith('video/'); |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/context-pills/image-pill.tsx (2)
1-6: Ensure a client boundary is presentThis component uses event handlers and motion; it must render within a Client Component. If not already under a parent 'use client' boundary, add it here.
+ 'use client'; import { type ImageMessageContext, MessageContextType } from '@onlook/models/chat'; import { Icons } from '@onlook/ui/icons'; import { isVideoFile } from '@onlook/utility'; import { motion } from 'motion/react'; import React from 'react';As per coding guidelines
41-54: A11y/perf: label video and avoid heavy preloading; add img loading hintsAdd an accessible name for the video and only load metadata; provide loading/decoding hints for the image.
- {isVideo ? ( - <video - src={context.content} - className="w-full h-full object-cover rounded-l-md" - muted - playsInline - /> + {isVideo ? ( + <video + src={context.content} + className="w-full h-full object-cover rounded-l-md" + muted + playsInline + preload="metadata" + aria-label={context.displayName} + /> ) : ( - <img + <img src={context.content} alt={context.displayName} - className="w-full h-full object-cover rounded-l-md" + className="w-full h-full object-cover rounded-l-md" + loading="lazy" + decoding="async" /> )}packages/utility/test/file.test.ts (1)
163-236: Add a few edge/negative cases to harden isVideoFile testsStrengthen coverage for unsupported extensions, missing extensions, and MIME parameters.
describe('edge cases', () => { it('handles filenames with special characters', () => { expect(isVideoFile('my-video_file.mp4')).toBe(true); expect(isVideoFile('video (1).webm')).toBe(true); expect(isVideoFile('clip@2x.mov')).toBe(true); }); it('handles filenames with unicode characters', () => { expect(isVideoFile('视频.mp4')).toBe(true); expect(isVideoFile('ビデオ.webm')).toBe(true); }); + + it('returns false for unknown/unsupported video-like extensions', () => { + expect(isVideoFile('movie.mkv')).toBe(false); + expect(isVideoFile('clip.mpeg')).toBe(false); + }); + + it('returns false for filenames without extension', () => { + expect(isVideoFile('noextension')).toBe(false); + }); + + it('returns true for video MIME types with parameters', () => { + expect(isVideoFile('video/mp4; codecs="avc1.42E01E"')).toBe(true); + }); }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/file-content/code-editor.tsx(2 hunks)apps/web/client/src/app/project/[id]/_components/left-panel/design-panel/image-tab/image-item.tsx(5 hunks)apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/context-pills/image-pill.tsx(3 hunks)packages/utility/src/file.ts(3 hunks)packages/utility/test/file.test.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/file-content/code-editor.tsx
- apps/web/client/src/app/project/[id]/_components/left-panel/design-panel/image-tab/image-item.tsx
- packages/utility/src/file.ts
🧰 Additional context used
📓 Path-based instructions (6)
apps/web/client/src/app/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/app/**/*.tsx: Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Do not use process.env in client code; import env from @/env insteadAvoid hardcoded user-facing text; use next-intl messages/hooks
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/context-pills/image-pill.tsx
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks insteadUse path aliases @/* and ~/* for imports mapping to src/*
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/context-pills/image-pill.tsx
apps/web/client/src/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable references across renders
Keep the active MobX store in a useRef and perform async cleanup with setTimeout(() => storeRef.current?.clear(), 0) to avoid route-change races
Avoid useMemo for creating MobX store instances
Avoid putting the MobX store instance in effect dependency arrays if it causes loops; split concerns by domain
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable identities across renders
Keep the active MobX store in a useRef and clean up asynchronously with setTimeout(() => storeRef.current?.clear(), 0)
Do not use useMemo to create MobX stores
Avoid placing MobX store instances in effect dependency arrays if it causes loops; split concerns instead
observer components must be client components; place a single client boundary at the feature entry; child observers need not repeat 'use client'
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/context-pills/image-pill.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/context-pills/image-pill.tsxpackages/utility/test/file.test.ts
apps/web/client/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Default to Server Components; add 'use client' only when using events, state/effects, browser APIs, or client-only libs
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/context-pills/image-pill.tsx
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/context-pills/image-pill.tsxpackages/utility/test/file.test.ts
🧬 Code graph analysis (2)
apps/web/client/src/app/project/[id]/_components/right-panel/chat-tab/context-pills/image-pill.tsx (1)
packages/utility/src/file.ts (1)
isVideoFile(120-128)
packages/utility/test/file.test.ts (1)
packages/utility/src/file.ts (2)
isImageFile(110-113)isVideoFile(120-128)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/utility/src/file.ts (1)
120-128: Normalize MIME type input to lowercase for case-insensitive matching.The MIME type prefix check at line 122 is case-sensitive. If a caller passes uppercase MIME types like
"VIDEO/MP4", the function will incorrectly treat it as a filename rather than a MIME type.Based on past review comments.
Apply this diff to normalize the input:
export const isVideoFile = (fileNameOrMimeType: string): boolean => { + const normalized = fileNameOrMimeType.toLowerCase(); // If it looks like a MIME type (starts with 'video/' pattern), check it directly - if (fileNameOrMimeType.startsWith('video/') || fileNameOrMimeType.startsWith('audio/') || fileNameOrMimeType.startsWith('image/')) { - return fileNameOrMimeType.startsWith('video/'); + if (normalized.startsWith('video/') || normalized.startsWith('audio/') || normalized.startsWith('image/')) { + return normalized.startsWith('video/'); } // Otherwise, treat it as a filename or file path const mimeType = getMimeType(fileNameOrMimeType); return mimeType.startsWith('video/'); };packages/utility/test/file.test.ts (1)
134-140: Semantic confusion:isImageFileshould not return true for videos.This test documents that
isImageFile('video.mp4')returnstrue, which is semantically incorrect. Images and videos are distinct media types and should be tested by separate predicates.Based on past review comments.
The underlying issue is that
IMAGE_EXTENSIONSincludes video MIME types. Consider one of these approaches:
- Keep
isImageFilestrictly for images, useisVideoFilefor videos- Rename
isImageFiletoisMediaFileorisVisualAssetFileto reflect that it handles both- Create a separate constant for video extensions and update
isImageFileto exclude videosThis change would require updating:
packages/constants/src/files.tsto separateIMAGE_EXTENSIONSandVIDEO_EXTENSIONS- All call sites that currently rely on
isImageFileaccepting videos
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/file-content/code-editor.tsx(2 hunks)packages/utility/src/file.ts(3 hunks)packages/utility/test/file.test.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
apps/web/client/src/app/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/app/**/*.tsx: Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Do not use process.env in client code; import env from @/env insteadAvoid hardcoded user-facing text; use next-intl messages/hooks
Files:
apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/file-content/code-editor.tsx
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks insteadUse path aliases @/* and ~/* for imports mapping to src/*
Files:
apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/file-content/code-editor.tsx
apps/web/client/src/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable references across renders
Keep the active MobX store in a useRef and perform async cleanup with setTimeout(() => storeRef.current?.clear(), 0) to avoid route-change races
Avoid useMemo for creating MobX store instances
Avoid putting the MobX store instance in effect dependency arrays if it causes loops; split concerns by domain
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable identities across renders
Keep the active MobX store in a useRef and clean up asynchronously with setTimeout(() => storeRef.current?.clear(), 0)
Do not use useMemo to create MobX stores
Avoid placing MobX store instances in effect dependency arrays if it causes loops; split concerns instead
observer components must be client components; place a single client boundary at the feature entry; child observers need not repeat 'use client'
Files:
apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/file-content/code-editor.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/file-content/code-editor.tsxpackages/utility/src/file.tspackages/utility/test/file.test.ts
apps/web/client/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Default to Server Components; add 'use client' only when using events, state/effects, browser APIs, or client-only libs
Files:
apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/file-content/code-editor.tsx
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/file-content/code-editor.tsxpackages/utility/src/file.tspackages/utility/test/file.test.ts
🧬 Code graph analysis (2)
apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/file-content/code-editor.tsx (2)
packages/utility/src/file.ts (1)
isVideoFile(120-128)apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/shared/types.ts (1)
BinaryEditorFile(15-19)
packages/utility/test/file.test.ts (1)
packages/utility/src/file.ts (2)
isImageFile(110-113)isVideoFile(120-128)
🪛 GitHub Actions: CI
apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/file-content/code-editor.tsx
[error] 1-1: Cannot find module '@codemirror/view' or its corresponding type declarations.
🔇 Additional comments (3)
apps/web/client/src/app/project/[id]/_components/left-panel/code-panel/code-tab/file-content/code-editor.tsx (1)
173-189: LGTM! Video rendering implementation is solid.The conditional rendering correctly distinguishes between video and image files. The video element includes:
- Controls for user playback interaction
- Consistent styling with the image element
- Accessible fallback text for unsupported browsers
packages/utility/src/file.ts (1)
98-104: LGTM! Video MIME type handling is comprehensive.The video format mappings cover common web video formats (mp4, webm, ogg, mov, avi) with correct MIME types.
packages/utility/test/file.test.ts (1)
163-244: LGTM! Comprehensive test coverage for video file detection.The
isVideoFiletest suite thoroughly validates:
- Common video extensions (mp4, webm, ogg, mov, avi)
- MIME type detection for video formats
- Negative cases (images, audio, documents)
- Edge cases (special characters, unicode, file paths)
...src/app/project/[id]/_components/left-panel/code-panel/code-tab/file-content/code-editor.tsx
Show resolved
Hide resolved
Kitenite
changed the title
Kitenite
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/utility/src/file.ts (1)
122-124: Normalize input before checking MIME type prefix.The case-sensitive check on line 122 means uppercase MIME types like "Video/MP4" won't be recognized as MIME types and will fall through to filename handling, which may not work correctly.
Apply this diff to normalize the input:
- if (fileNameOrMimeType.startsWith('video/') || fileNameOrMimeType.startsWith('audio/') || fileNameOrMimeType.startsWith('image/')) { - return fileNameOrMimeType.toLowerCase().startsWith('video/'); - } + const normalized = fileNameOrMimeType.toLowerCase(); + if (normalized.startsWith('video/') || normalized.startsWith('audio/') || normalized.startsWith('image/')) { + return normalized.startsWith('video/'); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/utility/src/file.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
packages/utility/src/file.ts
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
packages/utility/src/file.ts
🔇 Additional comments (2)
packages/utility/src/file.ts (2)
88-97: LGTM!The comment improves code organization by clearly delineating the image formats section.
98-104: LGTM!The video format handling correctly maps file extensions to their standard MIME types and follows the established pattern from the image formats section above.
| if (fileNameOrMimeType.startsWith('video/') || fileNameOrMimeType.startsWith('audio/') || fileNameOrMimeType.startsWith('image/')) { | ||
| return fileNameOrMimeType.toLowerCase().startsWith('video/'); | ||
| } | ||
| // Otherwise, treat it as a filename or file path | ||
| const mimeType = getMimeType(fileNameOrMimeType); | ||
| return mimeType.startsWith('video/'); |
There was a problem hiding this comment.
🧩 Analysis chain
Refine MIME type detection to avoid path ambiguity.
The current logic treats any string starting with 'video/', 'audio/', or 'image/' as a MIME type. This incorrectly identifies file paths like "video/intro.mp4" or "assets/image/logo.png" as MIME types instead of filenames.
Consider using a more robust pattern to distinguish MIME types from file paths. One approach is to check for a valid MIME type format (type/subtype with no additional path separators):
export const isVideoFile = (fileNameOrMimeType: string): boolean => {
- // If it looks like a MIME type (starts with 'video/' pattern), check it directly
- if (fileNameOrMimeType.startsWith('video/') || fileNameOrMimeType.startsWith('audio/') || fileNameOrMimeType.startsWith('image/')) {
- return fileNameOrMimeType.toLowerCase().startsWith('video/');
+ const normalized = fileNameOrMimeType.toLowerCase().trim();
+
+ // Check if it matches MIME type pattern: type/subtype with no additional slashes
+ const mimePattern = /^[a-z]+\/[a-z0-9+.-]+$/;
+ if (mimePattern.test(normalized)) {
+ return normalized.startsWith('video/');
}
+
// Otherwise, treat it as a filename or file path
const mimeType = getMimeType(fileNameOrMimeType);
return mimeType.startsWith('video/');
};Verify that the test suite covers these edge cases:
🏁 Script executed:
#!/bin/bash
# Description: Check if tests cover MIME type vs. file path edge cases
# Search for test cases in file.test.ts
rg -A 5 -B 2 "isVideoFile" --type=ts -g "**/file.test.ts"Length of output: 6678
Refine MIME type detection and add missing edge‐case tests
- packages/utility/src/file.ts (lines 122–127): replace the broad
startsWith('video/'|'audio/'|'image/')check with a regex that enforces a valid MIME format (e.g./^[a-z]+\/[a-z0-9+.-]+$/) before usingnormalized.startsWith('video/'). - packages/utility/test/file.test.ts: add tests for strings like
"video/intro.mp4"and"assets/image/logo.png"to ensure these are treated as file paths, not MIME types.
🤖 Prompt for AI Agents
In packages/utility/src/file.ts around lines 122–127, the current logic treats
any string containing "video/","audio/" or "image/" as a MIME type; replace that
broad startsWith check with a strict MIME format validation using a regex like
/^[a-z]+\/[a-z0-9+.\-]+$/i against a normalized lowercase input and only when it
matches treat it as a MIME (return normalized.startsWith('video/')); otherwise
fall back to treating the input as a filename/path and call getMimeType(...) as
before. Also update packages/utility/test/file.test.ts to add edge-case tests
verifying that strings like "video/intro.mp4" and "assets/image/logo.png" are
treated as file paths (not MIME) and that valid MIME strings (e.g. "video/mp4")
are detected as video.
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Important
This PR adds video handling capabilities alongside images, including upload, preview, and management, with UI updates and expanded test coverage.
code-editor.tsx,header-controls.tsx, andindex.tsxfor upload, preview, and management.image-item.tsx.image-grid.tsxandsearch-upload-bar.tsx.file.test.ts.This description was created by
for f64959f. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
UI/Copy
Tests