-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Citizen sidebar cache #1620
Citizen sidebar cache #1620
Conversation
📝 WalkthroughWalkthroughThe changes involve updates to 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
- micro-ui/web/micro-ui-internals/packages/modules/core/src/components/TopBarSideBar/SideBar/StaticCitizenSideBar.js (2 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/core/src/config/sidebar-menu.js (0 hunks)
- micro-ui/web/micro-ui-internals/packages/modules/hrms/src/pages/createEmployee.js (3 hunks)
💤 Files with no reviewable changes (1)
- micro-ui/web/micro-ui-internals/packages/modules/core/src/config/sidebar-menu.js
🧰 Additional context used
📓 Path-based instructions (2)
micro-ui/web/micro-ui-internals/packages/modules/core/src/components/TopBarSideBar/SideBar/StaticCitizenSideBar.js (1)
Pattern
**/*.js
: checkmicro-ui/web/micro-ui-internals/packages/modules/hrms/src/pages/createEmployee.js (1)
Pattern
**/*.js
: check
🪛 Biome
micro-ui/web/micro-ui-internals/packages/modules/hrms/src/pages/createEmployee.js
[error] 150-150: This let declares a variable that is only assigned once.
'selectedCity' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
🔇 Additional comments (5)
micro-ui/web/micro-ui-internals/packages/modules/core/src/components/TopBarSideBar/SideBar/StaticCitizenSideBar.js (2)
117-119
: LGTM: NewcloseSidebar
function added.The
closeSidebar
function is well-implemented. It correctly uses thehistory.push
method to navigate to the all-services page, and the use ofwindow?.contextPath
ensures compatibility with different deployment configurations.
Line range hint
1-324
: Overall assessment: Changes improve navigation and maintain code quality.The additions to the
StaticCitizenSideBar
component enhance the user experience by providing a clear way to return to the home page. The newcloseSidebar
function and the "HOME" menu item are well-implemented and consistent with the existing code style. These changes align with the PR objectives of updating the citizen sidebar.micro-ui/web/micro-ui-internals/packages/modules/hrms/src/pages/createEmployee.js (3)
19-19
: LGTM!The introduction of
isMultiRootTenant
correctly enhances tenant handling based on the multi-root tenant configuration.
154-155
:⚠️ Potential issueVerify if overwriting jurisdiction boundaries is intended
In the mapping of
data.Jurisdictions
, each jurisdiction'sboundary
is being overwritten withselectedCity
, which is derived from the first jurisdiction's boundary. This means all jurisdictions will have the same boundary. If jurisdictions are supposed to have different boundaries, this could lead to incorrect data.Please confirm if this behavior is intentional. If not, consider adjusting the code to preserve the original boundaries of each jurisdiction.
182-187
:⚠️ Potential issueEnsure correct assignment of
tenantId
for rolesThe assignment of
item["tenantId"]
usesele.boundary
whenisMultiRootTenant
isfalse
. Verify thatele.boundary
represents a validtenantId
. Typically,tenantId
andboundary
may not be interchangeable.Consider whether
ele.tenantId
should be used instead:- item["tenantId"] = ele.boundary; + item["tenantId"] = ele.tenantId;If
ele.boundary
is indeed the correct value fortenantId
in this context, no action is needed. Otherwise, adjust the assignment to use the appropriate tenant identifier.
...internals/packages/modules/core/src/components/TopBarSideBar/SideBar/StaticCitizenSideBar.js
Outdated
Show resolved
Hide resolved
micro-ui/web/micro-ui-internals/packages/modules/hrms/src/pages/createEmployee.js
Outdated
Show resolved
Hide resolved
…s/createEmployee.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…onents/TopBarSideBar/SideBar/StaticCitizenSideBar.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
home removed from citizen login screen
hrms boundary dropdown updated
Summary by CodeRabbit
New Features
Bug Fixes
Chores