-
Notifications
You must be signed in to change notification settings - Fork 346
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: UTApi.generateSignedURL
#1146
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
🦋 Changeset detectedLatest commit: ac1f2ca 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 |
Warning Rate limit exceeded@juliusmarminge has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 57 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. 📒 Files selected for processing (4)
WalkthroughThis pull request updates the API documentation and SDK code to replace the deprecated Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant UTApi
participant Shared
Client->>UTApi: Call generateSignedURL(key, options)
UTApi->>Shared: Validate "expiresIn" and generate presigned URL
Shared-->>UTApi: Return generated { ufsUrl }
UTApi->>Client: Return response { ufsUrl }
Possibly related PRs
Suggested reviewers
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
More templates
commit: |
📦 Bundle size comparison
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/uploadthing/src/sdk/index.ts (1)
388-444
: Consider extracting repeated expiration checks into a helper function.Both this new
generateSignedURL
method and the existinggetSignedURL
method implement very similar checks forexpiresIn
. To reduce duplication and ensure consistency, consider refactoring them into a shared utility (e.g.,validateExpiresIn()
).Additionally, note that the log span name is
"getSignedURL"
whereas this method is calledgenerateSignedURL
. For clarity, rename the log span or adopt a generic name like"signedURL"
.+// Example helper for expiration validation +function validateExpiresIn(expiresIn: number) { + if (isNaN(expiresIn)) { + throw new UploadThingError({ + code: "BAD_REQUEST", + message: "expiresIn must be a valid time string or number (in seconds)." + }); + } + if (expiresIn > 86400 * 7) { + throw new UploadThingError({ + code: "BAD_REQUEST", + message: "expiresIn must be less than 7 days (604800 seconds)." + }); + } + return expiresIn; +} + generateSignedURL = async (key: string, opts?: GetSignedURLOptions) => { guardServerOnly(); - const expiresIn = parseTimeToSeconds(opts?.expiresIn ?? "5 minutes"); - if (opts?.expiresIn && isNaN(expiresIn)) { - ... - } - if (expiresIn > 86400 * 7) { - ... - } + const expiresIn = validateExpiresIn( + parseTimeToSeconds(opts?.expiresIn ?? "5 minutes") + ); // ... };packages/uploadthing/src/_internal/config.ts (1)
95-102
: UnusedUtfsHost
constant.
UfsHost
is used in the SDK, butUtfsHost
currently appears unused. Consider removingUtfsHost
if it’s not needed or ensure it’s referenced where appropriate.packages/uploadthing/src/sdk/types.ts (1)
104-112
: Align the default value notation with actual usage.The code defaults
expiresIn
to"5 minutes"
, while the documentation states@default 5min
. Consistency helps avoid confusion.- * @default 5min + * @default 5 minutesdocs/src/app/(docs)/api-reference/ut-api/page.mdx (2)
422-461
: Consider enhancing the documentation forgenerateSignedURL
.The documentation would benefit from:
- A note about the performance improvement (e.g., "This method is significantly faster as it avoids the extra network round-trip").
- Adding the
keyType
parameter to maintain consistency withgetSignedURL
.## `generateSignedURL` {{ tag: 'method', since: '7.5'}} -Generate a presigned URL for a private file. +Generate a presigned URL for a private file. This method is significantly faster than `getSignedURL` as it avoids the extra network round-trip. ### Parameters <Properties> <Property name="key" type="string" since="7.5" required> The key of the file to get a signed URL for </Property> + <Property + name="options.keyType" + type="customId | fileKey" + since="7.5" + defaultValue="fileKey" + > + The type of key to use for the signed URL. + </Property> <Property name="options.expiresIn" type="number | TimeString" since="7.5">
466-468
: Fix grammar in the deprecation notice.Add a comma after "API" to separate the independent clauses.
-file. This method is no longer recommended as it makes a fetch request to the UploadThing API which incurs redundant latency. Use +file. This method is no longer recommended as it makes a fetch request to the UploadThing API, which incurs redundant latency. Use🧰 Tools
🪛 LanguageTool
[uncategorized] ~467-~467: Possible missing comma found.
Context: ...akes a fetch request to the UploadThing API which incurs redundant latency. Use [`g...(AI_HYDRA_LEO_MISSING_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/src/app/(docs)/api-reference/ut-api/page.mdx
(2 hunks)docs/src/app/(docs)/concepts/regions-acl/page.mdx
(1 hunks)docs/src/app/(docs)/working-with-files/page.mdx
(1 hunks)packages/uploadthing/src/_internal/config.ts
(1 hunks)packages/uploadthing/src/sdk/index.ts
(3 hunks)packages/uploadthing/src/sdk/types.ts
(1 hunks)packages/uploadthing/test/__test-helpers.ts
(1 hunks)packages/uploadthing/test/sdk.test.ts
(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/src/app/(docs)/api-reference/ut-api/page.mdx
[uncategorized] ~467-~467: Possible missing comma found.
Context: ...akes a fetch request to the UploadThing API which incurs redundant latency. Use [`g...
(AI_HYDRA_LEO_MISSING_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: typecheck
- GitHub Check: build
- GitHub Check: analyze-bundle (current-pr)
- GitHub Check: e2e-node (backend-adapters)
- GitHub Check: e2e-node (minimal-pagedir)
- GitHub Check: lint
- GitHub Check: e2e-node (minimal-appdir)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (8)
packages/uploadthing/src/sdk/index.ts (4)
16-20
: Imports appear valid and consistently used.All imported symbols—from
generateSignedURL
touploadThingError
—are referenced in this file. Good job keeping the dependencies tight and relevant.
22-27
: Imports from_internal/config
look appropriate.These new constants (
ApiUrl
,UfsHost
,UPLOADTHING_VERSION
,UTToken
) are now used throughout the file without redundancy. Everything appears consistent.
446-453
: Deprecation notice is clear.The documentation clearly communicates that
getSignedURL
will be deprecated. This helps guide users toward the recommended method. Nicely done.
480-480
: VerifyufsUrl
is consistently returned across the codebase.Adding
ufsUrl
toGetSignedUrlResponse
is beneficial. Ensure the server always populates it and no code paths return undefined.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for places where GetSignedUrlResponse or 'ufsUrl' is referenced rg -A 3 'ufsUrl'Length of output: 16846
Subject:
ufsUrl
Consistency Verified Across CodebaseThe search results confirm that
ufsUrl
is defined and consistently used in the response schemas, SDK utilities, and throughout the test suite (which validates its format viaufsUrlPattern
). There are no indications that any server code paths returnundefined
forufsUrl
.
- Confirmed definition in response schemas (e.g.,
S.String
inpackages/uploadthing/src/sdk/index.ts
and internal types).- Consistent usage in client-side and server-side examples.
- Tests validate
ufsUrl
with expected URL signature patterns.docs/src/app/(docs)/concepts/regions-acl/page.mdx (1)
75-77
: LGTM! Documentation updated correctly.The documentation has been updated to reflect the new
generateSignedURL
method name, maintaining consistency with the API changes.docs/src/app/(docs)/api-reference/ut-api/page.mdx (1)
1-560
: Documentation is well-structured and comprehensive!The documentation provides clear explanations, helpful code examples, and thorough parameter descriptions for all methods.
🧰 Tools
🪛 LanguageTool
[grammar] ~205-~205: Possible subject-verb agreement error.
Context: ...mUrl` {{ tag: 'method', since: '5.3'}} Have a file hosted somewhere else you want t...(WH_AUX_SG_NOUN_AGR)
[grammar] ~229-~229: The verb form ‘are’ does not appear to fit in this context.
Context: ...sponse[] ``` ### Parameters The first argument are the URLs of the files you want to uploa...(SINGULAR_NOUN_VERB_AGREEMENT)
[style] ~229-~229: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ... argument are the URLs of the files you want to upload. They may also be an object with...(REP_WANT_TO_VB)
[style] ~230-~230: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...bject with aurl
property in case you want to override thename
, or set a `customId...(REP_WANT_TO_VB)
[typographical] ~308-~308: Do not use a colon (:) before a series that is introduced by a preposition (‘since’). Remove the colon or add a noun or a noun phrase after the preposition.
Context: ...ct##
getFileUrls{{ tag: 'method', since: '4.0', deprecated: true }}
getFileUrl...(RP_COLON)
[uncategorized] ~467-~467: Possible missing comma found.
Context: ...akes a fetch request to the UploadThing API which incurs redundant latency. Use [`g...(AI_HYDRA_LEO_MISSING_COMMA)
packages/uploadthing/test/__test-helpers.ts (1)
125-126
: LGTM!The
ufsUrl
property follows the expected format and includes the required signature parameter.packages/uploadthing/test/sdk.test.ts (1)
378-429
: LGTM! Comprehensive test coverage for the newgenerateSignedURL
method.The test suite thoroughly covers:
- Basic URL generation
- Different formats of
expiresIn
parameter- Error handling for invalid inputs
- Validation of URL format and signature
[`UTApi.generateSignedURL`](/api-reference/ut-api#get-signed-url). Here's a | ||
reference implementation using Node.js crypto: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix incorrect API reference link.
The link to the API reference points to #get-signed-url
but should point to #generate-signed-url
to match the new method name.
-[`UTApi.generateSignedURL`](/api-reference/ut-api#get-signed-url)
+[`UTApi.generateSignedURL`](/api-reference/ut-api#generate-signed-url)
📝 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.
[`UTApi.generateSignedURL`](/api-reference/ut-api#get-signed-url). Here's a | |
reference implementation using Node.js crypto: | |
[`UTApi.generateSignedURL`](/api-reference/ut-api#generate-signed-url). Here's a | |
reference implementation using Node.js crypto: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/uploadthing/src/sdk/index.ts (1)
477-482
: Add JSDoc comments for response fields.Consider adding JSDoc comments to explain the difference between
url
andufsUrl
fields for better API documentation.class GetSignedUrlResponse extends S.Class<GetSignedUrlResponse>( "GetSignedUrlResponse", )({ + /** The legacy signed URL */ url: S.String, + /** The UFS (Unified File Storage) signed URL */ ufsUrl: S.String, }) {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/uploadthing/src/sdk/index.ts
(3 hunks)packages/uploadthing/test/__test-helpers.ts
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/uploadthing/test/__test-helpers.ts
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: typecheck
- GitHub Check: lint
- GitHub Check: analyze-bundle (current-pr)
- GitHub Check: build
- GitHub Check: e2e-node (backend-adapters)
- GitHub Check: e2e-node (minimal-pagedir)
- GitHub Check: e2e-node (minimal-appdir)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (3)
packages/uploadthing/src/sdk/index.ts (3)
16-27
: LGTM! Import statements are well-organized.The new imports are correctly structured and include all necessary dependencies for the URL generation functionality.
388-445
: Well-implemented URL generation with proper validation!The implementation is robust with several notable benefits:
- Eliminates extra API call for better performance
- Includes comprehensive validation for the
expiresIn
parameter- Uses proper protocol selection based on environment
447-454
: Excellent deprecation notice with clear migration path!The documentation effectively communicates:
- The deprecation timeline
- Performance implications of the old method
- Clear reference to the new recommended method
Co-authored-by: Mark R. Florkowski <[email protected]>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary by CodeRabbit
Documentation
generateSignedURL
method.New Features
ufsUrl
.Tests
generateSignedURL
method, including various scenarios for theexpiresIn
parameter.