Ba 1776 do not allow upload of too large logo #137
Conversation
|
WalkthroughThis pull request introduces updates to the changelogs and version numbers for the Changes
Possibly related PRs
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint (1.23.1)
packages/design-system/components/Dropzone/index.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Cannot read config file: /packages/design-system/.eslintrc.js
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: 0
🧹 Outside diff range and nitpick comments (4)
packages/design-system/components/Dropzone/types.ts (1)
18-18: Add JSDoc documentation for the maxFileSize propWhile the implementation is correct, please add JSDoc documentation to clarify:
- The unit of measurement (bytes, KB, MB?)
- The expected value range
- The default value (15MB as per PR description)
Example:
+ /** Maximum allowed file size in bytes. Defaults to 15MB (15728640 bytes). Must be a positive number. */ maxFileSize?: numberpackages/design-system/components/Dropzone/index.tsx (2)
34-37: Consider extracting size conversion logic for better maintainability.While the file size validation works correctly, the byte conversion logic could be more maintainable.
Consider extracting the conversion logic:
+ const MB_TO_BYTES = 1024 * 1024; const { sendToast } = useNotification() const { open, getRootProps, getInputProps, isFocused, isDragAccept, isDragReject } = useDropzone({ accept, onDrop: async (acceptedFiles: any) => { if (acceptedFiles.length === 0) return - if (acceptedFiles[0].size > maxFileSize * 1024 * 1024) { + if (acceptedFiles[0].size > maxFileSize * MB_TO_BYTES) { sendToast(`This file is too large (max ${maxFileSize} MB).`, { type: 'error' }) return }
54-66: Consider adding aria-label for better accessibility.The image preview implementation looks good and fixes the "Change Image" button functionality. However, consider adding an aria-label to improve accessibility.
return ( <> - <input {...getInputProps()} /> + <input {...getInputProps()} aria-label="Upload image input" /> <Card> <Box p={2} display="flex" flexDirection="column" alignItems="center"> <img key={files} src={files} alt="preview" style={{ maxHeight: '200px', maxWidth: '100%' }} /> </Box> </Card> </> )packages/design-system/CHANGELOG.md (1)
7-7: Enhance the changelog entry with more details.The current changelog entry could be more descriptive. Consider expanding it to include:
- The default file size limit (15MB)
- Mention of customizable limits
- The fix for the "Change Image" button functionality
Here's a suggested improvement:
-Impose file size limits on Dropzones +- Added file size validation to Dropzones (default: 15MB, customizable) +- Fixed "Change Image" button to properly trigger file selection when image is already uploaded
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between 08bfce4 and 01b5d0d0ae7f6e99899d7e39b872139ff4b2a577.
📒 Files selected for processing (6)
packages/components/CHANGELOG.md(1 hunks)packages/components/package.json(1 hunks)packages/design-system/CHANGELOG.md(1 hunks)packages/design-system/components/Dropzone/index.tsx(3 hunks)packages/design-system/components/Dropzone/types.ts(1 hunks)packages/design-system/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/components/CHANGELOG.md
- packages/components/package.json
- packages/design-system/package.json
🔇 Additional comments (4)
packages/design-system/components/Dropzone/types.ts (1)
18-19: Verify potential conflict with DropzoneOptions.maxSize
The react-dropzone library's DropzoneOptions already includes a maxSize property. Let's verify how these two size limitations interact to prevent confusion.
✅ Verification successful
Let me gather more information about how these properties are used in the implementation.
No conflict between maxFileSize and DropzoneOptions.maxSize
The implementation shows that maxFileSize and DropzoneOptions serve different purposes and work independently:
maxFileSizeis used for custom size validation in theonDropcallback before theDropzoneOptionsare appliedDropzoneOptionsis spread after the custom validation, allowing any additional configuration without conflicts
The current implementation is correct and maintains clear separation of concerns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if maxSize from DropzoneOptions is used elsewhere in the codebase
# Expected: No other usage of maxSize to avoid conflicts
# Search for maxSize usage in Dropzone component
rg "maxSize" --type ts --type tsx
# Search for DropzoneOptions type usage
ast-grep --pattern 'DropzoneOptions'
Length of output: 294
Script:
#!/bin/bash
# Check the implementation details of the Dropzone component
rg -t ts -t typescript "maxFileSize|maxSize" -A 5 packages/design-system/components/Dropzone/
# Get the full content of the types file to understand the interface
cat packages/design-system/components/Dropzone/types.ts
# Get the implementation file to see how these options are used
cat packages/design-system/components/Dropzone/index.tsx
Length of output: 4014
packages/design-system/components/Dropzone/index.tsx (2)
1-1: LGTM! Well-structured prop changes and imports.
The new maxFileSize prop with a default of 15MB and the dynamic subTitle are well implemented. The useNotification import is appropriately added for the new notification feature.
Also applies to: 3-3, 23-24
23-24: Verify maxFileSize prop usage across the codebase.
Let's ensure all instances of the Dropzone component are aware of the new maxFileSize prop and its implications.
✅ Verification successful
Let me gather more information to verify the Dropzone component usage across the codebase.
Let me try a different approach to search for Dropzone imports and usage.
Let me try one final search to find actual component usage in the codebase.
Based on the gathered information, I can now provide the final response:
The maxFileSize prop implementation appears to be self-contained and safe
The Dropzone component is properly defined with the maxFileSize prop in its types and implementation, with a default value of 15MB. Since no external usage of the component was found in the codebase, the changes are isolated to the design system package and won't cause any breaking changes. The prop is optional with a sensible default value, maintaining backward compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Dropzone component usage to verify maxFileSize prop adoption
rg -l "Dropzone" | xargs rg "<Dropzone" -A 5
Length of output: 1200
Script:
#!/bin/bash
# Search for files importing Dropzone component
rg -l "import.*Dropzone" --type typescript --type tsx
# Then search for actual usage of Dropzone in those files
echo "=== Dropzone Usage ==="
rg "import.*Dropzone" -A 20 --type typescript --type tsx
Length of output: 239
Script:
#!/bin/bash
# Search for files importing Dropzone component without file type restrictions
rg -l "import.*Dropzone"
# Then search for actual usage patterns
echo "=== Dropzone Usage ==="
rg "import.*Dropzone" -A 20
# Also search for JSX usage of Dropzone
echo "=== JSX Dropzone Usage ==="
rg "<Dropzone.*>" -A 3
Length of output: 5060
Script:
#!/bin/bash
# Search for files that might be using the Dropzone component
fd -e tsx -e ts -e jsx -e js | xargs rg -l "Dropzone"
# Then look for actual component usage in those files
echo "=== Component Usage ==="
fd -e tsx -e ts -e jsx -e js | xargs rg "<Dropzone[^a-zA-Z]" -B 2 -A 5
Length of output: 374
packages/design-system/CHANGELOG.md (1)
3-8: Version bump looks appropriate.
The changes (file size validation and button fix) are backward compatible improvements, making this an appropriate patch version bump.
Let's verify the version bump follows semantic versioning by checking recent changes:
✅ Verification successful
Let me gather more information about the recent changes to verify if the version bump is appropriate.
Version bump as patch is correct
The verification confirms this is an appropriate patch version bump because:
- The
DropzonePropsinterface remains unchanged, withmaxFileSizealready being an optional prop - No breaking changes were found in recent commit messages
- The change only adds validation using an existing optional prop, which is a backward compatible improvement
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify version bump follows semantic versioning by checking for breaking changes
# Test: Check for API changes in Dropzone component
ast-grep --pattern 'export interface DropzoneProps {
$$$
}'
# Test: Check for Dropzone component usage changes
rg -A 5 'Dropzone.*props'
Length of output: 1259
Script:
#!/bin/bash
# Get the git diff of recent changes to Dropzone component
git diff HEAD~1 HEAD -- packages/design-system/components/Dropzone/
# Check previous interface definition
git show HEAD~1:packages/design-system/components/Dropzone/types.ts
# Look for any breaking changes in commit messages
git log -n 5 --grep="BREAKING CHANGE\|breaking-change" packages/design-system/
Length of output: 701
tsl-ps2
force-pushed
the
BA-1776-do-not-allow-upload-of-too-large-logo
branch
from
01b5d0d to
ecfdced
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/design-system/components/Dropzone/index.tsx (2)
34-37: Consider extracting size conversion logic for better maintainability.While the file size validation works correctly, the byte conversion logic could be more maintainable.
Consider extracting the conversion logic:
+const MB_TO_BYTES = 1024 * 1024; + const Dropzone: FC<DropzoneProps> = ({ // ...props }) => { // ... onDrop: async (acceptedFiles: any) => { if (acceptedFiles.length === 0) return - if (acceptedFiles[0].size > maxFileSize * 1024 * 1024) { + if (acceptedFiles[0].size > maxFileSize * MB_TO_BYTES) { sendToast(`This file is too large (max ${maxFileSize} MB).`, { type: 'error' }) return }
Line range hint
31-43: Consider improving type safety in the onDrop handler.The
acceptedFilesparameter is typed asany, which could lead to runtime errors.Consider adding proper typing:
- onDrop: async (acceptedFiles: any) => { + onDrop: async (acceptedFiles: File[]) => { if (acceptedFiles.length === 0) return if (acceptedFiles[0].size > maxFileSize * 1024 * 1024) { sendToast(`This file is too large (max ${maxFileSize} MB).`, { type: 'error' }) return } - const imgString = await getImageString(acceptedFiles[0]) + const imgString = await getImageString(acceptedFiles[0] as File)packages/components/CHANGELOG.md (1)
7-8: Fix markdown list indentationThe unordered list indentation should be 0 spaces according to the markdown linting rules.
-Updated dependencies - - @baseapp-frontend/design-system@0.0.17 +Updated dependencies +- @baseapp-frontend/design-system@0.0.17🧰 Tools
🪛 Markdownlint (0.35.0)
8-8: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between 01b5d0d0ae7f6e99899d7e39b872139ff4b2a577 and ecfdced.
📒 Files selected for processing (6)
packages/components/CHANGELOG.md(1 hunks)packages/components/package.json(1 hunks)packages/design-system/CHANGELOG.md(1 hunks)packages/design-system/components/Dropzone/index.tsx(3 hunks)packages/design-system/components/Dropzone/types.ts(1 hunks)packages/design-system/package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/components/package.json
- packages/design-system/CHANGELOG.md
- packages/design-system/components/Dropzone/types.ts
- packages/design-system/package.json
🧰 Additional context used
🪛 Markdownlint (0.35.0)
packages/components/CHANGELOG.md
8-8: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
🔇 Additional comments (3)
packages/design-system/components/Dropzone/index.tsx (2)
1-1: LGTM! Props and imports are well structured.
The new maxFileSize prop with a sensible default of 15MB and the dynamic subTitle provide good configuration flexibility while maintaining backward compatibility.
Also applies to: 3-3, 23-24
54-66: LGTM! Rendering changes fix the "Change Image" button functionality.
The input element is now correctly rendered when files are present, ensuring the file input dialog can be opened via the "Change Image" button.
packages/components/CHANGELOG.md (1)
3-9: Verify version bump necessity
Both versions 0.0.16 and 0.0.17 update to the same design-system version (0.0.17). This might indicate an unnecessary version bump or a potential versioning issue.
Also applies to: 11-16
✅ Verification successful
Let me check the changelog history to verify the version bump necessity.
Let me try a different approach to locate and read the changelog files.
Based on the changelog analysis, I can now provide a final response:
Version bump appears necessary
The version bump from 0.0.16 to 0.0.17 in components package was necessary because:
- Version 0.0.16 had "Storybook cleanup" changes and updated design-system to 0.0.17
- Version 0.0.17 was needed to align with a new design-system dependency version (0.0.17)
The sequential version bumps had different purposes and changes, making them both valid updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check package.json files for version consistency
# Expected: Different versions or different changes between versions
# Check package.json in both packages
fd -e json package.json | xargs cat | jq -c 'select(.name | contains("@baseapp-frontend")) | {name: .name, version: .version, dependencies: .dependencies}'
Length of output: 3216
Script:
#!/bin/bash
# Check the CHANGELOG.md content for both packages
echo "=== components/CHANGELOG.md ==="
fd -g "components/CHANGELOG.md" --exec cat {}
echo -e "\n=== design-system/CHANGELOG.md ==="
fd -g "design-system/CHANGELOG.md" --exec cat {}
Length of output: 256
Script:
#!/bin/bash
# Search for CHANGELOG.md files in packages directory
echo "=== Finding and reading CHANGELOG.md files ==="
find packages -name "CHANGELOG.md" -type f -exec sh -c 'echo "=== $1 ==="; cat "$1"' sh {} \;
Length of output: 33796
🧰 Tools
🪛 Markdownlint (0.35.0)
8-8: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
| ## 0.0.17 | ||
|
|
||
| ### Patch Changes | ||
|
|
||
| Updated dependencies | ||
| - @baseapp-frontend/design-system@0.0.17 | ||
|
|
There was a problem hiding this comment.
Changelog entries don't reflect the PR objectives
The changelog entry for version 0.0.17 only mentions updating the design-system dependency, but doesn't describe the actual changes implemented in this PR:
- File size limit feature for the Dropzone component
- Fix for the "Change Image" button functionality
Consider adding these important changes to make the changelog more informative for users.
## 0.0.17
### Patch Changes
+ - Added file size limit feature to Dropzone component
+ - Fixed "Change Image" button functionality
Updated dependencies
- @baseapp-frontend/design-system@0.0.17📝 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.
| ## 0.0.17 | |
| ### Patch Changes | |
| Updated dependencies | |
| - @baseapp-frontend/design-system@0.0.17 | |
| ## 0.0.17 | |
| ### Patch Changes | |
| - Added file size limit feature to Dropzone component | |
| - Fixed "Change Image" button functionality | |
| Updated dependencies | |
| - @baseapp-frontend/design-system@0.0.17 |
🧰 Tools
🪛 Markdownlint (0.35.0)
8-8: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
anicioalexandre
deleted the
BA-1776-do-not-allow-upload-of-too-large-logo
branch
Dropzoneto files smaller than a certain maximum sizeSummary by CodeRabbit
Release Notes
New Features
Dependency Updates
@baseapp-frontend/design-systemdependency to version0.0.17and0.0.18for improved functionality and stability.Changelog Updates