-
Notifications
You must be signed in to change notification settings - Fork 392
feat: onFileUploadComplete (#1175) #1177
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: juliusmarminge <[email protected]>
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThis pull request introduces an Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UploadButton
participant UploadHook as useUploadThingInternal
participant UploadModule as upload-browser.ts
User->>UploadButton: Initiates file upload
UploadButton->>UploadHook: startUpload(config with onFileUploadComplete)
UploadHook->>UploadModule: uploadFiles(opts with onFileUploadComplete)
UploadModule->>UploadModule: Process file upload
UploadModule->>UploadModule: Invoke onFileUploadComplete callback
UploadModule-->>UploadHook: Return upload result
UploadHook-->>UploadButton: Complete upload flow with callback execution
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
Scope: all 3 workspace projects This error happened while installing a direct dependency of /tmp/eslint/packages/uploadthing Packages found in the workspace: ✨ Finishing Touches
🪧 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 (
|
More templates
commit: |
A new canary is available for testing. You can install this latest build in your project with: pnpm add @uploadthing/[email protected]
pnpm add @uploadthing/[email protected]
pnpm add @uploadthing/[email protected]
pnpm add @uploadthing/[email protected]
pnpm add @uploadthing/[email protected]
pnpm add @uploadthing/[email protected]
pnpm add @uploadthing/[email protected]
pnpm add [email protected]
pnpm add @uploadthing/[email protected] |
📦 Bundle size comparison
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/uploadthing/src/_internal/upload-browser.ts (1)
111-112
: LGTM! Consider adding JSDoc for the new callback.The addition of the
onFileUploadComplete
callback and the simplified type definition foronUploadProgress
are well-implemented. For improved developer experience, consider adding a JSDoc comment that describes the purpose and timing of the new callback.opts: { + /** + * Callback that is triggered when a file upload completes successfully + * @param data The uploaded file data including server response + */ onFileUploadComplete?: (_: ClientUploadedFileData<TServerOutput>) => void; onUploadProgress?: (_: { loaded: number; delta: number }) => void; },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/react/src/components/button.tsx
(2 hunks)packages/react/src/types.ts
(1 hunks)packages/react/src/use-uploadthing.ts
(1 hunks)packages/uploadthing/src/_internal/upload-browser.ts
(3 hunks)packages/uploadthing/src/types.ts
(2 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
packages/uploadthing/src/types.ts (1)
packages/uploadthing/src/_internal/shared-schemas.ts (1)
ClientUploadedFileData
(96-101)
packages/react/src/types.ts (1)
packages/uploadthing/src/types.ts (1)
ClientUploadedFileData
(25-25)
packages/uploadthing/src/_internal/upload-browser.ts (2)
packages/uploadthing/src/types.ts (1)
ClientUploadedFileData
(25-25)packages/uploadthing/src/_internal/shared-schemas.ts (1)
ClientUploadedFileData
(96-101)
🔇 Additional comments (8)
packages/react/src/types.ts (1)
56-74
: Well-structured callback addition for handling individual file upload completion.The new
onFileUploadComplete
callback is properly typed and documented, providing developers with a way to respond to individual file upload completions. This is a useful addition that complements the existingonClientUploadComplete
callback which only fires after all files are uploaded.packages/react/src/use-uploadthing.ts (1)
94-94
: Good integration of the new callback into the upload flow.Correctly passes the
onFileUploadComplete
callback from the props to theuploadFiles
function, ensuring the callback is properly integrated into the upload process.packages/uploadthing/src/types.ts (2)
12-12
: Appropriate import addition.Added import for
ClientUploadedFileData
which is needed for the new callback signature.
129-146
: Well-structured callback definition matching React implementation.The
onFileUploadComplete
callback is properly defined in theUploadFilesOptions
type with clear documentation. The structure mirrors the one inUseUploadthingProps
, maintaining consistency across the codebase.packages/react/src/components/button.tsx (2)
6-10
: Enhanced type imports for better clarity.Explicit imports of the necessary types from "@uploadthing/shared" improve code clarity and maintainability.
119-119
: Proper integration of callback in UploadButton component.The
onFileUploadComplete
property is correctly passed from component props to the internal upload logic, ensuring the callback works as expected when using theUploadButton
component.packages/uploadthing/src/_internal/upload-browser.ts (2)
157-157
: Good implementation of the callback invocation.The
Micro.tap
operator correctly invokes the callback with the file data after the upload is complete. The optional chaining ensures safety when the callback isn't provided.
206-212
:❓ Verification inconclusive
Well-structured callback with comprehensive context data.
The implementation provides rich context to the consumer by including not just the uploaded file data but also the original file and the complete files array. This gives developers flexibility in how they handle upload completions, especially in multi-file scenarios.
Consider adding tests to verify this callback is invoked correctly with the expected data structure.
🏁 Script executed:
#!/bin/bash # Check if tests exist for the new onFileUploadComplete functionality # Look for test files that might test this functionality echo "Searching for relevant test files..." rg -l "onFileUploadComplete" --type ts --type tsxLength of output: 160
Review Focus: Enhance Test Verification for onFileUploadComplete Callback
The callback implementation is solid and provides comprehensive context. However, our initial search for tests referencing this functionality was inconclusive due to a file type configuration issue (the
tsx
type wasn’t recognized by the search command). Please ensure that tests are added (or, if they already exist, verify their effectiveness) to confirm that theonFileUploadComplete
callback is invoked correctly with the expected data structure (i.e., it correctly passesfileData
,file
, andfiles
).
- Update the testing configuration or search parameters (e.g., by adding a
tsx
file type) to confirm test coverage, or manually verify that relevant tests exist.- If tests are not already present, consider adding them to cover multi-file and edge-case scenarios.
from #1175 -- Created for Canary (since that action fails on forks)
Summary by CodeRabbit