Tshepo/en/file component #3145
Tshepo/en/file component #3145
Conversation
WalkthroughThis PR updates multiple file upload and designer components. It enhances the FileUpload component by adding new icons, additional properties, and a preview mechanism, while refactoring functions for improved clarity. The DraggerStub component now accepts external styling instead of using internal hooks. In parallel, the styling utilities have been overhauled to support dynamic parameters, and the designer components received updates for configuration forms, default styles, and minor UI text adjustments. These changes collectively improve component configurability, presentation, and user interaction. Changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
🧹 Nitpick comments (5)
shesha-reactjs/src/components/fileUpload/styles/styles.ts (1)
3-310: Consider splitting large style blocks for better maintainability.This file contains substantial styling logic within a single function. Segmenting it into smaller, reusable style modules can simplify long-term maintenance and enhance readability.
shesha-reactjs/src/designer-components/fileUpload/settingsForm.ts (1)
1-746: Reduce repetition by extracting reusable logic.This file contains multiple repeated calls to create identical or highly similar input rows, each featuring the same
readOnly,hidden, orjsSettingstructures. Consider extracting common logic into helper functions to follow DRY (Don't Repeat Yourself) principles and improve maintainability.shesha-reactjs/src/components/fileUpload/index.tsx (1)
175-203: Break down the file rendering logic into smaller units.
renderFileItemincludes both text-based and thumbnail-based rendering paths. Consider extracting separate components or utility functions for these paths to make the code simpler and more testable.shesha-reactjs/src/designer-components/fileUpload/index.tsx (2)
41-45: Consider validating new style-related props
While these optional props add flexibility (listType,thumbnailWidth,thumbnailHeight,borderRadius,hideFileName), ensure that unexpected or invalid values (e.g., negativeborderRadius) are handled gracefully in the component logic.
70-106: Handle fetch cancellation to avoid possible race conditions
ThisuseEffectimplements object URL revocation carefully. Consider adding an AbortController to cancel the fetch if the component unmounts in mid-request. This can preempt potential “fetch resolved after unmount” warnings and free resources.Example (illustrative only):
useEffect(() => { let objectUrl = ''; + const controller = new AbortController(); + const { signal } = controller; const fetchStyles = async () => { ... const response = await fetch(url, { headers: httpHeaders, + signal }); }; fetchStyles(); return () => { if (objectUrl) { URL.revokeObjectURL(objectUrl); } + controller.abort(); }; }, [background, backendUrl, httpHeaders, jsStyle]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
shesha-starter/intent/ShaCompanyName.ShaProjectName/Intent.Metadata/Visual Studio/ShaCompanyName.ShaProjectName/Elements/ASP.NET Core Web Application/ShaCompanyName.ShaProjectName.Web.Host__d06tm1hb.xmlis excluded by none and included by noneshesha-starter/intent/ShaCompanyName.ShaProjectName/Intent.Metadata/Visual Studio/ShaCompanyName.ShaProjectName/Elements/Class Library (.NET Core)/ShaCompanyName.ShaProjectName.Application__ht499nxl.xmlis excluded by none and included by noneshesha-starter/intent/ShaCompanyName.ShaProjectName/Intent.Metadata/Visual Studio/ShaCompanyName.ShaProjectName/Elements/Class Library (.NET Core)/ShaCompanyName.ShaProjectName.Domain__1stf96vf.xmlis excluded by none and included by noneshesha-starter/intent/ShaCompanyName.ShaProjectName/Intent.Metadata/Visual Studio/ShaCompanyName.ShaProjectName/Elements/Class Library (.NET Core)/ShaCompanyName.ShaProjectName.Web.Core__k5ykvd6l.xmlis excluded by none and included by none
📒 Files selected for processing (12)
shesha-reactjs/src/components/fileUpload/index.tsx(4 hunks)shesha-reactjs/src/components/fileUpload/stubs.tsx(1 hunks)shesha-reactjs/src/components/fileUpload/styles/styles.ts(1 hunks)shesha-reactjs/src/components/storedFilesRendererBase/index.tsx(3 hunks)shesha-reactjs/src/designer-components/checkboxGroup/settingsForm.ts(1 hunks)shesha-reactjs/src/designer-components/fileUpload/index.tsx(4 hunks)shesha-reactjs/src/designer-components/fileUpload/settingsForm.ts(1 hunks)shesha-reactjs/src/designer-components/fileUpload/utils.ts(1 hunks)shesha-reactjs/src/designer-components/inputComponent/index.tsx(1 hunks)shesha-reactjs/src/designer-components/link/index.tsx(1 hunks)shesha-reactjs/src/designer-components/link/settingsForm.ts(1 hunks)shesha-reactjs/src/designer-components/text/settingsForm.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-attempt
🔇 Additional comments (23)
shesha-reactjs/src/designer-components/inputComponent/index.tsx (1)
1-1: Minor formatting improvement.The addition of an empty line at the beginning of the file follows code style guidelines for better readability.
shesha-reactjs/src/designer-components/text/settingsForm.ts (1)
967-967: Good practice to have newline at end of file.Adding a newline character at the end of the file adheres to common coding standards and improves compatibility with various text editors and source control systems.
shesha-reactjs/src/designer-components/checkboxGroup/settingsForm.ts (1)
121-121: Improved capitalization consistency.Updating "Api url" to "Api URL" correctly capitalizes the acronym URL (Uniform Resource Locator), improving readability and maintaining consistent terminology throughout the UI.
shesha-reactjs/src/designer-components/link/index.tsx (1)
117-117:❓ Verification inconclusive
Removed inline styles from link component.
The
styleattribute has been removed from the<a>tag, which means the calculatedfinalStyleis no longer applied to the link in non-designer mode. This could potentially change the visual appearance of links in the application.Please verify this change was intentional and that link styling is handled properly through CSS classes or other means. The
finalStylevariable is still being calculated at line 68 but is no longer used for the<a>tag whenhasChildrenis true.
🏁 Script executed:
#!/bin/bash # Check for CSS classes that might be styling the links echo "Searching for CSS classes related to sha-link..." grep -r "sha-link" --include="*.css" --include="*.scss" --include="*.less" .Length of output: 183
Critical: Confirm link styling implementation
- The inline
styleattribute was removed from the<a>tag (line 117 inshesha-reactjs/src/designer-components/link/index.tsx), meaning the previously calculatedfinalStyle(at line 68) is no longer applied whenhasChildrenis true.- A grep search for CSS definitions related to the
"sha-link"class in.css,.scss, and.lessfiles produced no results. This raises a concern that external styling for links might not be in place.- Action Required:
- Please verify that the removal was intentional and that styling is correctly handled by external CSS (possibly in a global stylesheet or via another mechanism).
- Confirm whether the computation of
finalStyleis obsolete or if it should be reapplied to ensure the intended visual appearance.shesha-reactjs/src/components/storedFilesRendererBase/index.tsx (3)
302-302: Style prop integration correctly implemented.The
DraggerStubcomponent now properly receives thestylesprop, aligning with the component's updated implementation. This enables consistent styling when rendering the stub in disabled mode.
316-316: Style prop integration correctly implemented.The
DraggerStubcomponent now receives thestylesprop, ensuring consistent styling when rendering within the Dragger component. This matches the updated implementation ofDraggerStubthat now expects external styling rather than using internal hooks.
355-355: Good addition of default export.Adding a default export for the component improves flexibility when importing, which is a good practice for component libraries.
shesha-reactjs/src/designer-components/fileUpload/utils.ts (1)
1-34: Well-structured default styles utility function.The
defaultStylesfunction provides a comprehensive set of default styling properties including border, dimensions, background, font, and shadow settings. This is a good pattern for maintaining consistent styling across file upload components.The implementation uses sensible default values and follows a clean structure that aligns with the
IStyleTypeinterface. This utility will help standardize component appearance and reduce code duplication.shesha-reactjs/src/designer-components/link/settingsForm.ts (2)
377-390: Conditional settings visibility properly implemented.The new settings input row correctly implements conditional visibility and read-only states based on the component properties:
- Hidden when the component doesn't have children
- Read-only when the component's
readOnlyproperty is trueThis enhances the user experience by showing relevant configuration options only when needed.
391-468: Comprehensive dropdown options for justifyItems.The dropdown provides a complete set of CSS justify-items options, giving users full control over layout alignment. The options are well-organized and use proper CSS values.
shesha-reactjs/src/components/fileUpload/stubs.tsx (1)
1-4: Improved component implementation with external styling.The component has been refactored to accept styles as a prop rather than using an internal hook. This is a better approach for component reusability and flexibility, allowing the parent component to control styling.
The simplified React import also removes unnecessary TypeScript typing, which makes the code cleaner.
shesha-reactjs/src/components/fileUpload/styles/styles.ts (1)
157-163: Avoid injecting arbitrary CSS without sanitization.The interpolation of
${style}within CSS might allow unsafe or unintended properties, especially ifstyleis user-controlled. Consider validating or sanitizing these values prior to injection.Would you like me to provide a script to detect possible injection sources across the codebase?
shesha-reactjs/src/components/fileUpload/index.tsx (1)
211-211: Unify usage of style props and generated styles.The
style: !isDragger && stylesPropline might overlap or conflict with the dynamically computedstylesfromuseStyles(props). Double-check that both are needed and won’t override each other unintentionally.shesha-reactjs/src/designer-components/fileUpload/index.tsx (10)
2-6: Newly added imports appear correctly utilized
No issues found with the expanded imports (CSSProperties,useEffect,useMemo,useState, etc.). They support the added functionality for styling and background handling.
11-12: Utility imports are straightforward
Bringing inevaluateValueandgetStylealigns well with the new style and form evaluation logic. No concerns here.
32-32: Extending theIFileUploadPropswithIStyleType
This addition looks appropriate to unify the style props with the component’s config.
55-55: Destructuring looks fine
No issues with retrievingbackendUrlandhttpHeadershere.
57-62: Safely accessing model properties
Using optional chaining (model?.foo) is a good approach, preventing runtime errors if the model is undefined.
64-68: Memoizing styles is a good practice
EmployinguseMemofor size, border, font, and shadow styles promotes performance by avoiding unnecessary recalculations.
108-120: Consolidating multiple style sources
Merging dimension, border, font, background, and shadow styles intofinalStyleis clean and helps keep logic centralized.
166-177: Confirm default ofhideFileName
By settinghideFileName: true, filenames are hidden by default, which may not always be expected. Please verify if that aligns with typical usage and user requirements.
215-215: Default export
ExportingFileUploadComponentas default is aligned with the rest of the codebase.
180-210:❓ Verification inconclusive
Check that migration logic is tested
Versioned migration steps look consistent with your pattern. Confirm the existence of unit tests or integration tests to ensure older models correctly migrate to the new structure.
🏁 Script executed:
#!/bin/bash # Attempt to locate tests covering 'FileUploadComponent' or its 'migrator' function. # We'll search for references to 'FileUploadComponent', 'migrator', or 'initModel'. fd -e ts -e tsx --exec rg -A 5 "FileUploadComponent|migrator|initModel" {}Length of output: 78
Ensure that the FileUpload migration logic is thoroughly tested
Our automated search did not reveal any tests explicitly covering the migrator logic inshesha-reactjs/src/designer-components/fileUpload/index.tsx(lines 180-210). Please manually verify that there are unit or integration tests validating that older models correctly migrate to the new structure. If no such tests exist or the coverage is insufficient, add test cases to safeguard the migration behavior.
| useEffect(() => { | ||
| if (fileInfo && url) { | ||
| fetch(url, { headers: { ...httpHeaders, 'Content-Type': 'application/octet-stream' } }) | ||
| .then((response) => response.blob()) | ||
| .then((blob) => URL.createObjectURL(blob)) | ||
| .then((url) => setImageUrl(url)); | ||
| } | ||
| }, [fileInfo]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Revoke object URLs to prevent memory leaks.
A new object URL is created on every fetch, but it’s never revoked. Call URL.revokeObjectURL within a cleanup function in useEffect to free memory once the image is no longer required.
Apply this diff to provide a cleanup:
useEffect(() => {
if (fileInfo && url) {
fetch(url, { headers: { ...httpHeaders, 'Content-Type': 'application/octet-stream' } })
.then(response => response.blob())
.then(blob => URL.createObjectURL(blob))
.then(newUrl => {
setImageUrl(newUrl);
+ return () => URL.revokeObjectURL(newUrl);
});
}
}, [fileInfo]);Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
New Features
Refactor & Style