Skip to content
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: return file hash from uploaded object #978

Merged
merged 14 commits into from
Oct 6, 2024
Merged

feat: return file hash from uploaded object #978

merged 14 commits into from
Oct 6, 2024

Conversation

juliusmarminge
Copy link
Collaborator

@juliusmarminge juliusmarminge commented Sep 23, 2024

syncs https://github.com/pingdotgg/uploadthing-infra/pull/556

image

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Enhanced upload functionality with the addition of a fileHash property in the upload completion callback, enabling better tracking of uploaded objects.
    • Updated return values for upload functions to include the new fileHash property, providing more comprehensive data post-upload.
    • Introduced a limit on the maximum file count for uploads, set to 4.
  • Documentation

    • Updated TypeScript documentation link for improved guidance on configuring TypeScript in Next.js applications.
  • Bug Fixes

    • Improved consistency in test cases by validating the presence of the new fileHash property across various upload operations.
    • Removed unnecessary console logging in the middleware function for cleaner operation.

Copy link

changeset-bot bot commented Sep 23, 2024

🦋 Changeset detected

Latest commit: 2dd7a1a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
uploadthing Minor

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

Copy link

vercel bot commented Sep 23, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
docs-uploadthing ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 3, 2024 8:20pm
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
legacy-docs-uploadthing ⬜️ Skipped (Inspect) Oct 3, 2024 8:20pm

Copy link
Contributor

coderabbitai bot commented Sep 23, 2024

Walkthrough

The changes introduced in this pull request enhance the upload functionality by adding a new fileHash property to various upload-related data structures. Key updates include modifications to the onUploadComplete callback to return an object hash, adjustments to return types across several functions, and updates to test cases to validate the presence of the new property. These changes aim to improve the tracking and identification of uploaded files through the inclusion of the hash value.

Changes

File Path Change Summary
.changeset/nice-crabs-compete.md Added enhancement to onUploadComplete callback to return an object hash for better tracking.
examples/minimal-appdir/next-env.d.ts Updated TypeScript documentation URL in comments.
packages/uploadthing/src/internal/shared-schemas.ts Added new properties hash and fileHash of type S.String to UploadedFileData class.
packages/uploadthing/src/internal/types.ts Introduced new type UploadPutResult to represent the response from the upload server.
packages/uploadthing/src/internal/upload.browser.ts Updated return type of uploadFile function to include fileHash in the response object.
packages/uploadthing/src/internal/upload.server.ts Modified return type of uploadWithoutProgress function to include UploadPutResult.
packages/uploadthing/src/sdk/utils.ts Updated return value of uploadFile function to include fileHash along with other properties.
packages/uploadthing/test/__test-helpers.ts Added a new property fileHash to the object returned in the extended it function.
packages/uploadthing/test/client.test.ts Updated expected output in test cases to include fileHash property.
packages/uploadthing/test/request-handler.test.ts Added fileHash property to objects in the .onUploadComplete() test suite.
packages/uploadthing/test/sdk.test.ts Updated test cases to expect fileHash property in various upload function outputs.
examples/minimal-appdir/src/server/uploadthing.ts Updated maxFileCount to 4 and modified middleware to return an object with property foo.

Possibly related PRs

  • chore(release): 📦 version packages #951: This PR includes updates to the uploadthing package, which addresses a regression in the Next.js pages adapter's response body and ensures the onUploadBegin function runs correctly. These changes are directly related to the upload functionality, which is also a focus of the main PR.
  • fix: Fix withUt in monorepos #987: This PR modifies the withUt function to improve module resolution, which may indirectly affect the upload functionality by ensuring that the upload-related modules are correctly resolved in a monorepo context.
  • docs: Fixed SolidStart and slightly changed Tanstack/Start #988: This PR enhances documentation related to the SolidStart setup and UploadThing integration, which is relevant as it clarifies the usage of the upload functionality introduced in the main PR.

Suggested labels

