Feature/bo val #1995
Feature/bo val #1995
Conversation
* validation from ui * empty top rows considered * info card added for error in place of toast * review changes
📝 WalkthroughWalkthroughThe pull request includes modifications across several components related to boundary management and file handling. Key changes involve the 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: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (2)
health/micro-ui/web/micro-ui-internals/package.jsonis excluded by!**/*.jsonhealth/micro-ui/web/workbench/package.jsonis excluded by!**/*.json
📒 Files selected for processing (4)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/XlsPreviewNew.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/BoundaryHome.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/ViewHierarchy.js(7 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/validateBoundaryExcel.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/validateBoundaryExcel.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/BoundaryHome.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/XlsPreviewNew.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/ViewHierarchy.js (1)
Pattern **/*.js: check
🪛 Biome (1.9.4)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/ViewHierarchy.js
[error] 586-586: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
[error] 592-592: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
[error] 600-600: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
🔇 Additional comments (2)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/utils/validateBoundaryExcel.js (1)
4-69: Validation function is well implemented
The validateBoundaryExcelContent function correctly reads and validates the Excel file content, handling various edge cases effectively.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/ViewHierarchy.js (1)
152-152: 🧹 Nitpick (assertive)
Remove commented-out code
The commented-out line on 152 is unnecessary and can be removed to keep the codebase clean.
Apply this diff to remove the redundant code:
- // setShowToast({ label: isValid.error, isError: "error" });Likely invalid or redundant comment.
...-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/XlsPreviewNew.js
Show resolved
Hide resolved
.../web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/BoundaryHome.js
Outdated
Show resolved
Hide resolved
...web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/ViewHierarchy.js
Outdated
Show resolved
Hide resolved
...web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/ViewHierarchy.js
Show resolved
Hide resolved
...web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/ViewHierarchy.js
Outdated
Show resolved
Hide resolved
| } catch (error) { | ||
| setShowToast({ label: error?.response?.data?.Errors?.[0]?.message ? error?.response?.data?.Errors?.[0]?.message : t("FILE_UPLOAD_FAILED") , isError:"error" }); | ||
| const file = event.target.files[0]; // Get the selected file | ||
| setFileName(file.name) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure file is defined before accessing file.name
To prevent potential runtime errors, check if file is defined before calling setFileName(file.name). This ensures that your code doesn't attempt to access properties of an undefined object.
Apply this diff to add the null check:
- setFileName(file.name)
if (file) {
+ setFileName(file.name);
// rest of the codeCommittable suggestion skipped: line range outside the PR's diff.
…gn-manager/src/pages/employee/BoundaryHome.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…gn-manager/src/pages/employee/ViewHierarchy.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…gn-manager/src/pages/employee/ViewHierarchy.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range comments (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/BoundaryHome.js (1)
Line range hint
43-43: Clean up unused state variables.The
authorizedandgeoPodeDatastates are declared but never used effectively. Consider removing them if they're not needed, or implement their intended functionality.Also applies to: 45-45
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/BoundaryHome.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/BoundaryHome.js (1)
Pattern **/*.js: check
🔇 Additional comments (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/BoundaryHome.js (1)
Line range hint 41-41: Implement proper error handling.
The error handling using Toast component is commented out, but the component receives an error from the useBoundaryHome hook. Consider implementing proper error handling to improve user experience.
Let's verify the error handling implementation in other boundary-related components:
Also applies to: 52-52
✅ Verification successful
Based on the search results, I can now provide a final response regarding the error handling implementation:
The error handling implementation is already properly handled in the codebase
The BoundaryHome component and its associated hook (useBoundaryHome) have proper error handling:
- The component receives error state from useBoundaryHome hook:
const {isLoading, data, error} = Digit.Hooks.campaign.useBoundaryHome(...)- The useBoundaryHome hook properly handles and propagates errors:
- Catches and logs errors during API calls
- Returns error objects to the component
- Uses React Query's error handling capabilities
- While there is a commented out Toast component, this is intentional as the error handling has been moved to the hook layer for better separation of concerns.
The current implementation follows React best practices by:
- Separating data fetching and error handling logic into a custom hook
- Using React Query for automatic error handling and retries
- Propagating errors up to the component level for flexible error UI handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Toast usage in boundary-related components
rg -l 'Toast.*USER_NOT_AUTHORISED' packages/modules/campaign-manager/src/
Length of output: 154
Script:
#!/bin/bash
# Search for Toast usage in boundary-related files
rg -l "Toast" health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/
# Search for error handling patterns in boundary-related files
rg "catch|error|Toast" health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/ -A 2
# Check the implementation of useBoundaryHome hook
ast-grep --pattern 'const useBoundaryHome = () => {
$$$
}'
Length of output: 107647
Script:
#!/bin/bash
# Search for error handling in BoundaryHome.js
rg -A 5 "error|Toast" health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/BoundaryHome.js
# Check the implementation of useBoundaryHome hook
rg -A 10 "useBoundaryHome" health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/
# Look for any error handling patterns in hooks directory
rg -A 5 "error|catch|throw" health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/
Length of output: 38637
.../web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/BoundaryHome.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/ViewHierarchy.js(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/ViewHierarchy.js (1)
Pattern **/*.js: check
🪛 Biome (1.9.4)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/ViewHierarchy.js
[error] 590-591: The elements with the following roles can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
[error] 599-599: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
🔇 Additional comments (2)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/ViewHierarchy.js (2)
133-134:
Ensure file is defined before accessing file.name
There is a potential runtime error if file is undefined when accessing file.name at line 134. Please check if file is defined before accessing its properties.
Apply this diff:
- setFileName(file.name)
if (file) {
+ setFileName(file.name);
// rest of the code600-600: 🧹 Nitpick (assertive)
Use self-closing tag for JSX element without children
The <div> element on line 600 has no children and can be self-closed for brevity and consistency.
Apply this diff:
- <div style={{height:"2rem"}}></div>
+ <div style={{height:"2rem"}} />
...web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/ViewHierarchy.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/ViewHierarchy.js(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/ViewHierarchy.js (1)
Pattern **/*.js: check
🪛 Biome (1.9.4)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/ViewHierarchy.js
[error] 590-591: The elements with the following roles can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
[error] 599-599: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
🔇 Additional comments (2)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/ViewHierarchy.js (2)
586-595: 🛠️ Refactor suggestion
Use <button> element instead of <div> with role="button" for better accessibility
Using a <button> element improves semantics and accessibility over a <div> with role="button". This enhances support for assistive technologies and keyboard navigation.
Apply this diff:
<div
className="dustbin-icon"
onClick={() => removeFile()}
onKeyDown={(e) => { if (e.key === 'Enter') removeFile(); }}
tabIndex="0"
role="button"
style={{ cursor: "pointer", marginTop:"1.15rem" }}
>
+ <button
+ className="dustbin-icon"
+ onClick={() => removeFile()}
+ style={{ cursor: "pointer", marginTop:"1.15rem", background: "none", border: "none", padding: 0 }}
+ >
<DustbinIcon />
- </div>
+ </button>🧰 Tools
🪛 Biome (1.9.4)
[error] 590-591: The elements with the following roles can be changed to the following elements:
For examples and more information, see WAI-ARIA Roles
(lint/a11y/useSemanticElements)
600-600: 🧹 Nitpick (assertive)
Use self-closing tag for JSX element without children
The <div> element on line 600 has no children and can be self-closed for cleaner code.
Apply this diff:
-<div style={{height:"2rem"}}></div>
+<div style={{height:"2rem"}} />| // Parse the file and validate its content | ||
| const isValid = await validateBoundaryExcelContent(file, t); | ||
| if (!isValid.success) { | ||
| // setShowToast({ label: isValid.error, isError: "error" }); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove commented-out code to maintain code cleanliness
The line // setShowToast({ label: isValid.error, isError: "error" }); is commented out. Consider removing it to keep the code clean and avoid confusion.
| const validExtensions = ['xls', 'xlsx']; | ||
| const fileExtension = file.name.split('.').pop().toLowerCase(); // Get the file extension | ||
|
|
||
| if (!validExtensions.includes(fileExtension)) { | ||
| setShowToast({ label: t("INVALID_FILE_FORMAT"), isError: "error" }); | ||
| setDisableFile(true); | ||
| event.target.value = ""; | ||
| return; // Exit the function if the file is not valid | ||
| } |
There was a problem hiding this comment.
Enhance file validation to prevent security issues
Relying solely on file extensions for validation can be insecure, as file extensions can be manipulated. It's recommended to validate the file's MIME type to ensure that the uploaded file is indeed an Excel file.
Apply this diff to improve file validation:
const validExtensions = ['xls', 'xlsx'];
const fileExtension = file.name.split('.').pop().toLowerCase(); // Get the file extension
+ const validMimeTypes = [
+ 'application/vnd.ms-excel',
+ 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet'
+ ];
+ if (!validMimeTypes.includes(file.type)) {
+ setShowToast({ label: t("INVALID_FILE_TYPE"), isError: "error" });
+ setDisableFile(true);
+ event.target.value = "";
+ return; // Exit the function if the file is not valid
+ } else
if (!validExtensions.includes(fileExtension)) {
setShowToast({ label: t("INVALID_FILE_FORMAT"), isError: "error" });
setDisableFile(true);
event.target.value = "";
return; // Exit the function if the file is not valid
}📝 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.
| const validExtensions = ['xls', 'xlsx']; | |
| const fileExtension = file.name.split('.').pop().toLowerCase(); // Get the file extension | |
| if (!validExtensions.includes(fileExtension)) { | |
| setShowToast({ label: t("INVALID_FILE_FORMAT"), isError: "error" }); | |
| setDisableFile(true); | |
| event.target.value = ""; | |
| return; // Exit the function if the file is not valid | |
| } | |
| const validExtensions = ['xls', 'xlsx']; | |
| const fileExtension = file.name.split('.').pop().toLowerCase(); // Get the file extension | |
| const validMimeTypes = [ | |
| 'application/vnd.ms-excel', | |
| 'application/vnd.openxmlformats-officedocument.spreadsheetml.sheet' | |
| ]; | |
| if (!validMimeTypes.includes(file.type)) { | |
| setShowToast({ label: t("INVALID_FILE_TYPE"), isError: "error" }); | |
| setDisableFile(true); | |
| event.target.value = ""; | |
| return; // Exit the function if the file is not valid | |
| } else | |
| if (!validExtensions.includes(fileExtension)) { | |
| setShowToast({ label: t("INVALID_FILE_FORMAT"), isError: "error" }); | |
| setDisableFile(true); | |
| event.target.value = ""; | |
| return; // Exit the function if the file is not valid | |
| } |
Choose the appropriate template for your PR:
Feature PR
Feature Request
JIRA ID
Module
Description
Related Issues
Bugfix PR
Bugfix Request
JIRA ID
Module
Description
Root Cause
Related Issues
Release PR
Summary by CodeRabbit
New Features
XlsPreviewNewcomponent.InfoCardfor displaying validation error messages in theViewHierarchycomponent.Bug Fixes
BoundaryHomecomponent to improve responsiveness based on boundary data.Improvements
ViewHierarchycomponent.