User Token cache remove for multiroottenant and css fix #1506
Conversation
📝 WalkthroughWalkthroughThe changes introduced in this pull request primarily focus on enhancing the CSS structure and logic for various components within the application. New CSS classes have been added to improve the styling and responsiveness of the UI, particularly for the input and sandbox components. Additionally, conditional checks have been implemented in the login-related JavaScript files to adjust the flow based on the multi-root tenant functionality, ensuring that certain actions are bypassed under specific conditions. Changes
Possibly related PRs
Suggested reviewers
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
🧹 Outside diff range comments (2)
micro-ui/web/micro-ui-internals/packages/css/src/components/PageBasedInput.scss (1)
Line range hint
1-41: Consider a plan for full migration to new class structure.While the addition of new classes improves the overall structure and follows better naming conventions, the coexistence of old (
.PageBasedInputWrapper) and new (.digit-page-based-input-wrapper) classes may lead to confusion and maintenance challenges in the future.Consider the following steps for a future refactoring:
- Gradually migrate components to use the new class names.
- Once all components are migrated, remove the old class definitions.
- Ensure that this refactoring is coordinated with any JavaScript/TypeScript files that may be applying these classes dynamically.
This approach will help maintain a consistent styling structure throughout the application and reduce technical debt.
micro-ui/web/micro-ui-internals/packages/modules/core/src/pages/citizen/Login/index.js (1)
Enhance User Data Handling Consistency Across Scenarios
The
Logincomponent continues to utilizelocalStorageextensively for managing user data, as evidenced by multiplelocalStorage.setItemcalls. This reliance may lead to inconsistencies between multiroottenant and non-multiroottenant scenarios.Recommendations:
- Implement a centralized data management service that abstracts away the storage mechanism, allowing flexibility between
localStorageand alternative storage solutions based on configuration.- Refactor all direct
localStorageinteractions within theLogincomponent to utilize this service, ensuring consistent user data handling across different tenant configurations.- Conduct comprehensive testing to verify that user data is managed correctly in both multiroottenant and single-tenant environments.
🔗 Analysis chain
Line range hint
1-285: Consider a more comprehensive approach to user data handling.While the change in
setCitizenDetailaddresses the multiroottenant scenario, theLogincomponent still heavily relies on localStorage for various operations. This might lead to inconsistencies in how user data is handled between multiroottenant and non-multiroottenant scenarios.Consider refactoring the component to use a more consistent approach for handling user data:
- Implement a data management service that can switch between localStorage and an alternative storage method based on the multiroottenant flag.
- Update all direct localStorage calls in the component to use this service instead.
- This will ensure consistent behavior across different scenarios and make future migrations easier.
To assess the impact of the current implementation, run the following script to identify all localStorage usages in the file:
This will help in identifying areas that might need attention for a more comprehensive solution.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for localStorage usage in the Login component rg --type js 'localStorage\.' micro-ui/web/micro-ui-internals/packages/modules/core/src/pages/citizen/Login/index.jsLength of output: 619
🧰 Tools
🪛 Biome
[error] 20-20: This let declares a variable that is only assigned once.
'locale' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (1)
micro-ui/web/micro-ui-internals/packages/css/package.jsonis excluded by!**/*.json
📒 Files selected for processing (6)
- micro-ui/web/micro-ui-internals/packages/css/src/components/PageBasedInput.scss (1 hunks)
- micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/sandbox.scss (2 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/core/src/pages/citizen/Login/index.js (1 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/core/src/pages/employee/Login/login.js (1 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/core/src/pages/employee/Otp/index.js (2 hunks)
- micro-ui/web/public/index.html (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
micro-ui/web/micro-ui-internals/packages/modules/core/src/pages/citizen/Login/index.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/modules/core/src/pages/employee/Login/login.js (1)
Pattern
**/*.js: checkmicro-ui/web/micro-ui-internals/packages/modules/core/src/pages/employee/Otp/index.js (1)
Pattern
**/*.js: check
🔇 Additional comments (7)
micro-ui/web/public/index.html (1)
10-10: CSS version update looks good.The update to the CSS version from
1.8.2-beta.40to1.8.2-beta.41aligns with the PR objectives. This change should incorporate the latest styles and potential bug fixes.To ensure this update doesn't introduce any breaking changes, please run the following verification steps:
- Test the UI thoroughly across different pages and components.
- Check for any visual regressions or layout issues.
- Verify that all interactive elements still function correctly.
You can use the following script to check if there are any other files referencing the old CSS version:
If the script returns any results, those files may need to be updated as well.
✅ Verification successful
CSS version update verified.
All references to
digit-ui-css@1.8.2-beta.40have been successfully updated to1.8.2-beta.41. No remaining references to the old version were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to the old CSS version # Test: Search for references to the old CSS version rg --type html --type javascript --type typescript "digit-ui-css@1.8.2-beta.40"Length of output: 114
Script:
#!/bin/bash # Description: Check for any remaining references to the old CSS version # Test: Search for references to the old CSS version rg --type html --type js --type ts "digit-ui-css@1.8.2-beta.40"Length of output: 63
micro-ui/web/micro-ui-internals/packages/modules/core/src/pages/employee/Otp/index.js (2)
121-121: Approved: Improved formatting.The addition of a space after the BackLink component improves code readability. This change is in line with good coding practices and doesn't affect the functionality.
Line range hint
1-153: Overall assessment: Changes meet PR objectives.The modifications in this file successfully address the PR objective of removing localStorage usage for the multiroottenant feature. The implementation is correct and the code remains clean and readable. The suggested minor improvements (adding a comment for clarity) will further enhance the code quality.
🧰 Tools
🪛 Biome
[error] 15-15: This let declares a variable that is only assigned once.
'locale' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
micro-ui/web/micro-ui-internals/packages/modules/core/src/pages/citizen/Login/index.js (1)
17-19: 🧹 Nitpick (assertive)LGTM: Early return for multiroottenant scenario.
The addition of the early return when
Digit.Utils.getMultiRootTenant()is true aligns with the PR objective of removing localStorage usage for the multiroottenant feature. This change effectively bypasses localStorage operations in multiroottenant scenarios.To ensure the multiroottenant functionality is working as expected, please verify:
- The behavior when
Digit.Utils.getMultiRootTenant()returns true.- How user details are handled in multiroottenant scenarios without localStorage.
Consider adding a comment explaining why localStorage is still used for non-multiroottenant scenarios. This will help maintain clarity for future developers working on this code.
micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/sandbox.scss (3)
781-803: Verify the usage oftheme(colors.white)Ensure that
theme(colors.white)correctly returns the desired color value. Depending on your SCSS setup or theming function, the syntax might need adjustment. Verify that this usage aligns with your theming conventions.
768-771:⚠️ Potential issueVerify the
border-leftwidth value for cross-browser compatibilityThe
border-leftis set to0.031rem, which is approximately0.5px. Subpixel values may not render consistently across different browsers and devices. Consider using a standard pixel value like1pxor0.5pxfor better compatibility.
417-427: 🛠️ Refactor suggestionConsider reducing the use of
!importantdeclarationsThe use of
!importantin styles for.digit-back-linkand.digit-card-headercan make the CSS harder to maintain and override. Overusing!importantcan lead to specificity issues. Try to increase the selector specificity or restructure the styles to avoid the need for!important.⛔ Skipped due to learnings
Learnt from: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#675 File: micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss:33-58 Timestamp: 2024-10-08T20:11:12.539Z Learning: The use of `!important` in the `.wbh-header-container` and `.guideline-actionbar-content` CSS classes within `microplanning.scss` is necessary to prevent these styles from being overridden.Learnt from: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#675 File: micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss:33-58 Timestamp: 2024-10-08T20:11:07.772Z Learning: The use of `!important` in the `.wbh-header-container` and `.guideline-actionbar-content` CSS classes within `microplanning.scss` is necessary to prevent these styles from being overridden.Learnt from: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#675 File: micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss:649-652 Timestamp: 2024-10-08T20:11:07.772Z Learning: The use of `!important` in the `.excel-wrapper` CSS class is necessary to prevent the styles from being overridden in certain contexts.Learnt from: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#675 File: micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss:649-652 Timestamp: 2024-10-08T20:11:12.539Z Learning: The use of `!important` in the `.excel-wrapper` CSS class is necessary to prevent the styles from being overridden in certain contexts.Learnt from: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#876 File: micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss:86-97 Timestamp: 2024-10-08T20:11:07.772Z Learning: The use of `!important` in the `.modal-header` CSS class within `microplanning.scss` is necessary to ensure the styles are applied as intended and are not overridden by other styles.Learnt from: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#876 File: micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss:86-97 Timestamp: 2024-06-15T11:20:20.645Z Learning: The use of `!important` in the `.modal-header` CSS class within `microplanning.scss` is necessary to ensure the styles are applied as intended and are not overridden by other styles.Learnt from: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#675 File: micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss:33-58 Timestamp: 2024-05-23T05:31:54.468Z Learning: The use of `!important` in the `.wbh-header-container` and `.guideline-actionbar-content` CSS classes within `microplanning.scss` is necessary to prevent these styles from being overridden.
| .digit-page-based-input-wrapper { | ||
| .digit-page-based-submit-bar { | ||
| @apply hidden; | ||
| } | ||
| .digit-submit-bar-container { | ||
| z-index: 60; | ||
| @apply p-sm fixed w-full bottom-0 left-0 text-right bg-white; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Approve new class structure with a minor suggestion.
The new .digit-page-based-input-wrapper class and its children are well-structured and follow better CSS naming conventions. This refactoring improves the separation of concerns and allows for more specific styling.
Consider adding a comment explaining the purpose of the .digit-page-based-submit-bar class, as it's currently set to hidden without any apparent use.
| .digit-page-based-input-wrapper { | ||
| .digit-submit-bar-container { | ||
| @apply hidden; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Approve desktop styles with a suggestion for consistency.
The addition of desktop-specific styles for the new .digit-page-based-input-wrapper class is appropriate and consistent with the existing responsive design approach.
For better consistency and maintainability, consider consolidating the desktop styles for both the new and existing classes. You could move the .PageBasedInputWrapper styles inside the .digit-page-based-input-wrapper block, like this:
@screen dt {
.digit-page-based-input-wrapper {
.digit-submit-bar-container {
@apply hidden;
}
.PageBasedInputWrapper {
.SubmitBar {
@apply hidden;
}
.SubmitBarInCardInDesktopView {
@apply block;
}
.card {
max-width: 960px;
}
button {
max-width: 240px;
}
}
}
}This structure would make it clearer that all these styles are related to the same component and its variations.
| if (Digit.Utils.getMultiRootTenant()) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Approved: Early return for multi-root tenant scenario.
The addition of the early return when Digit.Utils.getMultiRootTenant() is true aligns with the PR objective of removing localStorage usage for the multiroottenant feature. This change effectively prevents setting employee details in localStorage when multi-root tenant functionality is active.
Consider adding a comment explaining the implications of this early return. For example:
if (Digit.Utils.getMultiRootTenant()) {
// Skip setting employee details in localStorage for multi-root tenant scenarios
// Note: Ensure that any code relying on these localStorage values has appropriate fallbacks
return;
}This will help other developers understand the purpose and potential impact of this condition.
| if (Digit.Utils.getMultiRootTenant()) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Consider adding a comment for clarity.
The changes align well with the PR objective of removing localStorage usage for the multiroottenant feature. The early return effectively prevents unnecessary localStorage operations when the multiroottenant functionality is active.
Consider adding a brief comment explaining why we're returning early for multiroottenant scenarios:
if (Digit.Utils.getMultiRootTenant()) {
+ // Skip localStorage operations for multiroottenant scenarios
return;
}📝 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.
| if (Digit.Utils.getMultiRootTenant()) { | |
| return; | |
| } | |
| if (Digit.Utils.getMultiRootTenant()) { | |
| // Skip localStorage operations for multiroottenant scenarios | |
| return; | |
| } |
| .digit-card-component.digit-landing-page-card { | ||
| max-width: 30rem !important; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Minimize the use of !important in CSS
In the .digit-card-component.digit-landing-page-card, the max-width property is set with !important. Consider whether the !important is necessary here, as it can complicate maintenance and override of styles.
| @media (max-width: 768px) { | ||
| .homeWrapper > :last-child { | ||
| margin-left: 1rem !important; | ||
| } | ||
| .digit-landing-page-wrapper { | ||
| margin-left: 1rem !important; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor media query selectors to reduce redundancy
Both .homeWrapper > :last-child and .digit-landing-page-wrapper have the same margin-left property within the media query. You can combine them to simplify the CSS:
@media (max-width: 768px) {
.homeWrapper > :last-child,
.digit-landing-page-wrapper {
margin-left: 1rem !important;
}
}
Summary by CodeRabbit
New Features
Bug Fixes
Documentation