chore: add minimal-tanstack-start example #989
Conversation
🦋 Changeset detectedLatest commit: a9bae02 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
Warning Rate limit exceeded@juliusmarminge has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces significant changes to the Changes
Possibly related PRs
Poem
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 (
|
juraj98
force-pushed
the
tss-example
branch
from
ce3cf96
to
0b35d0e
Compare
juraj98
force-pushed
the
tss-example
branch
from
0b35d0e
to
15da943
Compare
juraj98
force-pushed
the
tss-example
branch
from
15da943
to
0108a3a
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: 13
🧹 Outside diff range and nitpick comments (14)
examples/minimal-tanstack-start/.env.example (1)
1-2: LGTM! Consider enhancing the comment slightly.The implementation of the
.env.examplefile is correct and follows best practices. The comment provides clear guidance on where to obtain the token.Consider slightly enhancing the comment to be more specific:
- # Go to https://uploadthing.com/dashboard to get your token + # Go to https://uploadthing.com/dashboard/api-keys to get your API tokenThis change would provide more precise guidance to users.
examples/minimal-tanstack-start/app.config.ts (1)
1-3: Add documentation and improve the exampleWhile this file serves as a minimal starting point for a TanStack Start configuration, it could be improved to provide more value as an example.
Consider the following improvements:
- Add a comment at the top of the file explaining its purpose and how to use/extend it.
- Include some basic configuration options (as suggested in the previous comment) to demonstrate common usage.
- Add inline comments explaining each configuration option.
For example:
/** * TanStack Start Configuration * * This file configures the TanStack Start application. * Customize the options below to fit your project needs. * * For more information, visit: https://tanstack.com/start/latest/docs/config */ import { defineConfig } from "@tanstack/start/config"; export default defineConfig({ // Set the application name name: 'MyTanStackApp', // Configure the server server: { port: 3000, // The port on which the server will run }, // Define routes routes: [ { path: '/', // The URL path component: './src/pages/Home', // The component to render for this route }, ], });These changes would make the example more informative and easier to understand for developers new to TanStack Start.
examples/minimal-tanstack-start/app/api.ts (1)
6-6: LGTM! Consider adding configuration options for a more comprehensive example.The exported default handler is correctly implemented using the
createStartAPIHandlerfunction withdefaultAPIFileRouteHandleras its argument. This setup provides a minimal working example of a TanStack API handler.For a more comprehensive example, you might consider:
- Adding configuration options to
createStartAPIHandler:export default createStartAPIHandler(defaultAPIFileRouteHandler, { // Add configuration options here // For example: // maxRequestBodySize: '1mb', // onError: (error) => console.error(error), });
- Wrapping the handler creation in a try-catch block for error handling:
try { export default createStartAPIHandler(defaultAPIFileRouteHandler); } catch (error) { console.error('Failed to create API handler:', error); // Provide a fallback handler or rethrow the error }These suggestions would make the example more robust and informative for users, while still maintaining its minimal nature.
examples/minimal-tanstack-start/app/client.tsx (2)
4-6: LGTM: Router creation looks good. Consider error handling.The router creation is implemented correctly and follows good practices by separating the router logic into its own module. This approach enhances maintainability and modularity.
Consider adding error handling around the router creation. For example:
let router; try { router = createRouter(); } catch (error) { console.error("Failed to create router:", error); // Handle the error appropriately (e.g., show an error message to the user) }This would make the application more robust by gracefully handling any potential errors during router creation.
8-8: LGTM: Hydration logic is correct. Consider safer root element access.The use of
hydrateRootfor client-side hydration is correct and follows React 18+ best practices. TheStartClientcomponent is properly used with the router passed as a prop.Consider a safer approach to accessing the root element. The current use of the non-null assertion (
!) assumes the element always exists, which might not be true. A more robust approach could be:const rootElement = document.getElementById("root"); if (rootElement) { hydrateRoot(rootElement, <StartClient router={router} />); } else { console.error("Root element not found"); // Handle the error appropriately (e.g., show an error message to the user) }This approach provides better error handling and type safety.
examples/minimal-tanstack-start/app/ssr.tsx (2)
9-12: LGTM: Correct setup for TanStack Start SSR handler.The default export is correctly set up using
createStartHandlerwith the appropriate options, and wrapped withdefaultStreamHandler. This follows the recommended pattern for TanStack Start applications.For improved readability, consider splitting the export into multiple lines:
const startHandler = createStartHandler({ createRouter, getRouterManifest, }); export default startHandler(defaultStreamHandler);This separation makes the code slightly more verbose but easier to read and modify if needed in the future.
1-12: Overall assessment: Well-implemented minimal TanStack Start SSR setup.This file successfully implements a minimal TanStack Start server-side rendering (SSR) handler. The code is concise, follows best practices, and aligns well with the PR objective of adding a "minimal-tanstack-start" example. It provides a good starting point for developers looking to implement SSR with TanStack Start.
As the project grows, consider the following architectural advice:
- Implement error handling and logging in the SSR handler for better debugging and monitoring in production.
- If the application scales, you might want to add performance optimization techniques such as caching or code splitting.
- Consider adding comments or documentation to explain the purpose and usage of this SSR setup for other developers who might work on this project in the future.
examples/minimal-tanstack-start/app/routes/api/uploadthing.ts (1)
1-5: LGTM! Consider grouping imports.The imports are appropriate for creating an API route for file uploads, aligning with the PR objective. They cover the necessary functionalities from TanStack, uploadthing, and local configurations.
Consider grouping the imports by their source (external libraries first, then local imports) for better readability:
import { createAPIFileRoute } from "@tanstack/start/api"; import { createRouteHandler } from "uploadthing/server"; import { uploadRouter } from "../../server/uploadthing";examples/minimal-tanstack-start/app/router.tsx (2)
1-3: LGTM! Consider using named import for clarity.The imports look good and are appropriate for setting up a TanStack router.
For better clarity, consider using a named import for
createTanStackRouter:-import { createRouter as createTanStackRouter } from "@tanstack/react-router"; +import { createRouter } from "@tanstack/react-router";This way, you can use the original name in your code, which might be more intuitive for other developers familiar with the TanStack library.
5-11: LGTM! Consider renaming for consistency.The
createRouterfunction is well-implemented and serves its purpose in this minimal example.For consistency with the import (assuming you keep the alias), consider renaming the function:
-export function createRouter() { +export function createTanStackRouter() { const router = createTanStackRouter({ routeTree, }); return router; }This change would make it clear that this function is creating a TanStack router specifically, which could be helpful if other router types are introduced in the future.
examples/minimal-tanstack-start/app/utils/uploadthing.ts (1)
1-7: LGTM! Consider grouping imports.The imports are appropriate for the functionality being implemented. Good use of named imports for clarity.
Consider grouping the imports by separating external and internal imports with a blank line for better readability:
import { generateReactHelpers, generateUploadButton, generateUploadDropzone, } from "@uploadthing/react"; import type { OurFileRouter } from "../server/uploadthing";examples/minimal-tanstack-start/package.json (1)
5-9: Scripts look good, consider adding a test script.The development, build, and start scripts using Vinxi are well-defined. However, to make this example more complete, consider adding a test script.
You might want to add a test script:
"scripts": { "dev": "vinxi dev", "build": "vinxi build", - "start": "vinxi start" + "start": "vinxi start", + "test": "vitest run" },Note: This assumes you're using Vitest for testing. Adjust the command if you're using a different testing framework.
examples/minimal-tanstack-start/app/routes/__root.tsx (1)
17-38: LGTM: Component structure is well-organized. Consider adding a lang attribute for accessibility.The
RootComponentandRootDocumentare structured correctly, providing a solid foundation for the application. The use ofOutletallows for flexible nested routing, and the inclusion ofScrollRestorationandScriptsensures proper functionality.For improved accessibility, consider adding a
langattribute to theHtmlcomponent. This helps screen readers and other assistive technologies to properly interpret the content. Here's a suggested change:- <Html> + <Html lang="en">Replace "en" with the appropriate language code for your application if it's not in English.
examples/minimal-tanstack-start/app/routes/index.tsx (1)
1-79: Overall: Good minimal example, with room for production-ready improvements.This file successfully implements a minimal example of using TanStack Router with UploadThing, providing multiple ways for users to upload files. It serves its purpose as a starting point for developers to understand how to integrate these technologies.
To make this example more robust and production-ready, consider implementing the suggested improvements in error handling, user feedback, and code cleanliness. These enhancements would make the example not just functional, but also demonstrative of best practices in React development.
For a more complete example, consider adding:
- A custom error boundary to handle unexpected errors gracefully.
- A reusable notification component for consistent user feedback across the application.
- A loading state indicator during file uploads to improve user experience.
- Unit tests to ensure the component behaves correctly under various scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
- examples/minimal-tanstack-start/.env.example (1 hunks)
- examples/minimal-tanstack-start/app.config.ts (1 hunks)
- examples/minimal-tanstack-start/app/api.ts (1 hunks)
- examples/minimal-tanstack-start/app/client.tsx (1 hunks)
- examples/minimal-tanstack-start/app/routeTree.gen.ts (1 hunks)
- examples/minimal-tanstack-start/app/router.tsx (1 hunks)
- examples/minimal-tanstack-start/app/routes/__root.tsx (1 hunks)
- examples/minimal-tanstack-start/app/routes/api/uploadthing.ts (1 hunks)
- examples/minimal-tanstack-start/app/routes/index.tsx (1 hunks)
- examples/minimal-tanstack-start/app/server/uploadthing.ts (1 hunks)
- examples/minimal-tanstack-start/app/ssr.tsx (1 hunks)
- examples/minimal-tanstack-start/app/utils/uploadthing.ts (1 hunks)
- examples/minimal-tanstack-start/package.json (1 hunks)
- examples/minimal-tanstack-start/tsconfig.json (1 hunks)
- packages/react/src/useUploadThing.ts (3 hunks)
✅ Files skipped from review due to trivial changes (1)
- examples/minimal-tanstack-start/app/routeTree.gen.ts
🧰 Additional context used
📓 Learnings (1)
examples/minimal-tanstack-start/app/utils/uploadthing.ts (1)
Learnt from: juliusmarminge PR: pingdotgg/uploadthing#980 File: packages/solid/src/create-uploadthing.ts:119-119 Timestamp: 2024-09-25T06:06:37.949Z Learning: In this codebase, the naming conventions differ between frameworks to align with ecosystem practices: - In React and Vue, functions are prefixed with `useX` (e.g., `useRouteConfig`) to match built-in hooks and conventions. - In Solid and Svelte, functions are prefixed with `createX` (e.g., `createRouteConfig`) to align with their respective built-in functions and ecosystem conventions.
🔇 Additional comments (25)
examples/minimal-tanstack-start/.env.example (1)
1-2: Consider additional environment variables for the example.The current
.env.examplefile includes only theUPLOADTHING_TOKEN. While this may be sufficient, it's worth considering if the minimal-tanstack-start example requires any additional configuration.To ensure we're not missing any required environment variables, let's check the example's code:
This will help us verify if there are any other environment variables used in the example that should be included in the
.env.examplefile.✅ Verification successful
Verified: No additional environment variables are used in the example.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential environment variable usage in the minimal-tanstack-start example # Search for potential environment variable usage echo "Searching for potential environment variable usage:" rg --type typescript --type javascript 'process\.env\.' examples/minimal-tanstack-start # Check for any existing .env files in the example directory echo "Checking for existing .env files:" fd -t f '^\.env' examples/minimal-tanstack-startLength of output: 573
Script:
#!/bin/bash # Description: Corrected script to check for potential environment variable usage in the minimal-tanstack-start example # Search for potential environment variable usage in specific file extensions echo "Searching for potential environment variable usage:" rg 'process\.env\.' --glob '*.ts' --glob '*.tsx' --glob '*.js' --glob '*.jsx' examples/minimal-tanstack-start # Check for any existing .env files in the example directory, including hidden files echo "Checking for existing .env files:" fd -t f '^\.env' --hidden examples/minimal-tanstack-startLength of output: 406
examples/minimal-tanstack-start/app.config.ts (1)
1-1: LGTM: Correct import statementThe import statement correctly imports the
defineConfigfunction from the "@tanstack/start/config" package, which is essential for setting up the TanStack Start configuration.examples/minimal-tanstack-start/app/api.ts (2)
1-4: Verify the package name and consider adding a comment.The imports look correct for setting up a minimal TanStack start example. However, "@tanstack/start/api" is not a widely recognized npm package.
Could you please confirm if this is the correct package name? If it's a custom or new/unreleased package, consider adding a brief comment explaining its purpose and where it can be found.
Additionally, to improve code readability, you might want to add a comment explaining the purpose of these imported functions:
// Import necessary functions from TanStack Start API to create a minimal API handler import { createStartAPIHandler, defaultAPIFileRouteHandler, } from "@tanstack/start/api";
1-6: Summary: Minimal TanStack start example successfully implementedThis file successfully implements a minimal TanStack start example, aligning well with the PR objectives. The code is concise, functional, and serves as an excellent starting point for developers working with the TanStack library.
A few suggestions have been made to enhance the example:
- Verifying the package name and adding explanatory comments.
- Considering the addition of configuration options and error handling for a more comprehensive example.
Overall, this addition contributes positively to the project's documentation and examples, facilitating better understanding and usage of the TanStack library within the context of the project.
examples/minimal-tanstack-start/tsconfig.json (1)
1-9: LGTM! Modern and minimal TypeScript configuration.The
tsconfig.jsonfile provides a modern and minimal configuration suitable for a React project using TanStack. The chosen compiler options are appropriate for contemporary web development practices.Consider adding these beneficial compiler options to enhance type safety and development experience:
{ "compilerOptions": { "jsx": "react-jsx", "moduleResolution": "Bundler", "module": "ESNext", "target": "ES2022", - "skipLibCheck": true + "skipLibCheck": true, + "strict": true, + "esModuleInterop": true, + "resolveJsonModule": true, + "isolatedModules": true } }
"strict": true: Enables all strict type checking options."esModuleInterop": true: Allows default imports from CommonJS modules."resolveJsonModule": true: Allows importing JSON files as modules."isolatedModules": true: Ensures each file can be safely transpiled without relying on other imports.Let's verify if this configuration aligns with other examples in the project:
This script will help us understand if the configuration is consistent across examples and if any additional options are commonly used in other parts of the project.
✅ Verification successful
Configuration Verified! Additional compiler options align with project standards.
The recommended compiler options have been added to
examples/minimal-tanstack-start/tsconfig.json, ensuring enhanced type safety and consistency with other project examples.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other tsconfig.json files in the examples directory # and compare their content with the current file. # Find all tsconfig.json files in the examples directory tsconfig_files=$(fd --type f --glob 'tsconfig.json' examples) # Print the content of each tsconfig.json file for file in $tsconfig_files; do echo "Content of $file:" cat "$file" echo "---" done # Count the number of tsconfig.json files echo "Total number of tsconfig.json files found: $(echo "$tsconfig_files" | wc -l)"Length of output: 17990
examples/minimal-tanstack-start/app/client.tsx (2)
1-2: LGTM: Imports are correct and follow best practices.The import statements are appropriate for the application's needs. The use of named imports is a good practice, improving code readability and potentially allowing for better tree-shaking.
1-8: Overall, the implementation is solid and aligns well with the PR objective.This file successfully sets up a minimal client-side entry point for a React application using the TanStack library. It follows common patterns and best practices for React applications, making it a good example for developers to reference.
The code is concise and focused, which is appropriate for a minimal example. The separation of router logic and the use of modern React APIs (
hydrateRoot) are particularly noteworthy.While the current implementation is good, consider the suggested improvements for error handling and type safety to make the example even more robust and educational.
examples/minimal-tanstack-start/app/ssr.tsx (1)
1-7: LGTM: Imports are appropriate and well-organized.The imports from TanStack packages and the local router module are correctly set up for creating an SSR handler. The separation of router configuration into its own file (
./router) promotes modularity and maintainability.examples/minimal-tanstack-start/app/routes/api/uploadthing.ts (3)
7-7: LGTM! Route handler creation looks good.The route handler is correctly created using
createRouteHandlerfrom uploadthing/server, and it's configured with theuploadRouter. This approach promotes modularity by separating the router configuration.
1-11: Overall, good implementation of a minimal TanStack start example.This file successfully implements a minimal TanStack start example for file uploads, meeting the PR objective. The code is concise, well-structured, and integrates nicely with the uploadthing library.
Key points:
- Appropriate use of imports from TanStack and uploadthing.
- Clear separation of concerns with the use of an external uploadRouter.
- Concise Route export using TanStack's createAPIFileRoute.
The main point for consideration is the use of identical handlers for both GET and POST methods, which should be verified for correctness.
8-11: LGTM! Verify the use of identical handlers for GET and POST.The Route export is well-structured and uses the appropriate TanStack function. The "/api/uploadthing" path is suitable for an upload API.
However, it's unusual to use identical handlers for both GET and POST methods. Please verify if this is intentional and aligns with the uploadthing library's expected usage.
To help verify this, you can run the following script to check if this pattern is used consistently across the project:
If this is a common pattern in the project or required by the uploadthing library, please add a comment explaining the rationale.
examples/minimal-tanstack-start/app/router.tsx (2)
13-17: LGTM! Well-implemented module augmentation.The module augmentation for
@tanstack/react-routeris correctly implemented. It extends theRegisterinterface to include therouterproperty with the correct type, ensuring type safety throughout the application when using the router.This approach allows for seamless integration with TypeScript's type system and will provide better autocompletion and type checking when using the router in other parts of the application.
1-17: Great implementation of a minimal TanStack router setup!This file successfully implements a minimal TanStack router configuration, which aligns perfectly with the PR objective. The code is concise, well-structured, and provides a solid foundation for the example.
Key points:
- Proper imports and router creation
- Effective use of the generated route tree
- Type-safe module augmentation for seamless TypeScript integration
The minor suggestions provided earlier (import naming and function renaming) are optional improvements for clarity and consistency. Overall, this is an excellent addition to the project's examples.
examples/minimal-tanstack-start/app/utils/uploadthing.ts (3)
9-10: LGTM! Well-structured component exports.The component exports are well-structured and follow best practices:
- Consistent use of the
OurFileRoutertype ensures type safety.- Component names follow React naming conventions (PascalCase).
- Utilizing generated components from the Uploadthing library promotes consistency and reduces boilerplate.
12-12: LGTM! Hook export follows React conventions.The hook export is well-implemented:
- The
useUploadThingname follows React hook naming conventions (camelCase prefixed with 'use').- Consistent use of the
OurFileRoutertype ensures type safety.- The use of object destructuring for the export is clean and efficient.
This naming convention aligns with the learning from a previous PR (#980) where it was noted that React functions are prefixed with
useXto match built-in hooks and conventions.
1-12: Great job! This file perfectly aligns with the PR objectives.This new file successfully introduces a minimal example for using the Uploadthing library with React, which was the main objective of the PR. The implementation is clean, concise, and follows best practices:
- Proper import structure
- Type-safe component and hook generation
- Adherence to React naming conventions
The file provides a solid foundation for developers to understand and utilize the Uploadthing library within a React application, serving as an excellent starting point or reference.
examples/minimal-tanstack-start/package.json (1)
1-4: LGTM: Package metadata is well-defined.The package metadata is correctly set up:
- The scoped package name is appropriate for an example.
- The package is correctly marked as private.
- The "type" field is set to "module", which is suitable for modern JavaScript projects.
examples/minimal-tanstack-start/app/routes/__root.tsx (2)
12-15: LGTM: Route definition is correct and includes necessary styles.The root route is properly defined using
createRootRoutefrom TanStack Router. It correctly specifies theRootComponentand includes the Uploadthing CSS stylesheet.
1-38: Summary: Excellent minimal TanStack Router setup.This file provides a well-structured, minimal example of a TanStack Router setup. It aligns perfectly with the PR objective of adding a "minimal-tanstack-start" example. The code is clean, follows best practices, and serves as an excellent starting point for developers working with TanStack Router.
Key points:
- Proper import structure
- Correct route definition
- Well-organized component hierarchy
- Inclusion of necessary TanStack components (Outlet, ScrollRestoration, Scripts)
The only minor suggestions were to investigate the TypeScript error for the CSS import and to consider adding a lang attribute for accessibility. Overall, this is a valuable addition to the project's examples.
examples/minimal-tanstack-start/app/server/uploadthing.ts (2)
61-61: LGTM: Type export is correctThe export of
OurFileRoutertype is implemented correctly, following TypeScript best practices by deriving the type from theuploadRouterobject.
21-33: 🛠️ Refactor suggestionReview ACL setting and consider using bytes for file sizes
The ACL for images is set to "public-read". Ensure this aligns with your security requirements, as it makes the uploaded images publicly accessible.
For consistency and precision, consider using bytes instead of MB/GB for
maxFileSize. This can prevent potential misinterpretations due to binary vs. decimal prefixes.To ensure the ACL setting is intentional and documented, please run the following script:
Consider updating the
maxFileSizevalues to use bytes:- maxFileSize: "32MB", + maxFileSize: 33554432, // 32 * 1024 * 1024 bytesRepeat this for other file size limits as well.
examples/minimal-tanstack-start/app/routes/index.tsx (2)
1-7: LGTM: Imports are appropriate and concise.The imports from "@tanstack/react-router" and the local "utils/uploadthing" file are relevant to the component's functionality. There are no unused imports, which is good for code cleanliness.
9-11: LGTM: Route definition is correct and follows TanStack Router conventions.The route is properly defined for the root path "/" and correctly associates with the
Homecomponent. This follows the expected pattern for TanStack Router usage.packages/react/src/useUploadThing.ts (2)
154-159: Verify that the refactoring maintains existing behaviorThe
useUploadThingfunction now delegates to__useUploadThing, which changes the internal implementation. While the external interface remains the same, it's essential to ensure that this refactoring doesn't introduce any regressions.Run the existing test suite to confirm that all functionalities of
useUploadThingwork as expected after the refactoring. Pay special attention to edge cases and any custom hooks that might depend on its internal behavior.
180-183: Confirm thatuseUploadThingis correctly exportedAt lines 180-183,
useUploadThingis included in the returned helpers fromgenerateReactHelpers. Ensure that this aligns with the intended public API and that consumers of this module can accessuseUploadThingwithout issues.Test the module in a consumer application to verify that
useUploadThingis accessible and functions correctly when imported.
| "devDependencies": { | ||
| "@types/react": "18.3.3", | ||
| "@types/react-dom": "18.3.0", | ||
| "@vitejs/plugin-react": "^4.3.1", | ||
| "typescript": "^5.5.2" | ||
| } |
There was a problem hiding this comment.
Fix devDependency versions and use consistent versioning strategy.
There are some issues with the devDependency versions:
- TypeScript version 5.5.2 doesn't exist. The latest stable version is 5.2.2 as of September 2023.
- There's an inconsistency in the versioning strategy. Some use caret (^) for flexible versioning, while others are pinned.
Please update the devDependencies as follows:
"devDependencies": {
- "@types/react": "18.3.3",
- "@types/react-dom": "18.3.0",
+ "@types/react": "^18.2.25",
+ "@types/react-dom": "^18.2.10",
"@vitejs/plugin-react": "^4.3.1",
- "typescript": "^5.5.2"
+ "typescript": "^5.2.2"
}This change ensures all devDependencies use flexible versioning and corrects the TypeScript version. The React types versions have also been updated to match the latest available versions compatible with React 18.2.0.
📝 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.
| "devDependencies": { | |
| "@types/react": "18.3.3", | |
| "@types/react-dom": "18.3.0", | |
| "@vitejs/plugin-react": "^4.3.1", | |
| "typescript": "^5.5.2" | |
| } | |
| "devDependencies": { | |
| "@types/react": "^18.2.25", | |
| "@types/react-dom": "^18.2.10", | |
| "@vitejs/plugin-react": "^4.3.1", | |
| "typescript": "^5.2.2" | |
| } |
There was a problem hiding this comment.
TypeScript version 5.5.2 doesn't exist. The latest stable version is 5.2.2 as of September 2023.
This is very very false Rabbit...
There was a problem hiding this comment.
@juliusmarminge, thank you for the correction. You're absolutely right; TypeScript version 5.5.2 exists, and the versions you've specified are accurate. I'll keep this in mind for future reviews.
🐰💡
✏️ Learnings added
Learnt from: juliusmarminge
PR: pingdotgg/uploadthing#989
File: examples/minimal-tanstack-start/package.json:19-24
Timestamp: 2024-09-30T21:49:47.751Z
Learning: As of September 2024, TypeScript version 5.5.2 is available and valid.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
packages/react/src/useUploadThing.ts
Outdated
| let error: UploadThingError<inferErrorShape<TRouter>>; | ||
| if (e instanceof UploadThingError) { | ||
| error = e as UploadThingError<inferErrorShape<TRouter>>; | ||
| } else { | ||
| error = INTERNAL_DO_NOT_USE__fatalClientError(e as Error); | ||
| console.error( | ||
| "Something went wrong. Please contact UploadThing and provide the following cause:", | ||
| error.cause instanceof Error ? error.cause.toString() : error.cause, | ||
| ); | ||
| } | ||
| await opts?.onUploadError?.(error); |
There was a problem hiding this comment.
Enhance error handling to prevent exposing internal details
The error handling between lines 111-121 logs detailed error information to the console, which might include sensitive internal data. Exposing such information can be a security risk in production environments.
Consider modifying the error logging to provide user-friendly messages without revealing sensitive details. For example:
console.error(
- "Something went wrong. Please contact UploadThing and provide the following cause:",
- error.cause instanceof Error ? error.cause.toString() : error.cause,
+ "An unexpected error occurred during the upload process. Please try again later.",
);Additionally, you might want to log the detailed error information separately in a secure manner accessible only to developers for debugging purposes.
📝 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.
| let error: UploadThingError<inferErrorShape<TRouter>>; | |
| if (e instanceof UploadThingError) { | |
| error = e as UploadThingError<inferErrorShape<TRouter>>; | |
| } else { | |
| error = INTERNAL_DO_NOT_USE__fatalClientError(e as Error); | |
| console.error( | |
| "Something went wrong. Please contact UploadThing and provide the following cause:", | |
| error.cause instanceof Error ? error.cause.toString() : error.cause, | |
| ); | |
| } | |
| await opts?.onUploadError?.(error); | |
| let error: UploadThingError<inferErrorShape<TRouter>>; | |
| if (e instanceof UploadThingError) { | |
| error = e as UploadThingError<inferErrorShape<TRouter>>; | |
| } else { | |
| error = INTERNAL_DO_NOT_USE__fatalClientError(e as Error); | |
| console.error( | |
| "An unexpected error occurred during the upload process. Please try again later.", | |
| ); | |
| } | |
| await opts?.onUploadError?.(error); |
packages/react/src/useUploadThing.ts
Outdated
| function __useUploadThing< | ||
| TRouter extends FileRouter, | ||
| TEndpoint extends keyof TRouter, | ||
| >( | ||
| url: URL, | ||
| endpoint: TEndpoint, | ||
| opts?: UseUploadthingProps<TRouter, TEndpoint>, | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider renaming __useUploadThing to avoid leading underscores
The function __useUploadThing is exported and used within the module. In JavaScript/TypeScript, leading underscores are typically used to indicate private or internal functions not intended for external use. Since this function is part of the module's internal API, consider renaming it without the leading underscores for clarity.
Apply this diff to rename the function and its references:
-function __useUploadThing<
+function useUploadThingInternal<And update the corresponding call:
-return __useUploadThing(initOpts.url, endpoint, opts);
+return useUploadThingInternal(initOpts.url, endpoint, opts);📝 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.
| function __useUploadThing< | |
| TRouter extends FileRouter, | |
| TEndpoint extends keyof TRouter, | |
| >( | |
| url: URL, | |
| endpoint: TEndpoint, | |
| opts?: UseUploadthingProps<TRouter, TEndpoint>, | |
| ) { | |
| function useUploadThingInternal< | |
| TRouter extends FileRouter, | |
| TEndpoint extends keyof TRouter, | |
| >( | |
| url: URL, | |
| endpoint: TEndpoint, | |
| opts?: UseUploadthingProps<TRouter, TEndpoint>, | |
| ) { |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (6)
packages/expo/src/image-picker.ts (1)
33-36: Approve changes with a minor suggestion for consistencyThe replacement of
INTERNAL_uploadthingHookGenwith__useUploadThingsimplifies the hook usage and improves code readability while maintaining the same functionality. This change is approved.For consistency with the function's parameter names, consider renaming the destructured variables:
- const { routeConfig, startUpload, isUploading } = __useUploadThing( + const { routeConfig, startUpload, isUploading } = __useUploadThing<TRouter, TEndpoint>( initOpts.url, endpoint, opts, );This change adds type parameters to
__useUploadThingfor better type inference and consistency with the function signature.packages/react/src/components/button.tsx (3)
25-25: Consider the implications of using an internal APIThe import of
__useUploadThingwith a double underscore prefix typically indicates an internal or private API. If this hook is intended for wider use within the project or by consumers of this package, consider removing the underscore prefix to make it a public API.If
__useUploadThingis meant to be internal, ensure it's not exposed in the public API documentation. If it's intended for public use, rename it touseUploadThingand update any related documentation.
Line range hint
112-133: Approved: Hook usage simplified and aligned with new importThe changes to the
__useUploadThinghook usage improve code clarity by directly passing the resolved URL and endpoint. The functionality appears to be maintained with this refactoring.Consider destructuring the props at the beginning of the component to improve readability:
const { url, endpoint, headers, onClientUploadComplete, onUploadProgress, onUploadError, onUploadBegin, onBeforeUploadBegin } = $props; const { startUpload, isUploading, routeConfig } = __useUploadThing( resolveMaybeUrlArg(url), endpoint, { signal: acRef.current.signal, headers, onClientUploadComplete: (res) => { // ... (rest of the implementation) }, onUploadProgress, onUploadError, onUploadBegin, onBeforeUploadBegin, }, );This change would make the code more readable and reduce the repetition of
$props.
Line range hint
1-324: Summary: Refactoring improves code structure, but consider API visibilityThe changes in this file are part of a larger refactoring effort to improve the upload functionality. The new
__useUploadThinghook simplifies the implementation and improves code clarity. However, the use of a double underscore prefix suggests it might be an internal API.Consider reviewing the API design of the
__useUploadThinghook:
- If it's intended for internal use only, ensure it's not exposed in the public API documentation.
- If it's meant for public use, rename it to remove the double underscore prefix and update any related documentation.
This will help maintain a clear distinction between public and internal APIs, improving the overall architecture and usability of the package.
packages/react/src/components/dropzone.tsx (2)
Line range hint
1-724: Update implementation to reflectonDropdeprecationThe
onDropprop in theUploadDropzonePropsinterface has been marked as deprecated in favor ofonChange. However, the current implementation still usesonDropinternally.To align with this deprecation:
- Update the
onDropfunction in the component to useonChangeinstead:const onDrop = useCallback( (acceptedFiles: File[]) => { - $props.onDrop?.(acceptedFiles); $props.onChange?.(acceptedFiles); setFiles(acceptedFiles); // If mode is auto, start upload immediately if (mode === "auto") void uploadFiles(acceptedFiles); }, [$props, mode, uploadFiles], );
Consider adding a console warning when
onDropis used, to inform developers about its deprecation.Update the component's documentation to reflect this change and guide users to use
onChangeinstead ofonDrop.If possible, provide a migration guide for users to update their existing implementations.
Line range hint
1-724: Summary of changes and recommendationsThis review has identified two main changes:
- The import of
useUploadThinghas been updated to__useUploadThing.- The
onDropprop has been deprecated in favor ofonChange.While these changes are minimal, they may have broader implications on the project. To ensure a smooth transition and maintain code quality:
Documentation Update: Revise the component's documentation to reflect the deprecation of
onDropand guide users towards usingonChange.Migration Guide: Consider creating a migration guide for users to update their existing implementations, especially regarding the
onDroptoonChangetransition.Consistency Check: Perform a codebase-wide audit to ensure consistent usage of
__useUploadThingand proper handling of the deprecatedonDropprop.Testing: Conduct thorough testing of the
UploadDropzonecomponent to verify that all functionalities, especially file uploads, work as expected with these changes.Performance Impact: Assess if these changes have any performance implications, particularly the use of
__useUploadThing.Code Cleanup: Consider refactoring this large component file for better maintainability, possibly splitting it into smaller, more focused components or hooks.
By addressing these points, you can ensure that the changes are properly integrated and minimize potential issues for users of this component.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- packages/expo/src/document-picker.ts (2 hunks)
- packages/expo/src/image-picker.ts (2 hunks)
- packages/react/src/components/button.tsx (2 hunks)
- packages/react/src/components/dropzone.tsx (2 hunks)
- packages/react/src/native.ts (1 hunks)
- packages/react/src/useUploadThing.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react/src/useUploadThing.ts
🔇 Additional comments (3)
packages/react/src/native.ts (1)
Line range hint
1-5: LGTM: Unchanged export ofgenerateReactHelpersThe export of
generateReactHelpersremains unchanged, which is good. The internal documentation comment is clear and helpful for users of the library.packages/expo/src/image-picker.ts (1)
Line range hint
1-138: Summary of changesThe modifications in this file primarily involve replacing the
INTERNAL_uploadthingHookGenfunction with the__useUploadThinghook from the@uploadthing/react/nativepackage. This change simplifies the hook usage and improves code readability while maintaining the same functionality.The overall structure and logic of the
useImageUploaderfunction remain intact, with the core functionality of handling image picking and uploading preserved. The changes are focused and don't introduce any apparent issues.To ensure the changes don't introduce any regressions, consider running the existing test suite and potentially adding new tests to cover the modified functionality.
packages/react/src/components/dropzone.tsx (1)
Line range hint
147-166: Verify functionality of renamed__useUploadThinghookThe
__useUploadThinghook (previouslyINTERNAL_uploadthingHookGen) is used here with the same interface as before. While the usage seems unchanged, it's crucial to ensure that all properties destructured from this hook are still available and functioning correctly.Please run the following verification steps:
- Confirm that all properties (
startUpload,isUploading,routeConfig) are still returned by the hook.- Test the component thoroughly to ensure that file uploads still work as expected.
- Check if there are any TypeScript errors related to this hook usage.
If any inconsistencies are found, please update the usage accordingly or consult with the team responsible for the hook's implementation.
✅ Verification successful
Verified:
__useUploadThinghook indropzone.tsxis correctly implemented with no issues found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the usage of __useUploadThing across the codebase echo "Searching for __useUploadThing usage:" rg "__useUploadThing" --type tsx --type ts echo "\nChecking for any remaining usage of INTERNAL_uploadthingHookGen:" rg "INTERNAL_uploadthingHookGen" --type tsx --type ts echo "\nVerifying imports from '../useUploadThing':" rg "from '../useUploadThing'" --type tsx --type tsLength of output: 574
Script:
#!/bin/bash # Verify the usage of __useUploadThing across the codebase by searching .ts and .tsx files echo "Searching for __useUploadThing usage in .ts and .tsx files:" rg "__useUploadThing" -g "*.ts" -g "*.tsx" echo "\nChecking for any remaining usage of INTERNAL_uploadthingHookGen in .ts and .tsx files:" rg "INTERNAL_uploadthingHookGen" -g "*.ts" -g "*.tsx" echo "\nVerifying imports from '../useUploadThing' in .ts and .tsx files:" rg "from '../useUploadThing'" -g "*.ts" -g "*.tsx"Length of output: 2255
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
.changeset/healthy-vans-trade.md (1)
1-7: Changeset looks good, consider adding more details.The changeset file is correctly formatted and includes patch updates for the relevant packages. The description of the chore is clear and concise.
Consider expanding the description to provide more context about the refactoring:
- chore: refactor internal hook generator to be more friendly for Vite React Refresh + chore: refactor internal hook generator to improve compatibility with Vite React Refresh + + This change enhances the internal hook generator to work more seamlessly with Vite's + React Refresh feature. It should improve the development experience when using these + packages in a Vite-based project.This additional information can help users and maintainers better understand the impact and motivation behind the change.
packages/expo/src/index.ts (1)
28-32: Consider adding documentation for the version check.The new version compatibility check is a valuable addition. To make it more visible to developers, consider adding a comment or updating the function's JSDoc to mention this check. This could help developers understand why they might see warnings related to peer dependencies.
Here's a suggested addition to the function's JSDoc:
/** * Generates React Native helpers for file uploads. * @param initOpts - Options for initializing the helpers. * @returns Object containing upload helpers. * @throws Warns if there's an invalid peer dependency for "@uploadthing/expo". */ export const generateReactNativeHelpers = <TRouter extends FileRouter>( initOpts?: GenerateTypedHelpersOptions, ) => { // ... existing code ... };packages/react/src/components/index.tsx (2)
24-29: LGTM: Added peer dependency validation for upload button.The addition of
warnIfInvalidPeerDependencyenhances the reliability of thegenerateUploadButtonfunction by validating peer dependencies.Consider extracting the common arguments to a constant to improve maintainability:
const PEER_DEPENDENCY_ARGS = [ "@uploadthing/react", peerDependencies.uploadthing, uploadthingClientVersion, ] as const; // Usage warnIfInvalidPeerDependency(...PEER_DEPENDENCY_ARGS);This would reduce duplication across the three generate functions and make future updates easier.
64-69: LGTM: Added peer dependency validation for uploader.The addition of
warnIfInvalidPeerDependencytogenerateUploadercompletes the implementation of peer dependency validation across all three generate functions.Given that this is the third occurrence of identical code, it's now clear that extracting the common arguments to a constant would significantly improve the maintainability of this file. Please consider implementing the suggestion from the previous comments:
const PEER_DEPENDENCY_ARGS = [ "@uploadthing/react", peerDependencies.uploadthing, uploadthingClientVersion, ] as const; // Usage in all three functions warnIfInvalidPeerDependency(...PEER_DEPENDENCY_ARGS);This refactoring will reduce code duplication and make future updates to the peer dependency validation process easier and less error-prone.
packages/shared/src/utils.ts (1)
255-265: LGTM with minor suggestions for improvementThe new
warnIfInvalidPeerDependencyfunction is well-implemented and serves its purpose effectively. It's a good addition for managing peer dependency versions. Here are a couple of suggestions for potential improvements:
- Consider making the warning message more generic by using the
pkgparameter in the message instead of hardcoding "uploadthing". This would make the function more reusable for other packages:console.warn( `!!!WARNING::: ${pkg} requires "${pkg}@${required}", but version "${toCheck}" is installed`, );
- While unlikely, it might be worth adding error handling for potential errors thrown by
semverLite:try { if (!semverLite(required, toCheck)) { console.warn(/* ... */); } } catch (error) { console.error(`Error checking semver for ${pkg}:`, error); }These changes would enhance the function's flexibility and robustness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- .changeset/healthy-vans-trade.md (1 hunks)
- packages/expo/src/document-picker.ts (2 hunks)
- packages/expo/src/image-picker.ts (2 hunks)
- packages/expo/src/index.ts (2 hunks)
- packages/react/src/components/button.tsx (2 hunks)
- packages/react/src/components/dropzone.tsx (2 hunks)
- packages/react/src/components/index.tsx (4 hunks)
- packages/react/src/native.ts (1 hunks)
- packages/react/src/useUploadThing.ts (3 hunks)
- packages/shared/src/utils.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/expo/src/document-picker.ts
- packages/expo/src/image-picker.ts
- packages/react/src/components/button.tsx
- packages/react/src/components/dropzone.tsx
- packages/react/src/native.ts
🔇 Additional comments (10)
packages/expo/src/index.ts (2)
4-5: LGTM: New imports for version checking.The added imports for
warnIfInvalidPeerDependency,uploadthingClientVersion, andpeerDependenciesare appropriate for implementing version compatibility checks. This is a good practice to ensure that the package versions are compatible.Also applies to: 8-8
28-32: LGTM: Added peer dependency version check.The addition of
warnIfInvalidPeerDependencyis a good practice. It helps to catch potential version incompatibilities early, which can prevent subtle bugs and improve the developer experience.packages/react/src/components/index.tsx (3)
1-8: LGTM: New imports for peer dependency validation.The added imports for
warnIfInvalidPeerDependency,uploadthingClientVersion, andpeerDependenciesare appropriate for implementing the new peer dependency validation feature.
44-49: LGTM: Added peer dependency validation for upload dropzone.The addition of
warnIfInvalidPeerDependencytogenerateUploadDropzoneis consistent with the changes made togenerateUploadButton.As mentioned in the previous comment, consider extracting the common arguments to a constant to improve maintainability across all three generate functions.
Line range hint
1-69: Summary: Peer dependency validation added to all generate functions.The changes in this file consistently implement peer dependency validation for
generateUploadButton,generateUploadDropzone, andgenerateUploaderfunctions. This enhancement improves the reliability of the package by ensuring compatibility between@uploadthing/react, the expected version inpackage.json, and theuploadthing/clientversion.While the implementation is correct and consistent, consider the suggested refactoring to improve code maintainability. Overall, these changes represent a valuable addition to the package's robustness.
packages/react/src/useUploadThing.ts (4)
12-12: LGTM: Added peer dependency checkThe addition of
warnIfInvalidPeerDependencyimport is a good practice. It helps ensure that the package is used with compatible versions of its dependencies, potentially preventing runtime issues.
45-141: LGTM: Improved code organization with__useUploadThingInternalThe introduction of
__useUploadThingInternalis a good refactoring step. It improves code organization by encapsulating the upload logic and separating concerns. The use of TypeScript generics ensures type safety throughout the upload process.Consider addressing the TODO comment in a future update
There's a TODO comment about potential API refactoring:
/** * This is the only way to introduce this as a non-breaking change * TODO: Consider refactoring API in the next major version */Consider creating a GitHub issue to track this potential refactoring for the next major version update.
Previous suggestion: Refactor
startUploadfunctionAs mentioned in a previous review, the
startUploadfunction (now part of__useUploadThingInternal) is still quite extensive. Consider breaking it down into smaller helper functions for improved readability and maintainability.
146-159: LGTM: EnhancedgenerateReactHelpersfunctionThe changes to
generateReactHelpersare well-implemented:
- The addition of
warnIfInvalidPeerDependencyenhances the robustness of the code by checking for compatible peer dependencies.- The refactoring of
useUploadThingto use__useUploadThingInternalis consistent with the new structure and improves code organization.These changes contribute to better code quality and maintainability.
Line range hint
1-184: LGTM: Improved overall structure and organizationThe refactoring of this file, particularly with the introduction of
__useUploadThingInternal, significantly improves the code organization. The separation of concerns enhances maintainability, readability, and allows for easier testing and future modifications.The changes demonstrate a thoughtful approach to code structure, which will benefit the project in the long term.
packages/shared/src/utils.ts (1)
Line range hint
1-265: Overall assessment of changesThe addition of the
warnIfInvalidPeerDependencyfunction to this utility file is appropriate and well-implemented. It enhances the project's ability to manage peer dependencies, which is crucial for maintaining compatibility across different package versions.The function integrates seamlessly with the existing codebase, utilizing the
semverLitefunction that was already present in the file. This demonstrates good code reuse and consistency in implementation.No other changes were made to the file, which helps maintain the stability of existing functionality. The new function adds value without disrupting the current structure or behavior of the utility module.
| // @ts-expect-error - input may not be defined on the type | ||
| input, |
There was a problem hiding this comment.
Address the TypeScript error instead of suppressing it
The @ts-expect-error comment is used to suppress a TypeScript error when passing input. Suppressing type errors can hide underlying issues and reduce type safety.
Consider adjusting the type definitions to properly reflect that input may be optional. For example, update the uploadFiles function signature to accept an optional input parameter:
async function uploadFiles<TEndpoint extends keyof TRouter>(
endpoint: TEndpoint,
{ ..., input?: InferredInput }
) {
// ...
}This change ensures that TypeScript correctly understands the optional nature of input, eliminating the need for @ts-expect-error.
| * The actual hook we export for public usage is generated from `generateReactHelpers` | ||
| * which has the URL and FileRouter generic pre-bound. | ||
| */ | ||
| export function __useUploadThingInternal< |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider renaming __useUploadThingInternal to avoid leading underscores
The function __useUploadThingInternal is exported and used within the module. In JavaScript/TypeScript, leading underscores are typically used to indicate private or internal functions not intended for external use. Since this function is part of the module's internal API, consider renaming it without the leading underscores for clarity.
Apply this diff to rename the function and its references:
-function __useUploadThingInternal<
+function useUploadThingInternal<And update the corresponding call:
-return __useUploadThingInternal(url, endpoint, opts);
+return useUploadThingInternal(url, endpoint, opts);Committable suggestion was skipped due to low confidence.
| } catch (e) { | ||
| /** | ||
| * This is the only way to introduce this as a non-breaking change | ||
| * TODO: Consider refactoring API in the next major version | ||
| */ | ||
| if (e instanceof UploadAbortedError) throw e; | ||
|
|
||
| let error: UploadThingError<inferErrorShape<TRouter>>; | ||
| if (e instanceof UploadThingError) { | ||
| error = e as UploadThingError<inferErrorShape<TRouter>>; | ||
| } else { | ||
| error = INTERNAL_DO_NOT_USE__fatalClientError(e as Error); | ||
| console.error( | ||
| "Something went wrong. Please contact UploadThing and provide the following cause:", | ||
| error.cause instanceof Error ? error.cause.toString() : error.cause, | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| const routeConfig = useRouteConfig(initOpts.url, endpoint as string); | ||
| await opts?.onUploadError?.(error); | ||
| } finally { |
There was a problem hiding this comment.
Enhance error handling to prevent exposing internal details
The error handling between lines 121-124 logs detailed error information to the console, which might include sensitive internal data. Exposing such information can be a security risk in production environments.
Consider modifying the error logging to provide user-friendly messages without revealing sensitive details. For example:
console.error(
- "Something went wrong. Please contact UploadThing and provide the following cause:",
- error.cause instanceof Error ? error.cause.toString() : error.cause,
+ "An unexpected error occurred during the upload process. Please try again later.",
);Additionally, you might want to log the detailed error information separately in a secure manner accessible only to developers for debugging purposes.
📝 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.
| } catch (e) { | |
| /** | |
| * This is the only way to introduce this as a non-breaking change | |
| * TODO: Consider refactoring API in the next major version | |
| */ | |
| if (e instanceof UploadAbortedError) throw e; | |
| let error: UploadThingError<inferErrorShape<TRouter>>; | |
| if (e instanceof UploadThingError) { | |
| error = e as UploadThingError<inferErrorShape<TRouter>>; | |
| } else { | |
| error = INTERNAL_DO_NOT_USE__fatalClientError(e as Error); | |
| console.error( | |
| "Something went wrong. Please contact UploadThing and provide the following cause:", | |
| error.cause instanceof Error ? error.cause.toString() : error.cause, | |
| ); | |
| } | |
| }); | |
| const routeConfig = useRouteConfig(initOpts.url, endpoint as string); | |
| await opts?.onUploadError?.(error); | |
| } finally { | |
| } catch (e) { | |
| /** | |
| * This is the only way to introduce this as a non-breaking change | |
| * TODO: Consider refactoring API in the next major version | |
| */ | |
| if (e instanceof UploadAbortedError) throw e; | |
| let error: UploadThingError<inferErrorShape<TRouter>>; | |
| if (e instanceof UploadThingError) { | |
| error = e as UploadThingError<inferErrorShape<TRouter>>; | |
| } else { | |
| error = INTERNAL_DO_NOT_USE__fatalClientError(e as Error); | |
| console.error( | |
| "An unexpected error occurred during the upload process. Please try again later.", | |
| ); | |
| } | |
| await opts?.onUploadError?.(error); | |
| } finally { |
| import { createRouter } from "./router"; | ||
| import { uploadRouter } from "./server/uploadthing"; | ||
|
|
||
| globalThis.__UPLOADTHING = extractRouterConfig(uploadRouter); |
There was a problem hiding this comment.
kinda hate this but it works... not sure where else we can import uploadRouter without leaking it to client bundle.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
examples/minimal-tanstack-start/app/ssr.tsx (2)
14-17: LGTM: Default export follows TanStack Start pattern.The default export correctly sets up the server-side handler using
createStartHandlerwith appropriate options. The use ofdefaultStreamHandlersuggests that streaming responses are being utilized, which is good for performance.Consider adding a brief comment explaining the purpose of this setup, especially for developers who might not be familiar with TanStack Start. For example:
// Set up the TanStack Start server-side handler with streaming responses export default createStartHandler({ createRouter, getRouterManifest, })(defaultStreamHandler);
1-17: LGTM: File structure is concise and logical.The file successfully sets up a minimal TanStack Start application with uploadthing integration. It covers all necessary aspects including imports, global variable declaration, and default export.
For future iterations or more complex setups, consider:
- Adding error handling to make the application more robust.
- Implementing additional configuration options if needed.
- Adding comments to explain the purpose and flow of the setup, especially for developers new to TanStack or uploadthing.
examples/minimal-tanstack-start/app/router.tsx (1)
8-24: LGTM: Effective UploadThing integration, with considerations for future improvements.The
routerWithUploadThingfunction successfully handles both server-side and client-side scenarios for UploadThing configuration. However, consider the following points:
- Security: Ensure that
globalThis.__UPLOADTHINGdoesn't contain sensitive information, as it's being transferred to the client.- Future refactoring: As the comment suggests, consider moving this functionality into
@uploadthing/reactfor better modularity and reusability.Consider implementing a more robust method for configuration transfer that doesn't rely on global variables, which could potentially be a security risk if not managed carefully.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- examples/minimal-tanstack-start/app/router.tsx (1 hunks)
- examples/minimal-tanstack-start/app/ssr.tsx (1 hunks)
🔇 Additional comments (5)
examples/minimal-tanstack-start/app/ssr.tsx (2)
1-10: LGTM: Imports are appropriate and well-organized.The imports cover all necessary dependencies for setting up a TanStack application with uploadthing integration. The separation of concerns between TanStack, uploadthing, and local modules is clear.
12-12: Global variable declaration looks necessary but exercise caution.The global variable
__UPLOADTHINGis likely required for uploadthing to function correctly in this context. However, be aware that global variables can potentially lead to naming conflicts and make the code harder to maintain. Ensure that this approach is recommended by the uploadthing documentation.To verify if this is a recommended approach, let's check the uploadthing documentation:
✅ Verification successful
Global variable declaration is necessary and properly used.
The global variable
__UPLOADTHINGis utilized consistently across multiple files to configure uploadthing functionality, indicating that its declaration is essential for the module's operation. Ensure that this approach aligns with uploadthing's recommended practices and consider documenting its usage for future maintainability.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for documentation about __UPLOADTHING global variable rg --type md -i "global.*__uploadthing"Length of output: 41
Script:
#!/bin/bash # Description: Search for usages of __UPLOADTHING in TypeScript and JavaScript files rg "__UPLOADTHING" --type ts --type tsx --type js --type jsxLength of output: 88
Script:
#!/bin/bash # Description: Search for usages of __UPLOADTHING in TypeScript and JavaScript files rg "__UPLOADTHING" -g "**/*.{ts,tsx,js,jsx}"Length of output: 1229
examples/minimal-tanstack-start/app/router.tsx (3)
1-6: LGTM: Imports are well-structured and appropriate.The imports are correctly set up, with a clear distinction between external and local imports. Renaming
createRoutertocreateTanStackRouteris a good practice to avoid naming conflicts with the localcreateRouterfunction.
26-32: LGTM: Clear and concise router creation with UploadThing integration.The
createRouterfunction efficiently combines the creation of a TanStack router with the UploadThing integration. This approach provides a clean API for consumers of this module.
34-38: LGTM: Proper type augmentation for enhanced type safety.The module augmentation for '@tanstack/react-router' is a good practice. It ensures type safety when using the custom router with TanStack's routing system, providing a better developer experience and reducing potential runtime errors.
this doesn't work as intened. let's ship the example as is and add the ssr improvements later
juliusmarminge
changed the title
juliusmarminge
changed the title
Adds minimal-tanstack-start example.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores