feat(file-upload) #3832
feat(file-upload) #3832
Conversation
|
|
@Arian94 is attempting to deploy a commit to the NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request introduces a new file upload component to the NextUI framework, along with comprehensive documentation and testing. It adds several new routes to the documentation site, including a dedicated section for the file upload feature. The implementation includes multiple React components for file upload functionalities, such as handling multiple files, custom buttons, and a top bar. Additionally, the sitemap has been updated to reflect new URLs related to the file upload documentation, and a new README file has been created for the Changes
Assessment against linked issues
Possibly related PRs
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
Documentation and Community
|
|
@wingkwong pls have a look. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Outside diff range and nitpick comments (39)
apps/docs/content/components/file-upload/usage.ts (2)
1-7: Enhance the example implementation for better documentation.The current example is too basic and doesn't demonstrate practical usage. Consider:
- Adding TypeScript types for the files parameter
- Implementing a basic example in the onChange handler
- Including common use cases like file validation
-const App = `import {FileUpload} from "@nextui-org/react"; +const App = `import {FileUpload, FileUploadProps} from "@nextui-org/react"; export default function App() { + const handleFileChange: FileUploadProps['onChange'] = (files) => { + if (files.length > 0) { + console.log('Selected files:', files); + // Example: Check file types + const validFiles = files.filter(file => + file.type.startsWith('image/') || file.type === 'application/pdf' + ); + console.log('Valid files:', validFiles); + } + }; + return ( - <FileUpload onChange={(files) => { /* handle files */}} /> + <FileUpload + accept=".jpg,.png,.pdf" + maxSize={5242880} // 5MB + onChange={handleFileChange} + /> ); }`;
9-15: Consider using TypeScript extension for the example path.Since NextUI supports TypeScript out of the box, consider using
.tsxextension to better align with the TypeScript ecosystem.const react = { - "/App.jsx": App, + "/App.tsx": App, }; -export default { - ...react, -}; +export default react;apps/docs/content/components/file-upload/multiple.ts (2)
1-7: Enhance the example with proper types and error handling.The example could be more comprehensive to better demonstrate the component's capabilities:
- Add TypeScript types for the files parameter
- Implement basic error handling
- Show commonly used props
Consider updating the example:
-const App = `import {FileUpload} from "@nextui-org/react"; +const App = `import {FileUpload, FileUploadProps} from "@nextui-org/react"; export default function App() { + const handleChange: FileUploadProps['onChange'] = (files) => { + try { + console.log('Selected files:', files); + // Process files here + } catch (error) { + console.error('Error handling files:', error); + } + }; + return ( - <FileUpload multiple onChange={(files) => { /* handle files */}} /> + <FileUpload + multiple + onChange={handleChange} + maxFileSize={10 * 1024 * 1024} // 10MB + accept={".jpg,.png,.pdf"} + onError={(error) => console.error(error)} + /> ); }`;
13-15: Consider making the export more explicit.While spreading the react object works, being explicit about what's being exported can improve code readability.
-export default { - ...react, -}; +export default react;apps/docs/content/components/file-upload/topbar.ts (2)
1-7: Enhance the example to be more comprehensive.The current example is quite basic. Consider improving it by:
- Using a more standardized format for
maxAllowedSize(e.g., "1MB" without space)- Adding error handling and validation examples
- Demonstrating more props and customization options
-const App = `import {FileUpload, FileUploadTopbar} from "@nextui-org/react"; +const App = `import {FileUpload, FileUploadTopbar, FileUploadProps} from "@nextui-org/react"; export default function App() { + const handleError = (error: Error) => { + console.log("Upload error:", error); + }; + return ( - <FileUpload topbar={<FileUploadTopbar maxAllowedSize="1 MB" />} /> + <FileUpload + topbar={ + <FileUploadTopbar + maxAllowedSize="1MB" + allowedTypes={[".jpg", ".png"]} + /> + } + onError={handleError} + maxFileSize={1024 * 1024} // 1MB in bytes + /> ); }`;
9-15: Improve the export structure and path format.Consider the following improvements:
- Remove the leading slash from the path
- Use .tsx extension since this is a TypeScript example
- Simplify the export structure
const react = { - "/App.jsx": App, + "App.tsx": App, }; -export default { - ...react, -}; +export default react;apps/docs/content/components/file-upload/index.ts (1)
9-17: Consider adding TypeScript type definitions.While the code is functional, adding TypeScript interfaces would improve maintainability and provide better documentation of the expected shape of each imported module.
+interface FileUploadModule { + App: React.ComponentType; + getStaticProps?: () => Promise<{ props: Record<string, unknown> }>; +} + +export const fileUploadContent: Record<string, FileUploadModule> = { usage, multiple, topbar, fileItem, button, buttons, controlled, };packages/components/file-upload/src/index.ts (1)
6-8: Consider using a more explicit type export syntax.While the current syntax is valid, using the more explicit
export typesyntax can make the type exports more obvious.-export type {FileUploadProps} from "./file-upload"; -export type {FileUploadItemProps} from "./file-upload-item"; -export type {FileUploadTopbarProps} from "./file-upload-topbar"; +export type {FileUploadProps} from "./file-upload" type; +export type {FileUploadItemProps} from "./file-upload-item" type; +export type {FileUploadTopbarProps} from "./file-upload-topbar" type;apps/docs/content/components/file-upload/button.ts (1)
17-23: Consider adding TypeScript types for better maintainability.While the export structure is correct, adding TypeScript types would improve code maintainability and provide better IDE support.
Consider adding types:
+interface CodeExample { + [path: string]: string; +} + -const react = { +const react: CodeExample = { "/App.jsx": App, }; -export default { +export default: CodeExample { ...react, };apps/docs/content/components/file-upload/buttons.ts (1)
9-15: Improve button accessibility and code structure.The buttons section could benefit from the following improvements:
- Add aria-labels for better accessibility
- Use the imported DeleteIcon for the reset button
- Improve code formatting for better readability
buttons={(onBrowse, onAdd, onReset) => { - return <div> - <Button onClick={() => onBrowse()}><SearchIcon /></Button> - <Button onClick={() => onAdd()}>Add New File</Button> - <Button onClick={() => onReset()}>Remove All</Button> - </div> + return ( + <div className="flex gap-2"> + <Button + onClick={onBrowse} + aria-label="Browse files" + > + <SearchIcon /> + </Button> + <Button + onClick={onAdd} + aria-label="Add new file" + > + Add New File + </Button> + <Button + onClick={onReset} + aria-label="Remove all files" + > + <DeleteIcon /> + Remove All + </Button> + </div> + ); }}packages/components/file-upload/src/file-upload-item.tsx (1)
5-9: Add documentation and improve type safety.Consider these enhancements for better maintainability and type safety:
- Add JSDoc documentation for the interface and its props
- Consider using a more specific type for accepted files
- Make the callback more type-safe by using the File type
+/** + * Props for the FileUploadItem component + */ export interface FileUploadItemProps extends HTMLNextUIProps<"div"> { + /** The file object to display */ file: File; - onFileRemove: (name: string) => void; + /** Callback when file is removed */ + onFileRemove: (file: File) => void; + /** Whether the remove button is disabled */ isDisabled?: boolean; }packages/core/theme/src/components/index.ts (1)
41-41: Consider maintaining alphabetical ordering of exports.While the export statement for the new file-upload component is correctly formatted, it would be better to maintain alphabetical ordering for consistency and easier maintenance. Consider moving it between "divider" and "image" exports.
export * from "./date-picker"; -export * from "./file-upload"; export * from "./alert"; export * from "./drawer"; export * from "./divider"; +export * from "./file-upload"; export * from "./image";packages/components/file-upload/package.json (1)
5-7: Enhance package discoverability with additional keywordsConsider adding more relevant keywords to improve package discoverability, such as: "nextui", "react", "upload", "component", "drag-drop", "file-input".
"keywords": [ - "file-upload" + "file-upload", + "nextui", + "react", + "upload", + "component", + "drag-drop", + "file-input" ],packages/core/theme/src/components/file-upload.ts (2)
6-23: Fix copy-paste error in JSDoc commentThe JSDoc comment refers to "Card" component instead of "FileUpload".
-* Card **Tailwind Variants** component +* FileUpload **Tailwind Variants** component
24-70: Consider enhancing the theme configurationWhile the basic styling is good, consider these improvements for better user experience:
- Add hover/focus states for interactive elements
- Include responsive design classes
- Consider dark mode variants
Example enhancement for the base slot:
base: [ "flex", "flex-col", "relative", "overflow-hidden", "h-auto", "outline-none", "text-foreground", "box-border", "bg-content1", + "transition-opacity", + "dark:bg-content1-dark", + "sm:min-h-[200px]", + "md:min-h-[300px]", ...dataFocusVisibleClasses, ],packages/components/file-upload/__tests__/file-upload.test.tsx (4)
8-18: Consider adding error case test coverage.While the test setup is good, consider adding test cases for error scenarios such as:
- File size limits
- Invalid file types
- Upload failures
Would you like me to provide examples of these test cases?
25-30: Strengthen ref testing.Consider enhancing ref testing by:
- Verifying ref properties
- Testing ref methods if any
- Checking ref updates
32-44: Improve disabled state test assertions.The test could be more explicit about click handler expectations:
it("should not browse when isDisabled", async () => { + const onBrowse = jest.fn(); - let renderResult = render(<FileUpload isDisabled />); + let renderResult = render(<FileUpload isDisabled onBrowse={onBrowse} />); let browseButton = renderResult.container.querySelector("button"); expect(browseButton).toBeDisabled(); renderResult = render(<FileUpload isDisabled browseButton={<Button>Browse files</Button>} />); browseButton = renderResult.container.querySelector("button"); browseButton && (await user.click(browseButton)); expect(browseButton).toBeDisabled(); + expect(onBrowse).not.toHaveBeenCalled(); });
46-65: Enhance button disabled state test.Consider these improvements:
- Use a more realistic file mock
- Add assertions for button labels
- Test interaction attempts
files={[new File([], "file1", {type: "jpg"})]} +files={[ + new File(['test content'], 'test.jpg', { + type: 'image/jpeg', + }), +]}packages/components/file-upload/src/file-upload-topbar.tsx (2)
4-4: Consider enhancing the FileSize type for better validation and flexibility.The current FileSize type could be improved to:
- Support more size units (bytes, GB)
- Prevent invalid sizes (negative or zero)
Consider this implementation:
type SizeUnit = 'B' | 'KB' | 'MB' | 'GB'; type FileSize = `${number} ${SizeUnit}`; // With validation helper const isValidFileSize = (size: FileSize): boolean => { const [value, unit] = size.split(' '); return Number(value) > 0 && ['B', 'KB', 'MB', 'GB'].includes(unit); };
6-47: Consider enhancing type safety and documentation.
- The maxItems prop should prevent negative values
- The inherited props from HTMLNextUIProps should be documented
Consider these improvements:
export interface FileUploadTopbarProps extends HTMLNextUIProps<"div"> { /** * Max number of items. * @default 1 * @minimum 1 */ maxItems?: number & { __brand: 'PositiveNumber' }; // ... other props ... /** * @inheritdoc * Additional HTML attributes and NextUI specific props */ } // Helper type guard function isPositiveNumber(n: number): n is number & { __brand: 'PositiveNumber' } { return n > 0; }packages/components/file-upload/stories/file-upload.stories.tsx (3)
8-23: Consider expanding argTypes configurationThe story configuration could be enhanced by adding controls for other important props of the FileUpload component, such as:
acceptfor file type restrictionsmaxFileSizefor size limitsonFileSelectcallbackargTypes: { multiple: { control: { type: "boolean", }, }, isDisabled: { control: { type: "boolean", }, }, + accept: { + control: "text", + description: "Accepted file types", + defaultValue: "*", + }, + maxFileSize: { + control: "number", + description: "Maximum file size in bytes", + }, },
55-70: Enhance ButtonsContainer template stylingThe buttons in ButtonsContainerTemplate lack proper spacing and styling, which might not represent the best practices for button layouts.
return ( <div> - <Button onClick={onBrowse}>Browse Files</Button> - <Button onClick={onReset}>Remove All</Button> - <Button onClick={onAdd}>Add New File</Button> + <div className="flex gap-2"> + <Button onClick={onBrowse}>Browse Files</Button> + <Button onClick={onReset}>Remove All</Button> + <Button onClick={onAdd}>Add New File</Button> + </div> </div> );
72-108: Consider adding error state storyThe stories cover various use cases but lack demonstration of error states (e.g., file size exceeded, invalid file type).
export const WithError = { render: Template, args: { ...defaultProps, maxFileSize: 1000, // 1KB to easily trigger size error onError: (error) => console.log('Error:', error), }, };apps/docs/content/docs/components/file-upload.mdx (2)
69-70: Improve sentence clarity in custom buttons descriptionThe sentence is split across lines and reads awkwardly. Consider combining and rephrasing for better readability.
-In case you need to upload files prior to submitting the form, you can implement a custom button. In addition, there are placeholders for -other buttons such as the Add Button to place your custom button (component) instead. +In case you need to upload files before form submission, you can implement a custom button. Additionally, there are placeholders for custom button components, such as the Add Button.🧰 Tools
🪛 LanguageTool
[style] ~69-~69: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...ttons In case you need to upload files prior to submitting the form, you can implement ...(EN_WORDINESS_PREMIUM_PRIOR_TO)
157-159: Enhance FileSize type definitionConsider improving the FileSize type to:
- Support additional units (B, GB, TB)
- Add validation for number values
- Include a helper function for conversion
-export type FileSize = `${number} KB` | `${number} MB`; +export type FileSizeUnit = 'B' | 'KB' | 'MB' | 'GB' | 'TB'; +export type FileSize = `${number} ${FileSizeUnit}`; + +// Helper function for conversion +export function convertFileSize(size: number, fromUnit: FileSizeUnit, toUnit: FileSizeUnit): FileSize;apps/docs/config/routes.json (1)
257-263: Consider expanding keywords for better discoverability.While the route configuration is correctly structured, consider enhancing the keywords to improve searchability. Include related terms that users might search for.
- "keywords": "file, upload, browse", + "keywords": "file upload, file input, drag and drop, file browser, attachment, document upload, multiple files",packages/components/file-upload/src/use-file-upload.ts (3)
10-65: Standardize JSDoc comments in thePropsinterfaceTo enhance readability and maintain consistent documentation, ensure that all JSDoc comments within the
Propsinterface follow the same style and punctuation. For instance, some comments start with uppercase letters while others don't, and some end with periods while others don't.
43-47: IncludeonUploadin thebuttonscallback parametersConsidering that there is an
uploadButtonprop, it would be beneficial to include anonUploadcallback in thebuttonsfunction parameters. This allows developers to customize the upload action within the custom buttons rendering function.
69-96: Add unit tests for theuseFileUploadhookImplementing unit tests for the
useFileUploadhook will help ensure its reliability and catch potential issues early. Tests can cover various scenarios, such as default behaviors, handling of props, and memoization logic.Would you like assistance in creating unit tests for this hook or should I open a new GitHub issue to track this task?
packages/components/file-upload/src/file-upload.tsx (9)
35-39: Ensure Dependency Array Completeness inuseEffectWhile the
useEffecthook updates thefilesstate wheninitialFileschanges, consider addingsetFilesto the dependency array to prevent potential stale closures.
47-48: Clarify the Purpose of Input ResettingThe comments explaining why input values are reset to
""can be clearer to enhance maintainability.Consider updating the comments:
- // Setting input values to "" in order to ignore previously-selected file(s). - // This will fix some bugs when "removing" and re-adding "the exact same" file(s) (e.g. removing foo.txt and adding foo.txt again). + // Reset input values to allow re-selection of the same files. + // Prevents issues when a user removes and re-adds the same file(s).
215-219: Simplify File Array Creation Using Modern JavaScriptThe loop used to construct
newFilesfromev.target.filescan be simplified for better readability.Apply this diff:
- const newFiles: File[] = []; - for (let index = 0; index < ev.target.files.length; index++) { - const file = ev.target.files.item(index); - file && newFiles.push(file); - } + const newFiles = Array.from(ev.target.files);
116-119: Provide Default Text for Browse ButtonEnsure that a default text is displayed when
browseButtonTextis not provided to enhance user experience.Apply this diff:
<Button disabled={props.isDisabled} onClick={() => onBrowse()}> - {browseButtonText} + {browseButtonText || "Browse Files"} </Button>
91-94: Consistent Handling of Disabled State in Event HandlersIn
onBrowse, the function checksprops.isDisabledbefore proceeding. Ensure that all event handlers consistently handle the disabled state to prevent unintended interactions when the component is disabled.
205-222: Consider Accept Attribute for File InputTo enhance user experience and file validation, consider adding an
acceptattribute to the file input to specify the types of files that can be selected.Example:
type="file" + accept={props.accept}
146-151: Event Ordering in Button Click HandlersIn the cloned button elements, the custom
onClickhandlers are called after the internal handlers. Consider whether the order should be reversed to allow custom handlers to prevent default behavior if necessary.Apply this diff:
resetButton.props.onClick?.(ev); onReset();
253-255: Export Component with Named Export for ConsistencyWhile the default export is acceptable, consider using a named export to maintain consistency and facilitate tree-shaking in bundlers.
Apply this diff:
-export default FileUpload; +export { FileUpload };
39-40: Prevent Unnecessary State UpdatesIn the
useEffect, add a check to prevent updating thefilesstate ifinitialFilesis the same as the currentfilesto avoid unnecessary re-renders.Apply this diff:
initialFiles && setFiles(initialFiles); + }, [initialFiles, setFiles]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (25)
apps/docs/config/routes.json(1 hunks)apps/docs/content/components/file-upload/button.ts(1 hunks)apps/docs/content/components/file-upload/buttons.ts(1 hunks)apps/docs/content/components/file-upload/controlled.ts(1 hunks)apps/docs/content/components/file-upload/file-item.ts(1 hunks)apps/docs/content/components/file-upload/index.ts(1 hunks)apps/docs/content/components/file-upload/multiple.ts(1 hunks)apps/docs/content/components/file-upload/topbar.ts(1 hunks)apps/docs/content/components/file-upload/usage.ts(1 hunks)apps/docs/content/docs/components/file-upload.mdx(1 hunks)apps/docs/public/sitemap-0.xml(1 hunks)packages/components/file-upload/README.md(1 hunks)packages/components/file-upload/__tests__/file-upload.test.tsx(1 hunks)packages/components/file-upload/package.json(1 hunks)packages/components/file-upload/src/file-upload-item.tsx(1 hunks)packages/components/file-upload/src/file-upload-topbar.tsx(1 hunks)packages/components/file-upload/src/file-upload.tsx(1 hunks)packages/components/file-upload/src/index.ts(1 hunks)packages/components/file-upload/src/use-file-upload.ts(1 hunks)packages/components/file-upload/stories/file-upload.stories.tsx(1 hunks)packages/components/file-upload/tsconfig.json(1 hunks)packages/components/file-upload/tsup.config.ts(1 hunks)packages/core/react/package.json(1 hunks)packages/core/theme/src/components/file-upload.ts(1 hunks)packages/core/theme/src/components/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- apps/docs/content/components/file-upload/file-item.ts
- packages/components/file-upload/tsconfig.json
- packages/components/file-upload/tsup.config.ts
🧰 Additional context used
🪛 LanguageTool
apps/docs/content/docs/components/file-upload.mdx
[style] ~69-~69: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...ttons In case you need to upload files prior to submitting the form, you can implement ...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
[typographical] ~109-~109: Consider adding a comma after ‘Usually’ for more clarity.
Context: ... | The contents of the collection. Usually the array of FileUploadItem ...
(RB_LY_COMMA)
[style] ~113-~113: This phrase is redundant. Consider using “outside”.
Context: ...l values or files to be controlled from outside of the FileUpload. ...
(OUTSIDE_OF)
[style] ~120-~120: In American English, abbreviations like “etc.” require a period.
Context: ...| Custom buttons for browsing, adding, .etc or any other ones that are needed for t...
(ETC_PERIOD)
packages/components/file-upload/README.md
[typographical] ~17-~17: Usually, a comma is necessary before ‘Yes’ at the end of a sentence. Did you mean “, please”?
Context: ...rg/file-upload ``` ## Contribution Yes please! See the [contributing guidelines](http...
(COMMA_BEFORE_PLEASE)
🔇 Additional comments (25)
apps/docs/content/components/file-upload/multiple.ts (1)
9-11: LGTM!
The React code mapping follows the expected pattern for documentation examples.
apps/docs/content/components/file-upload/index.ts (1)
1-7: LGTM! Clean and well-organized imports.
The imports follow a consistent pattern and are logically grouped based on their functionality (basic usage, variants, and sub-components).
packages/components/file-upload/src/index.ts (1)
1-14: LGTM! Well-organized barrel file structure.
The file follows a clean and organized structure with clear separation between different types of exports (components, types, and hooks).
Let's verify that all imported files exist:
✅ Verification successful
All imported files exist and match the exports
The verification confirms that all required files exist in the correct directory:
file-upload.tsxfile-upload-item.tsxfile-upload-topbar.tsxuse-file-upload.ts
Note that while the imports reference .ts files, finding .tsx files is perfectly valid as TypeScript allows importing .tsx files without the extension.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of all imported files
# Expected: All files should exist in the same directory
# Get the directory of the current file
cd "$(dirname "packages/components/file-upload/src/index.ts")"
# Check for the existence of each imported file
for file in "file-upload.ts" "file-upload-item.ts" "file-upload-topbar.ts" "use-file-upload.ts"; do
fd -t f "^${file}(x|)"
done
Length of output: 711
apps/docs/content/components/file-upload/buttons.ts (2)
20-26: LGTM! Export configuration is well-structured.
The export configuration follows the documentation pattern correctly, making the example available at the specified path.
1-26: Verify documentation integration.
Let's ensure this example is properly referenced in the documentation system and follows the established pattern.
✅ Verification successful
Let me verify the documentation integration by checking the content of the index file and the buttons example.
Documentation integration is properly set up
The example is correctly integrated into the documentation system:
- The
buttons.tsexample is properly exported in the index file - The file structure follows the established pattern with other examples
- The example code is properly formatted and follows the documentation standards
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the documentation integration
# Test 1: Check if this example is referenced in the main documentation file
echo "Checking main documentation file..."
rg -l "FileUpload.*buttons" apps/docs/content/components/file-upload/
# Test 2: Verify the file structure matches other example patterns
echo "Verifying file structure..."
fd -e ts -e tsx . apps/docs/content/components/ -x echo "Found example file: {}"
Length of output: 44737
Script:
#!/bin/bash
# Check if the buttons example is properly referenced in the index file
echo "Checking index file for buttons reference..."
cat apps/docs/content/components/file-upload/index.ts
# Check the content of the buttons example
echo -e "\nChecking buttons example content..."
cat apps/docs/content/components/file-upload/buttons.ts
Length of output: 1227
apps/docs/content/components/file-upload/controlled.ts (1)
25-31: Verify the export pattern consistency.
The export configuration looks correct, but let's verify if this pattern is consistent with other example files in the docs.
✅ Verification successful
Export pattern is consistent across example files
The search results show that all example files in the docs follow the same pattern of exporting an object that spreads the react variable. This is a consistent pattern used throughout the codebase for example files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if other example files follow the same export pattern
# Test: Search for similar export patterns in other example files
rg -g '*.ts' -g '*.tsx' "const react = {" "apps/docs/content/components/"
Length of output: 167
Script:
#!/bin/bash
# Let's fix the regex escape issue and search for export patterns
rg -g '*.ts' -g '*.tsx' 'const react \=' "apps/docs/content/components/"
# Also search for similar export configurations
rg -g '*.ts' -g '*.tsx' 'export default \{' -A 2 "apps/docs/content/components/"
# Let's also check if there are other export patterns
rg -g '*.ts' -g '*.tsx' '^export' "apps/docs/content/components/"
Length of output: 211229
packages/components/file-upload/src/file-upload-item.tsx (3)
1-3: LGTM! Imports are appropriate and well-organized.
The imports correctly utilize NextUI's internal components and types.
29-31: LGTM! Component metadata and export are well-defined.
The displayName is properly set for debugging purposes, and the export is appropriate.
1-31: Verify integration with parent FileUpload component.
Let's ensure this component integrates properly with its parent.
✅ Verification successful
Let me gather more information about the parent component's implementation to ensure complete verification.
Integration with parent FileUpload component is properly implemented
The FileUploadItem component is correctly integrated with its parent FileUpload component:
- The parent correctly passes all required props (file, onFileRemove, isDisabled)
- The onFileRemove handler in FileUpload properly manages file state and triggers the onChange callback
- The disabled state is properly propagated from parent to child
- The component is used exclusively within the FileUpload component as intended
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify FileUploadItem usage and integration
# Test 1: Check how FileUploadItem is used in parent components
echo "Checking FileUploadItem usage patterns..."
ast-grep --pattern 'import { FileUploadItem } from "$_"'
# Test 2: Verify consistent event handling
echo "Checking onFileRemove implementation in parent components..."
rg -A 5 'onFileRemove'
Length of output: 4135
Script:
#!/bin/bash
# Check the FileUpload component implementation
echo "Checking FileUpload component implementation..."
cat packages/components/file-upload/src/file-upload.tsx
# Check if FileUploadItem is used anywhere else
echo "Checking other FileUploadItem usages..."
rg -l "FileUploadItem"
Length of output: 7211
packages/components/file-upload/package.json (3)
36-54: Dependencies are well-structured
The dependency structure follows best practices:
- Appropriate peer dependencies for React and NextUI core packages
- Correct usage of workspace references for internal dependencies
- Development dependencies are properly separated
27-35: Build and development scripts are comprehensive
The scripts section includes all necessary commands for development, building, and type checking, following NextUI's standard patterns.
2-4: Consider starting with version 1.0.0
Starting with version 2.0.0 for a new package is unusual unless this is meant to align with NextUI's major version. If this isn't intentional, consider starting with 1.0.0 to follow semantic versioning conventions.
✅ Verification successful
Version 2.0.0 aligns with NextUI's major version
Based on the version information from other NextUI packages, starting with version 2.0.0 is correct as it aligns with the major version (v2.x.x) used across all NextUI components in the monorepo. All packages in the components directory follow the v2 major version, ranging from 2.0.x to 2.2.x.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the major version of other NextUI packages
echo "Checking major versions of other NextUI packages:"
fd -e json -x cat {} \; -p "packages/components" | grep "\"version\":" | sort | uniq
Length of output: 762
packages/core/theme/src/components/file-upload.ts (3)
1-4: LGTM: Imports are appropriate for theme configuration
The necessary dependencies for theme configuration are properly imported.
72-76: LGTM: Type exports are well-defined
The types are properly derived from the configuration and provide good type safety for consumers.
1-76: Verify theme integration with FileUpload components
Let's ensure these theme configurations are properly utilized by the FileUpload components.
✅ Verification successful
Theme configurations are properly integrated with FileUpload component
The verification shows that the theme slots defined in file-upload.ts are correctly used throughout the FileUpload component implementation:
- The
baseslot is applied to the root component with proper class merging - The
topbar,items,item, andbuttonsslots are all utilized in their respective component sections - The theme variants (like
isDisabled) are properly handled
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of fileUpload theme configurations
# Test: Search for imports of fileUpload theme
echo "Checking fileUpload theme imports:"
rg "from.*file-upload\"" --type ts
# Test: Search for usage of theme slots
echo -e "\nChecking usage of theme slots:"
rg "className=\{.*\.(base|topbar|items|item|buttons)\(" --type tsx
Length of output: 760
Script:
#!/bin/bash
# Let's try a different approach to find the component usage
# Search for FileUpload component files
echo "Searching for FileUpload component files:"
fd "file-upload" --type f
# Search for theme usage in these files with proper file extension
echo -e "\nChecking theme usage in component files:"
rg "className=\{.*\.(base|topbar|items|item|buttons)\(" --type ts
# Look for the actual component implementation
echo -e "\nChecking component implementation:"
ast-grep --pattern 'const FileUpload = $$$'
Length of output: 21618
packages/components/file-upload/__tests__/file-upload.test.tsx (1)
1-7: LGTM! Imports are well-organized and appropriate.
The imports cover all necessary testing utilities and components.
packages/components/file-upload/src/file-upload-topbar.tsx (1)
85-87: LGTM! Proper component naming and export.
The component follows React best practices by setting a descriptive displayName and using default export.
packages/components/file-upload/stories/file-upload.stories.tsx (3)
1-7: Well-structured imports with proper type definitions
The imports are well-organized, properly typed, and follow NextUI's conventions for theming integration.
25-28: LGTM! Good use of theme variants
The default props are well-defined and properly integrate with NextUI's theming system.
30-53: Well-structured templates demonstrating component flexibility
The templates effectively showcase various customization options of the FileUpload component, making it easy for users to understand the component's capabilities.
apps/docs/config/routes.json (1)
Line range hint 1-624: LGTM! Route structure maintains consistency.
The changes maintain proper alphabetical ordering and follow the established route configuration pattern. The JSON structure remains valid and well-organized.
packages/components/file-upload/src/use-file-upload.ts (1)
78-85: Verify useMemo dependencies for completeness
Ensure that all dependencies used within the useMemo hook are included in the dependency array. Specifically, confirm whether the fileUpload function is stable. If fileUpload can change between renders, it should be included in the dependencies to prevent stale styles.
If needed, consider updating the dependency array:
useMemo(
() =>
fileUpload({
...variantProps,
className,
}),
- [objectToDeps(variantProps), className],
+ [fileUpload, objectToDeps(variantProps), className],
);packages/components/file-upload/src/file-upload.tsx (3)
125-140: Handling of Optional Callback Props
When cloning elements like addButton, ensure that their original onClick handlers are preserved and called appropriately.
23-25: Type Definitions for Button Props
Ensure that the types for browseButton, addButton, resetButton, and uploadButton props are correctly defined in the FileUploadProps interface to provide better TypeScript support.
169-176:
Include uploadButtonElement in Rendered Output
The uploadButtonElement is defined but not rendered. Verify if it should be included in the buttons section.
Apply this diff to include it:
{browseButtonElement}
+ {uploadButtonElement}Likely invalid or redundant comment.
| const App = ` | ||
| import {FileUpload} from "@nextui-org/react"; | ||
| import {SearchIcon, DeleteIcon} from "@nextui-org/shared-icons"; | ||
|
|
There was a problem hiding this comment.
Add missing Button import and consider code structure improvements.
The code is missing the Button import while using it in the component. Additionally, since this is documentation code, consider using a more maintainable structure.
Apply this diff to fix the issues:
const App = `
import {FileUpload} from "@nextui-org/react";
+import {Button} from "@nextui-org/react";
import {SearchIcon, DeleteIcon} from "@nextui-org/shared-icons";📝 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 App = ` | |
| import {FileUpload} from "@nextui-org/react"; | |
| import {SearchIcon, DeleteIcon} from "@nextui-org/shared-icons"; | |
| const App = ` | |
| import {FileUpload} from "@nextui-org/react"; | |
| import {Button} from "@nextui-org/react"; | |
| import {SearchIcon, DeleteIcon} from "@nextui-org/shared-icons"; |
| <FileUpload | ||
| multiple | ||
| browseButton={<Button><SearchIcon /></Button>} | ||
| uploadButton={<Button onClick={() => alert("Files to upload are:\n" + files.map(file => file.name).join("\n"));}>Upload Files</Button>} | ||
| addButton={<Button>+</Button>} | ||
| resetButton={<Button><DeleteIcon /></Button>} | ||
| /> |
There was a problem hiding this comment.
Fix undefined files variable and improve user feedback.
The upload button's onClick handler references an undefined files variable, which will cause a runtime error.
Apply this diff to fix the issue:
<FileUpload
multiple
browseButton={<Button><SearchIcon /></Button>}
- uploadButton={<Button onClick={() => alert("Files to upload are:\n" + files.map(file => file.name).join("\n"));}>Upload Files</Button>}
+ uploadButton={
+ <Button
+ onClick={(files) => {
+ if (files?.length) {
+ alert("Files to upload are:\n" + files.map(file => file.name).join("\n"));
+ }
+ }}
+ >
+ Upload Files
+ </Button>
+ }
addButton={<Button>+</Button>}
resetButton={<Button><DeleteIcon /></Button>}
/>Committable suggestion skipped: line range outside the PR's diff.
🛠️ Refactor suggestion
Consider enhancing the example with proper file handling.
The current example lacks important features that should be demonstrated in documentation:
- File type validation
- File size limits
- Proper error handling
- User-friendly feedback instead of alerts
Consider enhancing the example:
<FileUpload
multiple
+ accept=".jpg,.png,.pdf"
+ maxFileSize={5 * 1024 * 1024} // 5MB
browseButton={<Button><SearchIcon /></Button>}
uploadButton={
<Button
onClick={(files) => {
try {
// Show proper upload feedback UI instead of alert
console.log("Uploading files:", files);
} catch (error) {
console.error("Upload failed:", error);
}
}}
>
Upload Files
</Button>
}
+ onError={(error) => console.error("Error:", error)}
addButton={<Button>+</Button>}
resetButton={<Button><DeleteIcon /></Button>}
/>Committable suggestion skipped: line range outside the PR's diff.
| const App = ` | ||
| import {FileUpload} from "@nextui-org/react"; | ||
| import {SearchIcon, DeleteIcon} from "@nextui-org/shared-icons"; | ||
|
|
There was a problem hiding this comment.
Add missing Button import statement.
The code uses the Button component but doesn't import it. Add the import to ensure the example is complete:
const App = `
import {FileUpload} from "@nextui-org/react";
+import {Button} from "@nextui-org/react";
import {SearchIcon, DeleteIcon} from "@nextui-org/shared-icons";📝 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 App = ` | |
| import {FileUpload} from "@nextui-org/react"; | |
| import {SearchIcon, DeleteIcon} from "@nextui-org/shared-icons"; | |
| const App = ` | |
| import {FileUpload} from "@nextui-org/react"; | |
| import {Button} from "@nextui-org/react"; | |
| import {SearchIcon, DeleteIcon} from "@nextui-org/shared-icons"; |
| export default function App() { | ||
| const [files, setFiles] = useState<File[]>(); | ||
|
|
||
| useEffect(() => { | ||
| filesFromApi().then(files => setFiles(files)); | ||
| }, []); | ||
|
|
||
| return ( | ||
| <FileUpload files={files} /> | ||
| ); | ||
| }`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and loading states.
The component should handle loading and error states for better user experience.
Apply this diff to improve the implementation:
export default function App() {
- const [files, setFiles] = useState<File[]>();
+ const [files, setFiles] = useState<File[]>([]);
+ const [isLoading, setIsLoading] = useState(false);
+ const [error, setError] = useState<Error | null>(null);
useEffect(() => {
- filesFromApi().then(files => setFiles(files));
+ const fetchFiles = async () => {
+ setIsLoading(true);
+ try {
+ const result = await filesFromApi();
+ setFiles(result);
+ } catch (err) {
+ setError(err instanceof Error ? err : new Error('Failed to fetch files'));
+ } finally {
+ setIsLoading(false);
+ }
+ };
+
+ fetchFiles();
}, []);
return (
- <FileUpload files={files} />
+ <>
+ {error && <div>Error: {error.message}</div>}
+ <FileUpload
+ files={files}
+ isDisabled={isLoading}
+ />
+ </>
);📝 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.
| export default function App() { | |
| const [files, setFiles] = useState<File[]>(); | |
| useEffect(() => { | |
| filesFromApi().then(files => setFiles(files)); | |
| }, []); | |
| return ( | |
| <FileUpload files={files} /> | |
| ); | |
| }`; | |
| export default function App() { | |
| const [files, setFiles] = useState<File[]>([]); | |
| const [isLoading, setIsLoading] = useState(false); | |
| const [error, setError] = useState<Error | null>(null); | |
| useEffect(() => { | |
| const fetchFiles = async () => { | |
| setIsLoading(true); | |
| try { | |
| const result = await filesFromApi(); | |
| setFiles(result); | |
| } catch (err) { | |
| setError(err instanceof Error ? err : new Error('Failed to fetch files')); | |
| } finally { | |
| setIsLoading(false); | |
| } | |
| }; | |
| fetchFiles(); | |
| }, []); | |
| return ( | |
| <> | |
| {error && <div>Error: {error.message}</div>} | |
| <FileUpload | |
| files={files} | |
| isDisabled={isLoading} | |
| /> | |
| </> | |
| ); | |
| }`; |
| ## API | ||
|
|
||
| ### FileUpload Props | ||
|
|
||
| | Attribute | Type | Description | Default | | ||
| | ------------------------- | ----------------------------------------------- | ------------------------------------------------------------------------------------------------------- | -------- | | ||
| | children | `ReactNode` \| `ReactNode[]` | The contents of the collection. Usually the array of `FileUploadItem` | | | ||
| | multiple | `boolean` | Whether multiple files can be selected from the device. | false | | ||
| | isDisabled | `boolean` | Whether the FileUpload items and buttons are disabled. | false | | ||
| | classNames | `Record<"base"｜ "items"｜ "buttons, string>` | Allows to set custom class names for the FileUpload slots. | - | | ||
| | files | `File[]` | Files as initial values or files to be controlled from outside of the FileUpload. | - | | ||
| | browseButton | `ReactHTMLElement<HTMLButtonElement>` | Custom button for browsing files. | - | | ||
| | browseButtonText | `Record<"base"｜ "items"｜ "buttons, string>` | Custom text for the default Browse Button. | - | | ||
| | addButton | `ReactHTMLElement<HTMLButtonElement>` | Custom button for adding a single file. | - | | ||
| | resetButton | `ReactHTMLElement<HTMLButtonElement>` | Custom button for reseting files to an empty array. | - | | ||
| | uploadButton | `ReactHTMLElement<HTMLButtonElement>` | Custom button for uploading files to a server. | - | | ||
| | topbar | `Record<"base"｜ "items"｜ "buttons, string>` | Custom topbar to show information regarding files. | - | | ||
| | buttons | `Record<"base"｜ "items"｜ "buttons, string>` | Custom buttons for browsing, adding, .etc or any other ones that are needed for the user. | - | | ||
| | fileItemElement | `Record<"base"｜ "items"｜ "buttons, string>` | Custom element for representing an item that is selected from the device. | - | | ||
|
|
||
| ### FileUpload Events | ||
|
|
||
| | Attribute | Type | Description | | ||
| | ------------- | ------------------------------ | --------------------------------------------------------------------------------------------------------------- | | ||
| | onChange | `(files: File[]) => void` | Handler called when file(s) are selected from device or removed from the list (`onFileRemove` on an item should be called). | | ||
|
|
||
| ### FileUpload Item Props | ||
|
|
||
| | Attribute | Type | Description | Default | | ||
| |---------------------------|---------------------------------------------------|------------------------------------------------------------------------------------------------------------------------------|---------| | ||
| | file | `File` | A file that is selected from the device. | | | ||
|
|
||
| ### FileUpload Item Events | ||
|
|
||
| | Attribute | Type | Description | | ||
| | ------------- | ------------------------------ | --------------------------------------------------------------------------------------------------------------- | | ||
| | onFileRemove | `(name: string) => void` | Handler called when a file is removed from the list of items. | | ||
|
|
||
| ### FileUpload Topbar Props | ||
|
|
||
| | Attribute | Type | Description | Default | | ||
| |---------------------------|---------------------------------------------------|------------------------------------------------------------------------------------------------------------------------------|---------| | ||
| | maxItems | `number` | Max number of items. | 1 | | ||
| | maxItemsText | `string` | Max number of items text. | | | ||
| | maxItemsElement | `ReactElement` | Custom Element to show Max number of items. | | | ||
| | maxAllowedSize | `FileSize` | Max file size allowed. | | | ||
| | maxAllowedSizeText | `string` | Max file size text. | "Max File Size" | | ||
| | maxAllowedSizeElement | `ReactElement` | Custom Element to show Max file size. | | | ||
| | totalMaxAllowedSize | `FileSize` | Total max size allowed for multiple files combined. | | | ||
| | totalMaxAllowedSizeText | `string` | Total max file size text. | "Total Max Files Size" | | ||
| | totalMaxAllowedSizeElement | `ReactElement` | Custom Element to show Total Max file size. | | | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add essential documentation sections
The documentation would benefit from additional sections covering:
-
Error Handling:
- How to handle file validation errors
- Error states and error messages
- Example of error handling implementation
-
Accessibility:
- ARIA attributes
- Keyboard navigation
- Screen reader support
-
Browser Compatibility:
- Supported browsers
- Known limitations
- Fallback behavior
Would you like me to help generate content for these sections?
🧰 Tools
🪛 LanguageTool
[typographical] ~109-~109: Consider adding a comma after ‘Usually’ for more clarity.
Context: ... | The contents of the collection. Usually the array of FileUploadItem ...
(RB_LY_COMMA)
[style] ~113-~113: This phrase is redundant. Consider using “outside”.
Context: ...l values or files to be controlled from outside of the FileUpload. ...
(OUTSIDE_OF)
[style] ~120-~120: In American English, abbreviations like “etc.” require a period.
Context: ...| Custom buttons for browsing, adding, .etc or any other ones that are needed for t...
(ETC_PERIOD)
|
|
||
| import {fileUploadContent} from "@/content/components/file-upload"; | ||
|
|
||
| # Field Upload |
There was a problem hiding this comment.
Fix typo in the title: "Field Upload" should be "File Upload"
The title contains a typo that should be corrected to maintain consistency with the component name used throughout the documentation.
-# Field Upload
+# File Upload📝 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.
| # Field Upload | |
| # File Upload |
| | classNames | `Record<"base"｜ "items"｜ "buttons, string>` | Allows to set custom class names for the FileUpload slots. | - | | ||
| | files | `File[]` | Files as initial values or files to be controlled from outside of the FileUpload. | - | | ||
| | browseButton | `ReactHTMLElement<HTMLButtonElement>` | Custom button for browsing files. | - | | ||
| | browseButtonText | `Record<"base"｜ "items"｜ "buttons, string>` | Custom text for the default Browse Button. | - | | ||
| | addButton | `ReactHTMLElement<HTMLButtonElement>` | Custom button for adding a single file. | - | | ||
| | resetButton | `ReactHTMLElement<HTMLButtonElement>` | Custom button for reseting files to an empty array. | - | | ||
| | uploadButton | `ReactHTMLElement<HTMLButtonElement>` | Custom button for uploading files to a server. | - | | ||
| | topbar | `Record<"base"｜ "items"｜ "buttons, string>` | Custom topbar to show information regarding files. | - | | ||
| | buttons | `Record<"base"｜ "items"｜ "buttons, string>` | Custom buttons for browsing, adding, .etc or any other ones that are needed for the user. | - | | ||
| | fileItemElement | `Record<"base"｜ "items"｜ "buttons, string>` | Custom element for representing an item that is selected from the device. | - | |
There was a problem hiding this comment.
Fix inconsistent and incorrect type definitions in props table
Several props have type definition issues that need to be addressed:
- The
classNamesprop has incorrect syntax in the type definition - Several props are using the same incorrect type
- Some props are marked as HTML elements but should be React elements
Apply these corrections:
-| classNames | `Record<"base"｜ "items"｜ "buttons, string>` |
+| classNames | `Record<"base" | "items" | "buttons", string>` |
-| browseButton | `ReactHTMLElement<HTMLButtonElement>` |
+| browseButton | `ReactElement` |
-| browseButtonText | `Record<"base"｜ "items"｜ "buttons, string>` |
+| browseButtonText | `string` |
-| addButton | `ReactHTMLElement<HTMLButtonElement>` |
+| addButton | `ReactElement` |
-| resetButton | `ReactHTMLElement<HTMLButtonElement>` |
+| resetButton | `ReactElement` |
-| uploadButton | `ReactHTMLElement<HTMLButtonElement>` |
+| uploadButton | `ReactElement` |
-| topbar | `Record<"base"｜ "items"｜ "buttons, string>` |
+| topbar | `ReactElement` |
-| buttons | `Record<"base"｜ "items"｜ "buttons, string>` |
+| buttons | `ReactElement` |
-| fileItemElement | `Record<"base"｜ "items"｜ "buttons, string>` |
+| fileItemElement | `ReactElement` |📝 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.
| | classNames | `Record<"base"｜ "items"｜ "buttons, string>` | Allows to set custom class names for the FileUpload slots. | - | | |
| | files | `File[]` | Files as initial values or files to be controlled from outside of the FileUpload. | - | | |
| | browseButton | `ReactHTMLElement<HTMLButtonElement>` | Custom button for browsing files. | - | | |
| | browseButtonText | `Record<"base"｜ "items"｜ "buttons, string>` | Custom text for the default Browse Button. | - | | |
| | addButton | `ReactHTMLElement<HTMLButtonElement>` | Custom button for adding a single file. | - | | |
| | resetButton | `ReactHTMLElement<HTMLButtonElement>` | Custom button for reseting files to an empty array. | - | | |
| | uploadButton | `ReactHTMLElement<HTMLButtonElement>` | Custom button for uploading files to a server. | - | | |
| | topbar | `Record<"base"｜ "items"｜ "buttons, string>` | Custom topbar to show information regarding files. | - | | |
| | buttons | `Record<"base"｜ "items"｜ "buttons, string>` | Custom buttons for browsing, adding, .etc or any other ones that are needed for the user. | - | | |
| | fileItemElement | `Record<"base"｜ "items"｜ "buttons, string>` | Custom element for representing an item that is selected from the device. | - | | |
| | classNames | `Record<"base" | "items" | "buttons", string>` | Allows to set custom class names for the FileUpload slots. | - | | |
| | files | `File[]` | Files as initial values or files to be controlled from outside of the FileUpload. | - | | |
| | browseButton | `ReactElement` | Custom button for browsing files. | - | | |
| | browseButtonText | `string` | Custom text for the default Browse Button. | - | | |
| | addButton | `ReactElement` | Custom button for adding a single file. | - | | |
| | resetButton | `ReactElement` | Custom button for reseting files to an empty array. | - | | |
| | uploadButton | `ReactElement` | Custom button for uploading files to a server. | - | | |
| | topbar | `ReactElement` | Custom topbar to show information regarding files. | - | | |
| | buttons | `ReactElement` | Custom buttons for browsing, adding, .etc or any other ones that are needed for the user. | - | | |
| | fileItemElement | `ReactElement` | Custom element for representing an item that is selected from the device. | - | |
🧰 Tools
🪛 LanguageTool
[style] ~113-~113: This phrase is redundant. Consider using “outside”.
Context: ...l values or files to be controlled from outside of the FileUpload. ...
(OUTSIDE_OF)
[style] ~120-~120: In American English, abbreviations like “etc.” require a period.
Context: ...| Custom buttons for browsing, adding, .etc or any other ones that are needed for t...
(ETC_PERIOD)
| files.push(singleFile); | ||
| updateFiles([...files]); |
There was a problem hiding this comment.
Avoid Direct State Mutation for files
In the onChange handler for the single file input, the files array is mutated directly using files.push(singleFile). Direct mutation of state can lead to unexpected behavior.
Apply this diff to prevent direct mutation:
- files.push(singleFile);
- updateFiles([...files]);
+ updateFiles([...files, singleFile]);📝 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.
| files.push(singleFile); | |
| updateFiles([...files]); | |
| updateFiles([...files, singleFile]); |
| files.map((file) => { | ||
| if (!fileItemElement) { | ||
| return ( | ||
| <FileUploadItem | ||
| key={file.name} | ||
| className={styles.item()} | ||
| file={file} | ||
| isDisabled={props.isDisabled} | ||
| onFileRemove={(name) => { | ||
| if (props.isDisabled) return; | ||
| const newFiles = files.filter((file) => file.name !== name); | ||
|
|
||
| updateFiles(newFiles); | ||
| }} | ||
| /> | ||
| ); | ||
| } | ||
|
|
||
| const customFileElm = fileItemElement(file); | ||
|
|
||
| return cloneElement(customFileElm, { | ||
| key: file.name, | ||
| className: styles.item({class: clsx(classNames?.item, customFileElm.props.className)}), | ||
| }); | ||
| }), | ||
| [styles, classNames, files, fileItemElement, updateFiles], |
There was a problem hiding this comment.
Potential Duplicate Keys in File List Rendering
Currently, the key prop for each file item is set to file.name. Since multiple files can have the same name, this may lead to rendering issues.
Apply this diff to use a more unique identifier:
- key={file.name}
+ key={`${file.name}-${file.lastModified}-${file.size}`}📝 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.
| files.map((file) => { | |
| if (!fileItemElement) { | |
| return ( | |
| <FileUploadItem | |
| key={file.name} | |
| className={styles.item()} | |
| file={file} | |
| isDisabled={props.isDisabled} | |
| onFileRemove={(name) => { | |
| if (props.isDisabled) return; | |
| const newFiles = files.filter((file) => file.name !== name); | |
| updateFiles(newFiles); | |
| }} | |
| /> | |
| ); | |
| } | |
| const customFileElm = fileItemElement(file); | |
| return cloneElement(customFileElm, { | |
| key: file.name, | |
| className: styles.item({class: clsx(classNames?.item, customFileElm.props.className)}), | |
| }); | |
| }), | |
| [styles, classNames, files, fileItemElement, updateFiles], | |
| files.map((file) => { | |
| if (!fileItemElement) { | |
| return ( | |
| <FileUploadItem | |
| key={`${file.name}-${file.lastModified}-${file.size}`} | |
| className={styles.item()} | |
| file={file} | |
| isDisabled={props.isDisabled} | |
| onFileRemove={(name) => { | |
| if (props.isDisabled) return; | |
| const newFiles = files.filter((file) => file.name !== name); | |
| updateFiles(newFiles); | |
| }} | |
| /> | |
| ); | |
| } | |
| const customFileElm = fileItemElement(file); | |
| return cloneElement(customFileElm, { | |
| key={`${file.name}-${file.lastModified}-${file.size}`}, | |
| className: styles.item({class: clsx(classNames?.item, customFileElm.props.className)}), | |
| }); | |
| }), | |
| [styles, classNames, files, fileItemElement, updateFiles], |
Arian94
force-pushed
the
feat/file-upload_#3772
branch
from
38d4056 to
7ca7d22
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (12)
apps/docs/content/components/file-upload/buttonsContainer.ts (1)
9-13: Enhance accessibility and user experience.The buttons section could benefit from improved accessibility and visual organization:
- Add aria-labels for screen readers
- Add spacing between buttons
- Consider adding icons for visual clarity
- return <div> - <Button onClick={() => onBrowse()}>Browse Files</Button> - <Button onClick={() => onAdd()}>Add New File</Button> - <Button onClick={() => onReset()}>Remove All</Button> + return <div className="flex gap-2"> + <Button + aria-label="Browse files" + onClick={() => onBrowse()} + startContent={<BrowseIcon />} + > + Browse Files + </Button> + <Button + aria-label="Add new file" + onClick={() => onAdd()} + startContent={<AddIcon />} + > + Add New File + </Button> + <Button + aria-label="Remove all files" + onClick={() => onReset()} + startContent={<DeleteIcon />} + color="danger" + > + Remove All + </Button> </div>packages/components/file-upload/src/file-upload-item.tsx (2)
5-9: Consider adding file validation types to the interface.The interface could benefit from additional type safety for file validation:
+type ValidFileType = string | string[]; +type ValidFileSize = number; export interface FileUploadItemProps extends HTMLNextUIProps<"div"> { file: File; onFileRemove: (file: File) => void; isDisabled?: boolean; + allowedTypes?: ValidFileType; + maxFileSize?: ValidFileSize; }
11-18: Consider optimizing and relocating the format utility.The
formatFileSizefunction is well-implemented but could be:
- Memoized for performance
- Moved to a shared utilities file for reuse across components
+import {useMemo} from 'react'; + -const formatFileSize = (bytes: number) => { +const useFormattedFileSize = (bytes: number) => useMemo(() => { if (bytes === 0) return "0 Bytes"; const k = 1024; const sizes = ["Bytes", "KB", "MB", "GB"]; const i = Math.floor(Math.log(bytes) / Math.log(k)); return parseFloat((bytes / Math.pow(k, i)).toFixed(2)) + " " + sizes[i]; -}; +}, [bytes]);packages/components/file-upload/__tests__/file-upload.test.tsx (2)
8-18: Consider enhancing test setup with common utilitiesWhile the basic setup is good, consider adding common test utilities:
- Mock for File API
- Common test files/data
- Helper functions for repeated operations
beforeEach(() => { user = userEvent.setup(); + // Setup common test files + global.testFile = new File(['test'], 'test.png', { type: 'image/png' }); + // Mock File API if needed + global.URL.createObjectURL = jest.fn(); }); afterEach(() => { jest.clearAllMocks(); + // Clean up any created object URLs + if (global.URL.createObjectURL.mockClear) { + global.URL.createObjectURL.mockClear(); + } });
55-55: Improve file mock creationThe current file mock is created with empty content and might not represent real-world scenarios accurately.
-files={[new File([], "file1", {type: "jpg"})]} +files={[new File(['test content'], "file1.jpg", {type: "image/jpeg"})]}apps/docs/content/components/file-upload/customButtons.ts (1)
1-28: Add TypeScript types and improve accessibilityConsider the following improvements for the DeleteIcon component:
- Add TypeScript interface for props
- Include aria-label for better accessibility
- Consider using a more maintainable format instead of template string
-const DeleteIcon = `export const DeleteIcon = ({ +const DeleteIcon = ` +interface DeleteIconProps { + fill?: string; + filled?: boolean; + size?: number; + height?: number; + width?: number; + label?: string; +} + +export const DeleteIcon = ({ fill = 'currentColor', filled, size, height, width, label, ...props -}) => { +}: DeleteIconProps) => { return ( <svg aria-hidden="true" fill="none" focusable="false" height="1em" role="presentation" - viewBox="0 0 20 20" width="1em"> + viewBox="0 0 20 20" width="1em" aria-label={label || "Delete"}> `;packages/components/file-upload/src/file-upload-topbar.tsx (3)
5-5: Consider enhancing the FileSize type definition.The current type definition has limitations:
- Missing common size units (B, GB, TB)
- Allows inconsistent spacing in format ("10 KB" vs "10KB")
Consider this improved type definition:
-type FileSize = `${number} KB` | `${number} MB` | `${number}KB` | `${number}MB`; +type FileSizeUnit = "B" | "KB" | "MB" | "GB" | "TB"; +type FileSize = `${number} ${FileSizeUnit}`;
7-48: Consider adding validation constraints to size-related props.While the interface is well-documented, it lacks validation constraints for size-related props. This could lead to unrealistic or invalid size configurations.
Consider adding zod schema or prop-types validation:
// Example validation using zod const FileSizeSchema = z.string().regex(/^\d+\s*(B|KB|MB|GB|TB)$/, { message: "Invalid file size format. Example: '10 MB'" }); // Add runtime validation in component if (maxAllowedSize && !FileSizeSchema.safeParse(maxAllowedSize).success) { console.warn("Invalid maxAllowedSize format"); }
50-90: Consider adding internationalization support.The component uses hardcoded English strings for default text values. For better global usability, consider extracting these strings into a localization system.
Example approach using a translation hook:
+import {useTranslation} from 'next-intl'; const FileUploadTopbar: React.FC<FileUploadTopbarProps> = ({ - maxItemsText = "Max number of items", - maxAllowedSizeText = "Max File Size", - totalMaxAllowedSizeText = "Total Max Files Size", + maxItemsText, + maxAllowedSizeText, + totalMaxAllowedSizeText, // ... other props }) => { + const {t} = useTranslation(); + const defaultTexts = { + maxItems: t('fileUpload.maxItems', 'Max number of items'), + maxSize: t('fileUpload.maxSize', 'Max File Size'), + totalMaxSize: t('fileUpload.totalMaxSize', 'Total Max Files Size'), + }; + + const finalMaxItemsText = maxItemsText ?? defaultTexts.maxItems; + // ... similar for other text propspackages/components/file-upload/src/use-file-upload.ts (2)
73-90: Optimize useMemo dependency and add cleanup
The
objectToDepsusage in the dependency array could lead to unnecessary re-renders. Consider memoizing the variant props separately.Consider adding cleanup logic for any ongoing file operations.
Here's the suggested optimization:
+ const memoizedVariantProps = useMemo( + () => variantProps, + [objectToDeps(variantProps)] + ); const styles = useMemo( () => fileUpload({ - ...variantProps, + ...memoizedVariantProps, className, }), - [objectToDeps(variantProps), className], + [memoizedVariantProps, className], );
71-72: Add JSDoc comments for exported typesConsider adding comprehensive JSDoc comments for the exported types to improve documentation:
+/** + * Props for the useFileUpload hook, combining base props with variant props + */ export type UseFileUploadProps = Props & FileUploadVariantProps; +/** + * Return type of the useFileUpload hook containing component props and styling information + */ export type UseFileUploadReturn = ReturnType<typeof useFileUpload>;Also applies to: 102-102
packages/components/file-upload/src/file-upload.tsx (1)
172-205: Optimize buttonsElement dependency array.The dependency array includes derived values that are already memoized. Consider removing redundant dependencies.
Apply this diff:
}, [ onBrowse, onAdd, onReset, styles, multiple, files.length, - browseButtonElement, - addButtonElement, - resetButtonElement, uploadButton, + buttons, + props.isDisabled, + classNames?.buttons ]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
packages/components/file-upload/__tests__/__snapshots__/file-upload.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (14)
apps/docs/content/components/file-upload/buttonsContainer.ts(1 hunks)apps/docs/content/components/file-upload/controlled.ts(1 hunks)apps/docs/content/components/file-upload/customButtons.ts(1 hunks)apps/docs/content/components/file-upload/file-item.ts(1 hunks)apps/docs/content/components/file-upload/index.ts(1 hunks)apps/docs/content/docs/components/file-upload.mdx(1 hunks)packages/components/file-upload/__tests__/file-upload.test.tsx(1 hunks)packages/components/file-upload/package.json(1 hunks)packages/components/file-upload/src/file-upload-item.tsx(1 hunks)packages/components/file-upload/src/file-upload-topbar.tsx(1 hunks)packages/components/file-upload/src/file-upload.tsx(1 hunks)packages/components/file-upload/src/use-file-upload.ts(1 hunks)packages/components/file-upload/stories/file-upload.stories.tsx(1 hunks)packages/core/react/src/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/docs/content/components/file-upload/controlled.ts
- apps/docs/content/components/file-upload/file-item.ts
- apps/docs/content/components/file-upload/index.ts
- packages/components/file-upload/package.json
- packages/components/file-upload/stories/file-upload.stories.tsx
🧰 Additional context used
🪛 LanguageTool
apps/docs/content/docs/components/file-upload.mdx
[style] ~70-~70: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ...n` prop for you in case uploading files prior to submitting the form is required. <Code...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
[typographical] ~109-~109: Consider adding a comma after ‘Usually’ for more clarity.
Context: ... | The contents of the collection. Usually the array of FileUploadItem ...
(RB_LY_COMMA)
[style] ~113-~113: This phrase is redundant. Consider using “outside”.
Context: ...l values or files to be controlled from outside of the FileUpload. ...
(OUTSIDE_OF)
[style] ~120-~120: In American English, abbreviations like “etc.” require a period.
Context: ...| Custom buttons for browsing, adding, .etc or any other ones that are needed for t...
(ETC_PERIOD)
🔇 Additional comments (12)
apps/docs/content/components/file-upload/buttonsContainer.ts (2)
7-7: Add validation for multiple file uploads.
The multiple prop is set without any file validation or error handling.
Consider adding validation props like maxFiles and accept to control the upload behavior.
<FileUpload
- multiple
+ multiple
+ maxFiles={5}
+ accept={{
+ 'image/*': ['.png', '.jpg', '.jpeg', '.gif'],
+ 'application/pdf': ['.pdf']
+ }}
+ onError={(error) => {
+ console.error('Upload error:', error);
+ }}19-25: LGTM: Path mapping and export structure.
The path mapping and export structure follows a clean pattern.
packages/components/file-upload/src/file-upload-item.tsx (1)
43-45: LGTM! Component exports follow React best practices.
The display name is properly set and the export is clean.
packages/core/react/src/index.ts (1)
48-48: LGTM! The export follows the established patterns.
The new export for the file-upload module maintains consistency with other NextUI package exports and preserves the alphabetical ordering.
Let's verify that the package exists and is properly set up:
✅ Verification successful
The file-upload package exists and is properly integrated
The package is correctly set up:
- Located at
packages/components/file-upload/ - Has necessary source files (file-upload.tsx, file-upload-item.tsx, etc.)
- Is properly listed as a dependency in
packages/core/react/package.json - Has documentation at
apps/docs/content/docs/components/file-upload.mdx
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and setup of the file-upload package
# Test 1: Check if package.json exists for the file-upload package
fd -t f "package.json" -p "packages/**/file-upload"
# Test 2: Check if the package exports a main entry point
fd -t f "index.ts" -p "packages/**/file-upload/src"
Length of output: 329
Script:
#!/bin/bash
# Let's verify if the file-upload package exists in a more comprehensive way
# Check if the package exists in the workspace packages
fd "file-upload" packages/
# Check package.json in core/react to see if the dependency is listed
cat packages/core/react/package.json
# Check if there are any references to file-upload in the codebase
rg "@nextui-org/file-upload" -l
Length of output: 4141
packages/components/file-upload/__tests__/file-upload.test.tsx (1)
1-7: LGTM: Imports are well-organized and complete
All necessary testing utilities and components are properly imported with appropriate typing.
apps/docs/content/components/file-upload/customButtons.ts (1)
51-58: LGTM!
The export configuration is clean and follows good practices.
packages/components/file-upload/src/file-upload-topbar.tsx (1)
92-94: LGTM!
The component export is properly configured with a meaningful displayName for better debugging experience.
packages/components/file-upload/src/use-file-upload.ts (1)
1-9: LGTM! Well-organized imports
The imports are well-structured and appropriately sourced from NextUI internal packages, showing good separation of concerns.
packages/components/file-upload/src/file-upload.tsx (4)
1-32: LGTM! Well-structured component setup.
The component follows React best practices with proper prop typing and ref forwarding.
34-52: LGTM! Clean state management implementation.
The state management is well-implemented with proper cleanup of file inputs to handle edge cases.
89-92: Use unique key for custom file items.
Using only file.name as key could cause issues with duplicate file names.
246-262: LGTM! Clean component rendering.
The component's render logic is well-structured with proper conditional rendering and className handling.
| @@ -0,0 +1,25 @@ | |||
| const App = ` | |||
| import {FileUpload} from "@nextui-org/react"; | |||
There was a problem hiding this comment.
Add missing Button import.
The code uses the Button component but doesn't import it from @nextui-org/react.
-import {FileUpload} from "@nextui-org/react";
+import {FileUpload, Button} from "@nextui-org/react";📝 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.
| import {FileUpload} from "@nextui-org/react"; | |
| import {FileUpload, Button} from "@nextui-org/react"; |
| const App = ` | ||
| import {FileUpload} from "@nextui-org/react"; | ||
|
|
||
| export default function App() { | ||
| return ( | ||
| <FileUpload | ||
| multiple | ||
| buttons={(onBrowse, onAdd, onReset) => { | ||
| return <div> | ||
| <Button onClick={() => onBrowse()}>Browse Files</Button> | ||
| <Button onClick={() => onAdd()}>Add New File</Button> | ||
| <Button onClick={() => onReset()}>Remove All</Button> | ||
| </div> | ||
| }} | ||
| /> | ||
| ); | ||
| }`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using TypeScript for better type safety.
The component is defined as a template string without proper TypeScript types for the callback functions. This could lead to type-related issues and makes the code harder to maintain.
Consider this implementation:
-const App = `
-import {FileUpload} from "@nextui-org/react";
+const App = `
+import {FileUpload, Button} from "@nextui-org/react";
+
+interface FileUploadHandlers {
+ onBrowse: () => void;
+ onAdd: () => void;
+ onReset: () => void;
+}
export default function App() {
return (
<FileUpload
multiple
- buttons={(onBrowse, onAdd, onReset) => {
+ buttons={({onBrowse, onAdd, onReset}: FileUploadHandlers) => {
return <div>
<Button onClick={() => onBrowse()}>Browse Files</Button>
<Button onClick={() => onAdd()}>Add New File</Button>
<Button onClick={() => onReset()}>Remove All</Button>
</div>
}}
/>
);
}`;📝 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 App = ` | |
| import {FileUpload} from "@nextui-org/react"; | |
| export default function App() { | |
| return ( | |
| <FileUpload | |
| multiple | |
| buttons={(onBrowse, onAdd, onReset) => { | |
| return <div> | |
| <Button onClick={() => onBrowse()}>Browse Files</Button> | |
| <Button onClick={() => onAdd()}>Add New File</Button> | |
| <Button onClick={() => onReset()}>Remove All</Button> | |
| </div> | |
| }} | |
| /> | |
| ); | |
| }`; | |
| const App = ` | |
| import {FileUpload, Button} from "@nextui-org/react"; | |
| interface FileUploadHandlers { | |
| onBrowse: () => void; | |
| onAdd: () => void; | |
| onReset: () => void; | |
| } | |
| export default function App() { | |
| return ( | |
| <FileUpload | |
| multiple | |
| buttons={({onBrowse, onAdd, onReset}: FileUploadHandlers) => { | |
| return <div> | |
| <Button onClick={() => onBrowse()}>Browse Files</Button> | |
| <Button onClick={() => onAdd()}>Add New File</Button> | |
| <Button onClick={() => onReset()}>Remove All</Button> | |
| </div> | |
| }} | |
| /> | |
| ); | |
| }`; |
| <div {...otherProps}> | ||
| <Button | ||
| aria-label={`Remove ${file.name}`} | ||
| isDisabled={isDisabled} | ||
| role="listitem" | ||
| onClick={() => onFileRemove(file)} | ||
| > | ||
| <CloseIcon /> | ||
| </Button> | ||
| <span>{file.name}</span> | ||
| <span>{formatFileSize(file.size)}</span> | ||
| <span>{file.type.split("/").at(1)?.toUpperCase()}</span> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve component structure and accessibility.
Several structural improvements are needed:
- <div {...otherProps}>
+ <div role="listitem" className="file-upload-item" {...otherProps}>
+ <div className="file-info">
+ <span className="file-name" title={file.name}>{file.name}</span>
+ <span className="file-size">{formatFileSize(file.size)}</span>
+ <span className="file-type">
+ {file.type ? file.type.split("/").at(1)?.toUpperCase() ?? 'UNKNOWN' : 'UNKNOWN'}
+ </span>
+ </div>
<Button
aria-label={`Remove ${file.name}`}
isDisabled={isDisabled}
- role="listitem"
onClick={() => onFileRemove(file)}
+ className="remove-button"
>
<CloseIcon />
</Button>
- <span>{file.name}</span>
- <span>{formatFileSize(file.size)}</span>
- <span>{file.type.split("/").at(1)?.toUpperCase()}</span>
</div>📝 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.
| <div {...otherProps}> | |
| <Button | |
| aria-label={`Remove ${file.name}`} | |
| isDisabled={isDisabled} | |
| role="listitem" | |
| onClick={() => onFileRemove(file)} | |
| > | |
| <CloseIcon /> | |
| </Button> | |
| <span>{file.name}</span> | |
| <span>{formatFileSize(file.size)}</span> | |
| <span>{file.type.split("/").at(1)?.toUpperCase()}</span> | |
| </div> | |
| <div role="listitem" className="file-upload-item" {...otherProps}> | |
| <div className="file-info"> | |
| <span className="file-name" title={file.name}>{file.name}</span> | |
| <span className="file-size">{formatFileSize(file.size)}</span> | |
| <span className="file-type"> | |
| {file.type ? file.type.split("/").at(1)?.toUpperCase() ?? 'UNKNOWN' : 'UNKNOWN'} | |
| </span> | |
| </div> | |
| <Button | |
| aria-label={`Remove ${file.name}`} | |
| isDisabled={isDisabled} | |
| onClick={() => onFileRemove(file)} | |
| className="remove-button" | |
| > | |
| <CloseIcon /> | |
| </Button> | |
| </div> |
| it("buttons should be disabled when isDisabled", () => { | ||
| const {container} = render( | ||
| <FileUpload | ||
| isDisabled | ||
| multiple | ||
| addButton={<Button>Add file</Button>} | ||
| browseButton={<Button>Browse files</Button>} | ||
| files={[new File([], "file1", {type: "jpg"})]} | ||
| resetButton={<Button>Remove all files</Button>} | ||
| />, | ||
| ); | ||
|
|
||
| const buttons = container.getElementsByTagName("button"); | ||
|
|
||
| for (let index = 0; index < buttons.length; index++) { | ||
| const btn = buttons.item(index); | ||
|
|
||
| expect(btn).toBeDisabled(); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing test scenarios
The disabled state test is good, but several critical scenarios are missing:
- File selection events
- Multiple file handling
- Error states (invalid file types, size limits)
Would you like me to provide example implementations for these test cases?
| it("should render correctly", () => { | ||
| const wrapper = render(<FileUpload />); | ||
|
|
||
| expect(wrapper.getByRole("button")).toBeInTheDocument(); | ||
| expect(wrapper.container).toMatchSnapshot(); | ||
| expect(() => wrapper.unmount()).not.toThrow(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance basic render test with accessibility checks
While the basic render test has been improved, consider adding accessibility-related assertions:
it("should render correctly", () => {
const wrapper = render(<FileUpload />);
expect(wrapper.getByRole("button")).toBeInTheDocument();
+ // Check for proper ARIA attributes
+ expect(wrapper.getByRole("button")).toHaveAttribute("aria-label");
+ // Verify the input is properly hidden but accessible
+ expect(wrapper.container.querySelector('input[type="file"]')).toHaveAttribute("aria-hidden", "true");
expect(wrapper.container).toMatchSnapshot();
expect(() => wrapper.unmount()).not.toThrow();
});📝 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.
| it("should render correctly", () => { | |
| const wrapper = render(<FileUpload />); | |
| expect(wrapper.getByRole("button")).toBeInTheDocument(); | |
| expect(wrapper.container).toMatchSnapshot(); | |
| expect(() => wrapper.unmount()).not.toThrow(); | |
| }); | |
| it("should render correctly", () => { | |
| const wrapper = render(<FileUpload />); | |
| expect(wrapper.getByRole("button")).toBeInTheDocument(); | |
| // Check for proper ARIA attributes | |
| expect(wrapper.getByRole("button")).toHaveAttribute("aria-label"); | |
| // Verify the input is properly hidden but accessible | |
| expect(wrapper.container.querySelector('input[type="file"]')).toHaveAttribute("aria-hidden", "true"); | |
| expect(wrapper.container).toMatchSnapshot(); | |
| expect(() => wrapper.unmount()).not.toThrow(); | |
| }); |
| describe("FileUpload", () => { | ||
| let user: UserEvent; | ||
|
|
||
| beforeEach(() => { | ||
| user = userEvent.setup(); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| it("should render correctly", () => { | ||
| const wrapper = render(<FileUpload />); | ||
|
|
||
| expect(wrapper.getByRole("button")).toBeInTheDocument(); | ||
| expect(wrapper.container).toMatchSnapshot(); | ||
| expect(() => wrapper.unmount()).not.toThrow(); | ||
| }); | ||
|
|
||
| it("ref should be forwarded", () => { | ||
| const ref = React.createRef<HTMLDivElement>(); | ||
|
|
||
| render(<FileUpload ref={ref} />); | ||
| expect(ref.current).not.toBeNull(); | ||
| }); | ||
|
|
||
| it("should not browse when isDisabled", async () => { | ||
| let renderResult = render(<FileUpload isDisabled />); | ||
| let browseButton = renderResult.container.querySelector("button"); | ||
|
|
||
| expect(browseButton).toBeDisabled(); | ||
|
|
||
| renderResult = render(<FileUpload isDisabled browseButton={<Button>Browse files</Button>} />); | ||
| browseButton = renderResult.container.querySelector("button"); | ||
|
|
||
| browseButton && (await user.click(browseButton)); | ||
|
|
||
| expect(browseButton).toBeDisabled(); | ||
| }); | ||
|
|
||
| it("buttons should be disabled when isDisabled", () => { | ||
| const {container} = render( | ||
| <FileUpload | ||
| isDisabled | ||
| multiple | ||
| addButton={<Button>Add file</Button>} | ||
| browseButton={<Button>Browse files</Button>} | ||
| files={[new File([], "file1", {type: "jpg"})]} | ||
| resetButton={<Button>Remove all files</Button>} | ||
| />, | ||
| ); | ||
|
|
||
| const buttons = container.getElementsByTagName("button"); | ||
|
|
||
| for (let index = 0; index < buttons.length; index++) { | ||
| const btn = buttons.item(index); | ||
|
|
||
| expect(btn).toBeDisabled(); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
Additional test coverage needed for key features
The review comment is correct. After examining the component files, several important features lack test coverage:
-
Top Bar functionality:
maxItemsconstraint display and validationmaxAllowedSizeconstraint display and validationtotalMaxAllowedSizeconstraint display and validation
-
File Item container features:
- File removal functionality
- File size formatting
- File type display
- Multiple file handling
-
File restrictions:
- File type validation through
acceptprop - File size restrictions
- Multiple file upload constraints
- File type validation through
-
Event handlers:
onChangecallback when files are added/removed- Custom button click handlers
- File selection events
The current tests only cover basic rendering, ref forwarding, and disabled state. Additional test cases should be added to ensure complete feature coverage.
🔗 Analysis chain
Verify test coverage for all component features
Based on the PR objectives, several key features of the FileUpload component are not covered by tests:
- Top Bar information display
- Items container functionality
- File type restrictions
Let's verify the component's exported features:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check exported features that need test coverage
# Find all exported features from the component
ast-grep --pattern 'export interface FileUploadProps {
$$$
}'
# Find all event handlers that need testing
ast-grep --pattern 'on[A-Z][a-zA-Z]+?: ($$$) => $$$'
Length of output: 120
Script:
#!/bin/bash
# Let's try a different approach to find the component's features
# Find the component file
fd "file-upload" --type f --extension tsx --exclude "__tests__"
# Search for prop types and component implementation
rg -U "interface.*Props|^export.*function FileUpload" --type tsx -A 10
Length of output: 394
Script:
#!/bin/bash
# Let's examine the component files directly to understand their features
# Check the main component implementation
cat "packages/components/file-upload/src/file-upload.tsx"
# Check the item component
cat "packages/components/file-upload/src/file-upload-item.tsx"
# Check the topbar component
cat "packages/components/file-upload/src/file-upload-topbar.tsx"
Length of output: 10899
| const App = ` | ||
| import React from "react"; | ||
| import {FileUpload} from "@nextui-org/react"; | ||
| import {Button} from "@nextui-org/react"; | ||
| import {DeleteIcon} from './DeleteIcon'; | ||
|
|
||
| export default function App() { | ||
| const [files, setFiles] = React.useState([]); | ||
|
|
||
| return ( | ||
| <FileUpload | ||
| multiple | ||
| onChange={newFiles => setFiles(newFiles)} | ||
| browseButton={<Button>Search</Button>} | ||
| uploadButton={<Button onClick={() => console.log("Files to upload:\\n" + files.map(file => file.name).join("\\n"))}>Upload Files</Button>} | ||
| addButton={<Button>+</Button>} | ||
| resetButton={<Button><DeleteIcon /></Button>} | ||
| /> | ||
| ); | ||
| }`; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance example with proper validation and error handling
The current example could be improved to demonstrate best practices:
- Remove console.log and show proper upload handling
- Add file type and size restrictions
- Include error handling
const App = `
import React from "react";
-import {FileUpload} from "@nextui-org/react";
-import {Button} from "@nextui-org/react";
+import {FileUpload, Button} from "@nextui-org/react";
import {DeleteIcon} from './DeleteIcon';
export default function App() {
const [files, setFiles] = React.useState([]);
+ const [error, setError] = React.useState("");
+
+ const handleUpload = async () => {
+ try {
+ // Add your upload logic here
+ const formData = new FormData();
+ files.forEach(file => formData.append('files', file));
+ // await uploadFiles(formData);
+ } catch (err) {
+ setError("Upload failed");
+ }
+ };
return (
<FileUpload
multiple
+ accept=".jpg,.png,.pdf"
+ maxSize={5242880} // 5MB
onChange={newFiles => setFiles(newFiles)}
browseButton={<Button>Search</Button>}
- uploadButton={<Button onClick={() => console.log("Files to upload:\\n" + files.map(file => file.name).join("\\n"))}>Upload Files</Button>}
+ uploadButton={<Button onClick={handleUpload}>Upload Files</Button>}
addButton={<Button>+</Button>}
resetButton={<Button><DeleteIcon /></Button>}
/>
);
}`;Committable suggestion skipped: line range outside the PR's diff.
| interface Props extends Omit<HTMLNextUIProps<"div">, "onChange"> { | ||
| /** | ||
| * Ref to the DOM node. | ||
| */ | ||
| ref?: ReactRef<HTMLElement | null>; | ||
| classNames?: SlotsToClasses<FileUploadSlots>; | ||
| /** | ||
| * A property to set initial files (which might be fetched) or to control files from outside of the component. | ||
| */ | ||
| files?: File[]; | ||
| /** | ||
| * Custom Browse Button. | ||
| */ | ||
| browseButton?: ReactElement<ButtonProps>; | ||
| /** | ||
| * A different text for the browse button. | ||
| */ | ||
| browseButtonText?: string; | ||
| /** | ||
| * Custom Add Button. | ||
| */ | ||
| addButton?: ReactElement<ButtonProps>; | ||
| /** | ||
| * Custom Reset Button. | ||
| */ | ||
| resetButton?: ReactElement<ButtonProps>; | ||
| /** | ||
| * Custom Upload Button. | ||
| */ | ||
| uploadButton?: ReactElement<ButtonProps>; | ||
| /** | ||
| * Custom element to display buttons such as the browse button in customized design and order. | ||
| */ | ||
| buttons?: ( | ||
| onBrowse: () => void, | ||
| onAdd: () => void, | ||
| onReset: () => void, | ||
| ) => ReactElement<HTMLElement>; | ||
| /** | ||
| * If set to true, multiple files can be selected from the device. | ||
| * @default false | ||
| */ | ||
| multiple?: boolean; | ||
| /** | ||
| * Accept certain file format(s). | ||
| */ | ||
| accept?: string; | ||
| /** | ||
| * Custom Element for an Upload File Item. | ||
| */ | ||
| fileItemElement?: (file: File) => ReactElement<HTMLElement>; | ||
| /** | ||
| * Custom Element for topbar of the component. | ||
| */ | ||
| topbar?: ReactElement<HTMLElement>; | ||
| /** | ||
| * Triggered when file(s) selected, added or removed. | ||
| */ | ||
| onChange?: (files: File[]) => void; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding essential file upload properties
While the Props interface is well-documented and provides good customization options, consider adding these essential properties for a more robust file upload component:
-
File validation props:
maxFileSize?: numbermaxTotalSize?: numbervalidateFile?: (file: File) => boolean | Promise<boolean>
-
Error handling props:
onError?: (error: Error, file?: File) => voiderrorMessage?: string | ReactNode
-
Loading state props:
onProgress?: (progress: number, file: File) => voidloading?: boolean
These additions would enhance the component's reliability and user experience.
interface Props extends Omit<HTMLNextUIProps<"div">, "onChange"> {
// ... existing props ...
+ /**
+ * Maximum size allowed for each file in bytes
+ */
+ maxFileSize?: number;
+ /**
+ * Maximum total size allowed for all files in bytes
+ */
+ maxTotalSize?: number;
+ /**
+ * Custom validation function for files
+ */
+ validateFile?: (file: File) => boolean | Promise<boolean>;
+ /**
+ * Callback fired when an error occurs
+ */
+ onError?: (error: Error, file?: File) => void;
+ /**
+ * Custom error message
+ */
+ errorMessage?: string | ReactNode;
+ /**
+ * Callback fired during file upload progress
+ */
+ onProgress?: (progress: number, file: File) => void;
+ /**
+ * Loading state of the upload process
+ */
+ loading?: boolean;
}📝 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.
| interface Props extends Omit<HTMLNextUIProps<"div">, "onChange"> { | |
| /** | |
| * Ref to the DOM node. | |
| */ | |
| ref?: ReactRef<HTMLElement | null>; | |
| classNames?: SlotsToClasses<FileUploadSlots>; | |
| /** | |
| * A property to set initial files (which might be fetched) or to control files from outside of the component. | |
| */ | |
| files?: File[]; | |
| /** | |
| * Custom Browse Button. | |
| */ | |
| browseButton?: ReactElement<ButtonProps>; | |
| /** | |
| * A different text for the browse button. | |
| */ | |
| browseButtonText?: string; | |
| /** | |
| * Custom Add Button. | |
| */ | |
| addButton?: ReactElement<ButtonProps>; | |
| /** | |
| * Custom Reset Button. | |
| */ | |
| resetButton?: ReactElement<ButtonProps>; | |
| /** | |
| * Custom Upload Button. | |
| */ | |
| uploadButton?: ReactElement<ButtonProps>; | |
| /** | |
| * Custom element to display buttons such as the browse button in customized design and order. | |
| */ | |
| buttons?: ( | |
| onBrowse: () => void, | |
| onAdd: () => void, | |
| onReset: () => void, | |
| ) => ReactElement<HTMLElement>; | |
| /** | |
| * If set to true, multiple files can be selected from the device. | |
| * @default false | |
| */ | |
| multiple?: boolean; | |
| /** | |
| * Accept certain file format(s). | |
| */ | |
| accept?: string; | |
| /** | |
| * Custom Element for an Upload File Item. | |
| */ | |
| fileItemElement?: (file: File) => ReactElement<HTMLElement>; | |
| /** | |
| * Custom Element for topbar of the component. | |
| */ | |
| topbar?: ReactElement<HTMLElement>; | |
| /** | |
| * Triggered when file(s) selected, added or removed. | |
| */ | |
| onChange?: (files: File[]) => void; | |
| } | |
| interface Props extends Omit<HTMLNextUIProps<"div">, "onChange"> { | |
| /** | |
| * Ref to the DOM node. | |
| */ | |
| ref?: ReactRef<HTMLElement | null>; | |
| classNames?: SlotsToClasses<FileUploadSlots>; | |
| /** | |
| * A property to set initial files (which might be fetched) or to control files from outside of the component. | |
| */ | |
| files?: File[]; | |
| /** | |
| * Custom Browse Button. | |
| */ | |
| browseButton?: ReactElement<ButtonProps>; | |
| /** | |
| * A different text for the browse button. | |
| */ | |
| browseButtonText?: string; | |
| /** | |
| * Custom Add Button. | |
| */ | |
| addButton?: ReactElement<ButtonProps>; | |
| /** | |
| * Custom Reset Button. | |
| */ | |
| resetButton?: ReactElement<ButtonProps>; | |
| /** | |
| * Custom Upload Button. | |
| */ | |
| uploadButton?: ReactElement<ButtonProps>; | |
| /** | |
| * Custom element to display buttons such as the browse button in customized design and order. | |
| */ | |
| buttons?: ( | |
| onBrowse: () => void, | |
| onAdd: () => void, | |
| onReset: () => void, | |
| ) => ReactElement<HTMLElement>; | |
| /** | |
| * If set to true, multiple files can be selected from the device. | |
| * @default false | |
| */ | |
| multiple?: boolean; | |
| /** | |
| * Accept certain file format(s). | |
| */ | |
| accept?: string; | |
| /** | |
| * Custom Element for an Upload File Item. | |
| */ | |
| fileItemElement?: (file: File) => ReactElement<HTMLElement>; | |
| /** | |
| * Custom Element for topbar of the component. | |
| */ | |
| topbar?: ReactElement<HTMLElement>; | |
| /** | |
| * Triggered when file(s) selected, added or removed. | |
| */ | |
| onChange?: (files: File[]) => void; | |
| /** | |
| * Maximum size allowed for each file in bytes | |
| */ | |
| maxFileSize?: number; | |
| /** | |
| * Maximum total size allowed for all files in bytes | |
| */ | |
| maxTotalSize?: number; | |
| /** | |
| * Custom validation function for files | |
| */ | |
| validateFile?: (file: File) => boolean | Promise<boolean>; | |
| /** | |
| * Callback fired when an error occurs | |
| */ | |
| onError?: (error: Error, file?: File) => void; | |
| /** | |
| * Custom error message | |
| */ | |
| errorMessage?: string | ReactNode; | |
| /** | |
| * Callback fired during file upload progress | |
| */ | |
| onProgress?: (progress: number, file: File) => void; | |
| /** | |
| * Loading state of the upload process | |
| */ | |
| loading?: boolean; | |
| } |
| <input | ||
| ref={inputFileRef} | ||
| accept={accept} | ||
| className="hidden" | ||
| multiple={multiple} | ||
| title="file upload" | ||
| type="file" | ||
| onChange={(ev) => { | ||
| if (!ev.target.files?.length) return; | ||
| const newFiles: File[] = []; | ||
|
|
||
| for (let index = 0; index < ev.target.files.length; index++) { | ||
| const file = ev.target.files.item(index); | ||
|
|
||
| file && newFiles.push(file); | ||
| } | ||
| updateFiles(newFiles); | ||
| }} | ||
| /> |
There was a problem hiding this comment.
💡 Codebase verification
File validation is indeed missing and should be implemented
The review comment is accurate. After examining the hook implementation and component code, there's no validation for:
- File size limits
- File type validation beyond the HTML
acceptattribute - Maximum number of files when
multipleis true
Key locations that need attention:
packages/components/file-upload/src/file-upload.tsx: TheonChangehandler where files are processedpackages/components/file-upload/src/use-file-upload.ts: The hook where validation logic could be centralized
🔗 Analysis chain
Consider adding file validation.
The file input handlers could benefit from additional validation:
- File size limits
- More robust file type checking
- Maximum number of files validation
Let's check if there's any file validation in the hook:
Let me check the implementation of the hook and any validation logic:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for file validation in the useFileUpload hook
rg -A 5 "useFileUpload" packages/components/file-upload/src/
Length of output: 2222
Script:
#!/bin/bash
# Check the hook implementation and validation logic
cat packages/components/file-upload/src/use-file-upload.ts
Length of output: 2724
| const addButtonElement = useMemo( | ||
| () => | ||
| addButton ? ( | ||
| cloneElement(addButton, { | ||
| disabled: props.isDisabled, | ||
| onClick: (ev) => { | ||
| onAdd(); | ||
| addButton.props.onClick?.(ev); | ||
| }, | ||
| }) | ||
| ) : ( | ||
| <Button color="secondary" disabled={props.isDisabled} onClick={() => onAdd()}> | ||
| Add | ||
| </Button> | ||
| ), | ||
| [addButton, onAdd], | ||
| ); |
There was a problem hiding this comment.
Add missing dependency to addButtonElement useMemo.
The props.isDisabled value is used inside the useMemo but is missing from the dependency array.
Apply this diff:
- [addButton, onAdd],
+ [addButton, onAdd, props.isDisabled],📝 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 addButtonElement = useMemo( | |
| () => | |
| addButton ? ( | |
| cloneElement(addButton, { | |
| disabled: props.isDisabled, | |
| onClick: (ev) => { | |
| onAdd(); | |
| addButton.props.onClick?.(ev); | |
| }, | |
| }) | |
| ) : ( | |
| <Button color="secondary" disabled={props.isDisabled} onClick={() => onAdd()}> | |
| Add | |
| </Button> | |
| ), | |
| [addButton, onAdd], | |
| ); | |
| const addButtonElement = useMemo( | |
| () => | |
| addButton ? ( | |
| cloneElement(addButton, { | |
| disabled: props.isDisabled, | |
| onClick: (ev) => { | |
| onAdd(); | |
| addButton.props.onClick?.(ev); | |
| }, | |
| }) | |
| ) : ( | |
| <Button color="secondary" disabled={props.isDisabled} onClick={() => onAdd()}> | |
| Add | |
| </Button> | |
| ), | |
| [addButton, onAdd, props.isDisabled], | |
| ); |
Closes #3772,
📝 Description
🚀 New behavior
Developers are allowed to customize all three sections. There are many props for modifications and/or using customizable parts in these sections.
💣 Is this a breaking change (Yes/No):
No
📝 Additional Information
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
@nextui-org/file-uploadpackage detailing functionality and installation.Tests