chore: add server uploads to pg #1055
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
WalkthroughThis pull request introduces new React functional components, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
📦 Bundle size comparison
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (14)
playground-v6/components/fieldset.tsx (2)
4-23: Consider extracting className constants for better maintainability.The component implementation is solid, but the long className strings could be moved to constants or a separate styles object for better maintainability.
Consider refactoring like this:
const INPUT_BASE_CLASSES = "border-input focus-visible:border-ring focus-visible:ring-ring/30 flex w-full rounded-lg border px-3 py-2 text-sm shadow-sm shadow-black/5 transition-shadow focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-offset-2 disabled:cursor-not-allowed disabled:opacity-50"; const INPUT_TYPE_CLASSES = { search: "[&::-webkit-search-cancel-button]:appearance-none [&::-webkit-search-decoration]:appearance-none [&::-webkit-search-results-button]:appearance-none [&::-webkit-search-results-decoration]:appearance-none", file: "file:border-input p-0 pr-3 italic file:me-3 file:h-full file:border-0 file:border-r file:border-solid file:bg-transparent file:px-3 file:text-sm file:font-medium file:not-italic" };
25-35: Consider enhancing accessibility with aria attributes.While the Label component is well-implemented, consider adding support for common accessibility patterns.
Consider adding these props:
interface LabelProps extends React.ComponentProps<"label"> { required?: boolean; htmlFor: string; // Make this required } // Usage example: export function Label({ className, required, htmlFor, ...props }: LabelProps) { return ( <label className={cx( "text-sm font-medium leading-4 peer-disabled:cursor-not-allowed peer-disabled:opacity-70", className, )} htmlFor={htmlFor} aria-required={required} {...props} /> ); }playground/components/fieldset.tsx (3)
4-23: Consider enhancing accessibility attributes.The Input component is well-implemented with comprehensive styling and proper type handling. Consider adding
aria-labeloraria-describedbyprops to improve accessibility for screen readers.
12-19: Consider extracting Tailwind classes for better maintainability.The className string is quite long. Consider extracting it into constants or using Tailwind's @apply directive in a separate CSS file for better maintainability.
+const baseInputStyles = "border-input focus-visible:border-ring focus-visible:ring-ring/30 flex w-full rounded-lg border px-3 py-2 text-sm shadow-sm shadow-black/5 transition-shadow focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-offset-2 disabled:cursor-not-allowed disabled:opacity-50" +const searchInputStyles = "[&::-webkit-search-cancel-button]:appearance-none [&::-webkit-search-decoration]:appearance-none [&::-webkit-search-results-button]:appearance-none [&::-webkit-search-results-decoration]:appearance-none" +const fileInputStyles = "file:border-input p-0 pr-3 italic file:me-3 file:h-full file:border-0 file:border-r file:border-solid file:bg-transparent file:px-3 file:text-sm file:font-medium file:not-italic" className={cx( className, - "border-input focus-visible:border-ring focus-visible:ring-ring/30 flex w-full rounded-lg border px-3 py-2 text-sm shadow-sm shadow-black/5 transition-shadow focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-offset-2 disabled:cursor-not-allowed disabled:opacity-50", + baseInputStyles, type === "search" && - "[&::-webkit-search-cancel-button]:appearance-none [&::-webkit-search-decoration]:appearance-none [&::-webkit-search-results-button]:appearance-none [&::-webkit-search-results-decoration]:appearance-none", + searchInputStyles, type === "file" && - "file:border-input p-0 pr-3 italic file:me-3 file:h-full file:border-0 file:border-r file:border-solid file:bg-transparent file:px-3 file:text-sm file:font-medium file:not-italic", + fileInputStyles, )}
25-35: Consider validating htmlFor prop.The Label component looks good, but consider adding validation to ensure the htmlFor prop is provided when the label is not wrapping an input directly. This helps maintain proper accessibility standards.
-export function Label({ className, ...props }: React.ComponentProps<"label">) { +export function Label({ className, htmlFor, ...props }: React.ComponentProps<"label">) { + if (!htmlFor && !props.children) { + console.warn('Label should either wrap an input or have an htmlFor prop'); + } return ( <label + htmlFor={htmlFor} className={cx( "text-sm font-medium leading-4 peer-disabled:cursor-not-allowed peer-disabled:opacity-70", className, )} {...props} /> ); }playground-v6/lib/actions.ts (2)
69-72: Consider allowing partial successThe current implementation marks the operation as successful only if all files are uploaded. Consider allowing partial success and communicating which files failed.
return { - success: uploadedCount === uploadResults.length, + success: uploadedCount > 0, uploadedCount, + failedFiles: uploadResults + .filter((result) => result.error !== null) + .map((result) => result.error), };
39-73: Add file validation and size limitsThe function should validate file types and sizes before attempting upload to prevent unnecessary network usage and potential security issues.
Consider:
- Adding file type validation
- Implementing size limits
- Adding rate limiting per session
- Documenting these limitations in the function's JSDoc
Would you like me to provide an implementation example?
playground/lib/actions.ts (2)
39-46: Consider removing unused parameter.The
previousStateparameter is not used in the function implementation. If it's not required for the server action framework, consider removing it.-export async function uploadFiles(previousState: unknown, form: FormData) { +export async function uploadFiles(form: FormData) {
39-73: Consider implementing rate limiting.To prevent potential abuse, consider implementing rate limiting for the upload functionality. This could be based on:
- Number of requests per time window
- Total upload size per time window
- Number of files per request
This would help protect server resources and prevent misuse of the upload functionality.
playground-v6/components/uploader.tsx (2)
29-52: Enhance form accessibilityThe form lacks proper ARIA labels and role attributes for better accessibility.
Consider these improvements:
-<form ref={formRef} action={dispatch}> +<form + ref={formRef} + action={dispatch} + aria-label={`Upload ${props.type} to server`} + role="form" +> <div className="space-y-1"> - <Label>Upload (server {props.type})</Label> + <Label htmlFor={`server-${props.type}-input`}> + Upload (server {props.type}) + </Label> <Input + id={`server-${props.type}-input`} name="files" // ... other props /> </div>
60-85: Consider improving responsive layoutThe current flex layout might not work well on smaller screens.
Consider making the layout more responsive:
-<div className="flex gap-4"> +<div className="flex flex-col sm:flex-row gap-4">playground/components/uploader.tsx (3)
28-46: Add visual feedback for upload stateThe component lacks visual feedback during uploads. Users might not know if their upload is in progress.
Consider adding a loading indicator:
<div className="space-y-1"> <Label>Upload (server {props.type})</Label> <Input name="files" multiple disabled={isUploading} className="h-10 p-0 file:me-3 file:border-0 file:border-e" type={props.type === "file" ? "file" : "text"} onChange={...} /> + {isUploading && ( + <div className="text-sm text-gray-500"> + Uploading... + </div> + )} </div>
15-54: Consider implementing security measuresThe server uploader should implement security measures to prevent abuse.
Consider adding:
- File type validation for file uploads
- File size limits
- Rate limiting
- URL allowlist/blocklist for URL uploads
- CSRF protection
Would you like me to provide implementation examples for these security measures?
66-71: Standardize error handling approachThe client-side error handling uses window.alert while the server-side uses useEffect with alert. Consider standardizing the error handling approach across both upload methods for consistency.
Consider extracting error handling to a shared utility:
const handleUploadError = (error: string) => { // Could be replaced with a toast notification or custom error UI window.alert(error); };Then use it consistently:
onUploadError={(error) => { - window.alert(error.message); + handleUploadError(error.message); }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
playground-v6/components/fieldset.tsx(1 hunks)playground-v6/components/uploader.tsx(1 hunks)playground-v6/lib/actions.ts(2 hunks)playground/components/fieldset.tsx(1 hunks)playground/components/uploader.tsx(1 hunks)playground/lib/actions.ts(2 hunks)
🔇 Additional comments (9)
playground-v6/components/fieldset.tsx (2)
1-2: LGTM: Appropriate imports for the components' needs.
The imports are minimal and necessary for the components' functionality.
15-18: Verify browser compatibility for search and file input styling.
The component includes browser-specific styles for search and file inputs. These styles should be tested across different browsers to ensure consistent appearance.
✅ Verification successful
Browser compatibility for search and file input styling is well-established
The styling patterns used in the fieldset component follow established practices seen across the codebase:
- The webkit-specific search input styles are consistently used in other components (e.g., docs/src/components/Search.tsx) to normalize the appearance of search inputs
- The file input styling using the
file:modifier is a standard pattern used in multiple components (e.g., examples/with-tailwindcss, examples/profile-picture)
These patterns are part of a design system that's been implemented across different parts of the application, suggesting they've been tested and validated for cross-browser compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any browser compatibility comments or issues related to input styling
rg -i "browser|webkit|firefox|safari" --type ts --type tsx
Length of output: 88
Script:
#!/bin/bash
# Search for any browser compatibility comments or issues related to input styling
# Using just ts extension and including tsx files in the search
rg -i "browser|webkit|firefox|safari" --type ts
# Also search specifically in the fieldset component
cat "playground-v6/components/fieldset.tsx"
# Look for similar styling patterns in other components
rg -g "*.{ts,tsx}" "\[&::-webkit-|file:"
Length of output: 10135
playground/components/fieldset.tsx (1)
1-3: LGTM! Dependencies are appropriate.
The imports are clean and use modern syntax. The clsx library is a good choice for managing conditional class names in React components.
playground-v6/lib/actions.ts (1)
8-8: LGTM: Import statement is correctly placed
The import for UploadFileResult type is appropriately placed and used in the new function.
playground/lib/actions.ts (2)
8-8: LGTM: Type import is correctly placed and utilized.
48-54: Add input validation for files.
Consider adding the following validations before processing uploads:
- Check if files array is empty
- Validate file types and sizes
- Implement a reasonable limit on number of files
✅ Verification successful
Let me search for any existing file validations in the codebase.
Let me search for any existing file validation implementations in the codebase.
Based on the search results, I can now generate the final response since I have found evidence of file validation capabilities in the codebase through the dropzone components and shared utilities.
Add file validations using built-in dropzone options
The codebase already has built-in validation capabilities that you can leverage:
- Use
maxFilesoption to limit the number of files - Use
maxSizeoption to validate file sizes - Use
acceptoption to validate file types
Example implementation:
const files = form.getAll("files") as File[] | string[];
if (files.length === 0) {
return { success: false, error: "No files provided" };
}
let uploadResults: UploadFileResult[];
if (files.some((file) => typeof file === "string")) {
uploadResults = await utapi.uploadFilesFromUrl(files as string[], {
maxSize: 1024 * 1024 * 4 // 4MB limit
});
} else {
uploadResults = await utapi.uploadFiles(files as File[], {
maxSize: 1024 * 1024 * 4 // 4MB limit
});
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any file validations in the codebase
rg -A 5 "getAll\(\"files\"\)"
rg -A 5 "utapi\.uploadFiles"
Length of output: 17219
Script:
#!/bin/bash
# Search for file validation patterns
rg -A 5 "validateFile|fileValidation|maxSize|maxFiles|fileType"
ast-grep --pattern 'if ($files.length $$$)'
Length of output: 43912
Script:
#!/bin/bash
# Search for file validation implementations
rg -A 5 "uploadFiles|uploadFilesFromUrl" "packages/uploadthing/src/"
ast-grep --pattern 'function $name($_, $_) {
$$$
if ($files $$$) {
$$$
}
$$$
}'
Length of output: 8903
playground-v6/components/uploader.tsx (2)
3-3: LGTM: Imports are well-organized and necessary
The new imports are properly organized and all serve a clear purpose in the implementation.
Also applies to: 9-10
9-9: Verify uploadFiles action implementation
Let's ensure the uploadFiles action is properly implemented with proper error handling and type safety.
Also applies to: 17-20
✅ Verification successful
uploadFiles action implementation is properly handled
The implementation in playground-v6/lib/actions.ts shows:
- Proper session validation
- Type-safe return values with success/error states
- Handles both URL and File uploads
- Validates upload results
- Includes cache invalidation
- Returns structured responses with error messages
The implementation demonstrates good practices for error handling and type safety, making it suitable for use in the uploader component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the implementation of uploadFiles action
ast-grep --pattern 'export async function uploadFiles($_) {
$$$
}'
# Check for proper error handling
rg -A 10 "catch.*error" playground*/lib/actions.ts
Length of output: 1119
Script:
#!/bin/bash
# Let's check the actual implementation in the playground-v6 directory
rg -A 10 "uploadFiles" playground-v6/lib/actions.ts
# Check for any type definitions
fd "actions.ts" --exec rg -l "type|interface"
# Look for error handling patterns in the codebase
rg "try.*catch" playground-v6/lib/actions.ts
Length of output: 923
playground/components/uploader.tsx (1)
9-9: Verify uploadFiles implementation and error handling
Let's verify the implementation of the uploadFiles action to ensure proper error handling and session validation.
✅ Verification successful
Implementation verified - proper error handling and session validation in place
The uploadFiles action and its integration are properly implemented with:
- Session validation before file operations
- Comprehensive error handling for both URL and File uploads
- Proper type checking and result validation
- Cache invalidation after successful uploads
- Consistent error response structure
The implementation aligns well with the uploader component's usage and includes all necessary security checks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check uploadFiles implementation and related files
# Check for proper session validation
rg -A 10 "uploadFiles.*export" ../lib/actions.ts
# Check for error handling patterns
rg -A 5 "throw.*Error" ../lib/actions.ts
# Look for related route handlers
fd -e ts -e tsx route.ts -x rg -l "uploadthing"
Length of output: 876
Script:
#!/bin/bash
# Let's check the correct path for actions and implementation
# First, find the uploadFiles implementation
rg -l "uploadFiles" --type ts --type tsx
# Then check its content
rg -A 10 "uploadFiles" --type ts --type tsx
# Also check the uploader component implementation
cat playground/components/uploader.tsx
Length of output: 2581
Script:
#!/bin/bash
# Let's find and check the lib/actions implementation relative to the playground directory
cat playground/lib/actions.ts
# Also check the uploadthing route implementation
cat playground/app/api/uploadthing/route.ts
# Look for any error handling patterns in these directories
rg "throw|catch|Error" playground/lib/ playground/app/api/uploadthing/
Length of output: 3779
| if (uploadResults.every((result) => result.error !== null)) { | ||
| return { | ||
| success: false as const, | ||
| error: "Failed to upload some files", | ||
| }; | ||
| } |
There was a problem hiding this comment.
Fix inverted error check logic
The current logic checks if ALL results have errors, but it should check if ANY result has an error.
-if (uploadResults.every((result) => result.error !== null)) {
+if (uploadResults.some((result) => result.error !== null)) {
return {
success: false as const,
error: "Failed to upload some files",
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (uploadResults.every((result) => result.error !== null)) { | |
| return { | |
| success: false as const, | |
| error: "Failed to upload some files", | |
| }; | |
| } | |
| if (uploadResults.some((result) => result.error !== null)) { | |
| return { | |
| success: false as const, | |
| error: "Failed to upload some files", | |
| }; | |
| } |
| }; | ||
| } | ||
|
|
||
| const files = form.getAll("files") as File[] | string[]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type validation for file input
The type casting of FormData values to File[] | string[] could be unsafe. Consider adding type validation before the cast.
-const files = form.getAll("files") as File[] | string[];
+const files = form.getAll("files").map((file) => {
+ if (!(file instanceof File) && typeof file !== "string") {
+ throw new Error("Invalid file type");
+ }
+ return file;
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const files = form.getAll("files") as File[] | string[]; | |
| const files = form.getAll("files").map((file) => { | |
| if (!(file instanceof File) && typeof file !== "string") { | |
| throw new Error("Invalid file type"); | |
| } | |
| return file; | |
| }); |
| if (uploadResults.every((result) => result.error !== null)) { | ||
| return { | ||
| success: false as const, | ||
| error: "Failed to upload some files", | ||
| }; | ||
| } | ||
|
|
||
| revalidateTag(CACHE_TAGS.LIST_FILES); | ||
|
|
||
| const uploadedCount = uploadResults.filter( | ||
| (result) => result.data != null, | ||
| ).length; | ||
|
|
||
| return { | ||
| success: uploadedCount === uploadResults.length, | ||
| uploadedCount, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling and logging.
The current error handling could be improved by:
- Including specific error details in the response
- Adding error logging for debugging purposes
if (uploadResults.every((result) => result.error !== null)) {
+ const errors = uploadResults
+ .map((result) => result.error)
+ .filter(Boolean);
+ console.error("File upload errors:", errors);
return {
success: false as const,
- error: "Failed to upload some files",
+ error: "Failed to upload files",
+ details: errors,
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (uploadResults.every((result) => result.error !== null)) { | |
| return { | |
| success: false as const, | |
| error: "Failed to upload some files", | |
| }; | |
| } | |
| revalidateTag(CACHE_TAGS.LIST_FILES); | |
| const uploadedCount = uploadResults.filter( | |
| (result) => result.data != null, | |
| ).length; | |
| return { | |
| success: uploadedCount === uploadResults.length, | |
| uploadedCount, | |
| }; | |
| } | |
| if (uploadResults.every((result) => result.error !== null)) { | |
| const errors = uploadResults | |
| .map((result) => result.error) | |
| .filter(Boolean); | |
| console.error("File upload errors:", errors); | |
| return { | |
| success: false as const, | |
| error: "Failed to upload files", | |
| details: errors, | |
| }; | |
| } | |
| revalidateTag(CACHE_TAGS.LIST_FILES); | |
| const uploadedCount = uploadResults.filter( | |
| (result) => result.data != null, | |
| ).length; | |
| return { | |
| success: uploadedCount === uploadResults.length, | |
| uploadedCount, | |
| }; | |
| } |
| useEffect(() => { | ||
| if (state.success === false && state.error) { | ||
| window.alert(state.error); | ||
| } | ||
| }, [state.success]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling UX
Using window.alert for error messages isn't ideal for accessibility and user experience.
Consider using a toast notification system or an inline error message:
-useEffect(() => {
- if (state.success === false && state.error) {
- window.alert(state.error);
- }
-}, [state.success]);
+const [showError, setShowError] = useState(false);
+useEffect(() => {
+ if (state.success === false && state.error) {
+ setShowError(true);
+ }
+}, [state.success]);
// In the JSX:
+{showError && (
+ <div role="alert" className="text-red-500 text-sm mt-2">
+ {state.error}
+ <button onClick={() => setShowError(false)} className="ml-2">Dismiss</button>
+ </div>
+)}Committable suggestion skipped: line range outside the PR's diff.
| <Input | ||
| name="files" | ||
| multiple | ||
| disabled={isUploading} | ||
| className="h-10 p-0 file:me-3 file:border-0 file:border-e" | ||
| type={props.type === "file" ? "file" : "text"} | ||
| onChange={() => { | ||
| if (props.type === "file") { | ||
| formRef.current?.requestSubmit(); | ||
| return; | ||
| } | ||
| }} | ||
| /> |
There was a problem hiding this comment.
Fix incomplete URL input handling
The Input component has multiple enabled for both file and URL types, but the onChange handler only processes file uploads. This could lead to confusion for URL inputs.
Consider this fix:
<Input
name="files"
- multiple
+ multiple={props.type === "file"}
disabled={isUploading}
className="h-10 p-0 file:me-3 file:border-0 file:border-e"
type={props.type === "file" ? "file" : "text"}
onChange={() => {
if (props.type === "file") {
formRef.current?.requestSubmit();
return;
}
+ // For URL type, submit on Enter key press instead
}}
+ onKeyDown={(e) => {
+ if (props.type === "url" && e.key === "Enter") {
+ e.preventDefault();
+ formRef.current?.requestSubmit();
+ }
+ }}
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Input | |
| name="files" | |
| multiple | |
| disabled={isUploading} | |
| className="h-10 p-0 file:me-3 file:border-0 file:border-e" | |
| type={props.type === "file" ? "file" : "text"} | |
| onChange={() => { | |
| if (props.type === "file") { | |
| formRef.current?.requestSubmit(); | |
| return; | |
| } | |
| }} | |
| /> | |
| <Input | |
| name="files" | |
| multiple={props.type === "file"} | |
| disabled={isUploading} | |
| className="h-10 p-0 file:me-3 file:border-0 file:border-e" | |
| type={props.type === "file" ? "file" : "text"} | |
| onChange={() => { | |
| if (props.type === "file") { | |
| formRef.current?.requestSubmit(); | |
| return; | |
| } | |
| // For URL type, submit on Enter key press instead | |
| }} | |
| onKeyDown={(e) => { | |
| if (props.type === "url" && e.key === "Enter") { | |
| e.preventDefault(); | |
| formRef.current?.requestSubmit(); | |
| } | |
| }} | |
| /> |
| onChange={() => { | ||
| if (props.type === "file") { | ||
| formRef.current?.requestSubmit(); | ||
| return; | ||
| } | ||
| }} |
There was a problem hiding this comment.
Incomplete onChange handler implementation
The onChange handler only handles the file type case but doesn't implement any logic for URL type. This could lead to a confusing user experience when entering URLs.
Consider implementing URL validation and auto-submission:
onChange={() => {
if (props.type === "file") {
formRef.current?.requestSubmit();
return;
}
+ if (props.type === "url") {
+ const input = formRef.current?.querySelector('input');
+ const url = input?.value;
+ if (url && isValidUrl(url)) {
+ formRef.current?.requestSubmit();
+ }
+ }
}}Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
InputandLabelcomponents for improved form element styling and accessibility.ServerUploadercomponent for server-side file and URL uploads, enhancing upload options.