fix: File size validation increase #3432
Conversation
sret-farhan2021
requested review from
Chethan-Fyle and
arjunaj5
as code owners
|
Warning Rate limit exceeded@sret-farhan2021 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 33 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)
WalkthroughThe application's file size limit has been increased from 5MB to 8MB! This change spans multiple components, updating user-facing messages and the core constant to reflect the new maximum file upload size. A systematic upgrade, it touches various parts of the application with precision, ensuring users are informed of the new limits. Changes
Possibly related PRs
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 (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
src/app/core/constants.ts(1 hunks)src/app/fyle/add-edit-expense/add-edit-expense-2.spec.ts(1 hunks)src/app/fyle/add-edit-expense/add-edit-expense.page.ts(1 hunks)src/app/fyle/add-edit-expense/camera-options-popup/camera-options-popup.component.spec.ts(1 hunks)src/app/fyle/add-edit-expense/camera-options-popup/camera-options-popup.component.ts(1 hunks)src/app/shared/components/expenses-card-v2/expenses-card.component.spec.ts(1 hunks)src/app/shared/components/expenses-card-v2/expenses-card.component.ts(1 hunks)src/app/shared/components/expenses-card/expenses-card.component.ts(1 hunks)
🔇 Additional comments (7)
src/app/core/constants.ts (1)
1-1: Mind it! Let's verify the impact of increasing file size limit, partner!The increase from 5MB to 9MB could affect mobile users with limited data plans or slower connections.
✅ Verification successful
Mind it! The file size limit increase is perfectly balanced, like my signature move!
The increase from 5MB to 9MB is well-implemented across the codebase with proper validations and clear error messages. The limit is reasonable for modern mobile devices while still being conservative enough to prevent excessive data usage. All components handle the new limit consistently.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if there are any comments or documentation about file size limits rg -i "file.*size.*limit|max.*file.*size" --type mdLength of output: 53
Script:
#!/bin/bash # Find where MAX_FILE_SIZE is used rg "MAX_FILE_SIZE" -B 2 -A 2 # Look for file upload related code rg -i "upload.*file|file.*upload" -g "*.{ts,js,tsx,jsx}" -B 2 -A 2 # Search for file size validation patterns ast-grep --pattern 'if ($_ > $_) { $$$ }' -l ts,tsx | xargs rg "size|file" -B 2 -A 2Length of output: 75671
Script:
#!/bin/bash # Check for any mobile-specific configurations rg -i "mobile.*upload|upload.*limit" --type ts # Look for API or backend configuration files fd -e json -e yaml -e yml -e config | xargs rg -i "upload|size.*limit|max.*size"Length of output: 380
src/app/fyle/add-edit-expense/camera-options-popup/camera-options-popup.component.ts (1)
79-79: Superstar approves this message update!The error message correctly reflects the new 9MB file size limit.
src/app/shared/components/expenses-card/expenses-card.component.ts (1)
442-442: Style-ah implement panirkeengale! (You've implemented it with style!)The error message is consistently updated across the component, maintaining harmony with the new file size limit.
src/app/shared/components/expenses-card-v2/expenses-card.component.ts (1)
464-464: Mind it! The file size limit message looks perfect, macha!The updated error message correctly reflects the new 9MB file size limit.
src/app/shared/components/expenses-card-v2/expenses-card.component.spec.ts (1)
703-703: Superstar approves this test case update!The test expectation has been properly updated to match the new 9MB file size limit message.
src/app/fyle/add-edit-expense/add-edit-expense.page.ts (1)
5225-5225: Kabali says this message change is spot on!The file size limit error message has been correctly updated to 9MB.
src/app/fyle/add-edit-expense/add-edit-expense-2.spec.ts (1)
1659-1659: Thalaiva gives a thumbs up to this test update!The test case has been properly updated to verify the new 9MB file size limit message.
| @@ -79,7 +79,7 @@ describe('CameraOptionsPopupComponent', () => { | |||
| component: PopupAlertComponent, | |||
| componentProps: { | |||
| title: 'Size limit exceeded', | |||
| message: 'The uploaded file is greater than 5MB in size. Please reduce the file size and try again.', | |||
| message: 'The uploaded file is greater than 9MB in size. Please reduce the file size and try again.', | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Time to power up our test game, machan!
While the message is updated correctly, we should add test cases to verify the file size validation logic:
- Test with file size < 9MB (should pass)
- Test with file size > 9MB (should show error)
- Test with file size = 9MB (edge case)
Would you like me to generate these test cases for you?
sret-farhan2021
force-pushed
the
FYLE-86cxr4fgh
branch
from
260b9dc to
599512e
Compare
src/app/core/constants.ts
Outdated
| @@ -1 +1 @@ | |||
| export const MAX_FILE_SIZE = 5000000; | |||
| export const MAX_FILE_SIZE = 8000000; | |||
There was a problem hiding this comment.
this is not 8MB, right? Shouldn't this be (8 * 1024 * 1204). If I upload a file that is 7.8 MB, won't this fail?
| @@ -439,7 +439,7 @@ export class ExpensesCardComponent implements OnInit { | |||
| component: PopupAlertComponent, | |||
| componentProps: { | |||
| title: 'Size limit exceeded', | |||
| message: 'The uploaded file is greater than 5MB in size. Please reduce the file size and try again.', | |||
| message: 'The uploaded file is greater than 8MB in size. Please reduce the file size and try again.', | |||
There was a problem hiding this comment.
Why not move the error message to a common place?. In stead of hardcoding 8MB, you can set the value using MAX_FILE_SIZE
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (6)
src/app/fyle/add-edit-expense/camera-options-popup/camera-options-popup.component.ts (1)
38-52: 🧹 Nitpick (assertive)Mind it! Let's simplify this logic, machan!
The file size check is duplicated in the if conditions. We can simplify this for better readability.
Here's a cleaner way to write this:
async uploadFileCallback(file: File): Promise<void> { - if (file?.size < MAX_FILE_SIZE) { + if (!file) { + this.closeClicked(); + return; + } + + if (file.size > MAX_FILE_SIZE) { + this.closeClicked(); + this.showSizeLimitExceededPopover(MAX_FILE_SIZE); + return; + } + const dataUrl = await this.fileService.readFile(file); this.popoverController.dismiss({ type: file.type, dataUrl, actionSource: 'gallery_upload', }); - } else { - this.closeClicked(); - - if (file?.size > MAX_FILE_SIZE) { - this.showSizeLimitExceededPopover(MAX_FILE_SIZE); - } - } }src/app/fyle/add-edit-expense/camera-options-popup/camera-options-popup.component.spec.ts (1)
131-174: 🛠️ Refactor suggestionTime to power up our test game, machan!
While the message is updated correctly, we should add test cases to verify the file size validation logic:
- Test with file size < 8MB (should pass)
- Test with file size > 8MB (should show error)
- Test with file size = 8MB (edge case)
Here's how we can add these test cases:
describe('uploadFileCallback():', () => { it('should accept file when size is less than 8MB', fakeAsync(() => { const file = new File([new ArrayBuffer(7 * 1024 * 1024)], 'test.jpg'); fileService.readFile.and.resolveTo('data:image/jpeg;base64,/9j/4AAQ'); component.uploadFileCallback(file); tick(500); expect(fileService.readFile).toHaveBeenCalledWith(file); expect(popoverController.dismiss).toHaveBeenCalled(); })); it('should show error when file size equals 8MB', fakeAsync(() => { const file = new File([new ArrayBuffer(8 * 1024 * 1024)], 'test.jpg'); component.uploadFileCallback(file); tick(500); expect(component.showSizeLimitExceededPopover).toHaveBeenCalledWith(MAX_FILE_SIZE); })); it('should show error when file size exceeds 8MB', fakeAsync(() => { const file = new File([new ArrayBuffer(9 * 1024 * 1024)], 'test.jpg'); component.uploadFileCallback(file); tick(500); expect(component.showSizeLimitExceededPopover).toHaveBeenCalledWith(MAX_FILE_SIZE); })); });src/app/shared/components/expenses-card/expenses-card.component.ts (1)
312-327: 🧹 Nitpick (assertive)Mind it! Let's handle errors with style, machan!
The file upload logic could be enhanced with better error handling and reusability.
Consider refactoring to use a common file validation service:
// Create a new service FileValidationService export class FileValidationService { validateFileSize(file: File): boolean { return file?.size < MAX_FILE_SIZE; } getFileSizeExceededMessage(maxFileSize: number): string { return `The uploaded file is greater than ${ maxFileSize / (1024 * 1024) }MB in size. Please reduce the file size and try again.`; } } // Then update the method to: async onFileUpload(nativeElement: HTMLInputElement): Promise<void> { const file = nativeElement.files[0]; if (!file) { return; } if (!this.fileValidationService.validateFileSize(file)) { this.showSizeLimitExceededPopover(MAX_FILE_SIZE); return; } try { const dataUrl = await this.fileService.readFile(file); this.trackingService.addAttachment({ type: file.type }); const receiptDetails: ReceiptDetail = { type: file.type, dataUrl, actionSource: 'gallery_upload', }; await this.attachReceipt(receiptDetails); } catch (error) { // Handle error appropriately console.error('File upload failed:', error); } }src/app/shared/components/expenses-card-v2/expenses-card.component.ts (1)
334-349: 🧹 Nitpick (assertive)Mind-blowing implementation, but let's make it even better!
The file size validation in
onFileUploadis looking sharp, but we can make it more robust by adding a null check for the file.async onFileUpload(nativeElement: HTMLInputElement): Promise<void> { const file = nativeElement.files[0]; + if (!file) { + return; + } let receiptDetails: ReceiptDetail; if (file?.size < MAX_FILE_SIZE) {src/app/shared/components/expenses-card-v2/expenses-card.component.spec.ts (1)
259-269: 🧹 Nitpick (assertive)Thalaiva-level test case, but let's make it even more powerful!
The test case correctly verifies the file size limit, but we can make it more precise by using the exact size limit.
- it('should show file size exceeded popover if uploaded file is larger than 8MB', fakeAsync(() => { + it('should show file size exceeded popover if uploaded file is larger than MAX_FILE_SIZE', fakeAsync(() => { spyOn(component, 'showSizeLimitExceededPopover'); const myBlob = new Blob([new ArrayBuffer(100 * 100 * 1000)], { type: 'application/octet-stream' }); - const file = new File([myBlob], 'file'); + const file = new File([myBlob], 'file', { size: MAX_FILE_SIZE + 1 });src/app/fyle/add-edit-expense/add-edit-expense-4.spec.ts (1)
259-269: 🧹 Nitpick (assertive)Style-ah style-ah! But let's make this test case even more robust!
The test case verifies the file size limit, but we can improve the assertion to be more specific.
- expect(component.showSizeLimitExceededPopover(MAX_FILE_SIZE)).toHaveBeenCalledOnceWith(); + expect(component.showSizeLimitExceededPopover).toHaveBeenCalledOnceWith(MAX_FILE_SIZE);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
src/app/core/constants.ts(1 hunks)src/app/fyle/add-edit-expense/add-edit-expense-2.spec.ts(2 hunks)src/app/fyle/add-edit-expense/add-edit-expense-4.spec.ts(3 hunks)src/app/fyle/add-edit-expense/add-edit-expense.page.ts(2 hunks)src/app/fyle/add-edit-expense/camera-options-popup/camera-options-popup.component.spec.ts(2 hunks)src/app/fyle/add-edit-expense/camera-options-popup/camera-options-popup.component.ts(2 hunks)src/app/shared/components/expenses-card-v2/expenses-card.component.spec.ts(3 hunks)src/app/shared/components/expenses-card-v2/expenses-card.component.ts(2 hunks)src/app/shared/components/expenses-card/expenses-card.component.ts(2 hunks)
🔇 Additional comments (4)
src/app/core/constants.ts (1)
1-1: Yo machan, this calculation is perfect!The file size calculation
8 * 1024 * 1024correctly represents 8MB in bytes. This is the proper way to handle binary file sizes.src/app/shared/components/expenses-card-v2/expenses-card.component.ts (1)
459-475: Superstar code right here! The error message is crystal clear!The
showSizeLimitExceededPopovermethod now dynamically calculates the size limit in MB, making it future-proof for any changes toMAX_FILE_SIZE.src/app/fyle/add-edit-expense/add-edit-expense-2.spec.ts (2)
114-114: Mind it! The import looks perfect!The import of MAX_FILE_SIZE constant makes the file size limit configurable. This is the way to do it!
1653-1660:❓ Verification inconclusive
Superstar style code change!
The function now uses the imported MAX_FILE_SIZE constant to display the correct file size limit in the error message. This is a proper way to handle dynamic configuration values.
However, let's verify if all test cases are updated to reflect this change.
🏁 Script executed:
#!/bin/bash # Search for any remaining hardcoded "5MB" references in test files rg -g "*.spec.ts" "5MB" .Length of output: 25
Action: Manual Verification Recommended for Test Case Updates
Superstar, the change to use the imported
MAX_FILE_SIZEconstant is in place and looks solid in the test filesrc/app/fyle/add-edit-expense/add-edit-expense-2.spec.ts(Lines 1653-1660). The search for any remaining hardcoded "5MB" references in the test files came up empty, which is encouraging. However, since the shell script output was minimal, please manually verify that no test cases still reference the old hardcoded values.
- Location to Verify:
src/app/fyle/add-edit-expense/add-edit-expense-2.spec.ts(Lines 1653-1660)- Key Code Snippet:
component.showSizeLimitExceededPopover(MAX_FILE_SIZE); tick(500); expect(popoverController.create).toHaveBeenCalledOnceWith({ component: PopupAlertComponent, componentProps: { title: 'Size limit exceeded', message: 'The uploaded file is greater than 8MB in size. Please reduce the file size and try again.',Keep up the blockbuster work, and make sure to double-check every corner of the tests manually for any lingering hardcoded values.
src/app/fyle/add-edit-expense/camera-options-popup/camera-options-popup.component.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/app/fyle/add-edit-expense/add-edit-expense-2.spec.ts(1 hunks)src/app/fyle/add-edit-expense/add-edit-expense-4.spec.ts(2 hunks)src/app/fyle/add-edit-expense/camera-options-popup/camera-options-popup.component.spec.ts(1 hunks)src/app/shared/components/expenses-card-v2/expenses-card.component.spec.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: gitStream.cm
- GitHub Check: build (12.x)
🔇 Additional comments (3)
src/app/fyle/add-edit-expense/camera-options-popup/camera-options-popup.component.spec.ts (1)
75-75: Mind it! The test changes look perfect, machan!The test correctly validates the new 8MB file size limit and error message. Keep rocking!
Also applies to: 82-82
src/app/shared/components/expenses-card-v2/expenses-card.component.spec.ts (1)
679-704: Style ah irukku! The test changes are spot on, machan!The test case beautifully handles the new 8MB file size limit with consistent updates to the test description, file size constant, and error message.
src/app/fyle/add-edit-expense/add-edit-expense-2.spec.ts (1)
1652-1652:❓ Verification inconclusive
Mind it! The test case looks good, but let's verify the byte value.
The test case correctly updates the file size limit from 5MB to 8MB. However, let's verify that 8388609 bytes is the exact value we want to use for validation.
Run this script to check if the byte value matches 8MB exactly:
Also applies to: 1659-1659
🏁 Script executed:
#!/bin/bash # Description: Calculate and compare byte values echo "8MB in bytes = 8 * 1024 * 1024 = 8388608 bytes" echo "Current validation uses: 8388609 bytes" echo "Difference: 1 byte"Length of output: 236
Punch it like Rajinikanth: Verify that one extra byte is your secret sauce!
The test case now elegantly checks that a file exceeding the 8MB limit triggers the warning popover. Our calculation shows 8MB exactly equals 8 × 1024 × 1024 = 8388608 bytes, yet the test uses 8388609 bytes—just one byte more. This off-by-one may be an astute choice to ensure that files strictly greater than 8MB trigger the popover. If that’s your intention, rock on! Otherwise, consider aligning the value to exactly 8MB or adding a clarifying comment for future maintainers.
- Inspect the test case in
src/app/fyle/add-edit-expense/add-edit-expense-2.spec.tsat lines 1652 and 1659.
| const sizeLimitExceededPopover = await this.popoverController.create({ | ||
| component: PopupAlertComponent, | ||
| componentProps: { | ||
| title: 'Size limit exceeded', | ||
| message: 'The uploaded file is greater than 5MB in size. Please reduce the file size and try again.', | ||
| message: `The uploaded file is greater than ${(maxFileSize / (1024 * 1024)).toFixed( |
There was a problem hiding this comment.
toFixedis not needed. It should show exact 8. Please verify
sret-farhan2021
force-pushed
the
FYLE-86cxr4fgh
branch
from
7790871 to
906e310
Compare
|
* intial change * 8mb validation * unit test fix * hardcoding fix * hardcoding fix * unit test fix * minor fix * minor fix --------- Co-authored-by: Mohammed farhan K <[email protected]>
Clickup
https://app.clickup.com/t/86cxr4fgh
Code Coverage
Please add code coverage here
UI Preview
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes