-
Notifications
You must be signed in to change notification settings - Fork 326
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
chore: add TB to internal file size converter #1094
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
WalkthroughThe changes in this pull request focus on enhancing the functionality of the 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 (
|
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/shared/src/utils.ts
(2 hunks)
🔇 Additional comments (2)
packages/shared/src/utils.ts (2)
139-139
: LGTM: TB unit addition is well-integrated
The addition of "TB" to FILESIZE_UNITS
is properly implemented and automatically works with existing parsing logic.
Line range hint 139-167
: Verify the impact of file size changes
Let's verify the impact of these changes on tests and documentation.
✅ Verification successful
Let me gather more specific information about the test usage of these utility functions.
Let me fix the import search pattern and look for any direct usage of these functions.
Let me try one final search with corrected patterns to find any usage of these functions.
Based on the gathered information, I can now provide the final response:
File size utility functions are actively used in core functionality
The fileSizeToBytes
and bytesToFileSize
functions are integral parts of the upload functionality:
- Used in
packages/uploadthing/src/internal/route-config.ts
for file size validation - Used in
packages/uploadthing/src/client.ts
for processing size limits - No breaking changes detected in the implementation
The changes appear to be maintaining the existing behavior while improving the code structure. The functions are used in critical paths for file size validation and display, making them essential utilities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential impacts of file size changes
# Check for test files using these functions
echo "Checking test files..."
rg -l "bytesToFileSize|fileSizeToBytes|FILESIZE_UNITS" --glob "**/*.test.*"
# Check for documentation references
echo -e "\nChecking documentation..."
rg -l "KB|MB|GB|TB" --glob "**/*.md"
# Check for examples in code
echo -e "\nChecking code examples..."
rg "(\\d+)\\s*(KB|MB|GB|TB)" --glob "**/*.{ts,js,tsx,jsx}"
Length of output: 6251
Script:
#!/bin/bash
# Check for direct usage of the utility functions in tests
echo "Checking test files for utility functions usage..."
rg -A 5 "fileSizeToBytes|bytesToFileSize" --glob "**/*.test.*"
# Check implementation details
echo -e "\nChecking implementation in utils.ts..."
rg -A 10 "^export const (fileSizeToBytes|bytesToFileSize)" packages/shared/src/utils.ts
# Check for any imports of these functions
echo -e "\nChecking imports..."
rg "import.*{.*(fileSizeToBytes|bytesToFileSize).*} from.*utils"
Length of output: 1259
Script:
#!/bin/bash
# Check for imports and usage
echo "Checking imports and usage..."
rg "import.*[{ ]+(fileSizeToBytes|bytesToFileSize)[} ].*from.*utils"
# Check for direct usage of these functions in the codebase
echo -e "\nChecking direct usage..."
rg "\b(fileSizeToBytes|bytesToFileSize)\(" --type ts --type tsx
# Check if there are any tests specifically for these utilities
echo -e "\nChecking for utility tests..."
rg -B 2 -A 5 "describe.*('|\")(fileSizeToBytes|bytesToFileSize)" --glob "**/*.test.*"
Length of output: 463
Script:
#!/bin/bash
# Search for any usage of these functions in the codebase
echo "Checking for function usage..."
rg "\b(fileSizeToBytes|bytesToFileSize)\b" --type ts
# Check for any test files that might contain these functions
echo -e "\nChecking test files..."
rg -l "(fileSizeToBytes|bytesToFileSize)" packages/shared/src/**/*.test.ts
# Check if these functions are exported from index files
echo -e "\nChecking exports..."
rg "export.*{.*\b(fileSizeToBytes|bytesToFileSize)\b.*}" packages/shared/src/index.ts
Length of output: 1363
const i = Math.floor(Math.log(bytes) / Math.log(1024)); | ||
return `${(bytes / Math.pow(1024, i)).toFixed(2)}${FILESIZE_UNITS[i]}`; |
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.
🛠️ Refactor suggestion
Consider aligning size units with standard conventions
The change from base 1000 to 1024 for size calculations introduces a discrepancy with the displayed units. Currently:
- Using 1024 as base (binary prefix standard)
- But displaying KB, MB, etc. (decimal prefix standard)
This mixing of standards could be confusing as:
- 1 KB should be 1000 bytes (SI standard)
- 1 KiB should be 1024 bytes (Binary standard)
Consider one of these solutions:
- Keep 1024 but use binary prefixes:
-export const FILESIZE_UNITS = ["B", "KB", "MB", "GB", "TB"] as const;
+export const FILESIZE_UNITS = ["B", "KiB", "MiB", "GiB", "TiB"] as const;
- Or revert to 1000 to match current decimal prefixes:
- const i = Math.floor(Math.log(bytes) / Math.log(1024));
- return `${(bytes / Math.pow(1024, i)).toFixed(2)}${FILESIZE_UNITS[i]}`;
+ const i = Math.floor(Math.log(bytes) / Math.log(1000));
+ return `${(bytes / Math.pow(1000, i)).toFixed(2)}${FILESIZE_UNITS[i]}`;
Committable suggestion skipped: line range outside the PR's diff.
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.
Gotta update tests b/c of 1000/1024 change
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: 0
🧹 Outside diff range and nitpick comments (1)
packages/uploadthing/test/request-handler.test.ts (1)
157-157
: Fix grammatical error in error messageThe error message contains a grammatical error. It should be "an image file" instead of "a image file" since "image" starts with a vowel sound.
- "Error: You uploaded a image file that was 3.00MB, but the limit for that type is 2MB", + "Error: You uploaded an image file that was 3.00MB, but the limit for that type is 2MB",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/uploadthing/test/request-handler.test.ts
(1 hunks)
🔇 Additional comments (1)
packages/uploadthing/test/request-handler.test.ts (1)
157-157
: Verify file size calculation consistency
The error message shows a file size of "3.00MB" which appears to be using the new base-1024 calculation (as mentioned in the AI summary). Let's verify that this is consistent with the actual test setup.
✅ Verification successful
Let me gather more information about the file size limits and calculations to ensure consistency.
Let me check one more thing to verify the file size limit configuration for the image uploader route.
File size calculation is consistent with base-1024 standard
The verification shows that:
- The test file correctly uses base-1024 calculation:
size: 3 * 1024 * 1024
- The error message accurately reflects this as "3.00MB"
- The size limit validation is handled by
FileSizeMismatch
error, which uses the same calculation standard
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for file size calculations and unit conversions in the codebase
# to ensure consistency with the new base-1024 standard.
# Look for file size calculations
rg -A 2 "size.*1024|1024.*size"
# Look for byte conversion functions
ast-grep --pattern 'function $_(bytes) {
$$$
1024
$$$
}'
Length of output: 777
Script:
#!/bin/bash
# Look for file size limit configurations and related code
rg -A 3 "fileSize|maxSize|sizeLimit" packages/uploadthing/
# Look for the specific test case setup
rg -B 5 -A 5 "3.*1024.*1024" packages/uploadthing/test/
Length of output: 3241
Script:
#!/bin/bash
# Look for the image uploader route configuration in the test file
rg -B 10 -A 5 "imageUploader.*=.*createUploadthing" packages/uploadthing/test/
# Also check for any file size related error messages
rg -B 2 "FileSizeMismatch|file.*size.*limit" packages/uploadthing/
Length of output: 1780
Summary by CodeRabbit
New Features
Bug Fixes
Chores