@uploadthing/react, 📚 documentation

Suggested reviewers

  • markflorkowski

Poem

🐇 In the meadow where uploads play,
A hash now joins the file ballet.
With every click, a trace to find,
A digital dance, so well-defined.
Hopping through code, we celebrate,
New paths to track, oh, isn’t it great! 🎉


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between ac31b41 and 2dd7a1a.

📒 Files selected for processing (1)
  • examples/minimal-appdir/src/server/uploadthing.ts (0 hunks)
💤 Files with no reviewable changes (1)
  • examples/minimal-appdir/src/server/uploadthing.ts

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

github-actions bot commented Sep 23, 2024

📦 Bundle size comparison

Bundle Size (gzip) Visualization
Main 26.03KB See Treemap 📊
PR (4c48622) 26.03KB See Treemap 📊
Diff ↑7.00B

Copy link

pkg-pr-new bot commented Sep 23, 2024

Open in Stackblitz

More templates

pnpm add https://pkg.pr.new/pingdotgg/uploadthing/@uploadthing/nuxt@978
pnpm add https://pkg.pr.new/pingdotgg/uploadthing@978

commit: 2dd7a1a

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Outside diff range and nitpick comments (2)
.changeset/nice-crabs-compete.md (1)

1-5: LGTM! Consider enhancing the description slightly.

The changeset file is correctly formatted and appropriately describes a minor version update for the new feature. The description is clear and concise.

Consider slightly expanding the description to provide more context. For example:

-feat: return object hash in onUploadComplete
+feat: return object hash in onUploadComplete callback for better file tracking

This minor enhancement would provide slightly more context about the purpose of the change without significantly increasing the length of the description.

packages/uploadthing/src/sdk/utils.ts (1)

Line range hint 181-192: LGTM! Consider adding JSDoc for the new hash property.

The implementation looks correct. The hash property is successfully extracted from the uploadWithoutProgress function and added to the return object of uploadFile. This change aligns well with the PR objective of returning the file hash from uploaded objects.

To improve code documentation, consider adding a JSDoc comment for the hash property in the return object. This will help other developers understand the purpose and type of this new property. For example:

return {
  // ... other properties ...
  /**
   * The hash of the uploaded file.
   * @type {string}
   */
  hash,
};
Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between e1490ba and 30f17e0.

Files selected for processing (6)
  • .changeset/nice-crabs-compete.md (1 hunks)
  • examples/minimal-appdir/next-env.d.ts (1 hunks)
  • packages/uploadthing/src/internal/shared-schemas.ts (1 hunks)
  • packages/uploadthing/src/internal/upload.browser.ts (2 hunks)
  • packages/uploadthing/src/internal/upload.server.ts (1 hunks)
  • packages/uploadthing/src/sdk/utils.ts (2 hunks)
Files skipped from review due to trivial changes (1)
  • examples/minimal-appdir/next-env.d.ts
Additional comments not posted (3)
packages/uploadthing/src/internal/upload.server.ts (1)

36-38: Approve the addition of hash property with suggestions for improvement

The addition of the hash property to the return type improves type safety and consistency. However, there are a few points to consider:

  1. Please add documentation explaining the purpose and format of the hash property.

  2. Ensure that all consumers of this function are updated to handle the new hash property.

  3. The use of unsafeCoerce bypasses TypeScript's type checking, which could lead to runtime errors if the actual return value doesn't match the specified type. Consider alternatives if possible, such as:

    • Using a type assertion (as keyword) instead of unsafeCoerce.
    • Implementing a type guard function to validate the structure of the returned object.

To verify the impact of this change, run the following script:

This script will help identify areas of the codebase that might need updating to handle the new hash property.

Verification successful

Approve the addition of hash property with verified usage

The addition of the hash property to the return type has been verified:

  1. Documentation: Ensure that documentation explaining the purpose and format of the hash property is added.
  2. Usage Verification: Confirmed that all current consumers of uploadWithoutProgress handle the new hash property appropriately.
  3. Type Safety: While unsafeCoerce is used, the controlled usage minimizes potential risks. Consider implementing type guards in the future for enhanced safety.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all usages of uploadWithoutProgress and verify they handle the new hash property

# Search for usages of uploadWithoutProgress
echo "Usages of uploadWithoutProgress:"
rg --type typescript "uploadWithoutProgress" -A 5 -B 5

# Search for destructuring of the result that includes the hash property
echo "\nDestructuring with hash property:"
rg --type typescript "const \{.*hash.*\} = .*uploadWithoutProgress" -A 5 -B 5

# Search for access to the hash property on the result
echo "\nAccess to hash property:"
rg --type typescript "uploadWithoutProgress.*\.hash" -A 5 -B 5

Length of output: 523


Script:

#!/bin/bash
# Description: Find all usages of uploadWithoutProgress and verify they handle the new hash property

# Search for usages of uploadWithoutProgress in .ts and .tsx files
echo "Usages of uploadWithoutProgress:"
rg --glob "*.ts" --glob "*.tsx" "uploadWithoutProgress" -A 5 -B 5

# Search for destructuring of the result that includes the hash property in .ts and .tsx files
echo "\nDestructuring with hash property:"
rg --glob "*.ts" --glob "*.tsx" "const \{.*hash.*\} = .*uploadWithoutProgress" -A 5 -B 5

# Search for access to the hash property on the result in .ts and .tsx files
echo "\nAccess to hash property:"
rg --glob "*.ts" --glob "*.tsx" "uploadWithoutProgress.*\.hash" -A 5 -B 5

Length of output: 3547

packages/uploadthing/src/internal/shared-schemas.ts (1)

81-81: LGTM! Consider adding documentation for the hash property.

The addition of the hash property to the UploadedFileData class aligns with the PR objective to return a file hash from the uploaded object. This can be useful for file integrity checks and deduplication.

Consider adding a comment to explain the purpose and format of the hash property. For example:

/**
 * A hash of the file content, useful for integrity checks and deduplication.
 * Format: [specify the hash algorithm, e.g., "SHA-256 in hexadecimal format"]
 */
hash: S.String,

To ensure this change doesn't break existing code, let's verify its usage:

Please review the script output to ensure that all usages of UploadedFileData are compatible with the new hash property.

Verification successful

Verified: Addition of hash property does not break existing code.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for usages of UploadedFileData that might need updating

# Search for UploadedFileData usages
echo "Searching for UploadedFileData usages:"
rg --type typescript "UploadedFileData" -A 5

# Search for potential serialization/deserialization of UploadedFileData
echo "Searching for potential serialization/deserialization of UploadedFileData:"
rg --type typescript "JSON\.(parse|stringify).*UploadedFileData" -A 5

Length of output: 432


Script:

#!/bin/bash
# Description: Check for usages of UploadedFileData that might need updating

# Define file extensions for TypeScript
extensions="\\.ts$|\\.tsx$"

# Search for UploadedFileData usages in TypeScript files
echo "Searching for UploadedFileData usages in .ts and .tsx files:"
rg "UploadedFileData" --type-add 'custom_ts:*.ts,*.tsx' --type custom_ts -A 5

# Search for potential serialization/deserialization of UploadedFileData
echo "Searching for potential serialization/deserialization of UploadedFileData in .ts and .tsx files:"
rg "JSON\.(parse|stringify).*UploadedFileData" --type-add 'custom_ts:*.ts,*.tsx' --type custom_ts -A 5

Length of output: 664


Script:

#!/bin/bash
# Description: Check for usages of UploadedFileData that might need updating

# Search for UploadedFileData usages in .ts and .tsx files
echo "Searching for UploadedFileData usages in .ts and .tsx files:"
rg "UploadedFileData" --glob "*.ts" --glob "*.tsx" -A 5

# Search for potential serialization/deserialization of UploadedFileData in .ts and .tsx files
echo "Searching for potential serialization/deserialization of UploadedFileData in .ts and .tsx files:"
rg "JSON\.(parse|stringify).*UploadedFileData" --glob "*.ts" --glob "*.tsx" -A 5

Length of output: 18342

packages/uploadthing/src/internal/upload.browser.ts (1)

Line range hint 98-116: Summary: Changes successfully implement file hash return

The modifications to the uploadFile function successfully implement the PR objective of returning a file hash from the uploaded object. The changes are well-typed and consistent throughout the affected parts of the code.

Key points:

  1. The return type has been correctly updated to include the hash property.
  2. The hash is properly mapped from the uploadResponse to the returned object.

Minor suggestions for improvement:

  1. Consider updating relevant documentation to reflect the new hash property.
  2. Add error handling or a default value for the hash property to improve robustness.

Overall, the changes are well-implemented and achieve the intended goal.

packages/uploadthing/src/internal/upload.browser.ts Outdated Show resolved Hide resolved
packages/uploadthing/src/internal/upload.browser.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Outside diff range and nitpick comments (1)
examples/minimal-appdir/src/server/uploadthing.ts (1)

Line range hint 55-60: Modify onUploadComplete to include file hash

To fully implement the PR objective of returning the file hash, consider modifying the onUploadComplete callback. This callback is an ideal place to return or log the calculated hash.

Here's a suggestion for how you might modify this section:

.onUploadComplete(({ file, metadata }) => {
  console.log("Upload completed", file);
  console.log("File hash:", file.hash); // Assuming hash was added in the middleware
  
  // Here you could also return the hash to the client if needed
  return { success: true, fileHash: file.hash };
})

This change assumes that you've implemented the hash calculation in the middleware as suggested earlier. Adjust the implementation based on how you've chosen to store the hash.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 30f17e0 and 211afd0.

Files selected for processing (1)
  • examples/minimal-appdir/src/server/uploadthing.ts (1 hunks)

examples/minimal-appdir/src/server/uploadthing.ts Outdated Show resolved Hide resolved
@juliusmarminge
Copy link
Collaborator Author

live tests fails cause infra doesn't yet return this field

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Outside diff range and nitpick comments (13)
packages/uploadthing/test/__test-helpers.ts (2)

123-125: LGTM: Hash calculation implemented correctly

The addition of the hash property to the response object is implemented correctly and aligns with the PR objective. It enhances the ability to verify file integrity after upload.

Consider using a stronger hash algorithm like SHA-256 instead of MD5 for improved security. While MD5 is faster, it's considered cryptographically weak. Here's a suggested improvement:

-            hash: createHash("md5")
+            hash: createHash("sha256")
              .update(new Uint8Array(await request.arrayBuffer()))
              .digest("hex"),

This change would provide a more secure hash while maintaining the same functionality.


Hash calculation not found in main codebase

The test helper includes hash calculation logic (createHash("md5")), but no corresponding implementation is present in the main code. Please verify whether the main code should include this feature to ensure consistency between testing and production environments.

Analysis chain

Line range hint 1-125: Verify main code implementation

The changes in this test helper file correctly implement the new feature of returning a file hash from uploaded objects. This aligns well with the PR objectives.

Please ensure that the corresponding changes have been made in the main code to actually calculate and return the file hash in the production environment. Run the following script to check for similar hash calculation logic in the main code:

If these searches don't yield results, it might indicate that the main code implementation is missing or implemented differently.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for hash calculation logic in the main code

# Test: Search for hash calculation logic in the main code
rg --type typescript -g '!test' 'createHash\(.+\).*\.update\(.+\).*\.digest\('

# Test: Search for new hash property being set or returned in the main code
rg --type typescript -g '!test' 'hash:.*createHash'

Length of output: 204


Script:

#!/bin/bash
# Description: Check for hash calculation logic in the main code using correct TypeScript file type

# Test: Search for hash calculation logic in the main code using 'ts' as the file type
rg --type ts -g '!test' 'createHash\(.+\).*\.update\(.+\).*\.digest\('

# Test: Search for new hash property being set or returned in the main code using 'ts' as the file type
rg --type ts -g '!test' 'hash:.*createHash'

Length of output: 118


Script:

#!/bin/bash
# Description: Check for hash calculation logic in the main code by targeting .ts and .tsx files

# Test: Search for hash calculation logic in .ts and .tsx files excluding test directories
rg -g '*.ts' -g '*.tsx' -g '!test/*' 'createHash\(.+\).*\.update\(.+\).*\.digest\('

# Test: Search for new hash property being set or returned in .ts and .tsx files excluding test directories
rg -g '*.ts' -g '*.tsx' -g '!test/*' 'hash\s*:\s*createHash'

Length of output: 228

packages/uploadthing/test/client.test.ts (3)

135-135: LGTM! Consider adding a comment explaining the hash property.

The addition of the hash property in the expected output aligns well with the PR objective. It correctly uses expect.any(String) to check for the presence and type of the hash.

Consider adding a brief comment explaining the purpose of the hash property for better test documentation. For example:

hash: expect.any(String), // Verify that a hash is returned for the uploaded file

216-216: LGTM! Consider refactoring for DRY principle.

The addition of the hash property is consistent with the previous changes and doesn't interfere with the test's primary focus on async custom headers.

Given that this exact expectation (hash: expect.any(String)) is now repeated in multiple test cases, consider refactoring to reduce duplication. You could create a shared expectation object or a helper function. For example:

const expectedFileProperties = {
  name: expect.any(String),
  size: expect.any(Number),
  type: expect.any(String),
  customId: null,
  serverData: null,
  lastModified: expect.any(Number),
  key: expect.stringMatching(/.+/),
  url: expect.stringMatching(fileUrlPattern),
  appUrl: expect.stringMatching(appUrlPattern()),
  hash: expect.any(String),
};

// Then in your tests:
expect(result).toMatchObject(expectedFileProperties);

This approach would make it easier to maintain consistency across tests and reduce the risk of inconsistencies when updating expectations in the future.


Line range hint 135-216: Summary: Consistent implementation of hash property in test expectations

The changes in this file successfully integrate the new hash property into the test expectations for the uploadFiles function. This aligns well with the PR objective of returning a file hash from uploaded objects. The implementation is consistent across different test scenarios and doesn't introduce any apparent issues.

Key points:

  1. The hash property is correctly expected to be a string in all cases.
  2. The addition doesn't interfere with the existing test logic or the specific focuses of individual test cases.
  3. The changes are minimal and targeted, reducing the risk of unintended side effects.

To further improve the test suite:

  1. Consider adding specific tests for the hash functionality to ensure it behaves as expected under various conditions.
  2. Explore refactoring opportunities to reduce duplication in test expectations, as suggested in the previous comment.
  3. Ensure that the hash property is documented in the relevant interfaces or types in the main code to maintain consistency between implementation and tests.
packages/uploadthing/test/request-handler.test.ts (4)

340-341: LGTM! Consider using a realistic MD5 hash.

The addition of the hash property is consistent with the PR objective. For improved test fidelity, consider using a realistic MD5 hash (e.g., "098f6bcd4621d373cade4e832627b4f6") instead of "some-md5-hash".


388-389: LGTM! Consider using a consistent realistic MD5 hash.

The addition of the hash property in the signature validation test case is appropriate. For consistency and improved test fidelity, consider using the same realistic MD5 hash suggested earlier (e.g., "098f6bcd4621d373cade4e832627b4f6") across all test cases.


421-422: LGTM! Maintain consistency with realistic MD5 hash.

The addition of the hash property in the invalid signature test case is appropriate. To maintain consistency and improve test fidelity, use the same realistic MD5 hash suggested earlier (e.g., "098f6bcd4621d373cade4e832627b4f6") across all test cases.


Line range hint 340-422: Overall LGTM! Consider using a consistent, realistic MD5 hash across all test cases.

The changes consistently implement the new hash property across various test cases, aligning well with the PR objective. To further improve test fidelity and maintainability, consider:

  1. Using a consistent, realistic MD5 hash (e.g., "098f6bcd4621d373cade4e832627b4f6") across all test cases.
  2. Defining the hash value as a constant at the top of the file to ensure consistency and ease of updates.

Example:

const TEST_FILE_HASH = "098f6bcd4621d373cade4e832627b4f6";

Then use TEST_FILE_HASH instead of "some-md5-hash" in all test cases.

These changes will enhance the robustness and readability of the tests while maintaining the intended functionality.

packages/uploadthing/test/sdk.test.ts (4)

57-57: LGTM! Consider adding a more specific hash expectation.

The addition of the hash property aligns well with the PR objective. Using expect.any(String) correctly tests for the presence and type of the hash.

For more robust testing, consider adding a separate test case that checks for a specific hash format (e.g., SHA-256) or length, if the hash algorithm is known and consistent.


480-480: LGTM! Comprehensive testing with live API.

The addition of the hash property in the live API test case ensures end-to-end functionality and helps catch any potential integration issues.

Consider adding a test case that verifies the consistency of the hash across multiple uploads of the same file. This would help ensure the hash generation is deterministic.


516-516: LGTM! Comprehensive hash verification across various scenarios.

The consistent addition of the hash property across multiple live API test cases ensures thorough testing of the new feature in different contexts.

To reduce repetition and improve maintainability, consider creating a helper function that generates the expected result object with the hash property. This function could be reused across all test cases, making it easier to update if the expected structure changes in the future.

Example:

function createExpectedResult(file: File, appId: string) {
  return {
    data: {
      customId: null,
      key: expect.stringMatching(/.+/),
      lastModified: file.lastModified,
      name: file.name,
      size: file.size,
      type: file.type,
      url: expect.stringMatching(fileUrlPattern),
      appUrl: expect.stringMatching(appUrlPattern(appId)),
      hash: expect.any(String),
    },
    error: null,
  };
}

Then use it in your tests like this:

expect(result).toEqual(createExpectedResult(file, appId));

This approach would make the tests more DRY and easier to maintain.

Also applies to: 547-547, 573-573, 616-616, 657-657


Line range hint 1-707: Overall, the changes look good and thoroughly test the new hash feature.

The additions consistently implement hash verification across various upload scenarios, including single file uploads, multi-file uploads, URL uploads, and live API tests. This comprehensive approach ensures that the new feature is well-tested in different contexts.

To further improve the test suite:

  1. Consider adding specific tests for hash consistency and format, if applicable.
  2. Explore opportunities to refactor common expectations into reusable helper functions to enhance maintainability.
  3. If not already present, consider adding error case tests to verify proper handling of hash generation failures.
Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 211afd0 and b336ddc.

<details>
Files selected for processing (4)
  • packages/uploadthing/test/__test-helpers.ts (2 hunks)
  • packages/uploadthing/test/client.test.ts (3 hunks)
  • packages/uploadthing/test/request-handler.test.ts (4 hunks)
  • packages/uploadthing/test/sdk.test.ts (10 hunks)
Additional comments not posted (6)
packages/uploadthing/test/__test-helpers.ts (1)

1-1: LGTM: Appropriate import for hash calculation

The import of createHash from the crypto module is correct and necessary for the new hash calculation feature. This built-in Node.js module is suitable for use in this context.

packages/uploadthing/test/client.test.ts (1)

181-181: LGTM! Consistent addition of the hash property.

The addition of the hash property in this test case is consistent with the previous change and doesn't interfere with the test's primary focus on custom headers.

packages/uploadthing/test/request-handler.test.ts (1)

370-371: LGTM! Consistent hash property verification.

The addition of the hash property in the expected arguments for uploadCompleteMock ensures proper verification of the new feature. This change maintains consistency with the UploadedFileData object and aligns with the PR objective.

packages/uploadthing/test/sdk.test.ts (3)

140-140: LGTM! Consistent implementation for URL uploads.

The addition of the hash property in the uploadFilesFromUrl test case is consistent with the previous change and ensures that file hashes are returned for URL uploads as well.


240-240: LGTM! Consistent hash implementation for multi-file uploads.

The addition of the hash property in the multi-file upload test case ensures consistency across different upload scenarios and verifies that file hashes are returned correctly for each uploaded file.


263-263: LGTM! Consistent hash verification for all files in multi-file upload.

The addition of the hash property for the second file in the multi-file upload test case maintains consistency and ensures that hashes are verified for all uploaded files.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (2)
examples/minimal-appdir/src/server/uploadthing.ts (2)

35-35: LGTM! Consider documenting the rationale for maxFileCount.

The addition of maxFileCount: 10 for blob uploads is a good practice to prevent potential abuse. However, it would be helpful to document the reasoning behind choosing 10 as the limit. Is this based on specific requirements or constraints?

Consider adding a comment explaining the rationale for this limit, e.g.:

maxFileCount: 10, // Limit set to 10 to balance user needs and server load

Line range hint 1-64: Summary: Implementation incomplete, needs refinement

Overall, the changes are moving in the right direction, but there are a few key points to address:

  1. The main PR objective of returning the file hash after upload is not yet implemented. This should be the primary focus of the next iteration.
  2. There are some code cleanliness issues, including unused references and commented-out code, that should be cleaned up.
  3. The new maxFileCount property is a good addition, but its value could use some explanation.

Please address these points in your next commit. Once the file hash return is implemented and the code is cleaned up, this PR will be much closer to meeting its objectives.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between b336ddc and f46dbbd.

📒 Files selected for processing (6)
  • examples/minimal-appdir/src/server/uploadthing.ts (2 hunks)
  • packages/uploadthing/src/internal/shared-schemas.ts (1 hunks)
  • packages/uploadthing/src/internal/types.ts (1 hunks)
  • packages/uploadthing/src/internal/upload.browser.ts (3 hunks)
  • packages/uploadthing/src/internal/upload.server.ts (2 hunks)
  • packages/uploadthing/src/sdk/utils.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • packages/uploadthing/src/internal/shared-schemas.ts
  • packages/uploadthing/src/internal/upload.browser.ts
  • packages/uploadthing/src/internal/upload.server.ts
  • packages/uploadthing/src/sdk/utils.ts
🔇 Additional comments (4)
examples/minimal-appdir/src/server/uploadthing.ts (1)

41-43: Consider removing or implementing the header validation.

The commented-out header validation code is still present. As noted in a previous review, this code appears in multiple files and is not utilized. To maintain code cleanliness and avoid confusion, consider either implementing this validation if it's needed or removing the commented code entirely.

If you decide to keep this validation, please uncomment and implement it. Otherwise, remove these lines:

- // if (!req.headers.get("x-some-header")) {
- //   throw new Error("x-some-header is required");
- // }
packages/uploadthing/src/internal/types.ts (3)

Line range hint 1-246: Summary: New type UploadPutResult added successfully.

The addition of the UploadPutResult type enhances the type definitions for the UploadThing library, specifically for handling the result of file uploads. This change aligns well with the PR objective and doesn't introduce any breaking changes to the existing type structure.

Key points:

  1. The new type is well-defined and uses a generic parameter for flexibility.
  2. It includes the fileHash property, which was the main goal of this PR.
  3. The addition doesn't impact existing type definitions in this file.

To ensure full integration:

  1. Verify the usage of this new type across the codebase.
  2. Ensure it's properly exported if intended for public API use.
  3. Update relevant documentation to reflect this new type.

241-246: LGTM! New type UploadPutResult looks good.

