fix: Add validation for gallery image upload #3457
Conversation
sret-farhan2021
requested review from
Chethan-Fyle and
arjunaj5
as code owners
WalkthroughThe Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
✨ Finishing Touches
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: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/app/shared/components/capture-receipt/capture-receipt.component.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (12.x)
🔇 Additional comments (1)
src/app/shared/components/capture-receipt/capture-receipt.component.ts (1)
Line range hint
482-525: Kabali da! This implementation is solid as a rock!The file size validation is implemented effectively with proper error handling and user feedback. The code is well-structured and maintainable. Just apply those small refactoring suggestions, and it's ready to go!
| async showSizeLimitExceededPopover(): Promise<void> { | ||
| 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.', | ||
| primaryCta: { | ||
| text: 'OK', | ||
| }, | ||
| }, | ||
| cssClass: 'pop-up-in-center', | ||
| }); | ||
|
|
||
| await sizeLimitExceededPopover.present(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Style ah? Let's make this popover message dynamic!
The size limit is hardcoded in the message. Let's use our constant to make it dynamic and consistent!
async showSizeLimitExceededPopover(): Promise<void> {
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 ${MAX_RECEIPT_SIZE_MB}MB in size. Please reduce the file size and try again.`,
primaryCta: {
text: 'OK',
},
},
cssClass: 'pop-up-in-center',
});Committable suggestion skipped: line range outside the PR's diff.
| calculateBase64Size(base64String: string): number { | ||
| const padding = (base64String.match(/(=+)$/) || [])[0]?.length || 0; | ||
| return (base64String.length * 3) / 4 - padding; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Magizhchi! Let's make this method bulletproof!
The base64 size calculation needs input validation and documentation. Here's what you need, boss!
+/**
+ * Calculates the size of a base64 string in bytes.
+ * @param base64String - The base64 encoded string
+ * @returns The size in bytes
+ * @throws Error if the input is not a valid base64 string
+ */
calculateBase64Size(base64String: string): number {
+ if (!base64String || typeof base64String !== 'string') {
+ throw new Error('Invalid input: base64String must be a non-empty string');
+ }
+
+ // Basic validation of base64 format
+ if (!/^[A-Za-z0-9+/]*={0,2}$/.test(base64String)) {
+ throw new Error('Invalid input: not a valid base64 string');
+ }
+
const padding = (base64String.match(/(=+)$/) || [])[0]?.length || 0;
return (base64String.length * 3) / 4 - padding;
}📝 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.
| calculateBase64Size(base64String: string): number { | |
| const padding = (base64String.match(/(=+)$/) || [])[0]?.length || 0; | |
| return (base64String.length * 3) / 4 - padding; | |
| } | |
| /** | |
| * Calculates the size of a base64 string in bytes. | |
| * @param base64String - The base64 encoded string | |
| * @returns The size in bytes | |
| * @throws Error if the input is not a valid base64 string | |
| */ | |
| calculateBase64Size(base64String: string): number { | |
| if (!base64String || typeof base64String !== 'string') { | |
| throw new Error('Invalid input: base64String must be a non-empty string'); | |
| } | |
| // Basic validation of base64 format | |
| if (!/^[A-Za-z0-9+/]*={0,2}$/.test(base64String)) { | |
| throw new Error('Invalid input: not a valid base64 string'); | |
| } | |
| const padding = (base64String.match(/(=+)$/) || [])[0]?.length || 0; | |
| return (base64String.length * 3) / 4 - padding; | |
| } |
| const oversizedReceipts = receiptsFromGallery.filter((receiptBase64) => { | ||
| const receiptSize = this.calculateBase64Size(receiptBase64); | ||
| return receiptSize > 5 * 1024 * 1024; // 5MB in bytes | ||
| }); | ||
|
|
||
| if (oversizedReceipts.length > 0) { | ||
| return this.showSizeLimitExceededPopover(); | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Mind it! Extract the size limit to a constant, partner!
The size limit of 5MB is hardcoded. Let's make it more maintainable by moving it to a constant at the top of the file.
+const MAX_RECEIPT_SIZE_MB = 5;
+const MAX_RECEIPT_SIZE_BYTES = MAX_RECEIPT_SIZE_MB * 1024 * 1024;
const oversizedReceipts = receiptsFromGallery.filter((receiptBase64) => {
const receiptSize = this.calculateBase64Size(receiptBase64);
- return receiptSize > 5 * 1024 * 1024; // 5MB in bytes
+ return receiptSize > MAX_RECEIPT_SIZE_BYTES;
});Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/manual-appflow-1.yml(2 hunks)src/app/shared/components/capture-receipt/capture-receipt.component.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (12.x)
🔇 Additional comments (3)
src/app/shared/components/capture-receipt/capture-receipt.component.ts (2)
27-27: Mind it! The import looks perfect, partner!The MAX_FILE_SIZE constant import is well-placed and follows Angular's style guide.
508-522: The hardcoded size limit in the message needs to be dynamic!.github/workflows/manual-appflow-1.yml (1)
54-54: Superb! Consistency is maintained!The upgrade to v4 is consistently applied for both Android and iOS artifact downloads.
| if (this.calculateImageSize(receiptBase64Data) < MAX_FILE_SIZE) { | ||
| this.base64ImagesWithSource.push({ | ||
| source: 'MOBILE_DASHCAM_GALLERY', | ||
| base64Image: receiptBase64Data, | ||
| }); | ||
| } else { | ||
| this.showSizeLimitExceededPopover(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Style ah? Let's make this code more powerful!
Instead of using forEach with conditional pushing, we can use filter for a more elegant solution.
Here's a more stylish way to handle this:
- if (this.calculateImageSize(receiptBase64Data) < MAX_FILE_SIZE) {
- this.base64ImagesWithSource.push({
- source: 'MOBILE_DASHCAM_GALLERY',
- base64Image: receiptBase64Data,
- });
- } else {
- this.showSizeLimitExceededPopover();
- }
+ const oversizedImages = receiptsFromGallery
+ .map(receipt => 'data:image/jpeg;base64,' + receipt)
+ .filter(base64Data => this.calculateImageSize(base64Data) >= MAX_FILE_SIZE);
+
+ if (oversizedImages.length > 0) {
+ this.showSizeLimitExceededPopover();
+ }
+
+ this.base64ImagesWithSource.push(
+ ...receiptsFromGallery
+ .map(receipt => 'data:image/jpeg;base64,' + receipt)
+ .filter(base64Data => this.calculateImageSize(base64Data) < MAX_FILE_SIZE)
+ .map(base64Data => ({
+ source: 'MOBILE_DASHCAM_GALLERY',
+ base64Image: base64Data
+ }))
+ );Committable suggestion skipped: line range outside the PR's diff.
| calculateImageSize(base64Image: string): number { | ||
| const stringLength = base64Image.length - 'data:image/jpeg;base64,'.length; | ||
| const sizeInBytes = 4 * Math.ceil(stringLength / 3) * 0.5624896334383812; | ||
| return sizeInBytes; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Magizhchi! Let's make this method more robust!
The magic number 0.5624896334383812 should be extracted as a constant, and the method needs proper documentation.
Here's a more professional approach:
+ private readonly BASE64_HEADER = 'data:image/jpeg;base64,';
+ private readonly BASE64_SIZE_MULTIPLIER = 0.5624896334383812;
+
+ /**
+ * Calculates the size of a base64 encoded image in bytes
+ * @param base64Image - The base64 encoded image string including the data URL prefix
+ * @returns The size of the image in bytes
+ */
calculateImageSize(base64Image: string): number {
- const stringLength = base64Image.length - 'data:image/jpeg;base64,'.length;
- const sizeInBytes = 4 * Math.ceil(stringLength / 3) * 0.5624896334383812;
+ const stringLength = base64Image.length - this.BASE64_HEADER.length;
+ const sizeInBytes = 4 * Math.ceil(stringLength / 3) * this.BASE64_SIZE_MULTIPLIER;
return sizeInBytes;
}📝 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.
| calculateImageSize(base64Image: string): number { | |
| const stringLength = base64Image.length - 'data:image/jpeg;base64,'.length; | |
| const sizeInBytes = 4 * Math.ceil(stringLength / 3) * 0.5624896334383812; | |
| return sizeInBytes; | |
| } | |
| private readonly BASE64_HEADER = 'data:image/jpeg;base64,'; | |
| private readonly BASE64_SIZE_MULTIPLIER = 0.5624896334383812; | |
| /** | |
| * Calculates the size of a base64 encoded image in bytes | |
| * @param base64Image - The base64 encoded image string including the data URL prefix | |
| * @returns The size of the image in bytes | |
| */ | |
| calculateImageSize(base64Image: string): number { | |
| const stringLength = base64Image.length - this.BASE64_HEADER.length; | |
| const sizeInBytes = 4 * Math.ceil(stringLength / 3) * this.BASE64_SIZE_MULTIPLIER; | |
| return sizeInBytes; | |
| } |
| @@ -22,7 +22,7 @@ jobs: | |||
| native-config: staging | |||
| upload-artifact: Android | |||
|
|
|||
| - uses: actions/download-artifact@v3 | |||
| - uses: actions/download-artifact@v4 | |||
There was a problem hiding this comment.
💡 Codebase verification
Breaking Changes Alert, Boss!
The v4 upgrade for actions/download-artifact introduces breaking changes. According to the release notes, artifacts created with versions v3 and below are incompatible with v4. This means that if your workflows use a mix of v3 and v4 (for download and upload respectively), you’re bound to face issues.
- Ensure corresponding changes for both upload and download artifact actions are made.
- Verify that all related steps in your workflows are updated to version 4 to prevent any incompatibilities.
🔗 Analysis chain
Mind it! Let's verify this upgrade, partner!
The upgrade to actions/download-artifact@v4 is good, but we should verify there are no breaking changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for breaking changes in actions/download-artifact v4
curl -s https://api.github.com/repos/actions/download-artifact/releases/tags/v4.0.0 | jq -r '.body'
Length of output: 1320
|
Clickup
https://app.clickup.com/t/86cxux3xk
Code Coverage
Please add code coverage here
UI Preview
Please add screenshots for UI changes
Summary by CodeRabbit