fix: dropzone behavior #980
Conversation
This reverts commit ddda408.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
🦋 Changeset detectedLatest commit: 3db2694 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Rate limit exceeded@juliusmarminge has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 33 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis pull request introduces significant changes across multiple files, primarily focusing on the removal of the Changes
Assessment against linked issues
Possibly related PRs
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
|
More templates
@uploadthing/nuxt
@uploadthing/react
@uploadthing/solid
@uploadthing/svelte
@uploadthing/vue
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 8
Outside diff range and nitpick comments (10)
packages/svelte/src/lib/index.ts (1)
6-7: Summary: Changes look good, ensure thorough testingThe changes to
createDropzoneand its type exports appear to be in line with the PR objectives. Moving to a local implementation should allow for better control over the dropzone behavior.To ensure the effectiveness of these changes:
- Implement comprehensive unit and integration tests for the new
createDropzoneimplementation, focusing on the multiple file upload scenario.- Conduct end-to-end testing to verify that the issue of multiple file uploads triggering twice has been resolved.
- If not already done, consider adding a test case that reproduces the original issue to prevent regression in future updates.
packages/react/package.json (1)
Line range hint
1-124: Summary of package.json changesThe modifications to the dependencies in this package.json file align well with the PR's objective of fixing the dropzone behavior issue (#965). By replacing the custom
@uploadthing/dropzonewith the standardizedfile-selectorlibrary, this change potentially addresses the problem of multiple file uploads triggering twice.However, to ensure a comprehensive fix:
- Verify that all components previously using
@uploadthing/dropzonehave been updated to usefile-selector.- Update the documentation to reflect these changes in the API or usage instructions.
- Consider adding or updating tests specifically for multiple file uploads to prevent regression.
These changes represent a significant shift in how file selection is handled. Ensure that this architectural change is consistently applied across all relevant parts of the codebase, including React, Solid, Vue, and Svelte implementations as mentioned in the PR checklist.
packages/shared/src/dropzone-utils.ts (2)
3-11: LGTM: DropzoneOptions type definition is comprehensive.The
DropzoneOptionstype definition covers essential configuration options for a dropzone component, including multiple file uploads, file type acceptance, size limits, and more. The requiredonDropfunction ensures that file uploads are always handled.Consider adding JSDoc comments to describe each option, especially for less obvious properties like
minSizeandmaxSize(are they in bytes?). This would improve the developer experience when using this type.Example:
export type DropzoneOptions = { /** Allow multiple file uploads */ multiple?: boolean; /** Accepted file types */ accept?: AcceptProp | undefined; /** Minimum file size in bytes */ minSize?: number; /** Maximum file size in bytes */ maxSize?: number; // ... (add comments for other properties) };
13-20: LGTM: DropzoneState type definition is comprehensive.The
DropzoneStatetype definition effectively captures the various states of a dropzone, including drag-and-drop interactions and the list of accepted files.Consider adding a
rejectedFilesproperty to theDropzoneStatetype. This would allow for easier handling and feedback for files that don't meet the acceptance criteria. Here's a suggested addition:export type DropzoneState = { // ... existing properties acceptedFiles: File[]; rejectedFiles: File[]; // Add this line };This addition would provide a more complete state representation and potentially simplify error handling and user feedback implementation.
packages/svelte/src/lib/component/UploadDropzone.svelte (1)
Line range hint
1-359: Consider future refactoring for improved maintainability.While this change addresses the immediate issue, the overall complexity of this component suggests that a future refactoring effort could be beneficial. Consider breaking down this large component into smaller, more manageable sub-components.
Here are some suggestions for future improvements:
- Extract the dropzone logic into a separate component.
- Create a dedicated component for the upload button.
- Consider using Svelte stores for managing the upload state.
- Implement proper error handling and user feedback mechanisms.
These refactoring efforts could improve the maintainability and testability of the code in the long run.
packages/solid/src/components/dropzone.tsx (2)
534-535: Prevent default action inonInputElementClickIn the
onInputElementClickhandler, onlyevent.stopPropagation()is called. To ensure that the default browser action doesn't occur, consider addingevent.preventDefault().Apply this diff:
const onInputElementClick = (event: MouseEvent) => { + event.preventDefault(); event.stopPropagation(); };
540-542: Update comment to reflect SolidJS instead of ReactThe comment mentions React handling state changes, but since this code uses SolidJS, the comment should be updated for accuracy.
Apply this diff:
// In IE11/Edge the file-browser dialog is blocking, therefore, use setTimeout() - // to ensure React can handle state changes + // to ensure SolidJS can handle state changes // See: https://github.com/react-dropzone/react-dropzone/issues/450packages/react/src/components/dropzone.tsx (3)
Line range hint
235-240: Avoid redundant calls to$props.onChangeinhandlePasteIn the
handlePastefunction, theonChangecallback is being called twice: once inside thesetFilesstate updater and once immediately after. This redundancy can lead to unnecessary executions of the callback.To prevent this, consider removing the inner call to
$props.onChangeinsidesetFiles:setFiles((prev) => { filesToUpload = [...prev, ...pastedFiles]; - $props.onChange?.(filesToUpload); return filesToUpload; }); $props.onChange?.(filesToUpload);
Line range hint
229-245: Clean up dependency array inuseEffectto avoid unnecessary re-rendersIn the
useEffecthandling the paste event, includingfilesandrootRefin the dependency array might cause unnecessary re-executions of the effect, sincefilesis updated within the effect androotRefis a ref which does not change.Consider updating the dependency array:
- }, [uploadFiles, $props, appendOnPaste, mode, fileTypes, rootRef, files]); + }, [uploadFiles, $props, appendOnPaste, mode, fileTypes]);
Line range hint
217-226: Ensure consistent prevention of default behavior inonUploadClickIn the
onUploadClickhandler,e.preventDefault()ande.stopPropagation()are called in multiple branches. To maintain consistency and prevent unintended side effects, consider structuring the function to handle these calls uniformly.Review and refactor the
onUploadClickfunction to ensure that default behaviors are prevented only when necessary, and that the code is clear and maintainable.
Comments failed to post (8)
packages/svelte/src/lib/index.ts (1)
6-6: 💡 Codebase verification
Missing
createDropzoneimplementation in React, Solid, and Vue frameworks.The
create-dropzone.tsfile was only found in the Svelte package. Please ensure that similar implementations are added for React, Solid, and Vue frameworks as outlined in the PR checklist.Analysis chain
LGTM! Verify the new implementation and check other frameworks.
The change from an external package to a local implementation for
createDropzonelooks good. This is likely part of the fix for the dropzone behavior issue.Please ensure that:
- The local implementation in
./component/create-dropzoneaddresses the issue of multiple file uploads triggering twice.- This change is consistent with the fix implemented for the React framework.
- Similar changes are made for Solid and Vue frameworks, as mentioned in the PR checklist.
Run the following script to verify the implementation across frameworks:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for createDropzone implementation across frameworks # Check for local implementation in Svelte echo "Checking Svelte implementation:" cat packages/svelte/src/lib/component/create-dropzone.ts # Check for similar changes in React echo "Checking React implementation:" cat packages/react/src/component/create-dropzone.ts # Check for Solid implementation echo "Checking Solid implementation:" cat packages/solid/src/component/create-dropzone.ts # Check for Vue implementation echo "Checking Vue implementation:" cat packages/vue/src/component/create-dropzone.tsLength of output: 1340
Script:
#!/bin/bash # Description: Search for create-dropzone.ts implementations across the entire repository echo "Searching for create-dropzone.ts files in the repository:" fd create-dropzone.tsLength of output: 201
packages/react/src/index.ts (1)
14-15: 💡 Codebase verification
Error:
useDropzoneis not exported from the local moduleThe local module
./components/dropzonedoes not exportuseDropzoneas expected.
- Ensure that
useDropzoneis correctly implemented and exported indropzone.tsx.Analysis chain
LGTM! Verify local implementation and update documentation.
The change to export
useDropzoneand its types from a local module aligns with the PR objective of fixing the dropzone behavior. This should allow for better control over the functionality and potentially resolve the issue with multiple file uploads triggering twice.Please run the following script to verify the local implementation:
Consider updating the package documentation to reflect this change in implementation. This will help users understand any potential differences in behavior or available properties compared to the previous external package implementation.
Please run the following revised script to verify the local implementation:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the local implementation of useDropzone # Test 1: Check if the local module exists if [ ! -f "./packages/react/src/components/dropzone.ts" ] && [ ! -f "./packages/react/src/components/dropzone.tsx" ]; then echo "Error: Local module './components/dropzone' not found" exit 1 fi # Test 2: Check if useDropzone is exported from the local module rg --type typescript "export.*useDropzone" ./packages/react/src/components/dropzone.ts* || echo "Error: useDropzone not exported from local module" # Test 3: Check for type exports in the local module rg --type typescript "export.*type" ./packages/react/src/components/dropzone.ts* || echo "Warning: No type exports found in local module"Length of output: 577
Script:
#!/bin/bash # Description: Verify the local implementation of useDropzone # Test 1: Check if the local module exists if [ ! -f "./packages/react/src/components/dropzone.ts" ] && [ ! -f "./packages/react/src/components/dropzone.tsx" ]; then echo "Error: Local module './components/dropzone' not found" exit 1 fi # Test 2: Check if useDropzone is exported from the local module rg "export\s+\{?\s*useDropzone\s*\}?" ./packages/react/src/components/dropzone.ts* || echo "Error: useDropzone not exported from local module" # Test 3: Check for type exports in the local module rg "export\s+type\s+" ./packages/react/src/components/dropzone.ts* || echo "Warning: No type exports found in local module"Length of output: 442
packages/solid/src/components/dropzone.tsx (2)
470-491: 🛠️ Refactor suggestion
Provide feedback for rejected files
In the
setFilesfunction, files that are rejected due to type, size, or quantity limits are silently discarded. Providing feedback to the user about rejected files and the reasons can enhance user experience.Consider modifying the function to collect rejected files and inform the user accordingly.
539-544:
⚠️ Potential issueSimplify
onClickhandler and update comment to reflect SolidJSThe
onClickhandler is unnecessarily wrapped in an extra function, and the comment refers to React instead of SolidJS. Simplifying the handler improves readability, and updating the comment ensures consistency with the framework in use.Apply this diff to fix the issue:
-const onClick = () => () => { - // In IE11/Edge the file-browser dialog is blocking, therefore, use setTimeout() - // to ensure React can handle state changes - // See: https://github.com/react-dropzone/react-dropzone/issues/450 - isIeOrEdge() ? setTimeout(openFileDialog, 0) : openFileDialog(); -}; +const onClick = () => { + // In IE11/Edge the file-browser dialog is blocking, therefore, use setTimeout() + // to ensure SolidJS can handle state changes + // See: https://github.com/react-dropzone/react-dropzone/issues/450 + isIeOrEdge() ? setTimeout(openFileDialog, 0) : openFileDialog(); +};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 onClick = () => { // In IE11/Edge the file-browser dialog is blocking, therefore, use setTimeout() // to ensure SolidJS can handle state changes // See: https://github.com/react-dropzone/react-dropzone/issues/450 isIeOrEdge() ? setTimeout(openFileDialog, 0) : openFileDialog(); };packages/vue/src/components/dropzone.tsx (2)
664-676:
⚠️ Potential issueCorrect the usage of
satisfieswithin object literalsThe use of the
satisfiesoperator within object literals that are immediately spread may not correctly enforce the types as intended. Thesatisfiesoperator is used during type checking but does not affect the runtime value. When spreading objects, the type information may not be preserved as you expect.To ensure type safety and proper typing, consider assigning the object to a variable with the appropriate type assertion before spreading it.
Apply this diff to fix the issue in
getRootProps:const getRootProps = (): HTMLAttributes & ReservedProps => ({ ref: rootRef, role: "presentation" as const, ...(!optionsRef.value.disabled ? { - tabindex: 0, - onKeydown, - onFocus, - onBlur, - onClick, - onDragenter, - onDragover, - onDragleave, - onDrop, - } satisfies HTMLAttributes & ReservedProps + ...({ + tabindex: 0, + onKeydown, + onFocus, + onBlur, + onClick, + onDragenter, + onDragover, + onDragleave, + onDrop, + } as HTMLAttributes & ReservedProps), : {}), });And in
getInputProps:const getInputProps = (): InputHTMLAttributes & ReservedProps => ({ ref: inputRef, type: "file", style: "display: none", accept: acceptAttr.value ?? "", // exactOptionalPropertyTypes: true multiple: optionsRef.value.multiple, tabindex: -1, ...(!optionsRef.value.disabled ? { - onChange: onDrop, - onClick: onInputElementClick, - } satisfies InputHTMLAttributes & ReservedProps + ...({ + onChange: onDrop, + onClick: onInputElementClick, + } as InputHTMLAttributes & ReservedProps), : {}), });Also applies to: 686-692
652-653:
⚠️ Potential issueAvoid assignments within arrow function return expressions
The assignments within the arrow functions
onFocusandonBlurcan be confusing, as they are performed within the return expression. Expressions are often considered side-effect free, so this pattern may reduce code readability. To improve clarity, consider using a block body for the arrow functions.Apply this diff to fix the issue:
-const onFocus = () => (state.isFocused = true); -const onBlur = () => (state.isFocused = false); +const onFocus = () => { + state.isFocused = true; +}; +const onBlur = () => { + state.isFocused = false; +};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 onFocus = () => { state.isFocused = true; }; const onBlur = () => { state.isFocused = false; };Tools
Biome
[error] 652-652: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 653-653: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
packages/react/src/components/dropzone.tsx (2)
494-495: 🛠️ Refactor suggestion
Remove unnecessary
event.persist()callsThe
event.persist()method is no longer necessary in modern versions of React (17+), as Synthetic Events are not pooled. These calls can be safely removed to simplify the code.Apply the following diffs to remove the unnecessary
event.persist()calls:// In onDragEnter handler - event.persist(); // In onDragOver handler - event.persist(); // In onDragLeave handler - event.persist(); // In onDropCb handler - event.persist();Also applies to: 532-533, 549-550, 605-606
658-661: 🛠️ Refactor suggestion
Evaluate the necessity of IE11/Edge-specific handling in
onClickThe
onClickhandler usesisIeOrEdge()to handle specific behavior for legacy browsers. Given that Internet Explorer 11 is no longer supported and Edge has transitioned to Chromium, this compatibility code may be unnecessary.Consider removing the IE11/Edge-specific code to simplify the
onClickfunction:const onClick = useCallback(() => { - isIeOrEdge() ? setTimeout(openFileDialog, 0) : openFileDialog(); + openFileDialog(); }, [openFileDialog]);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 onClick = useCallback(() => { openFileDialog(); }, [openFileDialog]);
vercel
bot
temporarily deployed
to
Preview – legacy-docs-uploadthing
vercel
bot
temporarily deployed
to
Preview – legacy-docs-uploadthing
vercel
bot
temporarily deployed
to
Preview – legacy-docs-uploadthing
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/svelte/src/lib/component/UploadDropzone.svelte (3)
105-107: Approved: Improved upload handling and error managementThe changes to the
uploadFilesfunction enhance its functionality:
- Removing the
awaitkeyword allows for non-blocking asynchronous uploads.- The added error handling for
UploadAbortedErrorimproves the robustness of the upload process.These modifications align well with the PR objectives to fix dropzone behavior.
Consider adding a generic error handler to log unexpected errors:
const uploadFiles = (files: File[]) => { const input = "input" in uploader ? uploader.input : undefined; startUpload(files, input).catch((e) => { if (e instanceof UploadAbortedError) { void uploader.onUploadAborted?.(); } else { + console.error('Unexpected error during upload:', e); throw e; } }); };
143-156: Approved: NewonUploadClickfunction improves upload controlThe new
onUploadClickfunction significantly improves the control over the upload process:
- It handles different states (uploading, manual mode with files) appropriately.
- The function includes logic to abort ongoing uploads and start new ones, addressing the issue of multiple file uploads triggering twice.
- The abort functionality aligns well with the PR objectives.
These changes enhance the overall user experience and fix the reported bug.
Consider adding a debounce mechanism to prevent rapid successive clicks:
+ import { debounce } from 'lodash-es'; - const onUploadClick = (e: MouseEvent) => { + const onUploadClick = debounce((e: MouseEvent) => { // ... existing function body ... - }; + }, 300);This will help prevent unintended multiple uploads if the user clicks rapidly.
232-233: Approved: Enhanced visual feedback and state handlingThe changes to the component's markup significantly improve the user experience and align with the PR objectives:
- Updated class bindings for the label provide better visual cues based on the component's state.
- Modified button classes and disabled state handling improve the dropzone behavior.
- The button content now reflects different states (readying, uploading, manual mode), providing clearer feedback to the user during the upload process.
These changes enhance the overall usability and visual consistency of the component.
Consider adding an aria-label to the button to improve accessibility:
<button class={cn( // ... existing classes ... )} style={styleFieldToClassName(appearance?.button, styleFieldArg)} on:click={onUploadClick} data-ut-element="button" data-state={state} type="button" disabled={files.length === 0 || state === "disabled"} + aria-label={state === "uploading" ? "Cancel upload" : "Upload files"} > <!-- ... button content ... --> </button>This will provide better context for screen reader users.
Also applies to: 262-268, 272-272, 275-276, 279-287
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/svelte/src/lib/component/UploadDropzone.svelte (8 hunks)
- packages/svelte/src/lib/component/create-dropzone.ts (10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/svelte/src/lib/component/create-dropzone.ts
🔇 Additional comments (4)
packages/svelte/src/lib/component/UploadDropzone.svelte (4)
31-32: Approved: Simplified component API and updated importThe changes to the import statements and component props appear to streamline the component's external API. The removal of internal state properties (
__internal_state,__internal_upload_progress,__internal_ready) and the use ofrouteConfiginstead ofpermittedFileInfosuggest an improvement in the underlying logic for handling file types and permissions.These changes may impact how the component is used in parent components, so ensure that any existing usage is updated accordingly.
Also applies to: 70-76, 85-89
123-123: Approved: Simplified drop callback and immediate upload in auto modeThe changes to the
onDropCallbackfunction and the subsequent logic improve the component's behavior:
- The function has been simplified, making it easier to understand and maintain.
- In "auto" mode, the upload is now triggered immediately after files are dropped, which enhances the user experience.
These modifications align well with the PR objectives to fix dropzone behavior and improve the overall functionality of the component.
Also applies to: 126-127
158-178: Approved: Improved paste handling and lifecycle managementThe changes to the
onMountlifecycle hook enhance the component's efficiency and resource management:
- The conditional return based on
appendOnPasteprevents unnecessary event listener setup, improving performance.- Encapsulating paste event handling within
onMountensures proper cleanup on component unmount, preventing potential memory leaks.- The paste handling logic aligns with the PR objectives by providing better control over file uploads.
These modifications contribute to a more robust and efficient component.
Line range hint
1-296: Summary: Significant improvements to UploadDropzone componentThis PR introduces several important enhancements to the UploadDropzone component:
- Simplified component API by removing internal state properties.
- Improved upload handling with better error management.
- Enhanced dropzone behavior, particularly in auto and manual modes.
- Added abort functionality for ongoing uploads.
- Optimized paste handling and lifecycle management.
- Improved visual feedback and state handling in the component's markup.
These changes effectively address the reported issues with multiple file uploads and align well with the PR objectives. The component now provides a more robust and user-friendly experience for file uploads.
A few minor suggestions have been made to further improve error handling, prevent rapid successive clicks, and enhance accessibility. Consider implementing these suggestions to further refine the component.
Overall, this is a high-quality update that significantly improves the UploadDropzone component.
| @@ -221,7 +237,7 @@ export function UploadButton< | |||
| return "Loading..."; | |||
| } | |||
| case "uploading": { | |||
| if (uploadProgress === 100) return <Spinner />; | |||
| if (uploadProgress >= 100) return <Spinner />; | |||
There was a problem hiding this comment.
Haha idk. Math is hard. Probably a bug in here somewhere but felt out of scope so this was a quick fix to get the UI to behave. Will look at the progress calculation later
https://github.com/pingdotgg/uploadthing/blob/main/packages/react/src/useUploadThing.ts#L84-L97
https://github.com/pingdotgg/uploadthing/blob/main/packages/uploadthing/src/internal/upload.browser.ts#L157-L168
|
Thanks for the fix! |
closes #965
thought this was fixed in #886
Done
Summary by CodeRabbit
Release Notes
New Features
Cancelcomponent for improved user feedback during upload processes.UploadDropzoneandUploadButtoncomponents with refined state management and event handling for better user experience.Dependency Updates
@uploadthing/dropzonedependency from various packages.file-selectordependency (version0.6.0) to support file selection functionality.Documentation
Bug Fixes
These updates streamline file upload processes and enhance the overall functionality of the application.