fix: prefer browser recognized mime type over file extension lookup #991
Conversation
🦋 Changeset detectedLatest commit: 662ab99 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 23 minutes and 23 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 changes introduce a new function, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
📦 Bundle size comparison
|
More templates
commit: |
juliusmarminge
changed the title
file.type as fallback type to getTypeFromFileNamefile.type as fallback type to getTypeFromFileName
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/shared/src/utils.ts (1)
41-48: LGTM! Consider adding explicit return type.The new
getDefaultRouteConfigValuesfunction is a great addition. It consolidates default configuration values, improving maintainability and reducing redundancy in the codebase.Consider adding an explicit return type to improve readability:
export function getDefaultRouteConfigValues(type: FileRouterInputKey): { maxFileSize: FileSize; maxFileCount: number; minFileCount: number; contentDisposition: ContentDisposition; } { // ... function body ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/shared/src/utils.ts (4 hunks)
- packages/uploadthing/src/internal/route-config.ts (1 hunks)
🔇 Additional comments (4)
packages/uploadthing/src/internal/route-config.ts (2)
Line range hint
32-124: Overall impact onassertFilesMeetConfigfunction is positive.The addition of
file.typeas an argument togetTypeFromFileNameenhances the function's ability to accurately determine file types. This change:
- Improves flexibility in type determination.
- Maintains the existing logic for file size and count validation.
- Does not introduce any new risks or potential issues.
The function's overall structure and purpose remain intact, with the added benefit of more robust file type handling.
77-77: LGTM! VerifygetTypeFromFileNameimplementation.The addition of
file.typeas an argument togetTypeFromFileNamealigns with the PR objective to use it as a fallback when determining the file type. This change enhances the flexibility of type determination.To ensure the change is fully implemented, please verify the
getTypeFromFileNamefunction:packages/shared/src/utils.ts (2)
67-67: LGTM! Good use of the new utility function.The changes to
fillInputRouteConfigeffectively utilize the newgetDefaultRouteConfigValuesfunction. This modification improves code organization while maintaining the existing functionality.Also applies to: 78-78
Line range hint
1-368: Overall, the changes look good and align with the PR objectives.The modifications to this file improve code organization by introducing the
getDefaultRouteConfigValuesfunction and enhance flexibility with the addition of thefallbackTypeparameter ingetTypeFromFileName. These changes are consistent with the existing codebase and maintain backward compatibility.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
examples/minimal-appdir/src/server/uploadthing.ts (1)
35-35: LGTM! Consider adding a comment for clarity.The addition of
maxFileCount: 10for blob files is a good improvement. It aligns with the PR objectives and is consistent with the configuration style of other file types.For consistency and clarity, consider adding a brief comment explaining the purpose of this limit, similar to other configurations in the file. For example:
blob: { maxFileSize: "8GB", + // Limit the number of blob files that can be uploaded maxFileCount: 10, },packages/uploadthing/src/client.ts (1)
Line range hint
69-74: Verify thattypematchesrouteConfigkeys inisValidFileSizeWhen accessing
routeConfig[type], ensure thattypecorresponds to a valid key inrouteConfig. Iftypeis a file extension (e.g.,"png") butrouteConfigkeys are MIME types (e.g.,"image/png"),routeConfig[type]will beundefined, leading to errors.Consider mapping the file extension to the correct MIME type before accessing
routeConfig:Micro.runSync( getTypeFromFileName(file.name, objectKeys(routeConfig), file.type).pipe( - Micro.flatMap((type) => fileSizeToBytes(routeConfig[type]!.maxFileSize)), + Micro.flatMap((type) => { + const mimeType = extensionToMimeType(type); + return fileSizeToBytes(routeConfig[mimeType]!.maxFileSize); + }), Micro.map((maxFileSize) => file.size <= maxFileSize), Micro.orElseSucceed(() => false), ), );Ensure you have a function
extensionToMimeTypethat correctly maps file extensions to MIME types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- examples/minimal-appdir/src/server/uploadthing.ts (1 hunks)
- packages/uploadthing/src/client.ts (2 hunks)
- packages/uploadthing/src/internal/handler.ts (1 hunks)
🔇 Additional comments (1)
packages/uploadthing/src/internal/handler.ts (1)
533-533: LGTM! The use offile.typeas a fallback is appropriateThe addition of
file.typeas a fallback parameter ingetTypeFromFileNameenhances the robustness of file type determination when the file name lacks an extension. This implementation aligns with the PR objective and improves functionality.
vercel
bot
temporarily deployed
to
Preview – legacy-docs-uploadthing
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/uploadthing/test/request-handler.test.ts (2)
204-228: LGTM! Consider adding an assertion for the response status code.This test case effectively verifies the behavior of using the file name to determine the MIME type when the browser doesn't recognize the file type. It aligns well with the PR objectives.
Consider adding an explicit assertion for the response status code before checking the response body:
expect(res.status).toBe(200); +expect(res.ok).toBe(true); await expect(res.json()).resolves.toMatchObject([ { name: "foo.png", url: expect.stringContaining("x-ut-file-type=image"), }, ]);This addition would make the test more robust by explicitly checking both the status code and the
okproperty.
230-257: LGTM! Remove duplicate status assertion and consider adding anokcheck.This test case effectively verifies the fallback behavior of using the browser-recognized type when the file name doesn't have an extension. It aligns well with the PR objectives.
There's a duplicate assertion for the response status. Consider removing the first one and adding an
okcheck:- expect(res.status).toBe(200); - expect(middlewareMock).toHaveBeenCalledWith( expect.objectContaining({ files: [{ name: "foo", size: 48, type: "image/png" }], }), ); expect(res.status).toBe(200); + expect(res.ok).toBe(true); await expect(res.json()).resolves.toMatchObject([ { name: "foo", url: expect.stringContaining("x-ut-file-type=image%252Fpng"), }, ]);This change removes the redundant assertion and adds an extra check for the
okproperty, making the test more robust.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/uploadthing/src/internal/handler.ts (1 hunks)
- packages/uploadthing/test/request-handler.test.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/uploadthing/src/internal/handler.ts
🔇 Additional comments (1)
packages/uploadthing/test/request-handler.test.ts (1)
204-257: Great addition of test cases for the new file type determination logic!These new test cases effectively cover the scenarios described in the PR objectives:
- Using the file name to determine the MIME type when the browser doesn't recognize the file type.
- Falling back to the browser-recognized type when the file name doesn't have an extension.
The tests are well-structured and align with the existing testing patterns in the file. They provide good coverage for the new functionality and help ensure the correct behavior of the
getTypeFromFileNamemethod.
juliusmarminge
changed the title
file.type as fallback type to getTypeFromFileName
Prefer
file.typewhen matching route config type. Fallback to lookup based on extensionFile { name: 'foo.png', type: 'image/png' } => image/pngFile { name: 'foo', type: 'image/png' } => image/pngFile { name: 'foo.jpg', type: 'image/png' } => image/pngFile { name: 'foo.jpg', type: '' } => image/jpgx-ref: https://discord.com/channels/966627436387266600/1102510616326967306/1289998476721262623
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes