-
Notifications
You must be signed in to change notification settings - Fork 46
Ccsd 595 #3007
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
base: develop
Are you sure you want to change the base?
Ccsd 595 #3007
Conversation
📝 WalkthroughWalkthroughUpdated changelogs and added a CSS rule for sandbox layout. The employee page wrapper now includes the "sandbox-module" classname in the non-collapsed rendering path; collapsed rendering remains unchanged. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 6
🔭 Outside diff range comments (1)
micro-ui/web/micro-ui-internals/packages/modules/core/src/pages/employee/index.js (1)
52-53
: Path check is brittle; use route basepath
to buildhideClass
Hardcoding
"employee/sandbox/productDetailsPage/"
risks false negatives with different context paths or base routes. Use the matchedpath
.- const hideClass = location.pathname.includes(`employee/sandbox/productDetailsPage/`); + const hideClass = location.pathname.includes(`${path}/sandbox/productDetailsPage/`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (5)
micro-ui/web/micro-ui-internals/example/package.json
is excluded by!**/*.json
micro-ui/web/micro-ui-internals/packages/css/package.json
is excluded by!**/*.json
micro-ui/web/micro-ui-internals/packages/modules/core/package.json
is excluded by!**/*.json
micro-ui/web/package.json
is excluded by!**/*.json
micro-ui/web/sandbox/package.json
is excluded by!**/*.json
📒 Files selected for processing (4)
micro-ui/web/micro-ui-internals/packages/css/CHANGELOG.md
(1 hunks)micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/sandbox.scss
(1 hunks)micro-ui/web/micro-ui-internals/packages/modules/core/CHANGELOG.md
(1 hunks)micro-ui/web/micro-ui-internals/packages/modules/core/src/pages/employee/index.js
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.js
⚙️ CodeRabbit Configuration File
check
Files:
micro-ui/web/micro-ui-internals/packages/modules/core/src/pages/employee/index.js
🧠 Learnings (2)
📚 Learning: 2025-06-26T10:24:08.628Z
Learnt from: Hari-egov
PR: egovernments/DIGIT-Frontend#2644
File: micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/sandbox.scss:1642-1662
Timestamp: 2025-06-26T10:24:08.628Z
Learning: In the sandbox SCSS file, tab styling with margin-bottom and border changes that cause layout shifts are intentional design requirements per updated UI specifications, not bugs to be fixed.
Applied to files:
micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/sandbox.scss
📚 Learning: 2024-11-07T11:02:33.520Z
Learnt from: rachana-egov
PR: egovernments/DIGIT-Frontend#1770
File: health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/viewVillage.js:320-322
Timestamp: 2024-11-07T11:02:33.520Z
Learning: In `health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/viewVillage.js`, the `data?.additionalFields` object is guaranteed to be defined, so checking for its existence before accessing its keys is unnecessary.
Applied to files:
micro-ui/web/micro-ui-internals/packages/modules/core/src/pages/employee/index.js
🧬 Code Graph Analysis (1)
micro-ui/web/micro-ui-internals/packages/modules/core/src/pages/employee/index.js (3)
micro-ui/web/micro-ui-internals/packages/modules/core/src/components/AppModules.js (2)
isSuperUserWithMultipleRootTenant
(38-38)hideClass
(39-40)micro-ui/web/micro-ui-internals/packages/modules/core/src/components/Home.js (1)
isSuperUserWithMultipleRootTenant
(197-197)micro-ui/web/micro-ui-internals/packages/modules/sandbox/src/pages/employee/index.js (1)
hideClass
(66-67)
🪛 LanguageTool
micro-ui/web/micro-ui-internals/packages/css/CHANGELOG.md
[grammar] ~3-~3: Use hyphens correctly
Context: ...# Changelog ## [1.8.39] - [18-Aug-2025] -Employee app wrapper classname css added...
(QB_NEW_EN_OTHER_ERROR_IDS_29)
[grammar] ~4-~4: Use correct spacing
Context: ...mployee app wrapper classname css added margin fixed for sandbox ## [1.8.38] - ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~4-~4: There might be a mistake here.
Context: ...name css added margin fixed for sandbox ## [1.8.38] - [14-Aug-2025] -Removed the lo...
(QB_NEW_EN_OTHER)
micro-ui/web/micro-ui-internals/packages/modules/core/CHANGELOG.md
[grammar] ~3-~3: Use correct spacing
Context: # Changelog ## [1.8.45] [18-Aug-2025] - Employee app wrapper mar...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~3-~3: There might be a mistake here.
Context: ...# Changelog ## [1.8.45] [18-Aug-2025] - Employee app wrapper margin fixed for sa...
(QB_NEW_EN)
[grammar] ~4-~4: There might be a mistake here.
Context: ...yee app wrapper margin fixed for sandbox ## [1.8.44] [15-July-2025] - Citizen login...
(QB_NEW_EN_OTHER)
## [1.8.39] - [18-Aug-2025] | ||
-Employee app wrapper classname css added margin fixed for sandbox | ||
|
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.
🧹 Nitpick (assertive)
Fix hyphen/spacing and clarify entry
There’s a missing space after the bullet and double spacing in the sentence. Suggested clearer phrasing:
-## [1.8.39] - [18-Aug-2025]
--Employee app wrapper classname css added margin fixed for sandbox
+## [1.8.39] - [18-Aug-2025]
+- Added CSS classname to Employee app wrapper; fixed Sandbox margin
📝 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.
## [1.8.39] - [18-Aug-2025] | |
-Employee app wrapper classname css added margin fixed for sandbox | |
## [1.8.39] - [18-Aug-2025] | |
- Added CSS classname to Employee app wrapper; fixed Sandbox margin |
🧰 Tools
🪛 LanguageTool
[grammar] ~3-~3: Use hyphens correctly
Context: ...# Changelog ## [1.8.39] - [18-Aug-2025] -Employee app wrapper classname css added...
(QB_NEW_EN_OTHER_ERROR_IDS_29)
[grammar] ~4-~4: Use correct spacing
Context: ...mployee app wrapper classname css added margin fixed for sandbox ## [1.8.38] - ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~4-~4: There might be a mistake here.
Context: ...name css added margin fixed for sandbox ## [1.8.38] - [14-Aug-2025] -Removed the lo...
(QB_NEW_EN_OTHER)
🤖 Prompt for AI Agents
In micro-ui/web/micro-ui-internals/packages/css/CHANGELOG.md around lines 3 to
5, the changelog entry uses inconsistent spacing and unclear wording; update the
bullet to include a space after the dash, remove the double spacing, and
rephrase for clarity (e.g., "Employee app wrapper classname CSS added; margin
fixed for sandbox" or "Added Employee app wrapper classname CSS and fixed
sandbox margin") so the entry is grammatically correct and concise.
.employee-app-wrapper .sandbox-module { | ||
min-height: calc(100vh - 4em); | ||
margin: -70px; | ||
} |
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.
🧹 Nitpick (assertive)
Optional: consider a token/variable for header height instead of hardcoded 4em
If a design token or CSS variable exists for topbar height, prefer it to keep layouts consistent across themes.
Example:
min-height: calc(100vh - var(--digit-topbar-height, 4em));
🤖 Prompt for AI Agents
In micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/sandbox.scss
around lines 553 to 556, the min-height uses a hardcoded "4em" for header/topbar
deduction which can diverge across themes; replace the literal with a design
token or CSS variable (e.g., use an existing topbar height token or define
--digit-topbar-height with a fallback) and update the calc to subtract that
variable so layouts stay consistent across themes and can be overridden by
consumers.
🛠️ Refactor suggestion
Selector will never match with current DOM; also avoid sweeping negative margins
You add both classes to the same element in index.js, but the selector here uses a descendant combinator. .employee-app-wrapper .sandbox-module
won’t match when both classes are on the same node. Also, margin: -70px
applies negative margins on all sides and can cause horizontal overflow/scroll.
Fix the selector to target the same element and constrain the negative margin to the top only.
Apply this diff:
-.employee-app-wrapper .sandbox-module {
- min-height: calc(100vh - 4em);
- margin: -70px;
-}
+.employee-app-wrapper.sandbox-module {
+ min-height: calc(100vh - 4em);
+ margin-top: -70px;
+}
Note: This preserves the intended vertical offset while preventing unintended horizontal overflow. The earlier learning about intentional layout shifts in sandbox styling is respected; this change only scopes the effect correctly.
📝 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.
.employee-app-wrapper .sandbox-module { | |
min-height: calc(100vh - 4em); | |
margin: -70px; | |
} | |
.employee-app-wrapper.sandbox-module { | |
min-height: calc(100vh - 4em); | |
margin-top: -70px; | |
} |
🤖 Prompt for AI Agents
In micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/sandbox.scss
around lines 553–556, the selector uses a descendant combinator but both classes
are applied to the same element; change the selector to target the same element
(e.g., .employee-app-wrapper.sandbox-module) and replace the sweeping negative
margin with a constrained top-only negative margin (e.g., margin-top: -70px;
leave left/right/bottom at their defaults or explicit 0 if needed) while
preserving the min-height calc(100vh - 4em).
## [1.8.45] [18-Aug-2025] | ||
- Employee app wrapper margin fixed for sandbox | ||
|
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.
🧹 Nitpick (assertive)
Polish wording and spacing for clarity
Minor grammar/formatting tweak for consistency with the rest of the changelog.
-## [1.8.45] [18-Aug-2025]
-- Employee app wrapper margin fixed for sandbox
+## [1.8.45] [18-Aug-2025]
+- Fix: Employee app wrapper margin for Sandbox
📝 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.
## [1.8.45] [18-Aug-2025] | |
- Employee app wrapper margin fixed for sandbox | |
## [1.8.45] [18-Aug-2025] | |
- Fix: Employee app wrapper margin for Sandbox |
🧰 Tools
🪛 LanguageTool
[grammar] ~3-~3: Use correct spacing
Context: # Changelog ## [1.8.45] [18-Aug-2025] - Employee app wrapper mar...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~3-~3: There might be a mistake here.
Context: ...# Changelog ## [1.8.45] [18-Aug-2025] - Employee app wrapper margin fixed for sa...
(QB_NEW_EN)
[grammar] ~4-~4: There might be a mistake here.
Context: ...yee app wrapper margin fixed for sandbox ## [1.8.44] [15-July-2025] - Citizen login...
(QB_NEW_EN_OTHER)
🤖 Prompt for AI Agents
In micro-ui/web/micro-ui-internals/packages/modules/core/CHANGELOG.md around
lines 3 to 5, the changelog entry needs wording and spacing consistency; change
the bullet from "- Employee app wrapper margin fixed for sandbox" to a clearer,
consistently styled line such as "- Fixed employee app wrapper margin for
sandbox." and ensure punctuation and capitalization match other entries
(sentence case and a trailing period), and preserve the surrounding blank
line/formatting to match the changelog's existing style.
<div className={!noTopBar ? `${(isSuperUserWithMultipleRootTenant) ? "" : "main"} ${DSO ? "m-auto" : ""} digit-home-main` : ""}> | ||
|
||
<div className={!noTopBar ? `${(isSuperUserWithMultipleRootTenant && hideClass) ? "" : "employee-app-wrapper"} digit-home-app-wrapper` : ""}> | ||
<div className={!noTopBar ? `${(isSuperUserWithMultipleRootTenant && hideClass) ? "" : "employee-app-wrapper sandbox-module"} digit-home-app-wrapper` : ""}> |
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.
🧹 Nitpick (assertive)
Optional: simplify conditional class composition
Consider using a helper like clsx
/classnames
for readability and to avoid stray spaces.
Example with clsx
:
import clsx from "clsx";
// ...
<div
className={
!noTopBar
? clsx(
"digit-home-app-wrapper",
"employee-app-wrapper",
{ "sandbox-module": !(isSuperUserWithMultipleRootTenant && hideClass) }
)
: ""
}
>
🤖 Prompt for AI Agents
In
micro-ui/web/micro-ui-internals/packages/modules/core/src/pages/employee/index.js
around line 136, the className expression with nested template strings is hard
to read and can produce stray spaces; replace it with a class composition helper
(e.g., import clsx or classnames at the top) and use it to build the classes:
when !noTopBar provide the base classes "digit-home-app-wrapper" and
"employee-app-wrapper" and include "sandbox-module" only when
!(isSuperUserWithMultipleRootTenant && hideClass), otherwise set className to an
empty string; add the necessary import and update the JSX to call the helper
instead of the current template literal.
💡 Verification agent
🧩 Analysis chain
Classes added on the same element; ensure CSS selector matches and keep base wrapper class when hiding sandbox modifier
You’re applying sandbox-module
on the same element as employee-app-wrapper
. The CSS introduced uses a descendant selector, so it won't match unless corrected (see SCSS review). Also, when (isSuperUserWithMultipleRootTenant && hideClass)
is true, you drop employee-app-wrapper
as well. If the intent is only to disable the sandbox-specific overrides, keep employee-app-wrapper
and toggle only sandbox-module
.
Apply this diff to keep the base class present:
- <div className={!noTopBar ? `${(isSuperUserWithMultipleRootTenant && hideClass) ? "" : "employee-app-wrapper sandbox-module"} digit-home-app-wrapper` : ""}>
+ <div className={!noTopBar ? `${(isSuperUserWithMultipleRootTenant && hideClass) ? "employee-app-wrapper" : "employee-app-wrapper sandbox-module"} digit-home-app-wrapper` : ""}>
Action items to verify:
- Confirm the intent: Should
employee-app-wrapper
be present on product details pages? If yes, the above change avoids unintended style regressions. - Ensure the SCSS selector is updated to
.employee-app-wrapper.sandbox-module
to match this DOM structure.
🏁 Script executed:
#!/bin/bash
# Display the relevant JSX snippet from the employee page index.js
sed -n '130,150p' micro-ui/web/micro-ui-internals/packages/modules/core/src/pages/employee/index.js || true
# Search SCSS/CSS for definitions of sandbox-module and employee-app-wrapper
rg -n "\.sandbox-module" -g '*.{scss,css}'
rg -n "\.employee-app-wrapper" -g '*.{scss,css}'
Length of output: 1829
Ensure base wrapper class is always present and update SCSS selector
The JSX currently drops employee-app-wrapper
when disabling sandbox overrides, and the SCSS uses a descendant selector (.employee-app-wrapper .sandbox-module
) that won’t match when both classes live on the same element. Adjust the markup to always include employee-app-wrapper
and toggle only sandbox-module
, and update the CSS selector to a class-combination.
• File:
micro-ui/web/micro-ui-internals/packages/modules/core/src/pages/employee/index.js
Line: 136
• SCSS:
micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/sandbox.scss
Update selector from:
.employee-app-wrapper .sandbox-module { … }
to:
.employee-app-wrapper.sandbox-module { … }
• JSX diff:
- <div className={!noTopBar ? `${(isSuperUserWithMultipleRootTenant && hideClass) ? "" : "employee-app-wrapper sandbox-module"} digit-home-app-wrapper` : ""}>
+ <div className={!noTopBar ? `${(isSuperUserWithMultipleRootTenant && hideClass) ? "employee-app-wrapper" : "employee-app-wrapper sandbox-module"} digit-home-app-wrapper` : ""}>
📝 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.
<div className={!noTopBar ? `${(isSuperUserWithMultipleRootTenant && hideClass) ? "" : "employee-app-wrapper sandbox-module"} digit-home-app-wrapper` : ""}> | |
<div | |
className={ | |
!noTopBar | |
? `${(isSuperUserWithMultipleRootTenant && hideClass) | |
? "employee-app-wrapper" | |
: "employee-app-wrapper sandbox-module" | |
} digit-home-app-wrapper` | |
: "" | |
} | |
> |
🤖 Prompt for AI Agents
In
micro-ui/web/micro-ui-internals/packages/modules/core/src/pages/employee/index.js
around line 136, the JSX conditionally removes the base "employee-app-wrapper"
class when sandbox overrides are disabled; change the markup so
"employee-app-wrapper" is always present and only "sandbox-module" is toggled
(i.e., ensure the class list always includes "employee-app-wrapper" and
conditionally adds "sandbox-module" and any other classes). Also update the SCSS
file
micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/sandbox.scss by
changing the selector from the descendant form ".employee-app-wrapper
.sandbox-module" to the combined-class form
".employee-app-wrapper.sandbox-module" so styles apply when both classes are on
the same element.
Choose the appropriate template for your PR:
Feature/Bugfix Request
JIRA ID
Module
Description
Summary by CodeRabbit
Bug Fixes
Documentation