feat: new uploadFiles API
#1176
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
🦋 Changeset detectedLatest commit: f8c7cd2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis update introduces a new, fully-typed, and controllable file upload client ("future" uploader) for the UploadThing SDK, including its core implementation, public API, and supporting React hooks and UI examples. It adds new internal and public modules for client-side upload management with pause, resume, and abort capabilities, and event-driven progress tracking. The playground app is extended with new upload pages and components demonstrating both asynchronous and controlled upload flows, as well as comprehensive UI examples using Origin UI. Styling across the playground is refactored to use the Changes
Sequence Diagram(s)High-level: Future Uploader FlowsequenceDiagram
participant User
participant Uploader (future_genUploader)
participant Server (UploadThing API)
participant Storage (Presigned URL)
User->>Uploader: Select files & trigger upload
Uploader->>Server: Request presigned URLs for files
Server-->>Uploader: Return presigned URLs
Uploader->>User: Emit presignedReceived event
loop For each file (concurrent, max 6)
Uploader->>Storage: PUT file data (with progress)
Storage-->>Uploader: Respond (success or error)
Uploader->>User: Emit progress/completed/failed events
end
Uploader->>User: All uploads done (done() promise resolves)
Controlled Upload: Pause/Resume/AbortsequenceDiagram
participant User
participant Uploader
participant Storage
User->>Uploader: Start upload
Uploader->>Storage: PUT file data (upload in progress)
User->>Uploader: Pause upload
Uploader--xStorage: Abort request (pause, no rejection)
User->>Uploader: Resume upload
Uploader->>Storage: Re-initiate PUT request
User->>Uploader: Abort upload
Uploader--xStorage: Abort request (throw UploadAbortedError)
Suggested labels
Suggested reviewers
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
juliusmarminge
changed the title
uploadFiles API
juliusmarminge
changed the title
uploadFiles APIuploadFiles API
juliusmarminge
changed the title
uploadFiles APIuploadFiles API
More templates
commit: |
📦 Bundle size comparison
|
juliusmarminge
force-pushed
the
future-uploadFiles
branch
from
7f0f686 to
9af1e7d
Compare
juliusmarminge
force-pushed
the
future-uploadFiles
branch
from
9af1e7d to
dc80314
Compare
juliusmarminge
force-pushed
the
future-uploadFiles
branch
from
dc80314 to
22076c0
Compare
| * Upload a file to the storage provider | ||
| * Throughout the upload, the file's status and progress will be updated | ||
| * @remarks This function never rejects | ||
| * @internal |
There was a problem hiding this comment.
I haven't implemented the public-facing uploadFiles yet, which would take just File[], transition to PendingFile and request the presigned URLs
Technically we could have this be public and also export a function to manually get the predesigned URLs
juliusmarminge
force-pushed
the
future-uploadFiles
branch
3 times, most recently
from
bff16c3 to
cb7d7fd
Compare
juliusmarminge
force-pushed
the
future-uploadFiles
branch
6 times, most recently
from
290fb78 to
f1c75b8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
playground/app/api/uploadthing/route.ts (1)
17-19: Naming consideration foranyPrivateroute.The route name
anyPrivatefollows the naming pattern established in this file, but there was a previous comment suggesting this name could be reconsidered.playground/app/originui/page.tsx (1)
179-184: Preview URLs leak memory across the entire Origin-UI pageEvery component here relies on the
previewobject URL produced by
useUploadThingDropzone, but none of them ever revokes it. Because users
can add/remove hundreds of files in a single session, un-revoked blob URLs
quickly exhaust memory.Once you fix the leak inside the hook (see previous comment) these
components will inherit the benefit automatically. Just ensure you upgrade
the hook everywhere.
🧹 Nitpick comments (10)
playground/app/global.css (3)
6-40: Consider fallbacks for OKLCH color values.Oklch is cutting-edge and not universally supported. To ensure graceful degradation, you might provide fallback formats (e.g. hex or rgb) alongside the
oklch()declarations:--background: oklch(1 0 0); /* OKLCH */ --background: #ffffff; /* HEX fallback */This will improve cross-browser compatibility.
42-91: Plan for theming beyond inline defaults.The
@theme inlineblock maps your root tokens into--color-*. If you intend to support dark mode or multiple themes, consider grouping alternative tokens (e.g.@theme dark { … }) rather than only inline defaults.
93-100: Scope global border and outline resets.Applying
@apply border-border outline-ring/50to*can bloat your generated CSS and may override third-party resets unexpectedly. Consider limiting this to focusable elements or specific components:*:focus { @apply outline-ring/50; }This will reduce CSS size and avoid styling conflicts.
playground/app/api/uploadthing/route.ts (1)
17-34: Significant code duplication between upload routes.The
anyPrivateandanyPublicroutes have identical implementation except for the ACL setting. This duplication increases maintenance burden as any changes need to be applied in multiple places.Consider extracting the common logic into a helper function that takes the ACL value as a parameter:
- anyPrivate: fileRoute({ - blob: { maxFileSize: "256MB", maxFileCount: 10, acl: "private" }, - }) - .input(z.object({})) - .middleware(async (opts) => { - const session = await getSession(); - if (!session) { - throw new UploadThingError("Unauthorized"); - } - - console.log("middleware ::", session.sub, opts.input); - - return {}; - }) - .onUploadComplete(async (opts) => { - console.log("Upload complete", opts.file); - revalidateTag(CACHE_TAGS.LIST_FILES); - }), - - anyPublic: fileRoute({ - blob: { maxFileSize: "256MB", maxFileCount: 10, acl: "public-read" }, - }) - .input(z.object({})) - .middleware(async (opts) => { - const session = await getSession(); - if (!session) { - throw new UploadThingError("Unauthorized"); - } - - console.log("middleware ::", session.sub, opts.input); - - return {}; - }) - .onUploadComplete(async (opts) => { - console.log("Upload complete", opts.file); - revalidateTag(CACHE_TAGS.LIST_FILES); - }), + // Helper function to create a blob upload route with the specified ACL + const createBlobRoute = (acl: "private" | "public-read") => + fileRoute({ + blob: { maxFileSize: "256MB", maxFileCount: 10, acl }, + }) + .input(z.object({})) + .middleware(async (opts) => { + const session = await getSession(); + if (!session) { + throw new UploadThingError("Unauthorized"); + } + + console.log("middleware ::", session.sub, opts.input); + + return {}; + }) + .onUploadComplete(async (opts) => { + console.log("Upload complete", opts.file); + revalidateTag(CACHE_TAGS.LIST_FILES); + }); + + anyPrivate: createBlobRoute("private"), + anyPublic: createBlobRoute("public-read"),Also applies to: 36-53
playground/app/future/page.tsx (2)
70-88:startTransitionshould wrap state updates, not async work
startTransitionis intended for render-only updates. Awaiting an async
function inside the transition blocks the “concurrent” benefit and can
throw unhandled promises if the component unmounts mid-await.- const handleSubmit = (e: React.FormEvent) => { - … - startTransition(async () => { - setIsUploading(true); - const controls = await future_createUpload("anyPrivate", { … }); - setUploadControls(controls); - }); - }; + const handleSubmit = async (e: React.FormEvent) => { + … + setIsUploading(true); + const controls = await future_createUpload("anyPrivate", { … }); + // wrap *only* the state writes + startTransition(() => setUploadControls(controls)); + };Keeps the async work out of the transition and still batches the state
updates.
160-168: Division by zero guard for progress calculationIf, for any reason,
file.sizecomes back as 0 the progress bar will throw
NaNand collapse the layout.- style={{ width: `${(file.sent / file.size) * 100}%` }} + const pct = file.size ? (file.sent / file.size) * 100 : 0; + … + style={{ width: `${pct}%` }}Minor, but avoids edge-case crashes.
packages/uploadthing/src/client-future.ts (2)
173-176:abortUploadleaks control flow via synchronous throw
abortUploadboth aborts the XHR viaAbortControllerand then immediately throws anUploadAbortedError().
Because this throw is synchronous, any caller that forgets to wrap the call intry/ catchwill crash the UI thread before the deferred promises settle.Prefer returning the error (or resolving the
done()promise with a failed state) and keepabortUploadside-effect-only:- // Abort the upload - throw new UploadAbortedError(); + // Aborted; caller can detect via the deferred(s) rejecting with UploadAbortedError + return;If a hard exception is desired, document it prominently in JSDoc so integrators know they must guard the call.
213-232: Replacevoidin the union return type withundefinedBiome flags
voidinside a union as confusing.voidwidens toundefined | null | void 0at the type level and makes narrowing harder. The API already treats “no single file specified” as an omitted parameter, whereundefinedis the natural sentinel.-const done = async <T extends AnyFile<TRoute[TEndpoint]> | void = void>( +const done = async <T extends AnyFile<TRoute[TEndpoint]> | undefined = undefined>(Resulting inference stays identical while satisfying the linter and preventing accidental
nullacceptance.🧰 Tools
🪛 Biome (1.9.4)
[error] 213-213: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/uploadthing/src/_internal/client-future.ts (2)
384-388: HEAD probe for every file can be expensive – consider conditional reuseA
HEADrequest is fired per file to fetchx-ut-range-start.
For large batches (dozens of files) this doubles the request count and increases latency on high-RTT networks.Possible optimisations:
- Add a feature flag in the presigned payload (
supports-resume) and skip the probe when it is0(fresh uploads).- Bundle range information together with the presigned URL response to remove the extra RTT entirely.
These changes preserve resumability while halving the number of network trips.
460-468:"uri" in fileruntime check lacks type-safetyThe
inoperator is used to detect React-NativeFileshims.
Becausefileis mutated in place to gain extra properties (status,sent, …) the structural check might collide in the future.Suggestion: narrow via a branded interface:
interface RNFileLike extends File { uri: string; }and cast only when
file as unknown as RNFileLikeafter verifyingtypeof (file as any).uri === "string".This avoids false positives when other URIs (e.g., metadata) are added to the enhanced file type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (19)
examples/profile-picture/package.json(1 hunks)examples/with-novel/package.json(1 hunks)packages/uploadthing/package.json(3 hunks)packages/uploadthing/src/_internal/client-future.ts(1 hunks)packages/uploadthing/src/_internal/ut-reporter.ts(2 hunks)packages/uploadthing/src/client-future.ts(1 hunks)packages/uploadthing/src/client.ts(1 hunks)packages/uploadthing/turbo.json(1 hunks)playground/app/api/uploadthing/route.ts(1 hunks)playground/app/future/page.tsx(1 hunks)playground/app/global.css(1 hunks)playground/app/originui/page.tsx(1 hunks)playground/components/button.tsx(1 hunks)playground/components/fieldset.tsx(1 hunks)playground/components/file-card.tsx(2 hunks)playground/components/skeleton.tsx(1 hunks)playground/components/uploader.tsx(1 hunks)playground/lib/uploadthing.ts(1 hunks)playground/package.json(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
playground/components/file-card.tsx (1)
playground/lib/actions.ts (1)
deleteFile(91-106)
playground/components/button.tsx (1)
playground-v6/components/button.tsx (1)
Button(11-26)
playground/app/api/uploadthing/route.ts (2)
playground-v6/lib/data.ts (1)
getSession(27-31)playground-v6/lib/const.ts (1)
CACHE_TAGS(3-5)
packages/uploadthing/src/_internal/client-future.ts (6)
packages/shared/src/tagged-errors.ts (1)
FetchError(63-72)packages/uploadthing/src/_internal/types.ts (1)
UploadPutResult(188-202)packages/shared/src/effect.ts (2)
FetchContext(7-11)fetchEff(20-55)packages/uploadthing/src/client-future.ts (1)
version(38-38)packages/shared/src/types.ts (1)
MaybePromise(14-14)packages/uploadthing/src/_internal/ut-reporter.ts (1)
createUTReporter(45-112)
🪛 Biome (1.9.4)
packages/uploadthing/src/client-future.ts
[error] 1-1: Do not shadow the global "Array" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
[error] 213-213: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
packages/uploadthing/src/_internal/client-future.ts
[error] 1-1: Do not shadow the global "Array" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (17)
playground/app/global.css (1)
1-5: Verify CSS import directives and scanning configuration.You’ve added
@import "uploadthing/tw/v4";and the nonstandard@source "../node_modules/@uploadthing/react/dist";. Please confirm that your PostCSS/Tailwind processor supports@sourcefor scanning CSS classes in node modules. If not, classes used in the React dist may be purged at build time.examples/with-novel/package.json (1)
14-14: Dependency version bump looks good.Upgrading
"class-variance-authority"to^0.7.1aligns with other examples. No conflicting versions ofclsxremain in this example’s dependencies.playground/components/fieldset.tsx (1)
2-2: Importcxfromclass-variance-authorityis correct.The switch from
clsxto the namedcxexport matches the new styling utility and keeps component logic unchanged. Ensure that your overall build no longer includes the oldclsxpackage.examples/profile-picture/package.json (1)
19-19: Dependency version bump is consistent.Updating
"class-variance-authority"to^0.7.1mirrors other example projects. No additional adjustments needed here.playground/components/skeleton.tsx (1)
1-1: Import adjustment forcxis applied correctly.The refactor from
clsxtoclass-variance-authorityaligns with the rest of the codebase. Component behavior remains unaffected.playground/components/uploader.tsx (1)
64-64: Endpoint change matches new upload route structure.The change from using the
anythingendpoint toanyPrivatealigns with the updated backend upload routes. This ensures the uploader component correctly targets the private upload route as part of the newuploadFilesAPI implementation.packages/uploadthing/turbo.json (1)
8-8: Build configuration updated for new client module.The addition of
"client-future/**"to the build outputs correctly includes the new module implementing theuploadFilesAPI in the package distribution.playground/package.json (1)
13-13:Details
✅ Verification successful
Dependencies updated to support new UI components.
The replacement of
clsxwithclass-variance-authorityand addition oflucide-reactandtailwind-mergesupport the new UI components and styling approach used in the playground for demonstrating the new upload functionality.Also applies to: 15-15, 19-19
🏁 Script executed:
#!/bin/bash # Verify that components are using the new dependencies instead of the removed clsx echo "Checking for any remaining clsx imports in the playground:" rg "from ['\\"]clsx['\\"]" playground/Length of output: 243
I’ve fixed the regex quoting to reliably catch any remaining
clsximports. Please rerun this check:
🏁 Script executed:
#!/bin/bash echo "Searching for `clsx` imports in the playground directory:" rg -n -e "import.*clsx" -e "from ['\\\"]clsx['\\\"]" playground/ || echo "No clsx imports found"Length of output: 456
Let’s simplify the check to catch any mention of
clsx(imports, requires, or usage) without complex regex:
🏁 Script executed:
#!/bin/bash echo "Searching for any occurrence of 'clsx' in the playground directory:" rg -nF "clsx" playground/ || echo "No clsx references found"Length of output: 235
Dependencies Updated for New Playground UI Components
- playground/package.json
• Line 13: added"class-variance-authority": "^0.7.1"
• Line 15: added"lucide-react"
• Line 19: added"tailwind-merge"Confirmed that all
clsxreferences have been removed and replaced. No further changes required.packages/uploadthing/src/_internal/ut-reporter.ts (1)
49-49: Made package property optional for more flexible reporting.Making the
packageproperty optional and conditionally setting thex-uploadthing-packageheader only when the property is defined provides more flexibility in how the reporter is used. This change supports the newuploadFilesAPI by allowing the reporter to work with or without a specific package identifier.Also applies to: 64-66
packages/uploadthing/src/client.ts (1)
44-46: Exported utilities for file size formatting and content label generation.The addition of these two utility exports from
@uploadthing/sharedhelps provide consistent formatting and label generation across the uploadthing ecosystem.playground/components/file-card.tsx (2)
4-4: Migration from clsx to class-variance-authority.This change aligns with the broader styling refactoring across the playground components.
43-53: Refactored delete button styling from props to direct classes.The button styling has been updated to use explicit Tailwind classes instead of the color prop system, with a proper size="icon" property. This approach gives more direct control over the button's appearance.
packages/uploadthing/package.json (3)
22-31: Added client-future module to package exports.This properly configures the new client-future module to be accessible to consumers of the package through both ESM and CJS module systems with proper type definitions.
131-131: Added client-future to files array.This ensures the client-future directory is included in the published package.
149-151: Increased Node memory allocation for build processes.The memory allocation for build and dev scripts has been doubled from 8GB to 16GB. This suggests the new client-future module may add significant complexity to the build process.
Consider monitoring the actual memory usage during builds to ensure this increase is necessary. If memory usage remains well below the new limit, you might be able to reduce it in the future.
playground/app/api/uploadthing/route.ts (1)
55-58: Images route configured differently from other routes.The
imagesroute uses a different configuration withawaitServerData: false, which means client uploads will complete before server-side processing finishes. This is appropriate for image uploads where immediate UI feedback is desired.playground/app/originui/page.tsx (1)
748-807: Repeated inline progress-bar logic could be extracted
Comp553(and siblings) contain identical inline closures that calculate
progress and paint bars. Duplicating the snippet in four places hampers
maintenance and increases bundle size.Consider a tiny, memoized
<ProgressBar progress={sent/size} />component
shared by all upload UIs.[ suggest_optional_refactor ]
| const [files, setFiles] = useState< | ||
| (AnyFile<UploadRouter["images"]> & { preview?: string })[] | ||
| >([]); | ||
|
|
There was a problem hiding this comment.
Incorrect generic narrows files state to the images route only
files is currently typed as
AnyFile<UploadRouter["images"]>[], so if the hook is used with
anyPrivate or anyPublic the compiler will (silently) lie – you’ll lose
type-safety and risk accessing props that do not exist on those route
definitions.
- const [files, setFiles] = useState<
- (AnyFile<UploadRouter["images"]> & { preview?: string })[]
- >([]);
+ const [files, setFiles] = useState<
+ (AnyFile<UploadRouter[TFileRoute]> & { preview?: string })[]
+ >([]);Apply the same substitution to every helper that takes/returns a file
(e.g. removeFile).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const [files, setFiles] = useState< | |
| (AnyFile<UploadRouter["images"]> & { preview?: string })[] | |
| >([]); | |
| const [files, setFiles] = useState< | |
| (AnyFile<UploadRouter[TFileRoute]> & { preview?: string })[] | |
| >([]); |
| const pendingFiles = acceptedFiles.map((file) => { | ||
| const pendingFile = makePendingFile(file); | ||
| const preview = file.type.startsWith("image") | ||
| ? URL.createObjectURL(file) | ||
| : undefined; | ||
| return Object.assign(pendingFile, { preview }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Object-URL never revoked – causes memory leaks
URL.createObjectURL allocates a blob URL that must be released with
URL.revokeObjectURL when the preview is no longer needed
(component unmount or file removed). Otherwise each drag-and-drop adds a
permanent entry in the browser’s blob registry.
A minimal fix inside removeFile and a cleanup useEffect is enough:
- const removeFile = (file: AnyFile<AnyFileRoute>) => {
- setFiles((prev) => prev.filter((f) => f !== file));
- };
+ const removeFile = (file: AnyFile<AnyFileRoute>) => {
+ if ('preview' in file && file.preview) URL.revokeObjectURL(file.preview);
+ setFiles((prev) => prev.filter((f) => f !== file));
+ };
+
+ useEffect(() => {
+ return () => {
+ files.forEach((f) => {
+ if ('preview' in f && f.preview) URL.revokeObjectURL(f.preview);
+ });
+ };
+ }, [files]);Without this the tab’s memory usage continuously grows while testing the
drop-zone.
Committable suggestion skipped: line range outside the PR's diff.
| const files = Arr.ensure(file ?? options.files); | ||
| for (const file of files) { | ||
| const upload = uploads.get(file); | ||
| if (!upload) throw "No upload found"; | ||
|
|
||
| if (upload.deferred.ac.signal.aborted === false) { | ||
| // Ensure the upload is paused |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Throw typed errors instead of raw strings
abortUpload, pauseUpload, and resumeUpload use throw "No upload found" when a file key is missing. Throwing a bare string:
- Loses stack-trace context in some runtimes.
- Violates the project’s tagged‐error convention.
- Is inconsistent with the rest of this module, which always throws an
UploadThing*Error.
- if (!upload) throw "No upload found";
+ if (!upload) {
+ throw new Error("uploadthing: no in-flight upload found for the provided File");
+ }Consider a dedicated UploadNotFoundError (mirroring UploadAbortedError) if callers need to discriminate the failure.
Also applies to: 185-187
| import { Array } from "effect"; | ||
| import * as Arr from "effect/Array"; | ||
| import * as Micro from "effect/Micro"; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid shadowing the global Array and remove the duplicate import
Importing Array from effect masks the built-in Array constructor and the subsequent explicit effect/Array import is redundant. This can be confusing for anyone reading or debugging the code and may break IDE ref-actors that rely on the global symbol.
-import { Array } from "effect";
-import * as Arr from "effect/Array";
+import * as Arr from "effect/Array"; // gives you Arr.zip & Arr.ensureThen replace every Array.zip usage in this file with Arr.zip.
This keeps one canonical alias, eliminates the shadowed global, and aligns with the rest of the codebase that already uses Arr.ensure.
📝 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 { Array } from "effect"; | |
| import * as Arr from "effect/Array"; | |
| import * as Micro from "effect/Micro"; | |
| import * as Arr from "effect/Array"; // gives you Arr.zip & Arr.ensure | |
| import * as Micro from "effect/Micro"; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Do not shadow the global "Array" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/happy-houses-hang.md (1)
9-9: Replace bare URL with Markdown link to satisfy MD034 lint rule. Embed the link in Markdown syntax, for example:- https://github.com/pingdotgg/uploadthing/blob/main/playground/app/originui/page.tsx + [Example usage here](https://github.com/pingdotgg/uploadthing/blob/main/playground/app/originui/page.tsx)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
9-9: Bare URL used
null(MD034, no-bare-urls)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.changeset/happy-houses-hang.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
.changeset/happy-houses-hang.md
9-9: Bare URL used
null
(MD034, no-bare-urls)
🔇 Additional comments (3)
.changeset/happy-houses-hang.md (3)
1-3: Frontmatter formatting is correct. The changeset header properly specifies theuploadthingpackage with aminorversion bump according to the Changesets spec.
5-5: Commit summary aligns with Conventional Commits. Thefeat:prefix clearly indicates a new experimental client API.
7-7: Explanatory text is clear. No changes needed.
juliusmarminge
force-pushed
the
future-uploadFiles
branch
from
bc9db78 to
f8c7cd2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
packages/uploadthing/src/client-future.ts (4)
225-225: Consider consistency in error handling between functions
abortUploadthrows an explicitUploadAbortedErrorafter pausing uploads, whilepauseUploadsimply returns if an upload isn't found without throwing anything. This creates an inconsistent API experience.Either make both functions no-op when the file isn't found, or make both throw the same style of error:
const pauseUpload = (file?: File) => { const files = Arr.ensure(file ?? options.files); for (const file of files) { const upload = uploads.get(file); - if (!upload) return; + if (!upload) { + // Either throw consistently like abortUpload: + // throw new Error("uploadthing: no in-flight upload found for the provided File"); + // Or just return silently (current behavior) + return; + }
1-2:⚠️ Potential issueAvoid shadowing the global
ArrayobjectImporting
Arrayfromeffectmasks the built-inArrayconstructor which can cause confusion. This was flagged in a previous review but wasn't addressed.-import { Array } from "effect"; -import * as Arr from "effect/Array"; +import * as Arr from "effect/Array";Then update all
Array.zipcalls in this file to useArr.zip.🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Do not shadow the global "Array" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
165-165: 🛠️ Refactor suggestionReplace string literals with proper Error objects
Throwing string literals loses stack trace information and is inconsistent with the project's error handling pattern.
- if (!upload) throw "No upload found"; + if (!upload) { + throw new Error("uploadthing: no in-flight upload found for the provided File"); + }
185-185: 🛠️ Refactor suggestionReplace string literals with proper Error objects
Throwing string literals loses stack trace information and is inconsistent with the project's error handling pattern.
- if (!upload) throw "No upload found"; + if (!upload) { + throw new Error("uploadthing: no in-flight upload found for the provided File"); + }packages/uploadthing/src/_internal/client-future.ts (1)
1-3:⚠️ Potential issueAvoid shadowing the global
ArrayobjectImporting
Arrayfromeffectmasks the built-inArrayconstructor which can cause confusion.-import { Array, Micro, Predicate } from "effect"; +import { Micro, Predicate } from "effect"; +import * as EffArray from "effect/Array";Then update all
Array.zipcalls to useEffArray.zip.🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Do not shadow the global "Array" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🧹 Nitpick comments (4)
.changeset/happy-houses-hang.md (1)
9-9: Use Markdown link syntax instead of bare URLFor better readability and markup compatibility, use proper Markdown link syntax instead of a bare URL.
-https://github.com/pingdotgg/uploadthing/blob/main/playground/app/originui/page.tsx +[Example usage](https://github.com/pingdotgg/uploadthing/blob/main/playground/app/originui/page.tsx)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
9-9: Bare URL used
null(MD034, no-bare-urls)
packages/uploadthing/src/client-future.ts (2)
213-219: Avoidvoidin union typesUsing
voidin a union type can be confusing. Consider usingundefinedinstead for better type clarity.-const done = async <T extends AnyFile<TRouter[TEndpoint]> | void = void>( +const done = async <T extends AnyFile<TRouter[TEndpoint]> | undefined = undefined>( file?: T, ): Promise< T extends AnyFile<TRouter[TEndpoint]> ? UploadedFile<TRouter[TEndpoint]> | FailedFile<TRouter[TEndpoint]> : (UploadedFile<TRouter[TEndpoint]> | FailedFile<TRouter[TEndpoint]>)[] > => {🧰 Tools
🪛 Biome (1.9.4)
[error] 213-213: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
146-147: Consider validating files against upload constraintsThe pauseUpload, resumeUpload, and abortUpload functions don't validate that the provided file was part of the original upload set. This might lead to unexpected behavior if consumers pass unrelated files.
Consider validating that any provided file was actually part of the initial upload request:
const pauseUpload = (file?: File) => { const files = Arr.ensure(file ?? options.files); for (const file of files) { + // Check if file was part of the original upload + if (file && !options.files.includes(file)) { + throw new Error("uploadthing: file was not part of the original upload request"); + } const upload = uploads.get(file); if (!upload) return;packages/uploadthing/src/_internal/client-future.ts (1)
594-618: Ensure concurrent uploads respect rate limitsThe code uses a concurrency limit of 6 for parallel uploads, but it's not clear if this was chosen based on specific API limitations or testing.
The hardcoded concurrency value of 6 may need adjustment based on:
- Storage provider rate limits
- Client device capabilities (memory, network)
- Server-side capacity
Consider making this configurable or dynamically adjustable based on network conditions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
.changeset/happy-houses-hang.md(1 hunks)examples/profile-picture/package.json(1 hunks)examples/with-novel/package.json(1 hunks)packages/uploadthing/package.json(3 hunks)packages/uploadthing/src/_internal/client-future.ts(1 hunks)packages/uploadthing/src/_internal/ut-reporter.ts(2 hunks)packages/uploadthing/src/client-future.ts(1 hunks)packages/uploadthing/src/client.ts(1 hunks)packages/uploadthing/turbo.json(1 hunks)playground/app/api/uploadthing/route.ts(1 hunks)playground/app/future/page.tsx(1 hunks)playground/app/global.css(1 hunks)playground/app/originui/page.tsx(1 hunks)playground/components/button.tsx(1 hunks)playground/components/fieldset.tsx(1 hunks)playground/components/file-card.tsx(2 hunks)playground/components/skeleton.tsx(1 hunks)playground/components/uploader.tsx(1 hunks)playground/lib/uploadthing.ts(1 hunks)playground/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- playground/components/fieldset.tsx
- examples/with-novel/package.json
🚧 Files skipped from review as they are similar to previous changes (15)
- examples/profile-picture/package.json
- playground/components/uploader.tsx
- playground/components/skeleton.tsx
- packages/uploadthing/src/_internal/ut-reporter.ts
- packages/uploadthing/turbo.json
- playground/package.json
- packages/uploadthing/package.json
- playground/components/file-card.tsx
- packages/uploadthing/src/client.ts
- playground/app/global.css
- playground/app/originui/page.tsx
- playground/components/button.tsx
- playground/app/future/page.tsx
- playground/lib/uploadthing.ts
- playground/app/api/uploadthing/route.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/uploadthing/src/_internal/client-future.ts
[error] 1-1: Do not shadow the global "Array" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
packages/uploadthing/src/client-future.ts
[error] 1-1: Do not shadow the global "Array" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
[error] 213-213: void is confusing inside a union type.
Unsafe fix: Use undefined instead.
(lint/suspicious/noConfusingVoidType)
🪛 markdownlint-cli2 (0.17.2)
.changeset/happy-houses-hang.md
9-9: Bare URL used
null
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: bundle-analyze-result
- GitHub Check: e2e-node (backend-adapters)
🔇 Additional comments (8)
.changeset/happy-houses-hang.md (1)
1-9: LGTM! Properly documented experimental featureThe changeset correctly marks this as a minor version change and clearly states that the API is experimental and not covered by semver.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
9-9: Bare URL used
null(MD034, no-bare-urls)
packages/uploadthing/src/client-future.ts (3)
72-74: LGTM! Strong typing for the endpoint resolutionThe implementation properly handles both function and direct string endpoint specifications, providing good type safety.
75-89: LGTM! Robust error handling with effect libraryThe code properly uses the effect library for error handling and transforms the errors into a more user-friendly format.
97-136: LGTM! Excellent implementation of concurrent uploadsThe implementation properly handles concurrent uploads with proper state management and event emission.
packages/uploadthing/src/_internal/client-future.ts (4)
20-67: LGTM! Well-structured error class hierarchyThe error classes follow a consistent pattern with the TaggedError approach, making it easy to distinguish between different error types and providing rich error information.
74-170: LGTM! Comprehensive file state interfacesThe file state interfaces (PendingFile, UploadingFile, FailedFile, UploadedFile) are well designed with clear status discriminants and appropriate properties for each state.
380-489: LGTM! Robust upload implementation with progress trackingThe uploadFile function is well-implemented with proper error handling, progress tracking, and state transitions. The special handling for iOS/React Native FormData is a nice touch.
460-473:Details
❓ Verification inconclusive
Verify React Native compatibility with real-world devices
The code has special handling for React Native FormData, but it's important to verify this works across various React Native versions and iOS devices.
🏁 Script executed:
#!/bin/bash # Verify if the React Native special case is documented or tested echo "Checking for React Native tests or documentation:" find . -type f -name "*.md" -o -name "*.test.ts" -o -name "*.test.tsx" | xargs grep -l "React Native" || echo "No React Native documentation or tests found" # Look for existing React Native integration in the codebase echo -e "\nChecking for React Native integration:" grep -r --include="*.ts" --include="*.tsx" "React Native" .Length of output: 1508
Please add automated tests and documentation for React Native FormData handling
I see that we already have React Native–specific handling in
client-future.ts(and mirrored inupload-browser.ts) and explanatory comments in the Expo packages, but there aren’t any corresponding tests or markdown docs.• Add integration or unit tests covering the
"uri" in filebranch on real devices or emulators (e.g., Jest+React Native Testing Library or E2E tests).
• Update your package README or add a dedicated MD file to document the React Native FormData behavior and any iOS quirks (nil-object crash workaround, supported RN versions, etc.).
|
|
||
| This API is not covered under semver. Check out some example usage here: | ||
|
|
||
| https://github.com/pingdotgg/uploadthing/blob/main/playground/app/originui/page.tsx No newline at end of file |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify that the example URL is correct
The URL points to /originui/page.tsx but the PR summary mentions examples in playground/app/future/page.tsx. Please verify this is the correct example location.
🏁 Script executed:
#!/bin/bash
# Check if both paths exist and compare them
echo "Checking if the files exist:"
ls -la playground/app/originui/page.tsx 2>/dev/null
ls -la playground/app/future/page.tsx 2>/dev/null
# Check if they contain examples of the new API
echo -e "\nChecking for usage of future_genUploader in files:"
grep -n "future_genUploader" playground/app/originui/page.tsx 2>/dev/null
grep -n "future_genUploader" playground/app/future/page.tsx 2>/dev/nullLength of output: 550
Update the example URL in the changeset
It looks like the linked example in .changeset/happy-houses-hang.md still points to:
https://github.com/pingdotgg/uploadthing/blob/main/playground/app/originui/page.tsx
but according to the PR summary the new API example lives in playground/app/future/page.tsx. Please update the URL on line 9 of that changeset to:
- https://github.com/pingdotgg/uploadthing/blob/main/playground/app/originui/page.tsx
+ https://github.com/pingdotgg/uploadthing/blob/main/playground/app/future/page.tsxand verify the linked file shows the intended example.
📝 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.
| https://github.com/pingdotgg/uploadthing/blob/main/playground/app/originui/page.tsx | |
| https://github.com/pingdotgg/uploadthing/blob/main/playground/app/future/page.tsx |
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
9-9: Bare URL used
null
(MD034, no-bare-urls)
Summary by CodeRabbit
New Features
Enhancements
class-variance-authority.Dependency Updates
Bug Fixes
Chores