feat: composable primitive components #947
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
I like how this is looking! I would probably add some basic styling to the examples, to at least identify the component, since right now, it kind of just looks like the page is broken, rather than there is another dropzone:
I think this is plenty in the examples, i'd potentially want to add an example
Tentatively I would say it doesn't need to be split for now, but cc @juliusmarminge -- thoughts? |
Kinda feels like it should be split? Should our default component use these primitives? A lot of code duplication if not? |
How would you want it split? I felt like the sub sections in theming wasn't bad
This is a good point, and also makes sure that they stay in sync. I don't see why we couldn't. |
I guess not, perhaps a leading section with the level of theming possible with links to respective section could be sufficient |
WalkthroughThis pull request introduces a comprehensive refactoring of the UploadThing React components, focusing on creating a more modular and flexible file upload system. The changes include developing primitive components for upload functionality, introducing new utility hooks, and updating documentation to reflect the new architecture. The refactoring aims to provide developers with more granular control over upload interfaces while maintaining a simple and intuitive API. Changes
Sequence DiagramsequenceDiagram
participant User
participant UploadRoot as UT.Root
participant UploadDropzone as UT.Dropzone
participant UploadButton as UT.Button
participant Backend
User->>UploadRoot: Initiate upload
UploadRoot->>UploadDropzone: Configure upload endpoint
User->>UploadDropzone: Drag and drop files
UploadDropzone->>UploadButton: Update upload state
UploadButton->>Backend: Upload files
Backend-->>UploadButton: Upload progress
UploadButton-->>User: Show upload status
Possibly related PRs
Suggested Labels
Suggested Reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
I'm currently refactoring the prebuilt components to use the primitives. For the dropzone, you can get the Exmaple: <Primitive.Dropzone
className={cn(
"data-[drag]:bg-blue-600/10",
className,
// isDragActive isn't defined here
styleFieldToClassName(appearance?.container, { isDragActive }),
)}
// or here
style={styleFieldToCssObject(appearance?.container, { isDragActive )}
>
{({ dropzone: { isDragActive } }) => /* we have it here */}
</Primitive.Dropzone>Should the <Primitive.Dropzone className={({ dropzone: { isDragActive }) => "your class"} />This feels a bit hacky imo as it still means you can't use <Primitive.Dropzone
dropzone={dropzone}
onDropzoneChange={setDropzone}
className={`${dropzone.isDragActive ? "bg-blue-500/10" : ""`}
/> |
|
We hope to be able to get this in a minor release, meaning no breaking changes so Can we not just wrap in another div or something? <Primitive.Dropzone>
{({ dropzone: { isDragActive } }) => (
<div
className={cn(
"data-[drag]:bg-blue-600/10",
className,
// isDragActive isn't defined here
styleFieldToClassName(appearance?.container, { isDragActive }),
)}
// or here
style={styleFieldToCssObject(appearance?.container, { isDragActive )}
>
...
</div>
</Primitive.Dropzone> |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Sorry this took so long. I've fixed all the merge conflicts, but I'm not very experienced with documentation and lack the time at the moment so I've added some extremely basic examples for the docs. Except from that the PR is ready for review. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (7)
examples/with-clerk-pagesdir/src/pages/index.tsx (1)
Line range hint
13-76: Remove duplicate upload implementations.The example currently shows both old and new upload implementations. This could confuse users about which approach to follow. Consider:
- Removing the older
UploadButtonandUploadDropzonecomponents- Adding comments explaining the composable nature of the new implementation
- Adding links to documentation for the new components
examples/minimal-appdir/src/app/page.tsx (1)
84-93: Consider extracting styles to a design system.While the inline styles work for a minimal example, consider extracting them to a design system or CSS module for better maintainability in a real application.
+// styles/dropzone.module.css +.dropzone { + margin-top: 24px; +} + +.container { + border-width: 2px; + border-style: dashed; + padding: 16px; +} + +.container[data-active="true"] { + border-color: #2563f5; +} + +.container[data-active="false"] { + border-color: #11182725; +}packages/react/src/components/primitive/root.tsx (2)
31-71: Enhance type safety and documentation for PrimitiveContextValuesConsider these improvements to the type definitions:
- Define state as a constant union type for better type safety:
- state: "readying" | "ready" | "uploading" | "disabled"; + type UploadState = "readying" | "ready" | "uploading" | "disabled"; + state: UploadState;
- Add JSDoc for the dropzone property:
/** * @remarks This will be only defined when nested in a <Dropzone> + * @property isDragActive - Indicates whether a file is being dragged over the dropzone */ dropzone?: { isDragActive: boolean; };
155-163: Add comprehensive documentation for RootPrimitiveComponentPropsThe TODO comment indicates missing documentation. Consider adding the following JSDoc:
+/** + * Props for the Root primitive component + * @template TRouter - The file router type + * @template TEndpoint - The endpoint key type + * @property {PrimitiveComponentChildren} children - Render function or React nodes + * @property {File[]} files - Controlled files state + * @property {(_: File[]) => void} onFilesChange - Callback for files state changes + */ export type RootPrimitiveComponentProps< TRouter extends FileRouter, TEndpoint extends keyof TRouter, > = UploadthingComponentProps<TRouter, TEndpoint> & { - // TODO: add @see comment for docs children?: PrimitiveComponentChildren; files?: File[]; onFilesChange?: (_: File[]) => void; };docs/src/app/(docs)/concepts/theming/page.mdx (3)
518-526: Enhance setup instructions for better clarity.Consider the following improvements to make the setup instructions more robust:
- Add explicit type imports
- Use absolute paths instead of the
~alias which might not work in all setups- Add context about file placement
```ts {{ title: 'src/utils/uploadthing.ts' }} +import type { FileRouter } from "uploadthing/next"; import { generateUploadPrimitives } from "@uploadthing/react"; -import type { OurFileRouter } from "~/server/uploadthing"; +import type { OurFileRouter } from "@/server/uploadthing"; export const UT = generateUploadPrimitives<OurFileRouter>();Add a note explaining where this file should be placed in the project structure and how it relates to the server-side configuration. --- `528-531`: **Enhance usage documentation with complete API reference.** The current overview is too brief. Consider adding: 1. A comprehensive list of all available state properties 2. TypeScript interfaces for the child function parameters Add a code block showing the TypeScript interfaces: ```ts type UploadState = "ready" | "uploading" | "error"; interface ChildFunctionProps { files: File[]; state: UploadState; dropzone?: { isDragActive: boolean; // ... other dropzone properties }; // ... document other available properties }
555-563: Enhance Button example with more features.The current example is too basic. Consider enhancing it to show:
- Error handling
- Upload progress
- File selection feedback
<UT.Root endpoint="mockRoute"> <UT.Button> - {({ state }) => state === "uploading" ? "Uploading" : "Upload file"} + {({ state, files, error, uploadProgress }) => { + if (error) return `Error: ${error.message}`; + if (state === "uploading") { + return `Uploading... ${uploadProgress}%`; + } + return files.length > 0 + ? `Upload ${files.length} files` + : "Select files"; + }} </UT.Button> + <UT.AllowedContent /> </UT.Root>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- docs/src/app/(docs)/concepts/theming/page.mdx (1 hunks)
- examples/minimal-appdir/src/app/page.tsx (2 hunks)
- examples/minimal-pagedir/src/pages/index.tsx (2 hunks)
- examples/with-clerk-appdir/src/app/page.tsx (2 hunks)
- examples/with-clerk-pagesdir/src/pages/index.tsx (2 hunks)
- packages/react/src/components/primitive/button.tsx (1 hunks)
- packages/react/src/components/primitive/clear-button.tsx (1 hunks)
- packages/react/src/components/primitive/root.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- examples/minimal-pagedir/src/pages/index.tsx
- examples/with-clerk-appdir/src/app/page.tsx
- packages/react/src/components/primitive/button.tsx
🧰 Additional context used
🪛 Biome
packages/react/src/components/primitive/clear-button.tsx
[error] 37-37: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
🔇 Additional comments (7)
packages/react/src/components/primitive/clear-button.tsx (1)
42-50: LGTM! Well-structured type definitions and exports.The type definitions provide good type safety for the polymorphic component while maintaining proper ref forwarding.
examples/with-clerk-pagesdir/src/pages/index.tsx (1)
4-4: Consider removing older component imports.Since you're introducing the new composable
UTcomponents, consider removing the olderUploadButtonandUploadDropzoneimports to avoid confusion and maintain a single pattern.examples/minimal-appdir/src/app/page.tsx (4)
7-7: LGTM: Import of primitive components.The import of
UTis correctly added alongside existing imports from the same module.
74-83: LGTM: Root component configuration.The
UT.Rootcomponent is properly configured with the endpoint and necessary callbacks, maintaining consistency with the existing implementation above.
104-107: Utilize render props for better content customization.Based on the previous discussion,
AllowedContentsupports render props for content customization. Consider utilizing this capability for better flexibility.<UT.AllowedContent as="p" style={{ fontSize: 12, width: "fit-content" }} + children={({ fileTypes }) => ( + `Allowed types: ${fileTypes.join(", ")}` + )} />
74-111: Verify coexistence with existing upload components.The new primitive components are added alongside existing
UploadButtonandUploadDropzone. Ensure they don't interfere with each other's functionality.✅ Verification successful
Based on the search results, I can now provide a final response since I have enough information to verify the coexistence of components:
No conflicts between primitive and existing upload components
The primitive components (
UT.Root,UT.Dropzone, etc.) and the existing components (UploadButton,UploadDropzone) are designed to work independently and do not share any state or event handlers that could cause interference. Each component:
- Maintains its own local state for files and upload progress
- Has its own instance of
useUploadThing/createUploadThinghooks- Handles its own lifecycle events (
onClientUploadComplete,onUploadBegin, etc.)The components can be safely used together on the same page as demonstrated in multiple example implementations (e.g., minimal-appdir, minimal-pagedir).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential conflicts between old and new components # Test: Search for any shared state or conflicting event handlers rg -A 5 'onClientUploadComplete|onUploadBegin|startUpload' # Test: Look for any shared DOM elements that might conflict ast-grep --pattern $'<(UploadButton|UploadDropzone|UT.Root)$$$'Length of output: 109022
docs/src/app/(docs)/concepts/theming/page.mdx (1)
514-517:⚠️ Potential issueRemove or update the
asprop reference.According to the PR objectives, the
asprop hasn't been implemented yet. This documentation should either remove the reference or be updated once the implementation is finalized.✅ Verification successful
Let me try a different search strategy to verify the implementation of the
asprop.
Based on the search results, I can now generate a final response. The results show that the
asprop is actually implemented in the primitive components:
- Found in
packages/react/src/components/primitive/button.tsx:{ children, onClick, as, ...props }: PrimitiveButtonProps<TTag>- Found in
packages/react/src/components/primitive/clear-button.tsx:{ children, onClick, as, ...props }: PrimitiveClearButtonProps<TTag>- Found in
packages/react/src/components/primitive/allowed-content.tsx:{ children, as, ...props }: PrimitiveAllowedContentProps<TTag>Documentation about the
asprop is accurateThe documentation correctly states that unstyled primitive components "can be assigned specific HTML tags through the
asprop". This is implemented in the codebase's primitive components, making the documentation accurate and consistent with the implementation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if the `as` prop is implemented in the codebase rg -A 5 "as: (string|React\.ElementType|ComponentType)"Length of output: 57
Script:
#!/bin/bash # Search for type definitions or interfaces that might include the `as` prop rg -g "*.ts*" -A 3 "interface.*Props|type.*Props" # Search for any usage or implementation of `as` prop in components ast-grep --pattern 'const $_ = $$$as$$$' # Search for any direct references to `as` prop in the codebase rg "\bas\b.*prop"Length of output: 21761
| }} | ||
| ref={ref} | ||
| > | ||
| <PrimitiveSlot default="Clear" children={children} /> |
There was a problem hiding this comment.
Use JSX children pattern instead of children prop.
The children prop should be passed using JSX syntax rather than as a prop to follow React conventions.
-<PrimitiveSlot default="Clear" children={children} />
+<PrimitiveSlot default="Clear">{children}</PrimitiveSlot>📝 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.
| <PrimitiveSlot default="Clear" children={children} /> | |
| <PrimitiveSlot default="Clear">{children}</PrimitiveSlot> |
🧰 Tools
🪛 Biome
[error] 37-37: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
| onClick={(e) => { | ||
| if (state === "disabled") return; | ||
| onClick?.(e); | ||
| setFiles([]); | ||
| }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add basic error handling for setFiles.
Consider adding basic error handling when clearing files to gracefully handle potential failures.
onClick={(e) => {
if (state === "disabled") return;
onClick?.(e);
- setFiles([]);
+ try {
+ setFiles([]);
+ } catch (error) {
+ console.error("Failed to clear files:", error);
+ }
}}📝 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.
| onClick={(e) => { | |
| if (state === "disabled") return; | |
| onClick?.(e); | |
| setFiles([]); | |
| }} | |
| onClick={(e) => { | |
| if (state === "disabled") return; | |
| onClick?.(e); | |
| try { | |
| setFiles([]); | |
| } catch (error) { | |
| console.error("Failed to clear files:", error); | |
| } | |
| }} |
| <UT.Button as="button"> | ||
| {state === "uploading" ? "Uploading" : "Upload file"} | ||
| </UT.Button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance button loading state feedback.
The button's state indication could be more informative. Consider adding a loading spinner and disabled state during upload.
<UT.Button as="button">
- {state === "uploading" ? "Uploading" : "Upload file"}
+ {state === "uploading" ? (
+ <>
+ <LoadingSpinner className="mr-2" />
+ <span>Uploading...</span>
+ </>
+ ) : (
+ "Upload file"
+ )}
</UT.Button>Committable suggestion was skipped due to low confidence.
| const state = (() => { | ||
| if (props.__internal_state) return props.__internal_state; | ||
|
|
||
| const ready = | ||
| props.__internal_ready ?? | ||
| (props.__internal_state === "ready" || fileTypes.length > 0); | ||
|
|
||
| if (fileTypes.length === 0 || !!props.disabled) return "disabled"; | ||
| if (!ready) return "readying"; | ||
| if (ready && !isUploading) return "ready"; | ||
| return "uploading"; | ||
| })(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify state computation logic
The state computation can be simplified using early returns:
const state = (() => {
if (props.__internal_state) return props.__internal_state;
+ if (fileTypes.length === 0 || props.disabled) return "disabled";
const ready =
props.__internal_ready ??
(props.__internal_state === "ready" || fileTypes.length > 0);
- if (fileTypes.length === 0 || !!props.disabled) return "disabled";
if (!ready) return "readying";
- if (ready && !isUploading) return "ready";
- return "uploading";
+ return isUploading ? "uploading" : "ready";
})();📝 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 state = (() => { | |
| if (props.__internal_state) return props.__internal_state; | |
| const ready = | |
| props.__internal_ready ?? | |
| (props.__internal_state === "ready" || fileTypes.length > 0); | |
| if (fileTypes.length === 0 || !!props.disabled) return "disabled"; | |
| if (!ready) return "readying"; | |
| if (ready && !isUploading) return "ready"; | |
| return "uploading"; | |
| })(); | |
| const state = (() => { | |
| if (props.__internal_state) return props.__internal_state; | |
| if (fileTypes.length === 0 || props.disabled) return "disabled"; | |
| const ready = | |
| props.__internal_ready ?? | |
| (props.__internal_state === "ready" || fileTypes.length > 0); | |
| if (!ready) return "readying"; | |
| return isUploading ? "uploading" : "ready"; | |
| })(); |
| startUpload(files, fileRouteInput).catch((e) => { | ||
| if (e instanceof UploadAbortedError) { | ||
| void onUploadAborted?.(); | ||
| } else { | ||
| throw e; | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in uploadFiles
Consider adding specific error handling for common upload scenarios:
startUpload(files, fileRouteInput).catch((e) => {
if (e instanceof UploadAbortedError) {
void onUploadAborted?.();
+ } else if (e instanceof TypeError) {
+ // Handle network errors
+ throw new Error(`Upload failed due to network error: ${e.message}`);
+ } else if (e.name === "QuotaExceededError") {
+ // Handle storage quota errors
+ throw new Error(`Upload failed due to storage quota: ${e.message}`);
} else {
throw e;
}
});Committable suggestion was skipped due to low confidence.
| export function PrimitiveSlot({ | ||
| children, | ||
| componentName, | ||
| default: defaultChildren, | ||
| }: { | ||
| children: PrimitiveComponentChildren; | ||
| componentName?: string; | ||
| default?: React.ReactNode; | ||
| }) { | ||
| if (!children) return defaultChildren; | ||
| return typeof children === "function" | ||
| ? children?.(usePrimitiveValues(componentName)) | ||
| : children; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize PrimitiveSlot component with memoization
The component re-renders on every parent update. Consider using useMemo for the function result:
export function PrimitiveSlot({
children,
componentName,
default: defaultChildren,
}: {
children: PrimitiveComponentChildren;
componentName?: string;
default?: React.ReactNode;
}) {
if (!children) return defaultChildren;
- return typeof children === "function"
+ return useMemo(() => (typeof children === "function"
? children?.(usePrimitiveValues(componentName))
- : children;
+ : children), [children, componentName]);
}Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/react/src/components/index.tsx (1)
63-81: Add JSDoc documentation for the new function.Consider adding JSDoc documentation to explain the purpose, usage, and return value of
generateUploadPrimitives. This would help developers understand how to use the composable primitive components effectively.Example documentation:
/** * Generates unstyled composable upload components that can be customized. * @param opts - Optional configuration options for the upload components * @returns An object containing primitive components: Root, Dropzone, Button, and AllowedContent * @example * const UT = generateUploadPrimitives(); * * function Upload() { * return ( * <UT.Root endpoint="imageUploader"> * <UT.Dropzone> * {({ isUploading }) => ( * <UT.Button>Upload a file</UT.Button> * )} * </UT.Dropzone> * </UT.Root> * ); * } */docs/src/app/(docs)/concepts/theming/page.mdx (2)
22-23: Fix grammatical issues in the introduction.There are a couple of grammatical issues that should be addressed:
- "These comes" should be "These come" (subject-verb agreement)
- Add a comma before "but" as it connects two independent clauses
-These comes with behavior built-in but you have full control over what's rendered when and where. +These come with behavior built-in, but you have full control over what's rendered when and where.🧰 Tools
🪛 LanguageTool
[grammar] ~22-~22: The verb ‘comes’ is singular. Did you mean: “this comes” or “These come”?
Context: ...tives](#unstyled-primitive-components). These comes with behavior built-in but you have ful...(SINGULAR_VERB_AFTER_THESE_OR_THOSE)
[uncategorized] ~23-~23: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...nts). These comes with behavior built-in but you have full control over what's rende...(COMMA_COMPOUND_SENTENCE)
527-612: Consider enhancing the Unstyled Primitive Components documentation.While the current documentation provides a good foundation, consider adding:
- A complete list of available state properties that can be accessed in children functions
- Examples of manual mode usage for both Button and Dropzone
- Examples of using the ClearButton component
- TypeScript examples showing proper type inference
Would you like me to help generate these additional documentation sections?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- docs/src/app/(docs)/concepts/theming/page.mdx (2 hunks)
- packages/react/src/components/index.tsx (2 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/src/app/(docs)/concepts/theming/page.mdx
[grammar] ~22-~22: The verb ‘comes’ is singular. Did you mean: “this comes” or “These come”?
Context: ...tives](#unstyled-primitive-components). These comes with behavior built-in but you have ful...(SINGULAR_VERB_AFTER_THESE_OR_THOSE)
[uncategorized] ~23-~23: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...nts). These comes with behavior built-in but you have full control over what's rende...(COMMA_COMPOUND_SENTENCE)
[style] ~27-~27: Opting for a less wordy alternative here can improve the clarity of your writing.
Context: ...e-upload-thing) hook that allows you to not only customize the look but also have full control of the behavior. ## Uploa...(NOT_ONLY_ALSO)
🔇 Additional comments (3)
packages/react/src/components/index.tsx (2)
17-18: LGTM! Clean import statements for new primitive components.The imports are well-structured and follow TypeScript best practices.
63-81: Verify the primitive components implementation.Let's ensure all the primitive components mentioned in the PR objectives are properly implemented.
✅ Verification successful
All primitive components are properly implemented
The verification confirms that all required primitive components are properly implemented in the codebase:
Rootcomponent (root.tsx): Implements the core upload functionality, context provider, and state managementDropzonecomponent (dropzone.tsx): Implements drag-and-drop functionality with proper file validationButtoncomponent (button.tsx): Implements file selection and upload trigger functionalityAllowedContentcomponent (allowed-content.tsx): Implements content display for allowed file typesEach component follows best practices with proper TypeScript typing, client-side directive, and accessibility considerations. The implementation matches the PR objectives and maintains consistency across the component suite.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and implementation of primitive components # Test 1: Check if all primitive components exist echo "Checking for primitive component files..." fd --type f --glob "*.tsx" --glob "*.ts" . packages/react/src/components/primitive # Test 2: Verify the exports from primitive/index echo -e "\nChecking primitive exports..." rg --type ts "export.*from" packages/react/src/components/primitive/index.ts # Test 3: Look for component implementations echo -e "\nChecking component implementations..." for component in Root Dropzone Button AllowedContent; do echo -e "\nLooking for $component implementation..." ast-grep --pattern "export function $component" --pattern "export const $component" doneLength of output: 8324
Script:
#!/bin/bash # Let's verify each primitive component individually # Check Root component implementation echo "Checking Root component..." ast-grep --pattern 'export const Root = ' echo -e "\nChecking Dropzone component..." ast-grep --pattern 'export const Dropzone = ' echo -e "\nChecking Button component..." ast-grep --pattern 'export const Button = ' echo -e "\nChecking AllowedContent component..." ast-grep --pattern 'export const AllowedContent = ' # Check exports from primitive/index.ts echo -e "\nChecking primitive/index.ts contents..." cat packages/react/src/components/primitive/index.ts # Check individual component files echo -e "\nChecking individual component implementations..." for file in root.tsx dropzone.tsx button.tsx allowed-content.tsx; do echo -e "\nContents of packages/react/src/components/primitive/$file:" cat "packages/react/src/components/primitive/$file" doneLength of output: 24707
docs/src/app/(docs)/concepts/theming/page.mdx (1)
585-602: Update Dropzone example for accuracy and completeness.The example needs several improvements:
- Remove
asprop usage since it's not implemented yet (as mentioned in PR objectives)- Add error handling example
- Demonstrate more state properties
<UT.Root endpoint="mockRoute"> <UT.Dropzone> - {({ state }) => ( + {({ state, files, error }) => ( <div> <p>Drag and drop</p> + {error && <p style={{ color: 'red' }}>{error.message}</p>} + {files.length > 0 && ( + <p>Selected files: {files.map(f => f.name).join(', ')}</p> + )} - <UT.Button as="button"> + <UT.Button> {state === "uploading" ? "Uploading" : "Upload file"} </UT.Button> <UT.AllowedContent - as="p" style={{ fontSize: 12, width: "fit-content" }} /> </div> )} </UT.Dropzone> </UT.Root>
|
Just played with this a bit and I love this - found some issues with pasting files. I'll try and have a look at fixing that when i got some time |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (14)
playground/lib/uploadthing.ts (2)
9-11: Add JSDoc comments to document the exported entitiesThe exports look well-structured and type-safe, but they would benefit from JSDoc comments explaining their purpose and usage patterns, especially since these are foundational primitives.
Here's a suggested improvement:
+/** + * Primitive components for building custom upload UI + * @example + * const MyUploader = () => ( + * <UT.Root> + * <UT.Dropzone> + * {({ isUploading }) => <div>{isUploading ? 'Uploading...' : 'Drop files'}</div>} + * </UT.Dropzone> + * </UT.Root> + * ); + */ export const UT = generateUploadPrimitives<UploadRouter>(); + +/** Pre-built button component for quick implementation */ export const UTButton = generateUploadButton<UploadRouter>(); + +/** Hook for custom upload implementations */ export const { useUploadThing } = generateReactHelpers<UploadRouter>();
9-9: Consider a more explicit name for primitive componentsWhile
UTis concise, a more explicit name likeUploadPrimitivesmight better convey its purpose, especially for new developers encountering the codebase.playground/app/primitives/page.tsx (1)
26-38: Consider extracting the dropzone UI into a separate componentThe inline JSX within the dropzone render prop is becoming complex. Consider extracting it into a separate component for better maintainability and reusability.
type DropzoneContentProps = { dropzone: { isDragActive: boolean }; state: string; uploadProgress: number; }; function DropzoneContent({ dropzone, state, uploadProgress }: DropzoneContentProps) { return ( <div data-dragactive={dropzone?.isDragActive} className="flex flex-col items-center rounded-md border-2 border-dashed border-zinc-200 p-4 data-[dragactive=true]:border-zinc-400" > <Subheading>Drag and drop</Subheading> <UT.Button as={Button} color="red" className="mt-2 px-4 py-2"> {state === "uploading" ? "Uploading" : "Upload file"} {!!uploadProgress && ` ${uploadProgress}%`} </UT.Button> <UT.AllowedContent as={Code} className="mt-2" /> </div> ); }playground/components/text.tsx (5)
1-6: LGTM! Consider adding readonly modifier for better type safety.The imports and type definitions are well-structured. For additional type safety, consider making the level union type readonly:
type HeadingProps = { - level?: 1 | 2 | 3 | 4 | 5 | 6; + level?: readonly [1, 2, 3, 4, 5, 6][number]; }
8-20: LGTM! Consider adding aria-level for accessibility.The component implementation is solid with proper type safety and responsive design.
<Element {...props} + aria-level={level} className={cx( "text-2xl/8 font-semibold text-zinc-950 sm:text-xl/8", className, )} />
36-44: LGTM! Document the purpose of data-slot attribute.The implementation is clean and follows best practices. Consider adding JSDoc to explain the purpose of the data-slot attribute.
+/** + * Text component that renders a paragraph with consistent styling. + * @param data-slot - Used for component targeting in the design system + */ export function Text({ className, ...props }: React.ComponentProps<"p">) {
46-59: Consider using :hover pseudo-class for better accessibility.The component correctly wraps Next.js Link, but the data-hover approach might not work with all input devices.
<Link {...props} className={cx( - "text-zinc-950 underline decoration-zinc-950/50 data-[hover]:decoration-zinc-950", + "text-zinc-950 underline decoration-zinc-950/50 hover:decoration-zinc-950 focus-visible:decoration-zinc-950", className, )} />
73-83: Consider adding semantic improvements for code elements.The styling provides good visual distinction, but could benefit from additional semantic attributes.
<code {...props} + translate="no" + role="code" className={cx( "rounded border border-zinc-950/10 bg-zinc-950/[2.5%] px-0.5 text-sm font-medium text-zinc-950 sm:text-[0.8125rem]", className, )} />The
translate="no"attribute prevents translation tools from modifying code content, androle="code"improves semantic meaning for screen readers.playground/components/uploader.tsx (4)
60-61: Consider adding endpoint validationThe endpoint callback is using a generic
anythingtype without validation. Consider adding type safety and validation for the endpoint configuration.-endpoint={(rr) => rr.anything} -input={{}} +endpoint={(rr) => { + if (!rr.anything) { + throw new Error("Endpoint configuration is invalid"); + } + return rr.anything; +}} +input={{ /* Add input validation schema here */ }}
73-76: Review CSS specificity overridesThe appearance props use
!to override styles. This might make it harder to maintain and customize styles in the future.Consider using a more maintainable approach:
-appearance={{ - button: "!text-sm/6", - allowedContent: "!h-0", -}} +appearance={{ + button: "text-sm leading-6", + allowedContent: "h-0", +}}
62-67: Enhance error handling and success feedbackThe current implementation uses basic window.alert for errors and only refreshes the page on success. Consider implementing a more user-friendly feedback mechanism.
onUploadError={(error) => { - window.alert(error.message); + // Consider using a toast notification or error boundary + console.error('Upload failed:', error); + // Add user-friendly error display }} onClientUploadComplete={() => { + // Consider adding success feedback before refresh router.refresh(); }}
55-80: Consider extracting upload configurationThe component mixes presentation with upload configuration. Consider extracting the upload configuration into a separate configuration object or custom hook for better maintainability.
Example approach:
// useUploadConfig.ts export const useUploadConfig = () => { const router = useRouter(); return { endpoint: (rr) => rr.anything, onError: (error) => { /* error handling */ }, onSuccess: () => router.refresh(), // ... other config }; }; // In component: const uploadConfig = useUploadConfig(); <UTButton {...uploadConfig} />playground/lib/actions.ts (1)
84-86: Consider extracting the expiration time constant and documenting the duration.While adding URL expiration is a good security practice, the magic number calculation could be more readable and maintainable.
Consider applying this change:
- const { url } = await utapi.getSignedURL(key, { - expiresIn: 60 * 60 * 24 * 7, - }); + // URL expires in 7 days + const SIGNED_URL_EXPIRATION = 60 * 60 * 24 * 7; // seconds + const { url } = await utapi.getSignedURL(key, { + expiresIn: SIGNED_URL_EXPIRATION, + });playground/app/layout.tsx (1)
70-73: Enhance navigation accessibility and user experience.Consider the following improvements:
- Add
aria-labelto the nav element for better screen reader support- Add visual indication for the current active route
- Include hover states for links
- <nav className="flex gap-4 bg-zinc-100 p-4"> + <nav + aria-label="Main navigation" + className="flex gap-4 bg-zinc-100 p-4" + > - <Link href="/">Home</Link> - <Link href="/primitives">Primitives</Link> + <Link + href="/" + className="hover:underline" + > + Home + </Link> + <Link + href="/primitives" + className="hover:underline" + > + Primitives + </Link> </nav>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
package.json(1 hunks)playground/app/layout.tsx(2 hunks)playground/app/primitives/page.tsx(1 hunks)playground/components/text.tsx(1 hunks)playground/components/uploader.tsx(2 hunks)playground/lib/actions.ts(1 hunks)playground/lib/uploadthing.ts(1 hunks)
🔇 Additional comments (13)
playground/lib/uploadthing.ts (2)
9-11: LGTM! Type-safe implementation
The consistent use of the UploadRouter type parameter ensures type safety across all exported entities. This will provide excellent TypeScript support for consumers of these primitives.
1-7: Consider making the import path more resilient
The relative import path might be brittle if the file structure changes. Consider using path aliases (e.g., @/app/api/uploadthing/route) to make imports more maintainable.
Let's check for potential circular dependencies and verify the route file location:
playground/app/primitives/page.tsx (3)
1-7: LGTM: Appropriate setup for client-side component
The imports and client-side directive are correctly configured for an interactive file upload component.
44-50: LGTM: Clean and focused page component
The Page component follows the single responsibility principle and provides appropriate layout spacing.
11-12: Consider restricting the upload endpoint and configuring input parameters
The current endpoint configuration allows "anything" which might be too permissive. Consider restricting to specific file types or sizes based on your requirements.
playground/components/text.tsx (1)
61-71: Verify text-foreground class in Tailwind configuration.
The implementation is clean, but let's ensure the text-foreground utility class is properly configured.
✅ Verification successful
The output shows that foreground is properly defined in the Tailwind configuration as "hsl(var(--foreground))", which means the text-foreground utility class is correctly configured and available for use in the Strong component.
The text-foreground utility class is properly configured in the Tailwind theme.
The class is defined using CSS variables and HSL color format, following a consistent pattern used across other color utilities in the theme configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if text-foreground is defined in Tailwind config
fd -e js -e ts -e json -g "*tailwind*config*" | xargs cat | grep -A 5 "foreground"
Length of output: 1125
package.json (1)
29-29: LGTM! Script addition follows established patterns.
The new dev:playground script follows the existing pattern of development scripts and will help in testing the new primitive components.
Let's verify the playground setup:
✅ Verification successful
The playground setup is properly configured and integrated
The verification confirms:
- The playground directory exists and contains the expected Next.js files (layout.tsx and page.tsx)
- The new primitive components are properly imported and used in the playground, specifically in
playground/app/primitives/page.tsx - The setup aligns with other examples in the repository and correctly implements the new UT components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the next-playground directory exists and contains necessary files
# Check if next-playground directory exists in examples
fd -t d "next-playground" examples/
# Check for typical Next.js files in the playground
fd -t f "^(page|layout)\.(tsx|js|jsx)$" -E "*.test.*" -E "*.spec.*"
# Check if the new primitive components are imported
rg -l "generateUploadPrimitives|UT\.(Root|Dropzone|Button|AllowedContent)"
Length of output: 1881
playground/components/uploader.tsx (2)
68-72: LGTM: Composable content implementation
The content prop implementation using render props pattern aligns well with the PR objectives for composable components. This allows for flexible customization while maintaining type safety.
7-7: Verify the local import path for UTButton
The import has been moved from @uploadthing/react to a local path. Let's ensure this path is correct and the component exists.
✅ Verification successful
Import path for UTButton is correct and verified
The UTButton component is correctly exported from playground/lib/uploadthing.ts, and the relative import path "../lib/uploadthing" in playground/components/uploader.tsx is accurate.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the UTButton export exists in the local path
rg -l "export.*UTButton" "lib/uploadthing"
Length of output: 98
Script:
#!/bin/bash
# Let's first find the correct path by searching for files containing "UTButton"
rg -l "UTButton"
# Also search for any file named "uploadthing" to verify the correct path
fd uploadthing
Length of output: 3341
Script:
#!/bin/bash
# Let's check the content of playground/lib/uploadthing.ts to verify UTButton export
rg "UTButton" "playground/lib/uploadthing.ts" -C 2
Length of output: 243
playground/lib/actions.ts (2)
84-86: Verify the URL expiration time requirements.
The 7-day expiration time for signed URLs should be validated against:
- Application requirements for URL validity
- Security policies regarding temporary access
- User experience expectations
Consider making this duration configurable through environment variables if different environments need different expiration times.
Let's check if this duration is documented or configurable elsewhere:
✅ Verification successful
7-day expiration time is within the documented limits and follows library constraints
The expiration time of 7 days (60 * 60 * 24 * 7 seconds) is:
- Explicitly enforced by the library which throws an error if expiration exceeds 7 days
- Well documented in the API reference
- Configurable through the
expiresInparameter which accepts both numeric seconds or human-readable time strings - Matches the maximum allowed duration (604800 seconds) as per the library's validation
The implementation is using the maximum allowed duration, which is a reasonable default for temporary access URLs. If different durations are needed, they can be configured per request as documented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any documentation or configuration related to URL expiration
rg -i "url.*expir|signed.*url" --type md
rg "process\.env.*EXPIR|config.*EXPIR"
Length of output: 290
Script:
#!/bin/bash
# Let's check for any mentions of URL expiration or signed URLs in the codebase
rg -A 5 "getSignedURL|signedUrl|expiresIn"
# Also check for any configuration or constant definitions
ast-grep --pattern 'const $_ = $$$* 60 * 60 * 24'
Length of output: 19204
84-86: Verify error handling for expired URLs.
Ensure that components consuming these signed URLs handle expiration gracefully. The application should:
- Detect expired URLs
- Provide appropriate user feedback
- Allow easy URL refresh when needed
Let's check how URL expiration errors are handled:
✅ Verification successful
URL expiration is properly handled in the application
The codebase already has appropriate error handling mechanisms in place:
- The uploader component has error handling that displays alerts to users when issues occur (
onUploadError) - The actions have proper error states and return structured error responses
- The URL expiration time is set to a reasonable 7 days, which is within the allowed limits as verified by the test cases
- Components using these URLs have error states and user feedback mechanisms
The implementation follows best practices for handling URL-related errors, including expired URLs, through the existing error handling infrastructure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling related to URL expiration
ast-grep --pattern 'catch ($_) {
$$$
}'
# Search for error messages related to URL expiration
rg -i "url.*expired|link.*expired"
Length of output: 80
Script:
#!/bin/bash
# Let's search for any error handling around the getSignedURL usage
rg -A 5 "getSignedURL"
# Also check for any error boundaries or error handling in components that might use these URLs
rg -A 5 "catch|throw" --type typescript
# Search for any error states or loading states in components
ast-grep --pattern 'const [$_] = useState<$_>($$$)'
Length of output: 6974
Script:
#!/bin/bash
# Let's check for error handling in the specific file and its imports
rg -A 5 "catch|throw" playground/lib/actions.ts
# Check for any error states or loading states in components that might use this action
rg -A 5 "actions.ts" playground/
# Look for any error handling in components
rg -A 5 "error|Error" playground/
Length of output: 4241
playground/app/layout.tsx (2)
51-61: Well-implemented loading state!
The Suspense fallback implementation follows best practices by:
- Maintaining layout stability
- Providing clear visual feedback
- Matching the structure of the loaded content
66-68: Review security implications of displaying token information.
Exposing the appId and region information in the UI might not be necessary in a production environment. Consider:
- Moving this information to environment variables
- Only showing it in development mode
- Or removing it entirely if not needed for debugging
|
Thanks! In the meantime, should I fix the linting issues? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
packages/react/src/components/primitive/clear-button.tsx (1)
30-35: Enhance error handling and user experience.Consider adding error handling and user feedback when clearing files:
onClick={(e) => { if (state === "disabled") return; // eslint-disable-next-line @typescript-eslint/no-unsafe-call onClick?.(e); - setFiles([]); + try { + setFiles([]); + } catch (error) { + console.error("Failed to clear files:", error); + // Consider adding an onError prop to handle errors + } }}packages/react/src/index.ts (1)
7-7: Add usage documentation forgenerateUploadPrimitives.This new export looks good, aligning well with the composable approach outlined in the PR. However, it's unclear how consumers should utilize this function. Consider adding (or referencing) documentation to explain how it's intended to be used, along with any patterns or best practices you recommend.
Would you like help drafting minimal usage docs or code examples to guide users?
packages/react/src/components/primitive/allowed-content.tsx (1)
20-22: Consider using a generic ref type to align with theaspropCurrently, the ref type is fixed to
HTMLDivElement, but the component allows rendering different element types via theasprop. To ensure accurate type inference, switch to a more generic type:- ref: Ref<HTMLDivElement>, + ref: Ref<React.ElementRef<TTag>>,packages/react/src/components/primitive/root.tsx (2)
230-237: Simplify state determination logicThe inline function returning the
statecan be simplified while retaining clarity. Consider early returns or consolidated checks for improved readability:
239-258: Refactor paste handling for better maintainabilityThe inline paste handling is beneficial, but extracting it into a dedicated custom hook (e.g.,
usePasteHandler) can enhance clarity and potential reuse.packages/react/src/components/primitive/dropzone.tsx (2)
206-207: Remove or reconsiderevent.persist()usageFrom React v17 onwards, the synthetic event pool has been removed, making
event.persist()largely unnecessary. Verify if you still need it, or remove for cleaner code.
371-375: Evaluate continued support for IE/Edge checkIE11 and legacy Edge are deprecated. If your project no longer supports them, remove the
isIeOrEdge()check and simplify the file dialog opening logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/react/src/components/index.tsx(2 hunks)packages/react/src/components/primitive/allowed-content.tsx(1 hunks)packages/react/src/components/primitive/button.tsx(1 hunks)packages/react/src/components/primitive/clear-button.tsx(1 hunks)packages/react/src/components/primitive/dropzone.tsx(1 hunks)packages/react/src/components/primitive/root.tsx(1 hunks)packages/react/src/index.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react/src/components/index.tsx
- packages/react/src/components/primitive/button.tsx
🧰 Additional context used
📓 Learnings (1)
packages/react/src/components/primitive/allowed-content.tsx (1)
Learnt from: veloii
PR: pingdotgg/uploadthing#947
File: packages/react/src/components/primitive/allowed-content.tsx:29-32
Timestamp: 2024-11-12T10:36:58.532Z
Learning: For slot components, sometimes it's preferred to manually specify `children` via props to avoid the appearance of nested elements when they aren't actually nested.
🪛 Biome (1.9.4)
packages/react/src/components/primitive/clear-button.tsx
[error] 38-38: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
packages/react/src/components/primitive/allowed-content.tsx
[error] 31-31: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
🔇 Additional comments (6)
packages/react/src/components/primitive/clear-button.tsx (3)
43-50: LGTM! Well-structured type definitions.The type definitions and component export are well-implemented, providing good type safety and proper display name support.
38-38: 🛠️ Refactor suggestionUse JSX children pattern instead of children prop.
The static analysis tool correctly flags that children should be passed using JSX syntax rather than as a prop.
-<PrimitiveSlot default="Clear" children={children} /> +<PrimitiveSlot default="Clear">{children}</PrimitiveSlot>Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 38-38: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
9-9: 🛠️ Refactor suggestionConsider using 'button' as the default tag instead of 'label'.
The current implementation uses 'label' as the default tag for the clear button. While this works, it might be more semantically correct and accessible to use a 'button' element by default, as labels are typically used to describe form controls rather than trigger actions.
-const DEFAULT_CLEAR_BUTTON_TAG = "label"; +const DEFAULT_CLEAR_BUTTON_TAG = "button" as const;Likely invalid or redundant comment.
packages/react/src/index.ts (1)
15-15: Verify consistency of path changes foruseDropzone.You've updated the import path to
./components/primitive/dropzone. Ensure that any references to the old path ("./components/dropzone") are removed or updated throughout the codebase.Below is a shell script you can run to find any outdated references:
✅ Verification successful
Let me verify if there are any other potential references to the dropzone component or its path.
Let me try a more comprehensive search without file type restrictions.
No outdated references to old dropzone path found
Based on the comprehensive search results, there are no references to the old path
"./components/dropzone"in the codebase. The only import ofuseDropzonefound is in the documentation example which uses the package import@uploadthing/react. The path change inpackages/react/src/index.tsto"./components/primitive/dropzone"appears to be consistent with the codebase structure.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for all imports of useDropzone from old or new paths rg 'import\s*\{?\s*useDropzone\s*\}?\s*from\s*"[^"]+"'Length of output: 155
Script:
#!/bin/bash # Search for any mentions of dropzone path or component rg -i "dropzone" --type ts --type tsxLength of output: 65
Script:
#!/bin/bash # Search for any mentions of dropzone in the codebase rg -i "dropzone"Length of output: 45781
packages/react/src/components/primitive/allowed-content.tsx (1)
29-32: Avoid passingchildrenvia prop ()Passing
childrenvia thechildrenprop can be seen as an anti-pattern in React. While you have a valid reason (avoiding nested DOM nodes), consider the standard pattern of passing children as JSX elements for consistency.🧰 Tools
🪛 Biome (1.9.4)
[error] 31-31: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
packages/react/src/components/primitive/root.tsx (1)
189-189: Ensure compatibility ofglobalThis.fetchYou are using
globalThis.fetchto make upload requests. If the library is meant to be used in older browsers or non-browser environments, confirm the availability ofglobalThis.fetchor provide a fallback.
| onDrop: onDropCb as any, | ||
| } | ||
| : {}), | ||
| }), | ||
| [ | ||
| disabled, | ||
| onBlur, | ||
| onClick, | ||
| onDragEnter, | ||
| onDragLeave, | ||
| onDragOver, | ||
| onDropCb, | ||
| onFocus, | ||
| onKeyDown, | ||
| ], | ||
| ); | ||
|
|
||
| const getInputProps = useMemo( | ||
| () => (): HTMLProps<HTMLInputElement> => ({ | ||
| ref: inputRef, | ||
| type: "file", | ||
| style: { display: "none" }, | ||
| accept: acceptAttr, | ||
| multiple, | ||
| tabIndex: -1, | ||
| ...(!disabled | ||
| ? { | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | ||
| onChange: onDropCb as any, |
There was a problem hiding this comment.
Eliminate as any casts in event handlers
Casting to any bypasses type safety. Instead, match the event signatures by adjusting the function’s argument type definition or refining TypeScript types.
This PR adds unstyled composable components through
generateUploadPrimitiveswhich returns:<Root><Dropzone><Button><AllowedContent>I've also added examples for
minimal-appdir,minimal-pagedir,with-clerk-appdir,with-clerk-pagedir. I can add it to all the rest of the react ones if needed.All these components accept parameters as children like
({ isUploading }) => <></>.There is no
asChildoras={YourComponent}prop at the moment as I'm not sure which one to use.I haven't implemented the docs either as I'm not sure if the current styling page should be split up.
Summary by CodeRabbit
Release Notes: UploadThing v6.0.0
New Features
generateUploadPrimitives()for creating flexible upload interfacesImprovements
Breaking Changes