The new UploadPutResult type is well-defined and aligns with the PR objective of returning a file hash from the uploaded object. The generic type parameter TServerOutput provides flexibility for different server response structures.

To ensure consistency, let's verify if this new type is being used correctly across the codebase:

✅ Verification successful

UploadPutResult type usage verified successfully.

The UploadPutResult type is correctly imported and utilized in the following files:

  • packages/uploadthing/src/internal/upload.server.ts
  • packages/uploadthing/src/internal/upload.browser.ts

No issues found with its usage.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for usage of UploadPutResult type in the codebase

# Search for UploadPutResult usage
echo "Searching for UploadPutResult usage:"
rg --type typescript "UploadPutResult"

# Search for potential places where UploadPutResult should be used
echo "\nSearching for potential places where UploadPutResult might be needed:"
rg --type typescript "upload.*result|result.*upload"

Length of output: 399


Script:

#!/bin/bash
# Description: Check for usage of UploadPutResult type in the codebase without relying on predefined file types.

# Search for UploadPutResult usage in TypeScript files
echo "Searching for UploadPutResult usage in .ts and .tsx files:"
rg --glob "*.ts" --glob "*.tsx" "UploadPutResult"

# Search for potential places where UploadPutResult might be needed in TypeScript files
echo "\nSearching for potential places where UploadPutResult might be needed in .ts and .tsx files:"
rg --glob "*.ts" --glob "*.tsx" "upload.*result|result.*upload"

Length of output: 2332


237-246: Consider documenting the new type and verifying its API integration.

While the new UploadPutResult type is well-defined, it's important to ensure it's properly integrated into the library's API. Consider the following suggestions:

  1. Add JSDoc comments to explain the purpose and usage of this type, especially for public API consumers.
  2. Verify that this type is exported if it's intended to be part of the public API.
  3. Update any relevant documentation or API references to include this new type.

Here's a suggested improvement with added JSDoc comments:

/**
 * Represents the result from a PUT request to the UploadThing Ingest server.
 * @template TServerOutput - The type of the server data returned. Defaults to unknown.
 */
export type UploadPutResult<TServerOutput = unknown> = {
  /** The URL of the uploaded file */
  url: string;
  /** The application-specific URL of the uploaded file */
  appUrl: string;
  /** The hash of the uploaded file */
  fileHash: string;
  /** Additional data returned by the server */
  serverData: TServerOutput;
};

Let's check if the type is being exported and used in the public API:

examples/minimal-appdir/src/server/uploadthing.ts Outdated Show resolved Hide resolved
examples/minimal-appdir/src/server/uploadthing.ts Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Oct 1, 2024

A new canary is available for testing. You can install this latest build in your project with:

pnpm add @uploadthing/expo@7.0.3-canary.c184a9f
pnpm add @uploadthing/mime-types@0.3.1-canary.c184a9f
pnpm add @uploadthing/nuxt@7.0.3-canary.c184a9f
pnpm add @uploadthing/react@7.0.3-canary.c184a9f
pnpm add @uploadthing/shared@7.0.3-canary.c184a9f
pnpm add @uploadthing/solid@7.0.3-canary.c184a9f
pnpm add @uploadthing/svelte@7.0.3-canary.c184a9f
pnpm add uploadthing@7.0.3-canary.c184a9f
pnpm add @uploadthing/vue@7.0.3-canary.c184a9f

@github-actions github-actions bot removed the release canary Trigger a canary release to npm label Oct 1, 2024
examples/minimal-appdir/src/server/uploadthing.ts Outdated Show resolved Hide resolved
examples/minimal-appdir/src/server/uploadthing.ts Outdated Show resolved Hide resolved
examples/minimal-appdir/src/server/uploadthing.ts Outdated Show resolved Hide resolved
examples/minimal-appdir/src/server/uploadthing.ts Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview – legacy-docs-uploadthing October 3, 2024 20:19 Inactive
@juliusmarminge juliusmarminge merged commit a3fa6af into main Oct 6, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants